react-remove-scroll icon indicating copy to clipboard operation
react-remove-scroll copied to clipboard

Violates safe CSPs because of dependency applying style tags

Open igboyes opened this issue 6 years ago • 28 comments

I originally submitted this issue at https://github.com/reach/reach-ui/issues/469, which uses your library as a dependency in their Dialog component.

react-remove-scroll has a dependency called react-style-singleton, of which you are also the author.

My understanding is react-style-singleton creates and modifies inline style tags after application mount. This is somewhat unsafe and if you are using a Content Security Policy (CSP), I don't think there is any way for this library to work as expected without allowing style-src 'unsafe-inline'.

Here is the trace from my CSP error: image

I may be completely missing a secure way to handle this problem.

Thoughts?

Cheers.

igboyes avatar Feb 13 '20 21:02 igboyes

style-src unsafe-inline right now is the default way to handle "CSS-in-JS", another way is to use __webpack-nonce__, like styled-components do. However - that's not going to work with parcel.

Solution - use webpack-nonce for now. That would solve the problem for the majority of users.

theKashey avatar Feb 13 '20 22:02 theKashey

Thanks for the reply and info. Sorry. You are right, unsafe-inline or no CSP is the default.

I am using Webpack and __webpack_nonce__, which is successfully consumed by styled-components. This is not working for react-style-singleton in my case.

Any ideas?

igboyes avatar Feb 13 '20 23:02 igboyes

Because react-style-singleton does not "consume" __webpack-nonce__, you were the first to hit this problem.

theKashey avatar Feb 14 '20 04:02 theKashey

Ok. So the react-style-singleton got updated. The question now is how to update all consumers. Probably asking to find that package in the lock file and update is not that good.

theKashey avatar Apr 16 '20 07:04 theKashey

Because react-style-singleton does not "consume" __webpack-nonce__, you were the first to hit this problem.

I guess I'm the second to hit this problem :(

TheThirdRace avatar Mar 14 '21 20:03 TheThirdRace

Isn't there anyway to pass a nonce?

TheThirdRace avatar Mar 14 '21 20:03 TheThirdRace

  • assigned here - https://github.com/theKashey/react-style-singleton/blob/5972081a19756bc4a578ac8bd87b5169d5c7ccdf/src/singleton.ts#L11
  • can be overridden here https://github.com/theKashey/get-nonce/blob/master/src/index.ts#L3 via API or dependency mocking

theKashey avatar Mar 15 '21 04:03 theKashey

@theKashey I've done some more research on my side and I've hit a snag with the nonce solution.

SSG

I use NextJs with SSG (Static Site Generation), which means there's actually no server generating the page on each request. NextJs doesn't offer anything that can generate a nonce every request with SSG. I guess that's a problem with all SSG frameworks.

Examples found on Google are very sparse and very barebone. After a few hours experimenting, I just can't make it work. It looks like if your page isn't Server-Side generated, you mostly can't use nonce.

I found 2 ways of making it work, but both are very problematic.

First, I could use a fixed nonce that doesn't refresh every request. This is "security through obscurity" at this point, so not really useful.

Second, I could use a hash of the <style> tag instead of a nonce. Unfortunately, the styles changes according to browsers so the hash method is not really scalable given I can't predict all the possible combinations.

Alternative

During my research, I found something interesting though. It seems the CSP applies to HTML attributes, but not to the DOM properties:

  • https://stackoverflow.com/a/57633457/5915000
  • https://stackoverflow.com/a/29089970/5915000

Basically, if you try to set the style attribute on an element or insert a <style> tag, you're gonna get blocked by the CSP. But if you set the DOM properties directly, you're fine.

// Here's an example of how to set a DOM property
element.style.backgroundColor = "blue"

Given the nature of this plugin, it might not be a huge problem to switch to this method instead of using a <style> tag. It would require a bit of work, but it would also remove the need for the nonce altogether and allow any site, even SSG sites, to work without any fuss or configuration.

I thought I'd bring it up to you in case you'd want to experiment with this solution.

TheThirdRace avatar Mar 19 '21 17:03 TheThirdRace

That is a very interesting solution and I don't see any reason why it can't be implemented. The underlaying react-remove-scroll-bar is doing many things - https://github.com/theKashey/react-remove-scroll-bar/blob/master/src/component.tsx#L17 - but actually it just needs to mend body only style, and the functionality for "other exposed class names" is already duplication by cssVariable, which works way better and gives more control.

Proposal:

  • release a v2 for react-remove-scroll-bar, with all code made imperative, removing all unnecessary functionality
  • use version resolution to replace v1 by v2 and test how it's working for some particular use case.
  • update this package and reach-ui. The latter should be safe, as "extra functionality" is not used (while it should).

theKashey avatar Mar 19 '21 23:03 theKashey

Looks good to me.

Once you get a v2 going, I could also test it out on my side to check if everything goes smoothly with my CSP.

TheThirdRace avatar Mar 21 '21 15:03 TheThirdRace

This issue has been marked as "stale" because there has been no activity for 2 months. If you have any new information or would like to continue the discussion, please feel free to do so. If this issue got buried among other tasks, maybe this message will reignite the conversation. Otherwise, this issue will be closed in 7 days. Thank you for your contributions so far.

stale[bot] avatar Apr 30 '23 07:04 stale[bot]