feat(security): collect CSP violation reports so report-only means something - #3470
Conversation
…mething The platform CSP floor is served report-only to every project that has not declared `security.csp`, and it carried no reporting endpoint. So it neither enforced nor reported: the violations it named reached only whoever happened to open devtools on the affected page, and the rollout it exists to enable had no instrument. There was no way to answer "which projects would break if we enforced?" short of enforcing and waiting for complaints, which is how the floor shipped the first time and broke roughly a hundred projects at once. Adds `POST /_vf/csp-report`, names it in the policy through both `report-to` and the deprecated `report-uri` (still the only spelling several shipping browsers honour), and defines the group in `Reporting-Endpoints`. The endpoint is unauthenticated by nature — browsers post to it without credentials and so can anyone else — so it treats every body as hostile: the size is capped before parsing, logged fields are truncated, both wire formats normalize to one log schema, and the response is always 204 so a poster learns nothing about what was accepted. A per-window ceiling stops one misconfigured project from drowning the log stream, and records what it dropped. Also: - `Reporting-Endpoints` joins the policy-owned header list and the override guard. A project-provided value would send reports elsewhere, or nowhere, while the policy still looked healthy. - Removes `isSecurityPolicyResponseHeaderName`, which had no call sites; the only mention was a comment explaining it was deliberately not reused. - Fixes `isCorsPolicyResponseHeaderName`, declared `value is string`. In the negative branch that told the compiler a non-matching header name was not a string at all, collapsing it to `never`. Nothing tripped it because the one caller only passed the value on to another `string` parameter.
|
Warning Review limit reached
Next review available in: 5 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds CSP reporting constants and response headers, a ChangesCSP reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant security-handler
participant AuthHandler
participant CsrfHandler
participant CspReportHandler
participant StructuredWarningLogger
Browser->>security-handler: Receive CSP reporting headers
security-handler-->>Browser: Provide report-to and Reporting-Endpoints
Browser->>AuthHandler: POST CSP violation report
AuthHandler->>CsrfHandler: Continue recognized report request
CsrfHandler->>CspReportHandler: Route report request
CspReportHandler->>CspReportHandler: Validate, normalize, and rate-limit
CspReportHandler->>StructuredWarningLogger: Log accepted reports
CspReportHandler-->>Browser: 204 No Content
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faf0978a36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/security/http/response/security-handler.ts (1)
437-443: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReport the reserved reporting header in the warning text.
Reporting-Endpointsnow setsignoredCspHeader, so a project that overrides onlyReporting-Endpointsreceives the message "Ignored Content-Security-Policy entries in security.headers; configure security.csp instead". That names the wrong header and the wrong remedy.🐛 Proposed fix for the warning text
logger.warn( - "Ignored Content-Security-Policy entries in security.headers; configure security.csp instead", + "Ignored policy-owned Content-Security-Policy and Reporting-Endpoints entries in security.headers; configure security.csp instead", );🤖 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 `@src/security/http/response/security-handler.ts` around lines 437 - 443, Update the warning text in the response security-header handling around ignoredCspHeader to distinguish Reporting-Endpoints from CSP headers. When the reserved REPORTING_ENDPOINTS_HEADER is ignored, report that header explicitly and direct users to the appropriate reporting-endpoints configuration rather than describing it as a Content-Security-Policy override.Source: Coding guidelines
🧹 Nitpick comments (4)
src/security/http/response/security-handler.ts (1)
42-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the header constant from the reserved list.
The literal
"reporting-endpoints"now exists twice in this file. Reference the frozen entry instead, so the reserved name and the emitted name cannot drift.♻️ Proposed deduplication
-/** Response header defining the reporting groups the policy refers to. */ -const REPORTING_ENDPOINTS_HEADER = "reporting-endpoints"; +/** Response header defining the reporting groups the policy refers to. */ +const REPORTING_ENDPOINTS_HEADER: string = SECURITY_POLICY_RESPONSE_HEADER_NAMES.find( + (name) => name === "reporting-endpoints", +)!;If the non-null assertion is unwanted, keep the literal and add a compile-time check that the list contains it.
🤖 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 `@src/security/http/response/security-handler.ts` around lines 42 - 53, Update REPORTING_ENDPOINTS_HEADER to derive its value from the frozen reserved-header list entry instead of repeating the "reporting-endpoints" literal, ensuring both names remain synchronized.src/server/handlers/request/csp-report.handler.test.ts (1)
81-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the log ceiling, not only the status code.
This test proves the endpoint never applies backpressure, which is the right guarantee. It does not prove the ceiling bounds log volume. Add a case that posts a Reporting API array of 16 violations repeatedly and asserts the logged line count stays at or below
MAX_LOGGED_PER_WINDOW. That case fails today, for the accounting reason raised onadmitToLoginsrc/server/handlers/request/csp-report.handler.ts.🤖 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 `@src/server/handlers/request/csp-report.handler.test.ts` around lines 81 - 92, Extend the CSP report rate-limit tests around CspReportHandler to verify log-volume enforcement, not just repeated 204 responses. Add a case that repeatedly submits a Reporting API array containing 16 violations and asserts the logged line count does not exceed MAX_LOGGED_PER_WINDOW, using the existing logging capture and reset helpers.Source: Coding guidelines
src/server/handlers/request/csp-report.handler.ts (1)
137-153: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftKey the log ceiling per project.
windowStartedAt,loggedInWindow, anddroppedInWindoware module-level, so all projects in the process share one window. One misconfigured project consumes the whole ceiling and silences reports for every other project, which removes the signal the enforcement rollout depends on. Key the window byctx.projectSlugwith a bounded map.Also note that line 143 compares
raw.length, a UTF-16 code-unit count, against a byte budget namedMAX_BODY_BYTES. Memory stays bounded, so rename the check or measure bytes for accuracy.🤖 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 `@src/server/handlers/request/csp-report.handler.ts` around lines 137 - 153, Update the CSP report admission flow around admitToLog to maintain window start, logged, and dropped counters per ctx.projectSlug using a bounded map, preventing one project from consuming another’s log ceiling; preserve the existing ceiling behavior within each project. Also replace the raw.length comparison with a byte-accurate measurement against MAX_BODY_BYTES, or rename the limit/check consistently if the intended budget is characters.src/security/http/response/security-handler.test.ts (1)
646-648: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing
getCsphelper.
getCspis in scope for this test and already handles both CSP header names.🤖 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 `@src/security/http/response/security-handler.test.ts` around lines 646 - 648, Update the test setup around applyHeaders to reuse the in-scope getCsp helper instead of manually reading Content-Security-Policy and Content-Security-Policy-Report-Only headers, preserving the existing fallback behavior provided by that helper.
🤖 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 `@src/server/handlers/request/csp-report.handler.test.ts`:
- Line 2: Update the test file’s BDD import to use the repository test module at
`#veryfront/testing/bdd.ts`, importing describe and it from there instead of
`@std/testing/bdd`.
In `@src/server/handlers/request/csp-report.handler.ts`:
- Around line 55-71: Update admitToLog to accept the caller’s intended log-line
count and enforce MAX_LOGGED_PER_WINDOW against that count rather than counting
admissions; increment loggedInWindow by the charged lines on every admitted
path, including rollover, while preserving dropped-count reporting. Update the
request handler call site to pass the batch’s actual number of lines (up to
MAX_REPORTS_PER_REQUEST), and add a focused test confirming a batched Reporting
API request cannot admit more than MAX_LOGGED_PER_WINDOW lines.
- Around line 73-76: Update the sanitization helpers near truncate to remove
control characters, including CR, LF, and ANSI escape sequences, before
truncating report fields; add URI-specific handling that strips query strings
before applying the sanitized truncation. In fromBody, use truncateUri for
documentUri and blockedUri while retaining truncate for other fields.
In `@src/server/runtime-handler/index.ts`:
- Line 168: Update the handler registry around CspReportHandler to exempt
CSP_REPORT_PATH from both authentication and CSRF checks, using the existing
exclude-path configuration mechanisms. Add a registry-level test covering an
unauthenticated POST to CSP_REPORT_PATH and verify it reaches CspReportHandler
without returning 401 or 403.
---
Outside diff comments:
In `@src/security/http/response/security-handler.ts`:
- Around line 437-443: Update the warning text in the response security-header
handling around ignoredCspHeader to distinguish Reporting-Endpoints from CSP
headers. When the reserved REPORTING_ENDPOINTS_HEADER is ignored, report that
header explicitly and direct users to the appropriate reporting-endpoints
configuration rather than describing it as a Content-Security-Policy override.
---
Nitpick comments:
In `@src/security/http/response/security-handler.test.ts`:
- Around line 646-648: Update the test setup around applyHeaders to reuse the
in-scope getCsp helper instead of manually reading Content-Security-Policy and
Content-Security-Policy-Report-Only headers, preserving the existing fallback
behavior provided by that helper.
In `@src/security/http/response/security-handler.ts`:
- Around line 42-53: Update REPORTING_ENDPOINTS_HEADER to derive its value from
the frozen reserved-header list entry instead of repeating the
"reporting-endpoints" literal, ensuring both names remain synchronized.
In `@src/server/handlers/request/csp-report.handler.test.ts`:
- Around line 81-92: Extend the CSP report rate-limit tests around
CspReportHandler to verify log-volume enforcement, not just repeated 204
responses. Add a case that repeatedly submits a Reporting API array containing
16 violations and asserts the logged line count does not exceed
MAX_LOGGED_PER_WINDOW, using the existing logging capture and reset helpers.
In `@src/server/handlers/request/csp-report.handler.ts`:
- Around line 137-153: Update the CSP report admission flow around admitToLog to
maintain window start, logged, and dropped counters per ctx.projectSlug using a
bounded map, preventing one project from consuming another’s log ceiling;
preserve the existing ceiling behavior within each project. Also replace the
raw.length comparison with a byte-accurate measurement against MAX_BODY_BYTES,
or rename the limit/check consistently if the intended budget is characters.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab4b2972-3964-447f-b467-808c3fefd912
📒 Files selected for processing (9)
docs/api-reference/veryfront/security.mdsrc/security/http/csp-report-endpoint.tssrc/security/http/response/security-handler.test.tssrc/security/http/response/security-handler.tssrc/server/handlers/request/csp-report.handler.test.tssrc/server/handlers/request/csp-report.handler.tssrc/server/handlers/request/index.tssrc/server/runtime-handler/index.tssrc/utils/cors-policy-limits.ts
Five problems in the handler as first written, found reviewing it rather than by a failing test: - `req.text()` buffered the whole body and only then measured it, so the 64 KiB cap was advisory. A body with no `content-length`, or a dishonest one, was already in memory by the time it was rejected. It is now read against a byte budget off the stream, which is the only version of the limit that holds. The declared-length check stays as a cheap early exit. - The cap compared `MAX_BODY_BYTES` against `String.length`, which counts UTF-16 code units, not bytes. Reading the stream measures bytes. - The window rollover returned early when the previous window had dropped reports, skipping the counter increment, so one report per window was free. - `slice` ran before `filter`, so a batch carrying other report types could push real violations out of the window. - The rate limiter kept module-level mutable state and exported a reset hook used only by tests. It is instance state now: one handler per registry, so production behaviour is unchanged, and each test gets a fresh window without the module exposing an API it does not otherwise need. The new test drives a body with no declared length and asserts on how much the handler pulled off the stream. It fails against the buffer-then-measure version, which I checked before keeping it.
Review found seven problems, five of them real defects in code I had already called finished. The endpoint was unreachable for the projects that most need it. `AuthHandler` (priority 0) and `CsrfHandler` (priority 5) both match all requests and both run ahead of this priority-100 handler, and a browser reports a violation with neither credentials nor a CSRF token, because a report is not a user action. So any project enabling auth or CSRF advertised a reporting endpoint that answered 403 or 401 and collected nothing. Both gates now exempt this one framework route. That is safe on the terms they exist for: it reads no credentials, changes no state, and answers 204 regardless of the body, so it discloses nothing about a protected project. Reports also lost their most useful fields. The Reporting API sends `effectiveDirective` and `statusCode` in camel case; the normalizer read only the hyphenated spellings, so modern reports arrived with no directive and no status -- exactly the two fields an enforcement decision is made from. The test that should have caught it asserted only the status code, and the payload it sent was camel case, so it passed while the data was being dropped. Remaining fixes: - Every field came from an unauthenticated body and was only length-capped, so CR/LF could forge additional log records (CWE-117). Control characters are stripped, and URI fields lose their query string, which can carry session identifiers with no business in a log. - One admission permitted a whole batch, so a sender posting 16 violations per request wrote 16x the ceiling. The window is charged per record now. - `readBoundedBody` duplicated the existing `readBodyWithLimit`, which also coalesces tiny transport chunks so chunk metadata cannot grow independently of the byte limit. Removed in favour of the shared helper. - Tests imported `describe`/`it` from `@std/testing/bdd` rather than the repository module. The log window is now a separate unit so per-record charging is tested as arithmetic, and `normalizeReports` is exported so field parsing is tested directly. Both were previously only reachable through log side effects, which is what let the camel-case bug pass.
`tests/docs/guide-code-examples.test.ts` derives its expectation from `buildCSP`, so adding `report-to` and `report-uri` to the floor made the security-headers guide fail — the page prints the policy verbatim and cannot drift from what is served. Adds the two directives, the `Reporting-Endpoints` header that defines the group they name, and a section covering what is recorded, that query strings are stripped, and why the endpoint is exempt from `security.auth` and `security.csrf`. Regenerates the security API reference for `isCspReportRequest`.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/security/http/auth.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the sibling relative import.
This file imports a same-directory sibling. Use
./csp-report-endpoint.tsinstead of a#veryfront/*alias.Proposed fix
-import { isCspReportRequest } from "`#veryfront/security/http/csp-report-endpoint.ts`"; +import { isCspReportRequest } from "./csp-report-endpoint.ts";Based on learnings: use relative imports for sibling files within the same module directory.
🤖 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 `@src/security/http/auth.ts` at line 1, Update the import of isCspReportRequest in the auth module to use the sibling relative path ./csp-report-endpoint.ts instead of the `#veryfront` alias.Source: Learnings
🤖 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 `@docs/guides/security-headers.md`:
- Line 46: Replace the em dash in the security headers documentation sentence
with approved punctuation, such as a comma or semicolon, while preserving the
existing meaning and wording.
---
Nitpick comments:
In `@src/security/http/auth.ts`:
- Line 1: Update the import of isCspReportRequest in the auth module to use the
sibling relative path ./csp-report-endpoint.ts instead of the `#veryfront` alias.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f86fc59-fc3e-45cc-bced-acba90024b0e
📒 Files selected for processing (8)
docs/api-reference/veryfront/security.mddocs/guides/security-headers.mdsrc/security/http/auth.tssrc/security/http/csp-report-endpoint.tssrc/security/http/csrf/csrf-handler.test.tssrc/security/http/csrf/csrf-handler.tssrc/server/handlers/request/csp-report.handler.test.tssrc/server/handlers/request/csp-report.handler.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/api-reference/veryfront/security.md
Closes the half-finished part of the CSP work rather than leaving it sitting there.
The gap. The floor is served
Content-Security-Policy-Report-Onlyto every project that has not declaredsecurity.csp, and it carried noreport-uriorreport-to. So it did neither job: it does not enforce, so it protects nothing, and with no endpoint the violations it names reach only whoever happens to open devtools on that page. The enforcement rollout the flag exists for had no instrument — no way to answer which projects would break if we enforced? short of enforcing and waiting for complaints, which is exactly how #3417 broke ~100 projects at once.That also matters for what derivation does not cover. #3465 derives origins for
img-src,media-srcandfont-srconly, deliberately, since a wrong origin there loads an image rather than executes code.script-src,connect-srcandframe-srcare not derived — codersociety's own policy needsconnect-src https://cdn.codersociety.comandscript-src https://esm.sh, neither of which derivation would supply. Those are precisely the projects that would break on a flip, and precisely what reports would name first.What this adds.
POST /_vf/csp-report, named in the policy viareport-toand the deprecatedreport-uri(still the only spelling several shipping browsers honour), with the group defined inReporting-Endpoints.The endpoint is unauthenticated by nature — browsers post to it without credentials, and so can anyone else — so nothing in the body is trusted: read against a byte budget off the stream rather than buffered whole, logged fields truncated, both wire formats (legacy
application/csp-reportand the Reporting API array) normalized to one log schema, and always 204 so a hostile poster learns nothing about what was accepted. A per-window log ceiling stops a single misconfigured project from drowning the log stream, and records how many it dropped. Reports land in the existing log pipeline — no new storage, no retention decision, no dashboard.Two bugs fixed on the way
isSecurityPolicyResponseHeaderNamehad zero call sites; the only other mention was a comment explaining it was deliberately not reused. Removed.isCorsPolicyResponseHeaderNamewas declared(value: unknown): value is string. In the negative branch that tells the compiler a non-matching header name is not a string at all, collapsing an ordinarystringtonever. Nothing had tripped it because the sole caller only passed the value straight into anotherstringparameter — it surfaced the moment I called.toLowerCase()after it. Now returns a plain boolean.Self-review found five things wrong with the first version, all fixed in 4909f4b — worth listing because the first one made a claim in this description false:
req.text()buffered the entire body and only then measured it, so the 64 KiB cap was advisory: a body with nocontent-length, or a dishonest one, was already in memory by the time it was rejected. Now read against a byte budget off the stream. The declared-length check stays as a cheap early exit.String.length, which counts UTF-16 code units.sliceran beforefilter, so a batch carrying other report types could push real violations out of the window.Review found two more defects that made the endpoint useless where it mattered most (2bc232c):
AuthHandler(priority 0) andCsrfHandler(priority 5) both match all requests and run ahead of this priority-100 handler, and a browser reports a violation with neither credentials nor a CSRF token — a report is not a user action. So those projects advertised a reporting endpoint that answered 401/403 and collected nothing. Both gates now exempt this one route through a sharedisCspReportRequest, rather than expecting projects to add it toexcludePaths— making reporting another prerequisite is the mistake this work exists to undo. Safe on the terms those gates exist for: no credentials read, no state changed, 204 regardless of body.effectiveDirectiveandstatusCodein camel case; the normalizer read only the hyphenated spellings, so modern reports arrived with no directive and no status — exactly what an enforcement decision needs. My own test payload used camel case and asserted only the 204, so it exercised the bug and could not see it.Also fixed: control characters stripped from logged fields (CWE-117 log forging) and query strings dropped from URI fields, since a violating URL carries whatever the page was called with; the log window charged per record instead of per request, so a 16-violation batch can no longer write 16x the ceiling; and
readBoundedBodyremoved in favour of the existingreadBodyWithLimit, which also coalesces tiny transport chunks so chunk metadata cannot grow independently of the limit.normalizeReportsand the log window are now separate testable units. Both were previously reachable only through log side effects, which is what let the camel-case bug pass.Test that earns its place: the failure mode here is silent — a
report-tonaming a group thatReporting-Endpointsdoes not define makes the browser send nothing, and the policy still looks correct in devtools. So there is a test asserting the directive and the header agree, and that both spellings aim at one path.Does not change delivery. Still report-only for projects that declared nothing, still enforced for those that did.
VERYFRONT_CSP_ENFORCEis untouched — this is what makes flipping it an informed decision instead of a repeat of #3417.Full unit suite green (3777 passed), typecheck, lint, fmt and
docs:api-reference:checkclean.Summary by CodeRabbit
New Features
/_vf/csp-report.204 No Content.Bug Fixes
Documentation