Skip to content

Commit

Permalink
fix(@angular-devkit/build-angular): support CSP on critical CSS link …
Browse files Browse the repository at this point in the history
…tags.

Based on angular#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.
  • Loading branch information
crisbeto committed Mar 28, 2023
1 parent 0ac5f27 commit 79aad28
Show file tree
Hide file tree
Showing 3 changed files with 192 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -163,4 +163,30 @@ describe('AppShell Builder', () => {
/<link rel="stylesheet" href="styles\.[a-z0-9]+\.css" media="print" onload="this\.media='all'">/,
);
});

it('applies CSP nonce to critical CSS', async () => {
host.writeMultipleFiles(appShellRouteFiles);
host.replaceInFile('src/index.html', /<app-root/g, '<app-root ngCspNonce="{% nonce %}" ');
const overrides = {
route: 'shell',
browserTarget: 'app:build:production,inline-critical-css',
};

const run = await architect.scheduleTarget(target, overrides);
const output = await run.result;
await run.stop();

expect(output.success).toBe(true);
const fileName = 'dist/index.html';
const content = virtualFs.fileBufferToString(host.scopedSync().read(normalize(fileName)));

expect(content).toContain('app-shell works!');
expect(content).toContain('<style nonce="{% nonce %}">p{color:#000}</style>');
expect(content).toContain('<style ng-app-id="ng" nonce="{% nonce %}">');
expect(content).toContain('<app-root ngcspnonce="{% nonce %}"');
expect(content).toContain('<script nonce="{% nonce %}">');
expect(content).toMatch(
/<link rel="stylesheet" href="styles\.[a-z0-9]+\.css" media="print" ngCspMedia="all">/,
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ import * as fs from 'fs';

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

/**
* Pattern used to extract the media query set by Critters in an `onload` handler.
*/
const MEDIA_SET_HANDLER_PATTERN = /^this\.media=["'](.*)["'];?$/;

/**
* Name of the attribute used to save the Critters media query so it can be re-assigned on load.
*/
const CSP_MEDIA_ATTR = 'ngCspMedia';

export interface InlineCriticalCssProcessOptions {
outputPath: string;
}
Expand All @@ -20,9 +30,40 @@ export interface InlineCriticalCssProcessorOptions {
readAsset?: (path: string) => Promise<string>;
}

/** Partial representation of an `HTMLElement`. */
interface PartialHTMLElement {
getAttribute(name: string): string | null;
setAttribute(name: string, value: string): void;
hasAttribute(name: string): boolean;
removeAttribute(name: string): void;
appendChild(child: PartialHTMLElement): void;
textContent: string;
tagName: string | null;
children: PartialHTMLElement[];
}

/** Partial representation of an HTML `Document`. */
interface PartialDocument {
head: PartialHTMLElement;
createElement(tagName: string): PartialHTMLElement;
querySelector(selector: string): PartialHTMLElement | null;
}

/** Signature of the `Critters.embedLinkedStylesheet` method. */
type EmbedLinkedStylesheetFn = (
link: PartialHTMLElement,
document: PartialDocument,
) => Promise<unknown>;

class CrittersExtended extends Critters {
readonly warnings: string[] = [];
readonly errors: string[] = [];
private initialEmbedLinkedStylesheet: EmbedLinkedStylesheetFn;
private addedCspScriptsDocuments = new WeakSet<PartialDocument>();
private documentNonces = new WeakMap<PartialDocument, string | null>();

// Inherited from `Critters`, but not exposed in the typings.
protected embedLinkedStylesheet!: EmbedLinkedStylesheetFn;

constructor(
private readonly optionsExtended: InlineCriticalCssProcessorOptions &
Expand All @@ -41,17 +82,112 @@ class CrittersExtended extends Critters {
pruneSource: false,
reduceInlineStyles: false,
mergeStylesheets: false,
// Note: if `preload` changes to anything other than `media`, the logic in
// `embedLinkedStylesheetOverride` will have to be updated.
preload: 'media',
noscriptFallback: true,
inlineFonts: true,
});

// We can't use inheritance to override `embedLinkedStylesheet`, because it's not declared in
// the `Critters` .d.ts which means that we can't call the `super` implementation. TS doesn't
// allow for `super` to be cast to a different type.
this.initialEmbedLinkedStylesheet = this.embedLinkedStylesheet;
this.embedLinkedStylesheet = this.embedLinkedStylesheetOverride;
}

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

return readAsset ? readAsset(path) : fs.promises.readFile(path, 'utf-8');
}

/**
* Override of the Critters `embedLinkedStylesheet` method
* that makes it work with Angular's CSP APIs.
*/
private embedLinkedStylesheetOverride: EmbedLinkedStylesheetFn = async (link, document) => {
const returnValue = await this.initialEmbedLinkedStylesheet(link, document);
const cspNonce = this.findCspNonce(document);

if (cspNonce) {
const crittersMedia = link.getAttribute('onload')?.match(MEDIA_SET_HANDLER_PATTERN);

if (crittersMedia) {
// If there's a Critters-generated `onload` handler and the file has an Angular CSP nonce,
// we have to remove the handler, because it's incompatible with CSP. We save the value
// in a different attribute and we generate a script tag with the nonce that uses
// `addEventListener` to apply the media query instead.
link.removeAttribute('onload');
link.setAttribute(CSP_MEDIA_ATTR, crittersMedia[1]);
this.conditionallyInsertCspLoadingScript(document, cspNonce);
}

// Ideally we would hook in at the time Critters inserts the `style` tags, but there isn't
// a way of doing that at the moment so we fall back to doing it any time a `link` tag is
// inserted. We mitigate it by only iterating the direct children of the `<head>` which
// should be pretty shallow.
document.head.children.forEach((child) => {
if (child.tagName === 'style' && !child.hasAttribute('nonce')) {
child.setAttribute('nonce', cspNonce);
}
});
}

return returnValue;
};

/**
* Finds the CSP nonce for a specific document.
*/
private findCspNonce(document: PartialDocument): string | null {
if (this.documentNonces.has(document)) {
return this.documentNonces.get(document) ?? null;
}

// HTML attribute are case-insensitive, but the parser used by Critters is case-sensitive.
const nonceElement = document.querySelector('[ngCspNonce], [ngcspnonce]');
const cspNonce =
nonceElement?.getAttribute('ngCspNonce') || nonceElement?.getAttribute('ngcspnonce') || null;

this.documentNonces.set(document, cspNonce);

return cspNonce;
}

/**
* Inserts the `script` tag that swaps the critical CSS at runtime,
* if one hasn't been inserted into the document already.
*/
private conditionallyInsertCspLoadingScript(document: PartialDocument, nonce: string) {
if (this.addedCspScriptsDocuments.has(document)) {
return;
}

const script = document.createElement('script');
script.setAttribute('nonce', nonce);
script.textContent = [
`(() => {`,
// Save the `children` in a variable since they're a live DOM node collection.
// We iterate over the direct descendants, instead of going through a `querySelectorAll`,
// because we know that the tags will be directly inside the `head`.
` const children = document.head.children;`,
// Declare `onLoad` outside the loop to avoid leaking memory.
// Can't be an arrow function, because we need `this` to refer to the DOM node.
` function onLoad() {this.media = this.getAttribute('${CSP_MEDIA_ATTR}');}`,
// Has to use a plain for loop, because some browsers don't support
// `forEach` on `children` which is a `HTMLCollection`.
` for (let i = 0; i < children.length; i++) {`,
` const child = children[i];`,
` child.hasAttribute('${CSP_MEDIA_ATTR}') && child.addEventListener('load', onLoad);`,
` }`,
`})();`,
].join('\n');
// Append the script to the head since it needs to
// run as early as possible, after the `link` tags.
document.head.appendChild(script);
this.addedCspScriptsDocuments.add(document);
}
}

export class InlineCriticalCssProcessor {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ describe('InlineCriticalCssProcessor', () => {
throw new Error('Cannot read asset.');
};

const getContent = (deployUrl: string): string => {
const getContent = (deployUrl: string, bodyContent = ''): string => {
return `
<html>
<head>
<link href="${deployUrl}styles.css" rel="stylesheet">
<link href="${deployUrl}theme.css" rel="stylesheet">
</head>
<body></body>
<body>${bodyContent}</body>
</html>`;
};

Expand Down Expand Up @@ -105,4 +105,32 @@ describe('InlineCriticalCssProcessor', () => {
);
expect(content).toContain('<style>body{margin:0}html{color:white}</style>');
});

it('should process the inline `onload` handlers if a CSP nonce is specified', async () => {
const inlineCssProcessor = new InlineCriticalCssProcessor({
readAsset,
});

const { content } = await inlineCssProcessor.process(
getContent('', '<app ngCspNonce="{% nonce %}"></app>'),
{
outputPath: '/dist/',
},
);

expect(content).toContain(
'<link href="styles.css" rel="stylesheet" media="print" ngCspMedia="all">',
);
expect(content).toContain(
'<link href="theme.css" rel="stylesheet" media="print" ngCspMedia="all">',
);
// Nonces shouldn't be added inside the `noscript` tags.
expect(content).toContain('<noscript><link rel="stylesheet" href="theme.css"></noscript>');
expect(content).toContain('<script nonce="{% nonce %}">');
expect(tags.stripIndents`${content}`).toContain(tags.stripIndents`
<style nonce="{% nonce %}">
body { margin: 0; }
html { color: white; }
</style>`);
});
});

0 comments on commit 79aad28

Please sign in to comment.