fix(csp): add per-request script nonces for Cloudflare JS detections#51
Merged
Conversation
|
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:
WalkthroughAdds per-response CSP nonce generation, injects nonces into HTML script tags, updates CSP headers, and applies the transformation to eligible HTML asset responses. ChangesDocument CSP nonce handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkerFetch
participant AssetResponse
participant applyDocumentCspNonce
Client->>WorkerFetch: request asset
WorkerFetch->>AssetResponse: obtain asset response
WorkerFetch->>applyDocumentCspNonce: transform eligible HTML
applyDocumentCspNonce->>AssetResponse: read body and CSP headers
applyDocumentCspNonce-->>WorkerFetch: return nonce-injected response
WorkerFetch-->>Client: return transformed HTML
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@worker/documentCsp.ts`:
- Around line 18-23: Replace the regex-based rewriting in injectScriptNonces
with Cloudflare’s native HTMLRewriter, registering a script element handler that
adds the nonce only when one is absent. Update the function to accept and return
a Response so transformation remains streaming and avoids buffering the entire
HTML body.
- Around line 25-36: Scope SCRIPT_NONCE_PATTERN matching and replacement within
the script-src directive in addScriptNonceToCsp, rather than testing or
replacing across the entire CSP string. Ensure an existing nonce is reused only
from script-src, and that unrelated directives such as style-src remain
unchanged while the nonce is added to script-src.
In `@worker/index.test.ts`:
- Around line 72-95: Strengthen the tests around injectScriptNonces and
applyDocumentCspNonce by extracting the nonce from the rewritten script/body and
asserting the CSP contains that exact same nonce. Add coverage for an input
script that already has a nonce, verifying the helper preserves the existing
value rather than replacing it.
- Around line 46-96: Extend the “document CSP nonce” tests to exercise the
worker.fetch path, using an HTML asset request and asserting the response
receives CSP and script nonce transformation. Add a second worker.fetch case for
a non-HTML response and verify its body and headers remain unchanged, covering
the wiring in worker.fetch rather than only applyDocumentCspNonce.
In `@worker/index.ts`:
- Around line 45-48: Guard the applyDocumentCspNonce call in worker/index.ts
within the isHtmlDocumentResponse path so null-body statuses 204, 205, and 304
return the original assetResponse unchanged. Alternatively, add the early return
in applyDocumentCspNonce in worker/documentCsp.ts, covering those statuses
before rebuilding the response; update only the selected root-cause location,
with the other site requiring no direct change.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: dcef2f37-e822-49ee-bf27-7e322ccf1f8a
📒 Files selected for processing (3)
worker/documentCsp.tsworker/index.test.tsworker/index.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
worker/documentCsp.ts (1)
65-85: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
applyDocumentCspNoncestill fully buffers the body, defeating the point of usingHTMLRewriter.This reads
response.text()upfront (L69), then whenHTMLRewriteris available,injectScriptNonceswraps that already-buffered string into a newResponse, streams it throughHTMLRewriter, and immediately reads it back to text (L49 ininjectScriptNonces). The original review comment recommendedHTMLRewriterspecifically to avoid buffering the whole body; that benefit isn't realized because the caller buffers first regardless of which branch runs.Restructure so
applyDocumentCspNoncepasses theresponseobject itself into a rewriter-based transform (only falling back to string buffering whenHTMLRewriteris unavailable), so the CSP header can be rewritten before the body stream is ever fully materialized in memory.🤖 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 `@worker/documentCsp.ts` around lines 65 - 85, Update applyDocumentCspNonce to pass the original response directly into the HTMLRewriter-based transformation instead of calling response.text() first. Move string buffering into the no-HTMLRewriter fallback within injectScriptNonces, while preserving the existing status, statusText, headers, and CSP nonce behavior.
♻️ Duplicate comments (2)
worker/index.test.ts (2)
93-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo previously-requested test-coverage gaps remain open.
- All assertions here call
injectScriptNonces/applyDocumentCspNoncedirectly — there's still no test that drives this through the actualworker.fetch()handler inworker/index.ts(e.g., an HTML asset request vs. a non-HTML request bypassing the transform), so a wiring regression inshouldApplyDocumentCsp/applyDocumentCspNonceusage insidefetch()wouldn't be caught.- No test exercises the "script tag already has a
nonceattribute" branch ininjectScriptNonces(both theHTMLRewriterand regex-fallback implementations explicitly preserve an existing nonce, but neither is verified).As per path instructions, test files should assert "Behavior-focused assertions on changed code paths" and "Edge cases for new logic."
it("preserves an existing nonce on script tags", async () => { expect( await injectScriptNonces( '<script nonce="existing" src="/trusted.js"></script>', "test-nonce", ), ).toBe('<script nonce="existing" src="/trusted.js"></script>'); });🤖 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 `@worker/index.test.ts` around lines 93 - 123, Expand the worker tests to cover both missing paths: add a `worker.fetch()` test that verifies HTML asset responses are transformed with a CSP nonce while non-HTML responses bypass transformation, exercising `shouldApplyDocumentCsp` and `applyDocumentCspNonce`; also add an `injectScriptNonces` test for a script with an existing nonce and assert that the original nonce is preserved.Source: Path instructions
117-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist these regex literals to module scope.
ESLint flags both as re-compiled on every call.
♻️ Proposed fix
+const CSP_NONCE_TOKEN_PATTERN = /'nonce-([^']+)'/; +const SCRIPT_NONCE_ATTRIBUTE_PATTERN = /nonce="([^"]+)"/; + describe("document CSP nonce", () => { @@ const body = await response.text(); const headerCsp = response.headers.get("Content-Security-Policy") ?? ""; - const headerNonce = headerCsp.match(/'nonce-([^']+)'/)?.[1]; - const bodyNonce = body.match(/nonce="([^"]+)"/)?.[1]; + const headerNonce = headerCsp.match(CSP_NONCE_TOKEN_PATTERN)?.[1]; + const bodyNonce = body.match(SCRIPT_NONCE_ATTRIBUTE_PATTERN)?.[1];🤖 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 `@worker/index.test.ts` around lines 117 - 118, Hoist the nonce-matching regular expressions used for headerNonce and bodyNonce to module scope, then reuse those constants in the existing match calls. Preserve both regex patterns and the current extraction behavior while avoiding recompilation on each call.Source: Linters/SAST tools
🤖 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 `@e2e/fixtures/session.ts`:
- Around line 175-180: Update the contextOptions object used by hostContext and
guestContext to remove the readonly `as const` assertion from permissions,
leaving it as a mutable string array containing "geolocation".
In `@worker/documentCsp.ts`:
- Around line 52-63: Hoist the nonce-stripping and whitespace-collapsing regular
expressions used by addScriptNonceToCsp to module-level constants, then reuse
those constants inside the replace callback while preserving the existing CSP
output behavior.
- Around line 30-50: Add a Workers-runtime test for injectScriptNonces that
provides or runs with HTMLRewriter available, exercising the rewriter branch
rather than the typeof HTMLRewriter fallback. Verify scripts without a nonce
receive the supplied nonce while existing nonce attributes remain unchanged.
---
Outside diff comments:
In `@worker/documentCsp.ts`:
- Around line 65-85: Update applyDocumentCspNonce to pass the original response
directly into the HTMLRewriter-based transformation instead of calling
response.text() first. Move string buffering into the no-HTMLRewriter fallback
within injectScriptNonces, while preserving the existing status, statusText,
headers, and CSP nonce behavior.
---
Duplicate comments:
In `@worker/index.test.ts`:
- Around line 93-123: Expand the worker tests to cover both missing paths: add a
`worker.fetch()` test that verifies HTML asset responses are transformed with a
CSP nonce while non-HTML responses bypass transformation, exercising
`shouldApplyDocumentCsp` and `applyDocumentCspNonce`; also add an
`injectScriptNonces` test for a script with an existing nonce and assert that
the original nonce is preserved.
- Around line 117-118: Hoist the nonce-matching regular expressions used for
headerNonce and bodyNonce to module scope, then reuse those constants in the
existing match calls. Preserve both regex patterns and the current extraction
behavior while avoiding recompilation on each call.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 0eeecfe5-da29-4588-9512-c4c25a7c5d7d
📒 Files selected for processing (5)
e2e/fixtures/map.tse2e/fixtures/session.tsworker/documentCsp.tsworker/index.test.tsworker/index.ts
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 `@worker/index.test.ts`:
- Around line 211-263: Add a test in the “worker fetch” suite for an HTML asset
response with Content-Type set to text/html but no Content-Security-Policy
header, verifying the response body receives a nonce while the CSP header
remains absent. Reuse the existing worker.fetch and ASSETS mock setup from the
adjacent tests.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: d97cbf1c-0367-4f5a-96d4-16749d97b601
📒 Files selected for processing (3)
e2e/fixtures/session.tsworker/documentCsp.tsworker/index.test.ts
gelbh
force-pushed
the
fix/csp-cf-js-detection-nonce
branch
from
c5ff47e to
df0d810
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@worker/documentCsp.ts`:
- Around line 54-67: The addScriptNonceToCsp function currently updates only
script-src; change it to authorize the nonce in script-src-elem first, otherwise
script-src, or create the applicable script policy from default-src when neither
exists. Add focused tests in worker/index.test.ts at lines 158-165 covering both
directive-selection paths.
- Around line 15-18: The isHtmlDocumentResponse function must parse the content
type as a media type instead of using substring matching: normalize it
case-insensitively, remove any parameters after the first semicolon, and compare
the result exactly with text/html. In worker/index.test.ts lines 48-63, add
coverage for uppercase valid values and values that merely contain text/html,
which must be rejected.
- Line 12: Hoist the three Base64 replacement regular expressions used by
generateCspNonce to module-level constants, then reuse those constants in the
return expression instead of recreating regex literals on each call.
In `@worker/index.test.ts`:
- Around line 158-165: Add behavior-focused tests alongside the existing
addScriptNonceToCsp coverage for policies containing script-src-elem and
policies containing only default-src. Assert that the nonce is appended to the
directive browsers use for script elements while preserving unrelated directives
and existing sources.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 578e71e2-ad3b-4d58-b8af-9fe043c58f2a
📒 Files selected for processing (3)
worker/documentCsp.tsworker/index.test.tsworker/index.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
worker/documentCsp.ts (1)
62-86: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDefault-src-only CSPs still bypass nonce injection.
addScriptNonceToCsponly rewritesscript-src-elemorscript-src. If a policy relies solely ondefault-src(no explicit script directive), neither regex matches,csp.replaceis a no-op, and the returned CSP has no nonce — so browsers enforcing that policy will block the nonce-tagged<script>elements injected byinjectScriptNonces, silently breaking the page under CSP. This exact gap was raised previously ("Authorize the nonce in the effective script policy") and does not appear to have been addressed for thedefault-src-only path.♻️ Suggested fallback
if (SCRIPT_SRC_ELEM_PATTERN.test(csp)) { return csp.replace(SCRIPT_SRC_ELEM_PATTERN, (_match, sources: string) => appendNonceToDirective("script-src-elem", sources), ); } - return csp.replace(SCRIPT_SRC_PATTERN, (_match, sources: string) => - appendNonceToDirective("script-src", sources), - ); + if (SCRIPT_SRC_PATTERN.test(csp)) { + return csp.replace(SCRIPT_SRC_PATTERN, (_match, sources: string) => + appendNonceToDirective("script-src", sources), + ); + } + + const defaultSrcMatch = csp.match(/default-src ([^;]+)/); + if (defaultSrcMatch) { + return `${csp}; ${appendNonceToDirective("script-src", defaultSrcMatch[1])}`; + } + + return csp;🤖 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 `@worker/documentCsp.ts` around lines 62 - 86, Update addScriptNonceToCsp so that when neither script-src-elem nor script-src exists, it appends the nonce to the existing default-src directive as the effective script policy. Preserve the current preference for script-src-elem, then script-src, and continue removing any existing nonce before adding the requested nonce.worker/index.test.ts (1)
179-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing test for
default-src-only CSP fallback.Deferred to the consolidated comment anchored on
worker/documentCsp.ts— shares the same root cause as the missingdefault-srcfallback inaddScriptNonceToCsp.🤖 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 `@worker/index.test.ts` around lines 179 - 186, Add a test in the addScriptNonceToCsp test suite covering a CSP containing only default-src, and assert that the nonce is appended to the default-src directive as the fallback when script-src and script-src-elem are absent.Source: Path instructions
🤖 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.
Duplicate comments:
In `@worker/documentCsp.ts`:
- Around line 62-86: Update addScriptNonceToCsp so that when neither
script-src-elem nor script-src exists, it appends the nonce to the existing
default-src directive as the effective script policy. Preserve the current
preference for script-src-elem, then script-src, and continue removing any
existing nonce before adding the requested nonce.
In `@worker/index.test.ts`:
- Around line 179-186: Add a test in the addScriptNonceToCsp test suite covering
a CSP containing only default-src, and assert that the nonce is appended to the
default-src directive as the fallback when script-src and script-src-elem are
absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 603d73a8-e942-4339-8dd6-cb00ad9e9bcb
📒 Files selected for processing (2)
worker/documentCsp.tsworker/index.test.ts
Owner
Author
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
gelbh
force-pushed
the
fix/csp-cf-js-detection-nonce
branch
from
424226f to
8327968
Compare
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.
Summary
Cloudflare Bot Fight / JavaScript Detections injects a dynamic inline script (
/cdn-cgi/challenge-platform/…) that cannot be allowlisted with static SHA-256 hashes. The Worker now adds a per-requestscript-srcnonce to HTML responses so Cloudflare can attach the same nonce to its injected script (per CF docs).Test plan
Summary by CodeRabbit
type="module") receive a matchingnonce="..."when they don’t already have one.Content-Security-Policyis updated to include the new nonce token forscript-src/script-src-elem.