feat(security): apply derived CSP origins so security.csp becomes an override - #3465
Conversation
…override #3461 derived a project's passive origins from its released source but nothing consumed them. This applies them, which is the change that stops a working site depending on configuration the project had no reason to know it needed. The policy is now built in three layers: the platform floor, which a project can never drop; origins derived from the project's own released source; and `security.csp` on top. A project that references its asset CDN in its own pages now loads it without writing any config, and `security.csp` becomes what it should have been -- an override for what static analysis cannot see, such as a URL assembled at runtime. Derivation is cached per content version. The input is a release's pinned sources, so the result is immutable for that version: computing it per response would read every source file on every request. The cache is bounded at 200 entries and evicts oldest-first, since every release of every project mints a key. It fails soft -- unreadable sources derive nothing, which leaves a project exactly where it was before this existed, and the empty result is cached so a broken adapter is not retried per request. `derivedCsp` is assigned, never merged. `SecurityConfig` carries an index signature, so a `security.derivedCsp` written in a project's own config would otherwise have been cloned straight through and become a second, unaudited way to widen the policy. deriveSecurityContext now overwrites it unconditionally and deletes it when absent, with a test asserting a project-declared `script-src` derived layer never reaches the served policy. An explicit `null` still wins. A project that drops a directive means it, and static analysis must not put back what was just removed. readProductionDefaults rejects any unrecognised option key by design and caught the new option immediately; `derivedCsp` is allowlisted rather than the check being loosened. Verified: full unit suite green -- 3814 passed, 28051 steps, 0 failed -- plus 7 new cache steps. deno check, lint and fmt clean.
|
Warning Review limit reached
Next review available in: 33 minutes 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 (2)
📝 WalkthroughWalkthroughProxy-mode requests derive CSP origins from released project sources. A bounded cache reuses derivations by project and content version. Security configuration replaces project-authored derived data, and CSP generation merges runtime-derived origins with baseline and project directives. ChangesDerived CSP integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant ProjectRuntimeContext
participant DerivedCspCache
participant deriveSecurityContext
participant buildCSP
Request->>ProjectRuntimeContext: resolve proxy-mode project context
ProjectRuntimeContext->>DerivedCspCache: derive origins from source files and content version
DerivedCspCache-->>ProjectRuntimeContext: return derived CSP origins
ProjectRuntimeContext->>deriveSecurityContext: pass derivedCsp origins
deriveSecurityContext-->>ProjectRuntimeContext: return security context
ProjectRuntimeContext->>buildCSP: build response CSP
buildCSP-->>Request: return security headers
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/types/server.ts (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the internal source alias.
Line 46 crosses from
src/typestosrc/security. Replace the relative import type query with#veryfront/security/http/derived-csp-origins.ts.Proposed fix
- derivedCsp?: import("../security/http/derived-csp-origins.ts").DerivedCspOrigins; + derivedCsp?: import("`#veryfront/security/http/derived-csp-origins.ts`").DerivedCspOrigins;As per coding guidelines, use
#veryfront/*for internal source imports. Based on learnings, use aliases when an import crosses a module boundary.🤖 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/types/server.ts` at line 46, Update the derivedCsp property type in the server type definition to use the `#veryfront/security/http/derived-csp-origins.ts` internal alias instead of the relative import path, preserving the existing DerivedCspOrigins type.Sources: Coding guidelines, 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 `@src/security/http/derived-csp-cache.test.ts`:
- Around line 11-28: Update getDerivedCspOrigins to cache and reuse an in-flight
promise keyed by projectScope and contentVersion before awaiting
loadSourceFiles, while preserving the resolved-value cache. Extend the “derives
once per content version” test to keep the first load pending, start a
concurrent second lookup, then resolve the load and assert loadSourceFiles runs
only once.
---
Nitpick comments:
In `@src/types/server.ts`:
- Line 46: Update the derivedCsp property type in the server type definition to
use the `#veryfront/security/http/derived-csp-origins.ts` internal alias instead
of the relative import path, preserving the existing DerivedCspOrigins type.
🪄 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: 054d515e-cea0-498c-a335-d8bc48acbde6
📒 Files selected for processing (8)
docs/api-reference/veryfront/security.mdsrc/security/http/config.tssrc/security/http/derived-csp-cache.test.tssrc/security/http/derived-csp-cache.tssrc/security/http/response/security-handler.test.tssrc/security/http/response/security-handler.tssrc/server/runtime-handler/project-runtime-context.tssrc/types/server.ts
There was a problem hiding this comment.
💡 Codex Review
veryfront-code/src/security/http/derived-csp-cache.ts
Lines 70 to 75 in 43f35c5
Cache in-flight derivations
When multiple first requests for the same release arrive concurrently, they all observe the cache miss before any caller reaches remember, so every request invokes loadSourceFiles() and scans the complete release. A deployment traffic burst can therefore multiply the expensive full-source read that this cache is intended to perform once. Store an in-flight promise per key, and replace it with the resolved value after completion.
ℹ️ 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".
…from the key Review catch: the resolved value was stored only after `await loadSourceFiles()`, so concurrent requests for the same key each read and scanned the whole source set. A key is coldest immediately after a release, when every pod serves that content version for the first time and requests arrive together -- precisely when the duplicate work is worst. An in-flight promise is now stored before the load, so the first caller does the work and the rest await its result. It is cleared only when it is still the current entry, since a later call may have started a fresh derivation under the same key after an eviction. This mirrors inFlightProjectCSS and inFlightPreparedCSSBuilds. Separately, the cache key held a literal NUL byte rather than the intended separator. NUL is a good separator -- neither component can contain one, so no scope/version pair can collide by concatenation -- but written as a raw byte it made the file binary to grep, which silently reports no matches rather than failing. It is now written as the escape sequence backslash-u-0000. Checked the concurrency test bites: reverting the dedup fails it, restoring passes. Verified: full unit suite green -- 3815 passed, 28059 steps, 0 failed.
Three review findings, all valid, and the first would have made the feature a silent no-op in production. The derivation ran after resolveAdapter returned, so outside runWithContext. MultiProjectFSAdapter resolves the tenant from AsyncLocalStorage: with no context getAdapter either throws -- getAllSourceFiles swallows that into an empty list, which would then be cached for the life of that content version -- or falls back to a default adapter, which would derive one project's origins from another project's source. The read now happens inside runWithContext with the resolved project and source identity, so it sees the right tenant or nothing. Config-less projects were skipped. Gating on `config !== undefined` excluded exactly the projects this exists for: a project with no veryfront.config.* is the one that never declared a policy. Those now get a request security context built from defaults plus their derived origins, rather than the process-wide config. Preview entries were keyed by a content version that does not move. Branch and environment versions are stable while the content under them changes, so a preview would serve a derivation from before a push until eviction. The adapter's source snapshot generation now forms part of the key. The deferred control-plane path is excluded from all of this. Those endpoints authenticate a signed operation envelope, expose no browser surface, and deliberately read no outer source; the existing boundary test caught the omission immediately. Verified: full unit suite green -- 3815 passed, 28059 steps, 0 failed.
CodeQL flagged the array `.includes` as substring sanitization. The receiver is an array so it was already exact, but asserting through a Set states it unambiguously and matches how the rest of this file spells the same contract.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/server/runtime-handler/project-runtime-context.ts (1)
362-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the new gating paths.
This change adds three behaviors: derivation is skipped when
configOutcome === "deferred", derivation applies to thehosted-absentconfig-less path, and the content version carries the snapshot suffix.src/server/runtime-handler/project-runtime-context.test.tsexists but is not part of this change. The repository guidelines require a focused test for behavior changes.Add cases for the deferred exclusion and the config-less path.
🤖 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/runtime-handler/project-runtime-context.ts` around lines 362 - 369, Add focused cases in the existing project runtime context tests for the gating logic around deriveSecurityContext: verify deferred control-plane requests skip derivation, and verify proxy-mode hosted projects with configOutcome "hosted-absent" receive a derived security context. Keep the assertions scoped to these two behaviors and use the existing test setup/helpers.Source: Coding guidelines
🤖 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/runtime-handler/project-runtime-context.ts`:
- Around line 344-355: Update the branch argument in the derivedProjectCsp call
within the runtime context flow to match the hosted config source precedence:
prefer reqCtx.branch, then projectRes.parsedDomain?.branch, and finally the
default "main" value. Keep this consistent with the existing config path so
derivation uses the branch that produced adapterRes.config.
- Around line 560-580: Update the CSP-loading flow around the local fs cast and
run function to use isExtendedFSAdapter and the shared underlying capability
type instead of an inline cast. Declare getSourceSnapshotVersion as
asynchronous, await its result before constructing contentVersion, and preserve
the existing fallback when no snapshot version is available.
---
Nitpick comments:
In `@src/server/runtime-handler/project-runtime-context.ts`:
- Around line 362-369: Add focused cases in the existing project runtime context
tests for the gating logic around deriveSecurityContext: verify deferred
control-plane requests skip derivation, and verify proxy-mode hosted projects
with configOutcome "hosted-absent" receive a derived security context. Keep the
assertions scoped to these two behaviors and use the existing test
setup/helpers.
🪄 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: 7671c323-5055-4df4-954c-903fa9cfb894
📒 Files selected for processing (1)
src/server/runtime-handler/project-runtime-context.ts
…th does Review catch. The hosted config source context resolves the branch as `reqCtx.branch ?? parsedDomain.branch ?? "main"`; the derivation had the first two the other way round. When both are present and differ, derivation reads source for one branch while the config -- and the rendered page -- come from another, so the derived origins describe content that was never served. Also reuse `isExtendedFSAdapter` rather than restating the adapter's capability surface as an inline structural type, so a change to that contract reaches this call site through the type system instead of silently missing it. Verified: full unit suite green -- 3815 passed, 28059 steps, 0 failed.
|
Thanks — already fixed, in
const pending = inFlight.get(key);
if (pending) return pending;
const derivation = deriveOnce(key, lookup);
inFlight.set(key, derivation);
return derivation.finally(() => {
// Clear only our own entry: after an eviction a later call may already
// have started a fresh derivation under the same key.
if (inFlight.get(key) === derivation) inFlight.delete(key);
});Agreed on the severity reasoning, and it is worth noting the timing is worse than "a traffic burst": the key is coldest immediately after a release, when every pod serves that content version for the first time and requests arrive together — precisely when the duplicate full-source read costs most. Covered by a test that holds the first load pending, starts a second lookup, then resolves — asserting one source read. I checked the test actually bites: reverting the dedup fails it, restoring passes. Full unit suite green at head: 3815 passed, 28059 steps, 0 failed. |
#3461 derived a project's passive origins from its released source but nothing consumed them. This applies them — the change that stops a working site depending on configuration the project had no reason to know it needed.
Layering
A project referencing its asset CDN in its own pages now loads it with zero config.
security.cspbecomes what it should have been: an override for what static analysis cannot see — a URL assembled at runtime, a CMS field, an env var.Concretely, this is what the ~100 projects the CSP floor broke actually needed. Report-only (#3455) stopped the bleeding; this removes the cause.
Caching
Derivation reads every source file a release pins — fine once per release, absurd per response. Keyed on project scope + content version, since a release's sources are immutable for that version. Bounded at 200 entries, oldest-first eviction, registered with the memory profiler.
Fails soft: unreadable sources derive nothing, leaving a project exactly where it was before this existed. The empty result is cached too, so a broken adapter isn't retried on every request.
Two decisions worth reviewing
derivedCspis assigned, never merged.SecurityConfigcarries an index signature, so asecurity.derivedCspwritten in a project's own config would have been cloned straight through and become a second, unaudited way to widen the policy.deriveSecurityContextnow overwrites it unconditionally and deletes it when absent. There's a test asserting a project-declaredscript-srcderived layer never reaches the served policy.An explicit
nullstill wins. A project that drops a directive (fontSrc: null) means it — derivation does not put back what was just removed.Note
readProductionDefaultsrejects any unrecognised option key by design and caught the new option immediately. I allowlistedderivedCsprather than loosening the check.Verification
Full unit suite green — 3814 passed, 28051 steps, 0 failed — plus 7 new cache steps covering per-version derivation, tenant isolation, soft failure, and eviction.
deno check,lint,fmtclean; API reference regenerated.Follow-up
With this live,
VERYFRONT_CSP_ENFORCEcan flip and end the report-only rollout — projects will no longer need config to keep working.Summary by CodeRabbit
New Features
Bug Fixes
Documentation