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

[crisbeto]

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.

[alan-agius4]

As discussed previously in the previous PR, IMHO, this is not the write place to do such changes. I was fine in doing the nonce as part of the index plugin to handle all the style tags, but in this case the link[onload] is specific to critical CSS Inlining which is used also in app-shell, and universal where the index html augmenter is not run as such currently nonce attribute is not added.

The correct place to do this would be the ExtendedCritters, until it's properly support by Critters proper, which is highly unlikely to happen any time soon. Doing the logic in ExtendedCritters would cover all the use-cases and can easily be ported over to the universal repo which does not have support for the nonce attribute yet. Adding this logic here only solves 1 out of the 4 use-cases (CSR, SSR, SSG and App-Shell). We cannot ignore the other use-cases and just focus on one.

To fix this across the board is simpler than you might think, as in ExtendedCritters you can access the document so there is not need to re-parse the HTML. What needs to be done is to override the embedLinkedStylesheet method https://github.com/GoogleChromeLabs/critters/blob/a590c05f9197b656d2aeaae9369df2483c26b072/packages/critters/src/index.js#L311, where the document can be queried to get the nonce value, add it to the style element and inject the link tag and onload handler script.

Eventually It would also be great if you can add some nonce tests in https://github.com/angular/angular-cli/blob/main/packages/angular_devkit/build_angular/src/builders/app-shell/app-shell_spec.ts

Happy to help if needed.

[crisbeto]

@alan-agius4 I've moved the logic into the CrittersExtended, but I had to do a hacky workaround to override embedLinkedStylesheet since it isn't part of the Critters typings.

[alan-agius4]

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);
  }

[crisbeto]

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

[alan-agius4]

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

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

Line 145 in 0ac5f27

it('critical CSS is inlined', async () => {

[crisbeto]

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.

[crisbeto]

The feedback should be addressed now @alan-agius4.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}