mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
92.97k stars 32.02k forks source link

[Content Security Policy] Issue with inline-style #19938

Open lwilkins opened 4 years ago

lwilkins commented 4 years ago

According to the documentation, to support the CSP header, a nonce needs to be added to the style-src directive (https://mui.com/material-ui/guides/content-security-policy/). However, a number of Material-UI components make use of inline style attributes (e.g. Tabs - https://github.com/mui/material-ui/blob/5c96f3cad765c259d029321a8f7d2f57c9bcc36b/packages/mui-material/src/Tabs/Tabs.js#L768-L773). These require the 'unsafe-inline' source (or a hash for each style although this isn't practical). The 'unsafe-inline' source is ignored if a hash or nonce source is set, thus necessitating the removal of the nonce.

So generally for Material-UI as I see it, I think one of two things are needed:

  1. Update the CSP documentation for a setup to allow inline style attribute setting (not a great CSP, but would make Material-UIs stance clear on styling then).
  2. Set a standard of no inline style attribute setting and we fix them all as bugs.

What do you think? I did an issue search and found this (https://github.com/mui-org/material-ui/issues/13364) suggesting that Material-UI as a whole allows inline style attributes and thus are happy with necessitating a non strict CSP setting. Is this still the case and we're happy with what this means for CSP?

Related:

eps1lon commented 4 years ago

Generally I think we shouldn't use inline styles to begin with since they make customizing harder. So ideally we wouldn't use inline styles at all.

In the meantime we should add this notice to the docs.

oliviertassinari commented 4 years ago

This concern seems important for the use of the components in high demanding environments, e.g; #13364 coming from SnapChat Inc.? However, from our issue history, most developers seem to loosen the CSP for it.

We do have a couple of components that rely on inline style:

Wow, ok, actually, it's much more than I thought.

I think that it would be interesting to explore:

lwilkins commented 4 years ago

Thanks for the input @eps1lon and @oliviertassinari . I'll loosen the CSP in my application for now, but exploring the removal of all inline style attributes would be great and would be something I'd support.

razor-x commented 4 years ago

Just coming here as I was surprised to find out I still had CSP errors after following the docs: https://mui.com/material-ui/guides/content-security-policy/

It looks like the docs were never updated as suggested, so I opened a PR. Hopefully, someone has an updated list of the "non-csp" components #21479.

oliviertassinari commented 4 years ago

https://developers.google.com/web/fundamentals/security/csp/#inline_code_is_considered_harmful makes a case in favor of removing all inline style.

oliviertassinari commented 3 years ago

@razor-x It seems that we have lost your contribution (#21479) along the way in v5: https://mui.com/guides/content-security-policy/. There are no more mentions of inline-styles and what needs to be done to handle them. We will also likely need to update the guide for emotion, our new default style engine for v5.

razor-x commented 3 years ago

@oliviertassinari just happen to be checking in on the progress towards Material UI v5 and remembered about this update. Ideally in v5 CSP would be fully supported by all the hard work done on switching everything to emotion 🤞🏻 .

That said, I do believe my update to the docs in https://github.com/mui-org/material-ui/pull/21479 should have been pushed to the v4 docs and not bumped to v5. After all, my original goal with that update was to save time for any CSP users on v4 by letting them know of this edge case. I'm sure there will still be quite some time before all v4 users are moved to v5, so I'd like to get that back in if possible. Best.

tasola commented 2 years ago

Hi @oliviertassinari! Any news on the issue?

ghost commented 2 years ago

Multiline TextFields are still broken in Material UI v5.4.1 with a nonce-based CSP, unfortunately. Any plans on fixing this?

tiennguyen1293 commented 2 years ago

we have the same issue on cssinjs here. we have any updates here? 👍

toastal commented 2 years ago

Bigger picture 2¢: Why wasn't CSS-in-CSS the default option? If CSS-in-JS wasn't a trend, we wouldn't see these issues popping up that require non-trivial workarounds. From my fresh-to-MUI viewpoint, this feels like it's bordering on bug not feature territory.

oliviertassinari commented 2 years ago

@toastal The issue we track here is the use of inline-style. For example, if we consider:

https://github.com/mui/material-ui/blob/3aa59189e566baa225a89f962214441f02787abc/packages/mui-base/src/TextareaAutosize/TextareaAutosize.js#L171-L177

How do we replace this to be compatible without style-src: 'unsafe-inline' https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src#unsafe_inline_styles?

CSS-in-CSS vs. CSS-in-JS is not really a variable on the problem faced, at best CSS-in-JS could be a solution because it allows dynamic values.

There are some discussions about this root issue in the standard working group, e.g. https://github.com/w3c/webappsec-csp/issues/45 but I couldn't find really anything concrete to help.

The option to use CSS-in-JS in the above example would mean that we should add to the unstyled component a dependency on a styling library, e.g. emotion, IMHO, it doesn't fly.

https://github.com/rrweb-io/rrweb/issues/816#issuecomment-1051065366 is encouraging. It suggests that approved JS scripts can use the CSSOM API so some of this list https://github.com/mui/material-ui/issues/19938#issuecomment-593618428 could be solved. If anybody has the bandwidth, feel free to dive deeper.

I suspect that there are components that we won't be able to migrate, hence it would be great to document this problem in https://mui.com/guides/content-security-policy/.

ramosbugs commented 2 years ago

How do we replace this to be compatible without style-src: 'unsafe-inline' https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src#unsafe_inline_styles?

CSS-in-CSS vs. CSS-in-JS is not really a variable on the problem faced, at best CSS-in-JS could be a solution because it allows dynamic values.

I wonder if this library can take a similar approach to what Next.JS does to solve this issue: hoist the dynamic CSS into a <style> block with a nonce that the CSP can reference: https://github.com/vercel/next.js/issues/18557 (implemented in https://github.com/vercel/next.js/pull/19150). Then, none of the CSS is actually rendered inline, but the developer experience is pretty similar (vs. static CSS-in-CSS).

Note that this approach shifts some security responsibility to the library that's inserting the dynamic CSS into the nonce-having <style> block. If the library allows untrusted user input to make it into the <style> block, then it's vulnerable to the same issues as style-src 'unsafe-inline'.

It's probably going to require a fair amount of work to migrate all of the current inline styles into hoisted dynamic <style> styles, but I'd strongly suggest moving in that direction since attacker-manipulated inline styles can lead to exfiltration of private user data: https://scarybeastsecurity.blogspot.com/2009/12/generic-cross-browser-cross-domain.html. Having a CSP without style-src 'unsafe-inline' significantly reduces the portion of a site's code that can insert an attacker's malicious CSS into the page, since only the code that directly maintains the <style> blocks needs to be trusted. Without CSP, or with style-src 'unsafe-line', essentially any buggy code on the page could provide an attack vector for inserting untrusted CSS into the page.

toastal commented 2 years ago

[TextareaAutosize.js] example; “dynamic values”

This could easily be a utility class that is pushed/popped instead of needing to edit the CSS “dynamically” (e.g. .MUITextarea.shouldHideOverflow { overflow: hidden }) (or if CSS variables where on the allowlist for CSP). JavaScript is allowed to dynamically add/remove items from the classList without a CSP concern so why is this not the first solution option? I don’t see what CSS-in-JS gives you other than slower processing speeds and extra complexity.

<style> block with a nonce

This is in the category of “non-trivial workarounds”… Needing to add complicated steps to the build pipeline is adding more complexity to the issue. This is a band-aid for what seems like a poor architecture decision. Users should not be required to whip out heavy guns like webpack to calculate nonces when there are simpler alternatives (see above).


I’m open to being wrong, but I am really not ‘getting it’ when it comes to CSS-in-JS. Currently in my multiple consulting gigs I’m suggesting clients migrate away from anything using CSS-in-JS now and has anecdotally appeared to have simplified the setup and tooling after a little CSS guidance.

zhdanouskikh commented 2 years ago

Any updates on this? Is there any date when mui won't have inline-styles or put them into