WT-536: improve our CSP unsafe-inline config by making it tighter and more specific#16994
WT-536: improve our CSP unsafe-inline config by making it tighter and more specific#16994stevejalim wants to merge 3 commits intomainfrom
Conversation
…in for CMS deployment
…nd is enabled Note that the previous cut of the code didn't add style-src: unsafe inline only in Transcend mode: it was enabled all the time
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16994 +/- ##
==========================================
+ Coverage 80.36% 81.42% +1.06%
==========================================
Files 163 162 -1
Lines 9100 8474 -626
==========================================
- Hits 7313 6900 -413
+ Misses 1787 1574 -213 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I think this might make sentry noise again, unless transcend was reconfigured in the meantime:
(I still believe transcend's/airgap's defaults should not necessitate any unsafe inline exclusions; maybe it's just not configured to what bedrock sets for them — as they also have functionality to be managing and setting CSP on their own — perhaps they think they're setting it?…)
| @@ -109,12 +109,12 @@ | |||
| "www.googletagmanager.com", | |||
| "www.youtube.com", | |||
| csp.constants.UNSAFE_EVAL, | |||
There was a problem hiding this comment.
[unrelated] Wondering whether to be adding UNSAFE_EVAL only if DEV? (Is there a risk of missing any legit use when this would always be allowed in dev, incl. demos, but never in prod/stage?)
| CSP_ASSETS_HOST, | ||
| csp.constants.UNSAFE_INLINE, | ||
| "cdn.transcend.io", # Transcend Consent Management | ||
| "transcend-cdn.com", # Transcend Consent Management |
There was a problem hiding this comment.
So, it was added for transcend. I still believe that has to be some misconfiguration, but… I don't see it added anywhere later for transcend — only for wagtail admin. (yay! but…)
There was a problem hiding this comment.
… but after reading the description again and looking around the lines yes transcend's already covered there:
bedrock/bedrock/settings/__init__.py
Lines 122 to 124 in 0c299c8
right 🤦♂️ — so airgap shouldn't break. And it's well before the RO deepcopy so it'll also silence any RO noise.
So what this PR does is, where UNSAFE_INLINE was removed only from RO before, then recently refactored out even from there, this actually finally enforces it (but will most likely be relaxed back via env where airgap's enabled, but only based on that) — so the sentry reports should be watched esp. in deployments that don't enable airgap yet — like prod — that should be restricted now more than some others. EDIT: Prod has transcend linked, so it'll also be unsafe-inline; not sure which deployments currently don't link it, so those might be the noisy ones if there's still some violation unaddressed… but prod won't break.
| CMS_ADMIN_IMAGES_CSP_RO = csp_ro_report_uri and _override_csp(CONTENT_SECURITY_POLICY_REPORT_ONLY, append={"img-src": {"blob:"}}) | ||
| # The CMS admin frames itself for page previews. | ||
| CMS_ADMIN_CSP = _override_csp(CONTENT_SECURITY_POLICY, replace={"frame-ancestors": {csp.constants.SELF}}) | ||
|
|
||
|
|
||
| # The CMS admin frames itself for page previews and needs script-src: allow-inline | ||
| CMS_ADMIN_CSP = _override_csp( | ||
| CONTENT_SECURITY_POLICY, | ||
| replace={"frame-ancestors": {csp.constants.SELF}}, | ||
| append={"script-src": {csp.constants.UNSAFE_INLINE}}, | ||
| ) | ||
| CMS_ADMIN_CSP_RO = csp_ro_report_uri and _override_csp(CONTENT_SECURITY_POLICY_REPORT_ONLY, replace={"frame-ancestors": {csp.constants.SELF}}) |
There was a problem hiding this comment.
You probably want to either have RO matching the prod, or… start with the rules in RO-only for now, and let it sit for a bit and see whether it would trigger anything before blocking things?
There was a problem hiding this comment.
Pull request overview
This PR tightens the Content Security Policy (CSP) configuration by making unsafe-inline directives more targeted and specific. Instead of allowing unsafe-inline broadly across the application, it restricts these permissions to only the contexts where they are actually needed.
Changes:
- Removed blanket
unsafe-inlinefrom defaultscript-srcCSP, adding it only for CMS admin pages - Removed default
unsafe-inlinefromstyle-srcCSP (appears to rely on Transcend-specific addition) - Updated CMS admin CSP configuration to explicitly include
script-src: unsafe-inlinewith explanatory comment
| "www.youtube.com", | ||
| csp.constants.UNSAFE_EVAL, | ||
| csp.constants.UNSAFE_INLINE, | ||
| # Don't allow csp.constants.UNSAFE_INLINE wholesale in the default CSP. Be more targetted with it |
There was a problem hiding this comment.
Corrected spelling of 'targetted' to 'targeted'.
| # Don't allow csp.constants.UNSAFE_INLINE wholesale in the default CSP. Be more targetted with it | |
| # Don't allow csp.constants.UNSAFE_INLINE wholesale in the default CSP. Be more targeted with it |
| _csp_style_src = { | ||
| csp.constants.SELF, | ||
| CSP_ASSETS_HOST, | ||
| csp.constants.UNSAFE_INLINE, | ||
| "cdn.transcend.io", # Transcend Consent Management | ||
| "transcend-cdn.com", # Transcend Consent Management | ||
| } |
There was a problem hiding this comment.
The removal of unsafe-inline from _csp_style_src may break styling functionality if Transcend or other components require inline styles. The PR description mentions that unsafe-inline should only be available if Transcend is enabled, but this code doesn't show conditional logic for that. Consider verifying that inline styles are properly handled either through nonces or that all inline styles have been eliminated.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This changeset is an additional fixup around https://mozilla-hub.atlassian.net/browse/WT-536
It does a few things. My recommendation is to read the code first, then the rest of this description, so you are not swayed (and in case I've not got this right)
for
script-src, it only allowsunsafe-inlinefor the Wagtail admin pages which are only enabled for the CMS deployment. This does not appear to affect anything else, but we really need to be sure (eg Transcend, GA, cookie banner)for
style-srcwe had code that enabledunsafe-inlinespecifically for Transcend, but a few lines above we also hadunsafe-inlineset as a default. This changeset moves it so thatstyle-src: unsafe-inlineis only available if transcend is enabled.Testing
I'd welcome a Slack chat about approaches here. I've pushed this branch to www-demo6.allizom.org where we can drive around and also try the CMS, but Transcend isn't enabled there - maybe we could enable it in demos too, tbc on @stephendherrera's blessing