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.

Sign up for GitHub

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

Jump to bottom

CSP docs are a little unclear - can I help improve them? #17566

Closed
johnnyreilly opened this issue Sep 25, 2019 · 6 comments · Fixed by #17594
Closed

CSP docs are a little unclear - can I help improve them? #17566

johnnyreilly opened this issue Sep 25, 2019 · 6 comments · Fixed by #17594
Labels
docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.

Comments

@johnnyreilly
Copy link
Contributor

johnnyreilly commented Sep 25, 2019 •
edited
Loading

First of all, thank you for Material UI - I love it; it's super cool!

I've been looking into how to support CSP with Material UI; specifically using a style-src. I've been reading the docs here: https://material-ui.com/styles/advanced/#content-security-policy-csp

They start off great but I must confess to getting a little lost by the end. Having spent a bunch of time googling and seeing others who also seem to be a little confused.

See:

I thought I'd see if I could start a discussion with the desire of discovering the answer and hopefully using that to subsequently improving the docs 🤗

The part where the docs start to get a little mysterious is here:

Then, you must pass this nonce to JSS so it can add it to subsequent <style> tags. The client-side gets the nonce from a header. You must include this header regardless of whether or not SSR is used.

<meta property="csp-nonce" content={nonce} />

This is not particularly clear to me; the "you must pass this nonce to JSS". How exactly does this happen? Is the idea that our page.html should include this meta property in the head? Like this:

<head>
<meta property="csp-nonce" content="123354-i-am-a-nonce" />
</head>

And that JSS looks for a csp-nonce meta property by default?

i.e. There's no need to write a line of code that says "hey JSS, this is the nonce you need", rather that JSS will by convention look for the csp-nonce meta property in your html>head when it boots and use the value as a nonce if it is found.

Or am I totally barking up the wrong tree? Totally possible!

It's worth saying that the JSS docs are similarly somewhat ambiguous: https://cssinjs.org/csp/?v=v10.0.0

Finally I'm wondering whether it's worth mentioning approaches around hashing in the docs:

https://github.com/slackhq/csp-html-webpack-plugin

I don't know enough around this topic to say though.
@oliviertassinari oliviertassinari added the package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. label Sep 25, 2019
@oliviertassinari
Copy link
Member

@johnnyreilly Thank you for raising this concern. Security is too often overlooked. To be honest, the last time I have faced this problem, I ended up using the following:

import helmet from 'helmet'

// ...

const app = express()

app.use(
  helmet({
    // TODO Should we restrict the policy ?
    // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy
    contentSecurityPolicy: {
      directives: {
        defaultSrc: ['*', 'blob:'], // Blob for upload support
        styleSrc: ['*', "'unsafe-inline'"],
        scriptSrc: ['*', "'unsafe-inline'", "'unsafe-eval'"],
        frameAncestors: ["'none'"],
        reportUri: config.get('sentry.reportCSP'),
      },
      // Don't support old version and help with CDN
      browserSniff: false,
      disableAndroid: true,
    },
    frameguard: {
      action: 'deny', // Disallow embedded iframe
    },
  })
)

And that JSS looks for a csp-nonce meta property by default?

Yes, you are right, it's the message we have tried to communicate. Do you think that we could improve the wording?

I have seen a related issue in styled-components/styled-components#2363. It doesn't seem to be a concern a lot of people handle/try to handle.
As far as I understand the concern, the only way to make this policy effective is to have a random dynamic nonce value. It needs to change frequently.

Regarding our documentation, we don't do anything about it: https://securityheaders.com/?q=https%3A%2F%2Fmaterial-ui.com%2F&followRedirects=on.

@johnnyreilly
Copy link
Contributor Author

Yes, you are right, it's the message we have tried to communicate. Do you think that we could improve the wording?

Awesome - that's good. Yes I think the wording could be made more explicit that that's the case. For instance, as I read the original I was wondering if:

  1. JSS was going to read the nonce from a response header or a meta tag in the head of the HTML. It turns out it's the latter but I wasn't sure.
  2. It wasn't clear to me that JSS was looking for this particular header; that it was a default behaviour of JSS.
  3. That the example read <meta property="csp-nonce" content={nonce} /> rather than an example of what a meta tag might look like confused me a little. I'm a simple man 😄

Perhaps the wording could be expanded to something like:

Then, you must pass this nonce to JSS so it can add it to subsequent <style> tags.

The way that you do that is by passing a <meta property="csp-nonce" content={nonce} /> tag in the <head> of your HTML. JSS then will, by convention, look for a <meta property="csp-nonce" tag and use the content value as the nonce.

You must include this header regardless of whether or not SSR is used.

<head>
<!-- ... -->
<meta property="csp-nonce" content="this-is-a-nonce-123" />
<!-- ... -->
</head>

What do you think?

@oliviertassinari
Copy link
Member

I think that the proposal will help 👍. Do you want to open à pull request? :)

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Sep 25, 2019
@johnnyreilly
Copy link
Contributor Author

Yeah I'll try to. Crazy busy right now though - ping me if I forget

@johnnyreilly johnnyreilly mentioned this issue Sep 27, 2019
Merged
1 task
@oliviertassinari oliviertassinari mentioned this issue Oct 21, 2019
Closed
@jessewoo
Copy link

jessewoo commented Sep 2, 2021

hi @johnnyreilly - I'm developing with a standard stack with Create React App (CRA) with Material UI (MUI) running with CSP that the security team wanted to put in place, but it's failing with 'style-src' with inline css because MUI has components with inline styling. Wondering if that's something that you ran into. I've been looking around for solution but I too am feeling lost.

@oliviertassinari
Copy link
Member

There is still an open discussion about it in #19938.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[docs] Improve CSP nonce docs
3 participants
@johnnyreilly @oliviertassinari @jessewoo