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(core): add API to provide CSP nonce for inline stylesheets #49444

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Mar 16, 2023

Angular uses inline styles to insert the styles associated with a component. This violates the strict styles Content Security Policy which doesn't allow inline styles by default. One way to allow the styles to be applied is to set a nonce attribute on them, but because the code for inserting the stylesheets is deep inside the framework, users weren't able to provide it without accessing private APIs.

These changes add a new CSP_NONCE injection token that will allow users to provide a nonce, if their app is using CSP. If the token isn't provided, the framework will look for an ngCspNonce attribute on the app's root node instead. The latter approach is provided as a convenience for apps that render the index.html through a server, e.g. <app ngCspNonce="{% randomNonceAddedByTheServer %}"></app>.

This PR addresses adding the nonce to framework-generated styles. There will be follow-up PRs that add support for it in critical CSS tags in the CLI, and in Angular Material.

Fixes #6361.

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Mar 16, 2023

constructor(@Inject(DOCUMENT) doc: any, @Inject(APP_ID) private appId: string) {
super();
this.head = doc.getElementsByTagName('head')[0];
this.head = doc.head || doc.getElementsByTagName('head')[0];
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated change, I just saw that we could get the head directly without querying the DOM. I left the old approach as a fallback in case it was there as a workaround for Domino.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: major This PR is targeted for the next major release labels Mar 16, 2023
@ngbot ngbot bot modified the milestone: Backlog Mar 16, 2023
@crisbeto crisbeto modified the milestones: Backlog, v16-candidates Mar 16, 2023
@crisbeto crisbeto force-pushed the 6361/csp-nonce branch 3 times, most recently from 5e3b71e to 92db19c Compare March 16, 2023 14:43
@@ -91,7 +91,8 @@ export class DomRendererFactory2 implements RendererFactory2, OnDestroy {
constructor(
private eventManager: EventManager, private sharedStylesHost: DomSharedStylesHost,
@Inject(APP_ID) private appId: string,
@Inject(REMOVE_STYLES_ON_COMPONENT_DESTROY) private removeStylesOnCompDestory: boolean) {
@Inject(REMOVE_STYLES_ON_COMPONENT_DESTROY) private removeStylesOnCompDestory: boolean,
@Inject(CSP_NONCE) @Optional() private nonce?: string|null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This uses @Inject instead of inject, because we had a bunch of tests that were constructing classes manually which lacked a DI context.

@crisbeto crisbeto marked this pull request as ready for review March 16, 2023 15:03
@@ -7,26 +7,35 @@
*/

import {DOCUMENT, ɵgetDOM as getDOM} from '@angular/common';
import {APP_ID, Inject, Injectable} from '@angular/core';
import {APP_ID, CSP_NONCE, Inject, Injectable, Optional} from '@angular/core';
import {ɵSharedStylesHost as SharedStylesHost} from '@angular/platform-browser';

@Injectable()
export class ServerStylesHost extends SharedStylesHost {
Copy link
Contributor

Choose a reason for hiding this comment

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

JFYI, the ServerStyleHost is being removed in #49424, so we'll need to update the location of this logic.

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

Aside from @AndrewKushnir's comment, this looks good to me

reviewed-for: fw-core, fw-platform-server, public-api

Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@crisbeto crisbeto removed the request for review from pkozlowski-opensource March 17, 2023 05:51
@crisbeto crisbeto 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 17, 2023
crisbeto added a commit to crisbeto/angular-cli that referenced this pull request Mar 17, 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).
@@ -149,6 +150,11 @@ export class SharedStylesHost implements OnDestroy {
private addStyleToHost(host: Node, style: string): void {
const styleEl = this.getStyleElement(host, style);

if (this.nonce) {
// Uses a keyed write to avoid issues with property minification.
Copy link
Contributor

Choose a reason for hiding this comment

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

OOC, does closure minify this property?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, but I wanted to keep it safe since I've seen it minify things like el.style.foo before.

Angular uses inline styles to insert the styles associated with a component. This violates the strict styles [Content Security Policy](https://web.dev/strict-csp/) which doesn't allow inline styles by default. One way to allow the styles to be applied is to set a `nonce` attribute on them, but because the code for inserting the stylesheets is deep inside the framework, users weren't able to provide it without accessing private APIs.

These changes add a new `CSP_NONCE` injection token that will allow users to provide a nonce, if their app is using CSP. If the token isn't provided, the framework will look for an `ngCspNonce` attribute on the app's root node instead. The latter approach is provided as a convenience for apps that render the `index.html` through a server, e.g. `<app ngCspNonce="{% randomNonceAddedByTheServer %}"></app>`.

This PR addresses adding the nonce to framework-generated styles. There will be follow-up PRs that add support for it in critical CSS tags in the CLI, and in Angular Material.

Fixes angular#6361.
@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit 17e9862.

crisbeto added a commit to crisbeto/angular-cli that referenced this pull request 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).
angular-robot bot pushed a commit to angular/angular-cli that referenced this pull request 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).
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Mar 30, 2023
Companion change to angular/angular#49444. That adds support to add nonce attributes on inline styles
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Mar 30, 2023
Companion change to angular/angular#49444. That adds support to add nonce attributes on inline styles
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Mar 30, 2023
Companion change to angular/angular#49444. That adds support to add nonce attributes on inline styles
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Mar 30, 2023
Companion change to angular/angular#49444. That adds support to add nonce attributes on inline styles
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Mar 31, 2023
Companion change to angular/angular#49444. That adds support to add nonce attributes on inline styles
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Mar 31, 2023
Companion change to angular/angular#49444. That adds support to add nonce attributes on inline styles
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Mar 31, 2023
Companion change to angular/angular#49444. That adds support to add nonce attributes on inline styles
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Mar 31, 2023
Companion change to angular/angular#49444. That adds support to add nonce attributes on inline styles
alan-agius4 added a commit to angular/universal that referenced this pull request Mar 31, 2023
Companion change to angular/angular#49444. That adds support to add nonce attributes on inline styles
@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 17, 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 area: core Issues related to the framework runtime 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.

Inline <style> elements violates style-src Content Security Policy
7 participants