fix(config): replace deploy-web CSP nonce/strict-dynamic with host allowlist - #3523
Open
baktun14 wants to merge 8 commits into
Open
fix(config): replace deploy-web CSP nonce/strict-dynamic with host allowlist#3523baktun14 wants to merge 8 commits into
baktun14 wants to merge 8 commits into
Conversation
…lowlist The report-only CSP added in #3473 used a per-request nonce plus 'strict-dynamic'. That is incompatible with deploy-web's statically optimized Pages Router routes: their HTML is generated at build time with no nonce, and 'strict-dynamic' makes browsers ignore 'self', so every first-party script chunk (framework-*.js) was reported. This flooded Sentry with 14k events / 650 users under DEPLOY-WEB-2JT and would white-screen every static page if CSP_MODE were set to enforce. Switch script-src to a host allowlist ('self' 'unsafe-inline' + vendor hosts) that needs no nonce and behaves identically on static and dynamic pages, so the policy is safe to enforce. Remove the now-dead nonce plumbing: generateNonce, the x-nonce request header, and getCspNonce/nonce application in domUtils. 'unsafe-inline' is an intentional, reviewed tradeoff: the app's GTM loader injects an inline bootstrap client-side, and the static Pages Router architecture cannot attach a per-request nonce. The host allowlist plus object-src 'none', base-uri 'self', frame-ancestors 'none' and form-action 'self' are retained. Keep CSP_MODE=report-only for now; watch Sentry for any GTM-loaded origins now that 'strict-dynamic' is gone and add them to the allowlist before flipping to enforce. Fixes DEPLOY-WEB-2JT
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDeploy-web replaces request-based CSP nonces with a hashed theme script, applies CSP headers directly to responses, introduces an app theme provider, and refactors Google Tag Manager bootstrapping with focused tests. ChangesDeploy-web CSP and script integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3523 +/- ##
==========================================
- Coverage 74.18% 73.42% -0.76%
==========================================
Files 1142 1053 -89
Lines 29723 27371 -2352
Branches 7441 6981 -460
==========================================
- Hits 22049 20098 -1951
+ Misses 6779 6408 -371
+ Partials 895 865 -30
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/deploy-web/src/lib/csp/csp.spec.ts (1)
53-62: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the new Google Analytics allowlist entries.
The policy adds
https://www.google-analytics.comandhttps://*.google-analytics.com, but this test only protects GTM and Stripe. A regression removing either GA source would still pass.Suggested assertions
expect(scriptSrc).toContain("https://www.googletagmanager.com"); expect(scriptSrc).toContain("https://js.stripe.com"); + expect(scriptSrc).toContain("https://www.google-analytics.com"); + expect(scriptSrc).toContain("https://*.google-analytics.com");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/deploy-web/src/lib/csp/csp.spec.ts` around lines 53 - 62, Update the test case “allows first-party and vendor scripts via a host allowlist without a nonce or strict-dynamic” to assert that scriptSrc contains both https://www.google-analytics.com and https://*.google-analytics.com, alongside the existing GTM and Stripe allowlist assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/deploy-web/src/lib/csp/csp.ts`:
- Around line 82-88: Update buildContentSecurityPolicy so enforce mode does not
emit "'unsafe-inline'" in script-src; retain report-only behavior until required
inline scripts are covered by hashes or nonces, or construct a separate enforce
policy without that directive.
---
Nitpick comments:
In `@apps/deploy-web/src/lib/csp/csp.spec.ts`:
- Around line 53-62: Update the test case “allows first-party and vendor scripts
via a host allowlist without a nonce or strict-dynamic” to assert that scriptSrc
contains both https://www.google-analytics.com and
https://*.google-analytics.com, alongside the existing GTM and Stripe allowlist
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d9e58ef-ab97-4163-a980-dd0de6036189
📒 Files selected for processing (4)
apps/deploy-web/src/lib/csp/csp.spec.tsapps/deploy-web/src/lib/csp/csp.tsapps/deploy-web/src/middleware.tsapps/deploy-web/src/utils/domUtils.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/src/utils/domUtils.ts
middleware.ts had no test, so the CSP-header lines changed in this PR showed 0% patch coverage and failed codecov/patch/deploy-web (which in turn failed validate/result and the security-scan gate). Add middleware.spec.ts asserting the middleware attaches the CSP header (report-only by default, enforcing under CSP_MODE=enforce) and no longer emits an x-nonce header.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/deploy-web/src/middleware.spec.ts`:
- Around line 11-16: Expand the test for the default response created by setup
in middleware.spec.ts to assert the complete approved script-src CSP allowlist,
including all required vendor and GA hosts. Also assert that the policy excludes
'unsafe-inline', 'strict-dynamic', and nonce sources, while preserving the
existing report-only header and absent enforcing-header checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 585262fa-2558-45cb-a58d-240193aae50e
📒 Files selected for processing (1)
apps/deploy-web/src/middleware.spec.ts
Strengthen the CSP contract tests so a regression is caught: assert every approved script-src host (GTM, GA, growth-channel, cloudflare, stripe) and that 'strict-dynamic'/nonce sources stay absent. The middleware test now rejects those directives too, instead of a loose 'self' substring check.
…op-nonce-strict-dynamic
…op-nonce-strict-dynamic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/deploy-web/src/components/layout/TrackingScripts.tsx`:
- Around line 15-38: Guard loadGoogleTagManager and appendGtmNoscriptFallback
against a missing gtmId before injecting the Google Tag Manager script or
iframe. Return without modifying the data layer or DOM when NEXT_PUBLIC_GTM_ID
is unset, while preserving the existing loading behavior for valid IDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03c1d15c-75b3-4fae-8589-aa92c8ccb246
📒 Files selected for processing (9)
apps/deploy-web/src/components/layout/AppThemeProvider.spec.tsxapps/deploy-web/src/components/layout/AppThemeProvider.tsxapps/deploy-web/src/components/layout/TrackingScripts.spec.tsxapps/deploy-web/src/components/layout/TrackingScripts.tsxapps/deploy-web/src/lib/csp/csp.spec.tsapps/deploy-web/src/lib/csp/csp.tsapps/deploy-web/src/middleware.spec.tsapps/deploy-web/src/pages/_app.tsxapps/deploy-web/src/utils/domUtils.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/src/utils/domUtils.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Sentry DEPLOY-WEB-2JT was escalating — 14,090 report-only CSP violations across 650 users since 2026-07-23, all reporting the app's own Next.js framework chunk (
/_next/static/chunks/framework-*.js) as blocked byscript-src-elem.Root cause: the report-only CSP added in #3473 uses a per-request
'nonce-…'+'strict-dynamic'script policy. deploy-web is Pages Router with statically optimized routes (/,/deployments, …) whose HTML is generated at build time with no nonce.'strict-dynamic'makes browsers ignore'self', so every un-nonced first-party script chunk is reported. Nothing is broken today (it's report-only), but flippingCSP_MODE=enforceon this policy would white-screen every static page.Fixes DEPLOY-WEB-2JT
What
Replace the nonce/strict-dynamic script policy with a fully static policy — host allowlist plus a single script hash — with no
'unsafe-inline':csp.ts—script-src 'self' 'sha256-…' <vendor hosts>; dropped thenonceparameter,generateNonce,'strict-dynamic', and the redundantwww.google-analytics.comentry (covered by*.google-analytics.com).middleware.ts— removed nonce generation, the unusedx-nonceheader, and the CSP-on-request-header plumbing (NextResponse.next()with the CSP on the response only).TrackingScripts.tsx— GTM no longer bootstraps via the stock inline IIFE: the effect seedswindow.dataLayerdirectly and loadsgtm.jsas an external allowlisted script. Timing is identical — the IIFE already ran post-hydration from this same effect, so the inline wrapper was pure overhead.domUtils.ts— removed the deadgetCspNonceand theinnerHTMLoption fromScriptOptions; the script-injection helper can now only inject external scripts, making inline-script regressions impossible at the type level.AppThemeProvider.tsx— extracted the next-themes provider from_app.tsx. Its inline anti-FOUC theme script is the one remaining inline script (it must run before first paint and is library-rendered, so it can't be externalized) and is allowed by sha256 (THEME_SCRIPT_HASH).AppThemeProvider.spec.tsxrecomputes the hash from the rendered provider, so a next-themes upgrade or prop change fails CI with the new hash in the failure message instead of silently breaking production.Why hash + host allowlist, not nonce or unsafe-inline
'unsafe-inline'would neuter script-src's XSS protection, and both prior inline scripts had clean non-inline solutions — so nothing needs it.Verification
csp,middleware,TrackingScripts, andAppThemeProvider(including the hash drift guard); lint and types clean on changed files; full deploy-web unit suite green apart from a pre-existinguseProviderJwtfailure that reproduces without these changes (passes in CI).next start, report-only header):/and/deployments(the pages from the Sentry issue): 0 CSP violations — framework chunks and the hashed theme script all execute, and the theme class/color-schemeare applied (no FOUC regression).gtm.jsscript element injected fromwww.googletagmanager.com: no violation (load permitted by the allowlist).example.com:script-src-elemviolation reported immediately — the policy is live and reports non-allowlisted sources as expected.Rollout
Stays
report-only(the default —CSP_MODEunset). The report-only window now also validates the GTM container: if a Custom HTML tag ever injects an inline script, it will surface in Sentry as a script-src report before enforcement. Watch Sentry to confirm no new script-src reports, then flipCSP_MODE=enforcein a follow-up.Summary by CodeRabbit
strict-dynamic-based script allowances, and instead rely on an exported hashed theme inline script plus approved first-party and vendor origins (excluding'unsafe-inline').