feat(security): enforce what a project declared, report what it did not - #3489
Merged
Conversation
Enforcement was all-or-nothing and hinged on whether `security.csp` existed at all. So a project adding one image origin to unblock an asset also bound `script-src` across its site, and would break the moment it loaded a third-party script it had never listed. Allowing a CDN and binding script execution are different decisions and should not share a lever. They are separated per directive now. A declared directive is enforced, with the project's own sources in it; a directive the project never mentioned keeps reporting. Both policies are served together, which browsers apply independently, so violations stay visible for everything that does not yet bind. `object-src 'none'` and `base-uri 'self'` are enforced for every project regardless. Both close real injection routes -- `<object>` payloads and a `<base>` tag rewriting every relative URL -- and neither has a use a site would notice losing. `form-action` and `frame-ancestors` are deliberately excluded: projects post forms to third parties and are legitimately embedded. Removes VERYFRONT_CSP_ENFORCE. It existed to end a staged rollout that is not going to happen: derivation covers `img-src`, `media-src` and `font-src`, and cannot cover `script-src`, `connect-src` or `frame-src`, because deriving script origins from source would grant execution to whatever a scan turned up. A flag that will never be turned on is scaffolding implying a plan that does not exist. `VERYFRONT_CSP` remains as the ops-level full override. No project-level opt-in flag was added: nothing needs one yet, and the day something does is the day to design it against a real requirement.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe security handler now delivers the complete CSP as report-only by default and emits an enforced companion policy containing required and explicitly configured directives. Tests, request handlers, integration checks, and documentation now reflect the dual-header behavior. ChangesCSP delivery and documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RequestHandler
participant SecurityHandler
participant Response
RequestHandler->>SecurityHandler: compute CSP policies
SecurityHandler->>Response: set full report-only CSP
SecurityHandler->>Response: set enforced companion CSP
Response-->>RequestHandler: return nonce-aligned headers
Possibly related PRs
Suggested reviewers: 🚥 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: 2
🤖 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`:
- Around line 46-48: Update the security headers documentation to accurately
describe buildEnforcedCSP: object-src and base-uri are always enforced, every
directive declared in security.csp—including form-action and frame-ancestors—is
enforced, and undeclared directives remain report-only. Replace the “cost you
nothing” wording with compatibility guidance covering object/embed/applet
blocking and cross-origin base elements; document that security.csp adds allowed
sources while VERYFRONT_CSP replaces the complete policy.
In `@src/security/http/response/security-handler.ts`:
- Around line 341-343: Update the declared-directives construction near the
`declared` Set so CSP entries whose value is `undefined` are filtered out before
`toCspDirectiveName` maps their keys. Preserve enforcement for configured values
and add a regression test covering an `undefined` directive such as `scriptSrc`.
🪄 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: cfc35486-2275-4a5d-9787-019cdc0d722b
📒 Files selected for processing (7)
docs/api-reference/veryfront/security.mddocs/guides/security-headers.mdsrc/security/http/response/security-handler.test.tssrc/security/http/response/security-handler.tssrc/server/handlers/request/openapi-docs.handler.test.tssrc/server/handlers/request/rsc/index.test.tssrc/server/handlers/request/static.handler.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d206e1829
ℹ️ 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".
Four review findings, one of which would have blocked working sites. CSP resolves a missing directive by walking a fallback chain, so a policy carrying `script-src` and no `worker-src` does not leave workers unconstrained -- it constrains them by `script-src`. Emitting a subset of directives is therefore not the same as emitting those directives alone. A project declaring only `scriptSrc` would have found its `blob:` workers blocked by an enforced `script-src` that never mentioned them, while the reported `worker-src` said they were fine. The companion policy now carries the fallback closure of whatever it enforces, with the values the reported policy gives those directives, so declaring one directive cannot tighten another. `undefined` is now treated as absent when deciding what was declared, matching what the merge already does. Without it `scriptSrc: undefined` enforced a merged `script-src` the project never configured. The integration suite's `readCsp` preferred the enforced header, which now carries only the binding subset, so its nonce and policy-content assertions were reading the wrong header. The guide claimed the always-enforced pair "costs you nothing" and omitted that declaring `formAction` or `frameAncestors` binds them too. It now says what each blocks and states the contract as implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 53: Update the paragraph describing buildEnforcedCSP enforcement to say
that directives with defined values in security.csp are enforced, while
directives set to undefined are treated as undeclared and remain report-only.
Preserve the existing examples and explanation about independently deciding
which directives to bind.
- Line 46: Update the security-headers documentation text to qualify that the
companion enforced Content-Security-Policy header is added by default, and
explicitly note that setting VERYFRONT_CSP replaces it with the override header
without adding the companion enforced header.
🪄 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: a906d494-1353-43ef-89bb-59a463d1a9fd
📒 Files selected for processing (5)
docs/api-reference/veryfront/security.mddocs/guides/security-headers.mdsrc/security/http/response/security-handler.test.tssrc/security/http/response/security-handler.tstests/integration/server/production-server.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/api-reference/veryfront/security.md
- src/security/http/response/security-handler.ts
- src/security/http/response/security-handler.test.ts
Two inaccuracies in the section I added. "For every project" was wrong: `VERYFRONT_CSP` replaces the policy wholesale and is served enforced on its own, with neither the reported floor nor the always-enforced pair beside it. It is the default that is described here, not an invariant. "Anything you declare" was wrong in the same direction: a directive written as `undefined` counts as unconfigured, matching what the merge does, so it keeps reporting rather than binding. The text now asks for a value rather than a mention.
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.
The problem. Enforcement was all-or-nothing and hinged on whether
security.cspexisted:So a project adding one image origin to unblock an asset also bound
script-srcacross its site, and would break the moment it loaded a third-party script it had never listed. Allowing a CDN and binding script execution are different decisions; they should not share a lever.The change. Separated per directive. A declared directive is enforced, with the project's own sources in it. A directive the project never mentioned keeps reporting. Both policies are served together — browsers apply an enforced and a report-only policy independently — so violations stay visible for everything that does not yet bind.
object-src,base-uricsp: { imgSrc: [...] }img-srcwith those originsVERYFRONT_CSPEnforced for everyone:
object-src 'none'andbase-uri 'self'. Both close real injection routes —<object>/<embed>payloads, and a<base>tag rewriting every relative URL on the page — and neither has a use a site would notice losing. A report-only policy protects nothing, so the directives that are safe to bind unconditionally now do.Deliberately not enforced:
form-action(projects post forms to HubSpot and similar — tomcode's own derived origins includeapi-eu1.hsforms.com) andframe-ancestors(projects are legitimately embedded).Removes
VERYFRONT_CSP_ENFORCE. It existed to end a staged rollout that is not going to happen. Derivation coversimg-src,media-srcandfont-src, and cannot coverscript-src,connect-srcorframe-src— deriving script origins from source would grant execution to whatever a scan turned up, so anyone landing a URL in project source would have it allowlisted. A flag that will never be turned on is scaffolding implying a plan that does not exist.VERYFRONT_CSPremains as the ops-level full override.No project-level opt-in flag. I built one (
cspEnforce) and removed it before pushing: nothing needs it yet, zero projects have deliberately asked for enforcement, and the two that had it got there by accident. The day something needs it is the day to design it against a real requirement.Correction to something I claimed earlier: I said this rule was "the likeliest way a project broke itself". That was speculation — codersociety was broken by #3417 shipping the floor enforced for everyone, not by this. The intent gap is real; my evidence for that particular story was not.
Lint,
lint:test-typecheck, typecheck, fmt, docs and the full unit suite green by exit code. Three handler tests read the nonce out of the enforced header and now read the reported one — worth noting that no production code parses the nonce back out of a header, so this was test-only.Summary by CodeRabbit
New Features
Documentation
Bug Fixes