Skip to content

Commit 955b493

Browse files
crisbetoangular-robot[bot]
authored andcommitted
fix(@angular-devkit/build-angular): support CSP on critical CSS link tags.
Based on #24880 (review). Critters can generate `link` tags with inline `onload` handlers which breaks CSP. These changes update the style nonce processor to remove the `onload` handlers and replicate the behavior with an inline `script` tag that gets the proper nonce. Note that earlier we talked about doing this through Critters which while possible, would still require a custom HTML processor, because we need to both add and remove attributes from an element.
1 parent dc5cc89 commit 955b493

File tree

3 files changed

+192
-2
lines changed

3 files changed

+192
-2
lines changed

packages/angular_devkit/build_angular/src/builders/app-shell/app-shell_spec.ts

+26
Original file line numberDiff line numberDiff line change
@@ -163,4 +163,30 @@ describe('AppShell Builder', () => {
163163
/<link rel="stylesheet" href="styles\.[a-z0-9]+\.css" media="print" onload="this\.media='all'">/,
164164
);
165165
});
166+
167+
it('applies CSP nonce to critical CSS', async () => {
168+
host.writeMultipleFiles(appShellRouteFiles);
169+
host.replaceInFile('src/index.html', /<app-root/g, '<app-root ngCspNonce="{% nonce %}" ');
170+
const overrides = {
171+
route: 'shell',
172+
browserTarget: 'app:build:production,inline-critical-css',
173+
};
174+
175+
const run = await architect.scheduleTarget(target, overrides);
176+
const output = await run.result;
177+
await run.stop();
178+
179+
expect(output.success).toBe(true);
180+
const fileName = 'dist/index.html';
181+
const content = virtualFs.fileBufferToString(host.scopedSync().read(normalize(fileName)));
182+
183+
expect(content).toContain('app-shell works!');
184+
expect(content).toContain('<style nonce="{% nonce %}">p{color:#000}</style>');
185+
expect(content).toContain('<style ng-app-id="ng" nonce="{% nonce %}">');
186+
expect(content).toContain('<app-root ngcspnonce="{% nonce %}"');
187+
expect(content).toContain('<script nonce="{% nonce %}">');
188+
expect(content).toMatch(
189+
/<link rel="stylesheet" href="styles\.[a-z0-9]+\.css" media="print" ngCspMedia="all">/,
190+
);
191+
});
166192
});

packages/angular_devkit/build_angular/src/utils/index-file/inline-critical-css.ts

+136
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,16 @@ import * as fs from 'fs';
1010

1111
const Critters: typeof import('critters').default = require('critters');
1212

13+
/**
14+
* Pattern used to extract the media query set by Critters in an `onload` handler.
15+
*/
16+
const MEDIA_SET_HANDLER_PATTERN = /^this\.media=["'](.*)["'];?$/;
17+
18+
/**
19+
* Name of the attribute used to save the Critters media query so it can be re-assigned on load.
20+
*/
21+
const CSP_MEDIA_ATTR = 'ngCspMedia';
22+
1323
export interface InlineCriticalCssProcessOptions {
1424
outputPath: string;
1525
}
@@ -20,9 +30,40 @@ export interface InlineCriticalCssProcessorOptions {
2030
readAsset?: (path: string) => Promise<string>;
2131
}
2232

33+
/** Partial representation of an `HTMLElement`. */
34+
interface PartialHTMLElement {
35+
getAttribute(name: string): string | null;
36+
setAttribute(name: string, value: string): void;
37+
hasAttribute(name: string): boolean;
38+
removeAttribute(name: string): void;
39+
appendChild(child: PartialHTMLElement): void;
40+
textContent: string;
41+
tagName: string | null;
42+
children: PartialHTMLElement[];
43+
}
44+
45+
/** Partial representation of an HTML `Document`. */
46+
interface PartialDocument {
47+
head: PartialHTMLElement;
48+
createElement(tagName: string): PartialHTMLElement;
49+
querySelector(selector: string): PartialHTMLElement | null;
50+
}
51+
52+
/** Signature of the `Critters.embedLinkedStylesheet` method. */
53+
type EmbedLinkedStylesheetFn = (
54+
link: PartialHTMLElement,
55+
document: PartialDocument,
56+
) => Promise<unknown>;
57+
2358
class CrittersExtended extends Critters {
2459
readonly warnings: string[] = [];
2560
readonly errors: string[] = [];
61+
private initialEmbedLinkedStylesheet: EmbedLinkedStylesheetFn;
62+
private addedCspScriptsDocuments = new WeakSet<PartialDocument>();
63+
private documentNonces = new WeakMap<PartialDocument, string | null>();
64+
65+
// Inherited from `Critters`, but not exposed in the typings.
66+
protected embedLinkedStylesheet!: EmbedLinkedStylesheetFn;
2667

2768
constructor(
2869
private readonly optionsExtended: InlineCriticalCssProcessorOptions &
@@ -41,17 +82,112 @@ class CrittersExtended extends Critters {
4182
pruneSource: false,
4283
reduceInlineStyles: false,
4384
mergeStylesheets: false,
85+
// Note: if `preload` changes to anything other than `media`, the logic in
86+
// `embedLinkedStylesheetOverride` will have to be updated.
4487
preload: 'media',
4588
noscriptFallback: true,
4689
inlineFonts: true,
4790
});
91+
92+
// We can't use inheritance to override `embedLinkedStylesheet`, because it's not declared in
93+
// the `Critters` .d.ts which means that we can't call the `super` implementation. TS doesn't
94+
// allow for `super` to be cast to a different type.
95+
this.initialEmbedLinkedStylesheet = this.embedLinkedStylesheet;
96+
this.embedLinkedStylesheet = this.embedLinkedStylesheetOverride;
4897
}
4998

5099
public override readFile(path: string): Promise<string> {
51100
const readAsset = this.optionsExtended.readAsset;
52101

53102
return readAsset ? readAsset(path) : fs.promises.readFile(path, 'utf-8');
54103
}
104+
105+
/**
106+
* Override of the Critters `embedLinkedStylesheet` method
107+
* that makes it work with Angular's CSP APIs.
108+
*/
109+
private embedLinkedStylesheetOverride: EmbedLinkedStylesheetFn = async (link, document) => {
110+
const returnValue = await this.initialEmbedLinkedStylesheet(link, document);
111+
const cspNonce = this.findCspNonce(document);
112+
113+
if (cspNonce) {
114+
const crittersMedia = link.getAttribute('onload')?.match(MEDIA_SET_HANDLER_PATTERN);
115+
116+
if (crittersMedia) {
117+
// If there's a Critters-generated `onload` handler and the file has an Angular CSP nonce,
118+
// we have to remove the handler, because it's incompatible with CSP. We save the value
119+
// in a different attribute and we generate a script tag with the nonce that uses
120+
// `addEventListener` to apply the media query instead.
121+
link.removeAttribute('onload');
122+
link.setAttribute(CSP_MEDIA_ATTR, crittersMedia[1]);
123+
this.conditionallyInsertCspLoadingScript(document, cspNonce);
124+
}
125+
126+
// Ideally we would hook in at the time Critters inserts the `style` tags, but there isn't
127+
// a way of doing that at the moment so we fall back to doing it any time a `link` tag is
128+
// inserted. We mitigate it by only iterating the direct children of the `<head>` which
129+
// should be pretty shallow.
130+
document.head.children.forEach((child) => {
131+
if (child.tagName === 'style' && !child.hasAttribute('nonce')) {
132+
child.setAttribute('nonce', cspNonce);
133+
}
134+
});
135+
}
136+
137+
return returnValue;
138+
};
139+
140+
/**
141+
* Finds the CSP nonce for a specific document.
142+
*/
143+
private findCspNonce(document: PartialDocument): string | null {
144+
if (this.documentNonces.has(document)) {
145+
return this.documentNonces.get(document) ?? null;
146+
}
147+
148+
// HTML attribute are case-insensitive, but the parser used by Critters is case-sensitive.
149+
const nonceElement = document.querySelector('[ngCspNonce], [ngcspnonce]');
150+
const cspNonce =
151+
nonceElement?.getAttribute('ngCspNonce') || nonceElement?.getAttribute('ngcspnonce') || null;
152+
153+
this.documentNonces.set(document, cspNonce);
154+
155+
return cspNonce;
156+
}
157+
158+
/**
159+
* Inserts the `script` tag that swaps the critical CSS at runtime,
160+
* if one hasn't been inserted into the document already.
161+
*/
162+
private conditionallyInsertCspLoadingScript(document: PartialDocument, nonce: string) {
163+
if (this.addedCspScriptsDocuments.has(document)) {
164+
return;
165+
}
166+
167+
const script = document.createElement('script');
168+
script.setAttribute('nonce', nonce);
169+
script.textContent = [
170+
`(() => {`,
171+
// Save the `children` in a variable since they're a live DOM node collection.
172+
// We iterate over the direct descendants, instead of going through a `querySelectorAll`,
173+
// because we know that the tags will be directly inside the `head`.
174+
` const children = document.head.children;`,
175+
// Declare `onLoad` outside the loop to avoid leaking memory.
176+
// Can't be an arrow function, because we need `this` to refer to the DOM node.
177+
` function onLoad() {this.media = this.getAttribute('${CSP_MEDIA_ATTR}');}`,
178+
// Has to use a plain for loop, because some browsers don't support
179+
// `forEach` on `children` which is a `HTMLCollection`.
180+
` for (let i = 0; i < children.length; i++) {`,
181+
` const child = children[i];`,
182+
` child.hasAttribute('${CSP_MEDIA_ATTR}') && child.addEventListener('load', onLoad);`,
183+
` }`,
184+
`})();`,
185+
].join('\n');
186+
// Append the script to the head since it needs to
187+
// run as early as possible, after the `link` tags.
188+
document.head.appendChild(script);
189+
this.addedCspScriptsDocuments.add(document);
190+
}
55191
}
56192

57193
export class InlineCriticalCssProcessor {

packages/angular_devkit/build_angular/src/utils/index-file/inline-critical-css_spec.ts

+30-2
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,14 @@ describe('InlineCriticalCssProcessor', () => {
3030
throw new Error('Cannot read asset.');
3131
};
3232

33-
const getContent = (deployUrl: string): string => {
33+
const getContent = (deployUrl: string, bodyContent = ''): string => {
3434
return `
3535
<html>
3636
<head>
3737
<link href="${deployUrl}styles.css" rel="stylesheet">
3838
<link href="${deployUrl}theme.css" rel="stylesheet">
3939
</head>
40-
<body></body>
40+
<body>${bodyContent}</body>
4141
</html>`;
4242
};
4343

@@ -105,4 +105,32 @@ describe('InlineCriticalCssProcessor', () => {
105105
);
106106
expect(content).toContain('<style>body{margin:0}html{color:white}</style>');
107107
});
108+
109+
it('should process the inline `onload` handlers if a CSP nonce is specified', async () => {
110+
const inlineCssProcessor = new InlineCriticalCssProcessor({
111+
readAsset,
112+
});
113+
114+
const { content } = await inlineCssProcessor.process(
115+
getContent('', '<app ngCspNonce="{% nonce %}"></app>'),
116+
{
117+
outputPath: '/dist/',
118+
},
119+
);
120+
121+
expect(content).toContain(
122+
'<link href="styles.css" rel="stylesheet" media="print" ngCspMedia="all">',
123+
);
124+
expect(content).toContain(
125+
'<link href="theme.css" rel="stylesheet" media="print" ngCspMedia="all">',
126+
);
127+
// Nonces shouldn't be added inside the `noscript` tags.
128+
expect(content).toContain('<noscript><link rel="stylesheet" href="theme.css"></noscript>');
129+
expect(content).toContain('<script nonce="{% nonce %}">');
130+
expect(tags.stripIndents`${content}`).toContain(tags.stripIndents`
131+
<style nonce="{% nonce %}">
132+
body { margin: 0; }
133+
html { color: white; }
134+
</style>`);
135+
});
108136
});

0 commit comments

Comments
 (0)