Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(@angular-devkit/build-angular): support CSP on critical CSS link tags #24903

Merged
merged 1 commit into from
Mar 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not seeing any handling of adding nonce to the processed stylesheet by critters. This is needed for scenarios where the index augmenter is not run.

Since we need to have access to the style element that was added (In a performant way), I think that we need to port some of the logic into this method example;

  async embedLinkedStylesheet(link, document) {
    if (this.cspNonceValue === undefined) {
      // Avoid querying the body for every stylesheet.
      // This would cause unnecessary processing on pages with a lot of DOM nodes.
      const nonceElement = document.querySelector('[ngCspNonce], [ngcspnonce]');
      const cspNonce = nonceElement?.getAttribute('ngCspNonce') || nonceElement?.getAttribute('ngcspnonce');
      this.cspNonceValue = cspNonce || null
    }

    // We can actually have the same logic for both CSP and non-CSP cases.
    // IE: to always load the stylesheets using the custom script as this would also increase test coverage of for the logic.
        if (!this.cspNonceValue) {
      this.originalEmbedLinkedStylesheet(link, document);
      return;
    }

    const href = link.getAttribute('href');
    const media = link.getAttribute('media');
    // skip filtered resources, or network resources if no filter is provided
    if (!(href || '').match(/\.css$/)) {
      return undefined;
    }

    // the reduced critical CSS gets injected into a new <style> tag
    const sheet = await this.getCssAsset(href);
    if (!sheet) {
      return;
    }

    const style = document.createElement('style');
    style.setAttribute('nonce', this.cspNonceValue);
    style.$$external = true;
    style.textContent = sheet;
    style.$$name = href;
    style.$$links = [link];
    link.parentNode.insertBefore(style, link);

    // @see https://github.com/filamentgroup/loadCSS/blob/af1106cfe0bf70147e22185afa7ead96c01dec48/src/loadCSS.js#L26
    link.setAttribute('rel', 'stylesheet');
    link.removeAttribute('as');
    link.setAttribute('media', 'print');
    link.setAttribute(CSP_MEDIA_ATTR,  media || 'all');


    // No script fallback
    const noscript = document.createElement('noscript');
    const noscriptLink = document.createElement('link');
    noscriptLink.setAttribute('rel', 'stylesheet');
    noscriptLink.setAttribute('href', href);

    if (media) {
      noscriptLink.setAttribute('media', media);
    }

    noscript.appendChild(noscriptLink);
    link.parentNode.insertBefore(noscript, link.nextSibling);
    style.$$links.push(noscript);

    this.conditionallyInsertCspLoadingScript(document, cspNonce);
  }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what case does the style tag get produced then? I didn't see any other unit tests checking for the style tags.

Copy link
Collaborator

@alan-agius4 alan-agius4 Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style tag get produced by critters in embedLinkedStylesheet outside of the index augmentation phase in app-shell builder, this means that nonce will not be added to style tags unless critters is modified to include the nonce attribute to style tags.

Add an integration test in

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, although I ended up just iterating over all <style> tags in the document.head. This seemed like a lot of logic to duplicate from Critters.

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 () => {
crisbeto marked this conversation as resolved.
Show resolved Hide resolved
const inlineCssProcessor = new InlineCriticalCssProcessor({
readAsset,
});

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

expect(content).toContain(
crisbeto marked this conversation as resolved.
Show resolved Hide resolved
'<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>`);
});
});