-
Notifications
You must be signed in to change notification settings - Fork 966
WT-536: improve our CSP unsafe-inline config by making it tighter and more specific #16994
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -109,12 +109,12 @@ | |||||||
| "www.googletagmanager.com", | ||||||||
| "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 | ||||||||
|
||||||||
| # 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… 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
| # Transcend Consent Management UI uses CSS-in-JS which requires inline styles. | |
| if TRANSCEND_AIRGAP_URL: # noqa: F405 | |
| _csp_style_src.add(csp.constants.UNSAFE_INLINE) |
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.
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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?)