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

crisbeto · angular-robot[bot] · commit 955b493b13e0 · 2023-03-28T14:45:40.000Z
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.
diff --git a/packages/angular_devkit/build_angular/src/builders/app-shell/app-shell_spec.ts b/packages/angular_devkit/build_angular/src/builders/app-shell/app-shell_spec.ts
@@ -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">/,
+    );
+  });
 });
diff --git a/packages/angular_devkit/build_angular/src/utils/index-file/inline-critical-css.ts b/packages/angular_devkit/build_angular/src/utils/index-file/inline-critical-css.ts
@@ -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;
 }
@@ -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 &
@@ -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 {
diff --git a/packages/angular_devkit/build_angular/src/utils/index-file/inline-critical-css_spec.ts b/packages/angular_devkit/build_angular/src/utils/index-file/inline-critical-css_spec.ts
@@ -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>`;
   };
 
@@ -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>`);
+  });
 });
