Tighten CSP around script-src and style-src
#6270
Conversation
Instead of allowing every src, require scripts from unknown sources to have have a nonce. This makes it harder to exploit potential XSS vulnverabilities as the attacker needs to somehow know the nonce beforehand. This also adds the nonce to all internal scripts. As per sidekiq#3913 (comment) the assets may be hosted outside of the main app. Adding the nonce continues to support this usecase. A nonce is incompatible with `unsafe-inline` (which only style has). Adapt the chart to not assign inline styles directly. Extensions that load external scripts/styles must either vendor or add the nonce to their tags. Closes sidekiq#6268
|
Looks great. I'll need to do some work on Pro/Ent to make them compliant so give me a week or two to get this merged. |
|
Yeah, of course. If you have questions regarding CSP for that, let me know. While I'm no expert on the matter I do have the misfortune of knowing it more closely. |
| "script-src 'self' https: http:", | ||
| "style-src 'self' https: http: 'unsafe-inline'", | ||
| "script-src 'self' 'nonce-!placeholder!'", | ||
| "style-src 'self' 'nonce-!placeholder!'", |
There was a problem hiding this comment.
I'm wondering if we can allow style-src 'unsafe-inline' until Sidekiq 8.0. Most people can extract JavaScript pretty easily but a lot of dynamic CSS isn't as easy to extract. Are you aware of any issues or trade offs here?
There was a problem hiding this comment.
It would be great to allow both nonce and unsafe-inline for 7.3 so extensions can migrate from inline to nonce smoothly.
There was a problem hiding this comment.
nonce and unsafe-inline are not compatible with each other, when both are specified only nonce is respected, at least on modern browsers.
It seems fine to just undo the policy changes for style-src to allow for a smoother upgrade path, script-src is what this PR was all about. Loading arbitrary css is not exactly what you'd want to do but loading arbitrary scripts even less so.
Let me know what you think, I can make a PR for that. Or just do that yourself, either is fine for me.
There was a problem hiding this comment.
Ok, we'll continue to implement nonce but need to allow unsafe-inline for styles for now. We can enable nonce and disable unsafe-inline in 8.0. Extensions can use 7.3 to implement nonce so they can remain compatible with 7.3 and 8.0. I don't want to require a hard break in compatibility between major versions if possible.
There was a problem hiding this comment.
You're welcome to send a PR for that if you're in a good place to do so.
There was a problem hiding this comment.
Yeah, sure. I will do so. I'm currently looking into something else but will probably have time to do that tomorrow
Followup to sidekiq#6270 (comment) A nonce doesn't work with `unsafe-inline`, so just go back to how it was previously. Keep this as a task for later instead
Instead of allowing every src, require scripts from unknown sources to have have a nonce. This makes it harder to exploit potential XSS vulnverabilities as the attacker needs to somehow know the nonce beforehand.
This also adds the nonce to all internal scripts. As per #3913 (comment) the assets may be hosted outside of the main app. Adding the nonce continues to support this usecase.
A nonce is incompatible with
unsafe-inline(which only style has). Adapt the chart to not assign inline styles directly.Extensions that load external scripts/styles must either vendor or add the nonce to their tags.
Closes #6268
Using nonces is the least disruptive change I believe. However, extensions that load resources from external sources (like from a CDN) will have the browser refuse to load them.
Similarly, because nonces are incompatible with
unsafe-inline, things like<span style="background-color: red;"></span>won't work anymore. You can look at the chart changes what this actually means since sidekiq itself requires a small change for this. It is very similar in scope to the changes about removingunsafe-inlinefromscript-srcwhich you did in the past.