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

feat(@angular-devkit/build-angular): add CSP support for inline styles #24880

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Mar 19, 2023

Companion change to angular/angular#49444. Adds an HTML processor that finds the ngCspNonce attribute and copies its value to any inline style tags in the HTML. The processor runs late in the processing pipeline in order to pick up any style tag that might've been added by other processors (e.g. critical CSS).

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Mar 19, 2023
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Mar 19, 2023
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

I am not certain that creating a new plugin is the right direction.

The plugin addresses only one use case (CSR), but it does not add nonce to the app-shell, which uses extended Critters directly. See:

if (inlineCriticalCssProcessor) {
const { content, warnings, errors } = await inlineCriticalCssProcessor.process(html, {
outputPath,
});
html = content;
if (warnings.length || errors.length) {
spinner.stop();
warnings.forEach((m) => context.logger.warn(m));
errors.forEach((m) => context.logger.error(m));
spinner.start();
}
}
Extending this class and doing the logic in here IMHO would be better as we’ll share the same logic and it would be easier to port this change to Universal https://github.com/angular/universal/blob/main/modules/common/tools/src/inline-css-processor.ts.

I was under the impression, that we were going to add nonce support in Critters, as adding nonce to the style elements is also not sufficient to make the page CSP complaint, crtitters adds onload events to link elements, as such we will need to modify critters to output this is a CSP complaint way.

Currently critters will change

<link rel="stylesheet" href="styles.css”>

to

<link rel="stylesheet" href="styles.css" media="print" onload="this.media='all'">

Now, we need to amend the above and make it CSP complaint when nonce is provided (Or we can do the same logic irrespective if CSP is enabled.)

<link rel="stylesheet" href=“styles.css” ngAsyncStyle>
<script nonce=“..">
  document.querySelectorAll('link[ngAsyncStyle]’)
      .forEach(s => s.addEventListener(‘load’, () => s.media = ‘all'));
</script>

All the above, would be complex to do with this sort of plugin created, but should be more doable using by extended Critters.

Copy link
Member Author

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

The change I was planning to make to Critters was to allow for custom attributes to be specified and we can achieve the same without touching Critters at all using the changes in this PR. I can bake this logic into the Critters plugin, but I think that it makes sense to apply it to all inline style tags, not just the ones generated by Critters.

I think that the link use case would require a much larger change to Critters so for now I would rather focus only on the style tags.

@alan-agius4
Copy link
Collaborator

I can bake this logic into the Critters plugin, but I think that it makes sense to apply it to all inline style tags, not just the ones generated by Critters.

Fair enough.

@alan-agius4 alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 20, 2023
Companion change to angular/angular#49444. Adds an HTML processor that finds the `ngCspNonce` attribute and copies its value to any inline `style` tags in the HTML. The processor runs late in the processing pipeline in order to pick up any `style` tag that might've been added by other processors (e.g. critical CSS).
@crisbeto crisbeto removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 20, 2023
@crisbeto
Copy link
Member Author

Comments have been addressed.

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 20, 2023
@angular-robot angular-robot bot merged commit ff5ebf9 into angular:main Mar 20, 2023
crisbeto added a commit to crisbeto/angular-cli that referenced this pull request Mar 24, 2023
…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.
crisbeto added a commit to crisbeto/angular-cli that referenced this pull request Mar 24, 2023
…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.
crisbeto added a commit to crisbeto/angular-cli that referenced this pull request Mar 28, 2023
…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.
angular-robot bot pushed a commit that referenced this pull request Mar 28, 2023
…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.
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker detected: feature PR contains a feature commit target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants