fix(csp): match script-src-elem/attr when extracting script nonce - #2049
Conversation
Select the CSP nonce directive via startsWith("script-src") /
startsWith("default-src"), matching Next.js get-script-nonce-from-header.
Previously the exact-match predicate skipped script-src-elem/script-src-attr
directives, so a CSP declaring the nonce only on script-src-elem fell through
to default-src and SSR scripts could be emitted without the expected nonce
and blocked by the browser CSP.
Closes cloudflare#1989
commit: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: LGTM ✅
The fix is correct, minimal, and faithfully matches Next.js. I verified the change against the current canary source of get-script-nonce-from-header.tsx:
const directive =
directives.find((dir) => dir.startsWith('script-src')) ||
directives.find((dir) => dir.startsWith('default-src'))This PR now does exactly the same directive selection (startsWith("script-src") then startsWith("default-src")), which correctly resolves #1989: a CSP declaring the nonce only on script-src-elem/script-src-attr is now matched instead of falling through to default-src or no nonce.
Verified
- Parity correct: directive selection matches Next.js canary exactly, including the single-directive semantics (nonce read only from the first matching
script-src*directive). The test attests/csp.test.ts:54correctly encodes this (script-src-elem 'self'; script-src 'nonce-x'→undefined), which is the same behavior Next.js exhibits. - Dead code removed cleanly:
matchesDirectiveNamehad no other references in the repo (rgconfirms zero), and this is the only directive matcher, so App Router / Pages Router / dev are all fixed together as described. - Tests updated correctly: the two prior tests encoded the old buggy exact-match behavior; the rewritten/added tests assert the new (Next.js-parity) behavior.
vp test run tests/csp.test.ts→ 15 passed;vp test run tests/nextjs-compat/script-nonce.test.ts→ 4 passed (no SSR regression).
Non-blocking observations (pre-existing, out of scope)
The nonce source parsing in getScriptNonceFromHeader still diverges slightly from Next.js — vinext splits on a single space and uses a startsWith("'nonce-") + endsWith("'") guard, whereas Next.js splits on /\s+/ and matches /^'nonce-([A-Za-z0-9+/_-]+={0,2})'$/. The existing whitespace test passes because empty split segments fail the startsWith guard, and the ESCAPE_REGEX guard backstops invalid characters. This is unchanged by this PR and not a regression, so it shouldn't block the merge — just noting it as a potential future parity follow-up (e.g., a nonce containing internal whitespace would be parsed differently, though that's not valid base64 anyway).
Nice, focused fix with clear parity documentation in both the code comment and the PR description.
|
Review submitted successfully. Summary of my findings: Review: Approved ✅PR #2049 fixes the CSP nonce directive matching (#1989). My assessment: The fix is correct and faithfully matches Next.js. I verified against canary Verified:
Non-blocking observation: The nonce source parsing (single-space split + Clean, well-scoped fix with good documentation. |
Problem
Next.js's
get-script-nonce-from-header.tsxselects the nonce directive withdir.startsWith('script-src'), which also matchesscript-src-elemandscript-src-attr. vinext used an exact-match predicate (matchesDirectiveName) that requireddirective === "script-src"or ascript-srcprefix, so a CSP declaring the nonce only onscript-src-elem(with no plainscript-src) was skipped. vinext then fell through todefault-src(or found no nonce), and SSR scripts could be emitted without the expected nonce and get blocked by the browser CSP.Fixes #1989.
Fix
Select the directive via
startsWith("script-src")thenstartsWith("default-src"), matching Next.js exactly. The existing nonce-source parsing (slice +ESCAPE_REGEXguard) is unchanged. This is the single directive matcher in the repo, so App Router (app-rsc-handler.ts), Pages Router (pages-page-handler.ts), and dev (dev-server.ts) are all fixed together. The now-unusedmatchesDirectiveNamehelper was removed.Next.js parity notes
packages/next/src/server/app-render/get-script-nonce-from-header.tsx, which usesdirectives.find(dir => dir.startsWith('script-src')) || directives.find(dir => dir.startsWith('default-src')).script-src,script-src-elem,script-src-attrstart withscript-src(all script-related); onlydefault-srcstarts withdefault-src.script-src*directive, so a nonce on a later plainscript-srcis not used.Tests
Updated
tests/csp.test.ts(the two prior tests encoded the old buggy behavior). Added coverage for:script-src-elem 'nonce-abc'/script-src-attr 'nonce-attr'with no plainscript-src→ nonce extractedscript-src*/default-src*directive selected when multiple existscript-src*directive (script-src-elem 'self'; script-src 'nonce-x'→undefined)Validation
vp test run tests/csp.test.ts— 15 passedvp test run tests/nextjs-compat/script-nonce.test.ts— 4 passed (no SSR regression)vp check— format, lint, typecheck clean