theKashey / react-remove-scroll

Removes and disables 📜in a "React" way
MIT License
804 stars 31 forks source link

Violates safe CSPs because of dependency applying style tags #21

Closed igboyes closed 1 year ago

igboyes commented 4 years ago

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.

theKashey commented 4 years ago

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.

igboyes commented 4 years ago

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?

theKashey commented 4 years ago

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

theKashey commented 4 years ago

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.

TheThirdRace commented 3 years ago

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 commented 3 years ago

Isn't there anyway to pass a nonce?

theKashey commented 3 years ago
TheThirdRace commented 3 years ago

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

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.

theKashey commented 3 years ago

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:

TheThirdRace commented 3 years ago

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.

stale[bot] commented 1 year ago

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.

devnull commented 3 months ago

Was this v2 ever sorted? I'm hitting this issue and it's quite problematic.

TheThirdRace commented 3 months ago

Was this v2 ever sorted? I'm hitting this issue and it's quite problematic.

I never checked back since, but there seem to be a mention in the 2.5.5 release about fixing the CSP (https://github.com/theKashey/react-remove-scroll/releases/tag/v2.5.5)

theKashey commented 3 months ago

It was "supported" via nonce tag. Still creating runtime styles - https://github.com/theKashey/react-style-singleton/commit/560cc190d219a7c16f5557f11b508c01ecd67ccd