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

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

Open
lwilkins opened this issue Mar 2, 2020 · 28 comments
Open

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

lwilkins opened this issue Mar 2, 2020 · 28 comments
Labels
core Infrastructure work going on behind the scenes discussion

Comments

@lwilkins
Copy link

lwilkins commented Mar 2, 2020

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 -

style={{
overflow: scrollerStyle.overflow,
[vertical ? `margin${isRtl ? 'Left' : 'Right'}` : 'marginBottom']: visibleScrollbar
? undefined
: -scrollerStyle.scrollbarWidth,
}}
). 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 (#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:

@mbrookes mbrookes added the docs Improvements or additions to the documentation label Mar 2, 2020
@eps1lon
Copy link
Member

eps1lon commented Mar 2, 2020

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
Copy link
Member

oliviertassinari commented Mar 2, 2020

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:

  • SpeedDial
  • Skeleton
  • AvatarGroup
  • Autocomplete
  • TextareaAutosize
  • Tabs
  • SwipeableDrawer
  • Slider
  • Modal
  • LinearProgress
  • CircularProgress
  • GridList
  • Collapse
  • Fade
  • Grow
  • Zoom
  • CardMedia
  • ButtonBase

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

I think that it would be interesting to explore:

  • What's the alternative? (cost)
  • How much better is it without inline-style? Who actually needs it?

@lwilkins
Copy link
Author

lwilkins commented Mar 3, 2020

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
Copy link
Contributor

razor-x commented Jun 17, 2020

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
Copy link
Member

oliviertassinari commented Jun 18, 2020

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

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 23, 2020

@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
Copy link
Contributor

razor-x commented Jun 29, 2021

@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 #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

This comment was marked as spam.

@ghost
Copy link

ghost commented Feb 15, 2022

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

@tiennguyen1293

This comment was marked as off-topic.

@toastal
Copy link

toastal commented Mar 1, 2022

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 oliviertassinari changed the title Content Security Policy support clarification CSP issue with inline-style Mar 26, 2022
@oliviertassinari
Copy link
Member

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

style={{
height: state.outerHeightStyle,
// Need a large enough difference to allow scrolling.
// This prevents infinite rendering loop.
overflow: state.overflow ? 'hidden' : null,
...style,
}}

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. w3c/webappsec-csp#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.

rrweb-io/rrweb#816 (comment) is encouraging. It suggests that approved JS scripts can use the CSSOM API so some of this list #19938 (comment) 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
Copy link

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: vercel/next.js#18557 (implemented in vercel/next.js#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

This comment was marked as resolved.

@zhdanouskikh

This comment was marked as resolved.

@indefinitelee
Copy link

also interested in knowing if there are any updates to this.
is there a list of components that have/have not been migrated?

@zhdanouskikh

This comment was marked as off-topic.

@iMrDJAi
Copy link

iMrDJAi commented Nov 29, 2022

@oliviertassinari Not sure if the issue is related to my setup or not, but for some components that rely on inline style such as textField, the style attributes are missing on SSR. I'm facing a style mismatch between SSR and CSR after hydration.

Before hydration:
Screenshot 2022-11-29 104007

After hydration:
Screenshot 2022-11-29 104132

Maybe they are being added at a later stage after rendering is done?

@wmertens
Copy link

wmertens commented Feb 4, 2023

I think some dynamic styling is unavoidable, in the case of images. When you want to provide a pleasant loading experience you need to tell the browser what the aspect ratio is, and if you have a lot of images that means a lot of possible aspect ratios.

Currently I pass the ratio as a CSS variable in a style attribute on the wrapping component. How to solve this, especially when you have a SPA that needs to dynamically insert images?

@RanVaknin
Copy link

Any updates on this? We are also blocked from using material UI for our project because of strict CSP policies mainly unsafe-inline.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 20, 2023

I created a reproduction with TextareaAutosize: https://codesandbox.io/p/sandbox/throbbing-browser-rkms3j?file=/next.config.js:33,18.

Screenshot 2023-08-21 at 13 14 54 Screenshot 2023-08-21 at 13 15 01

As for solutions,

  1. CSS variables
import * as React from "react";

export default function UnstyledTextareaIntroduction() {
  return (
    <div
      style={{
        "--foo": "bar",
      }}
    >
      Hello
    </div>
  );
}

doesn't work, not unless w3c/webappsec-csp#174 makes progress.

  1. ref.current.style access. This seems to work, it's explained in Policy to allow only custom properties in inline CSS w3c/webappsec-csp#174 (comment). It was applied in fix: remove inline styles carbon-design-system/carbon#12784.
  2. When SSR is required, <style> with nonce attributes, e.g. https://github.com/shuding/react-wrap-balancer/blob/eb363827ef806119caaa5ffd3e4135bde1f43811/src/index.tsx#L113

@oliviertassinari oliviertassinari changed the title CSP issue with inline-style [Content Security Poilicy] Issue with inline-style Aug 21, 2023
@samuelsycamore samuelsycamore changed the title [Content Security Poilicy] Issue with inline-style [Content Security Policy] Issue with inline-style Nov 24, 2023
@samuelsycamore samuelsycamore removed the docs Improvements or additions to the documentation label Nov 28, 2023
@sneko
Copy link

sneko commented Mar 11, 2024

The documentation should be more clear. In my case Accordion was also doing inline style breaking the CSP.

My workaround is to use style-src-attr 'unsafe-inline' instead of doing it on style-src which would break in case of already using a nonce for third-party files. It's not perfect, but the attack vector seems minimal to me like that.

@frattaro
Copy link

not sure why this is an issue when emotion is available. Isn't the OP code trivially

 css={css`
   overflow: ${scrollerStyle.overflow};
   ${!visibleScrollbar
     ? (vertical
       ? `margin-${isRtl ? "left" : "right"}`
       : 'margin-bottom') + `: ${-scrollerStyle.scrollbarWidth};`
     : "" }
 `} 

?

@yesudeep
Copy link

yesudeep commented Jul 17, 2024

Can we please remove inline styles entirely? What prevents us from using CSS modules or plain old classes? That would simplify CSPs and allow Web applications to be less lenient with security. If we add a nonce to style-src, the unsafe-inline keyword doesn't appear to work, so this is currently a blocker for us.

@iMrDJAi
Copy link

iMrDJAi commented Jul 17, 2024

Agree, inline styles break SSR as well.

@yesudeep
Copy link

We've dropped MUI in favor of https://github.com/rmwc/rmwc

@oliviertassinari oliviertassinari changed the title [Content Security Policy] Issue with inline-style [CSP] Issue with inline-style and Content Security Policy Sep 11, 2024
@saislam10
Copy link

We've dropped MUI in favor of https://github.com/rmwc/rmwc

Does rmwc allow to pass CSP without 'unsafe-inline'?

@yesudeep
Copy link

yesudeep commented Oct 2, 2024

@saislam10 we extract all static inline styles known at build time. There doesn't appear to be a runtime component so far, so I'd be inclined to say yes.

bhavberi added a commit to Clubs-Council-IIITH/web that referenced this issue Oct 4, 2024
Dileepadari added a commit to Clubs-Council-IIITH/web that referenced this issue Oct 9, 2024
* Content-Security-Policy: add strict csp headers and use a nonce

* Content-Security-Policy: add iiit main website in iframe whitelist

* Fixed some icons are not loading

* Allow icons to load from different sources

* Minor Variable name change for better understanding

* Redirect to the correct url between clubs and student-bodies

* Simplify Gallery Images Bugger View, and added loading for images

Helps in Google Indexing, as otherwise it treats /gallery and /gallery?img=index as different pages

* Reduce showImage function

* placeholder animation added for the images.

* Minor change

* Apply Prettier Formatting Fixes

* handle merge conflicts: when rebasing the scp

* Enable styled components

* Use unsafe-inline with styles (mui/material-ui#19938)

* csp: add unsafe to specific style

---------

Co-authored-by: Bhav Beri <43399374+bhavberi@users.noreply.github.com>
Co-authored-by: Bhav Beri <bhavberi@gmail.com>
Co-authored-by: dileepadari <adaridileep@gmail.com>
Co-authored-by: bhavberi <bhavberi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes discussion
Projects
None yet
Development

No branches or pull requests