fix(security): stop caching an empty CSP derivation - #3474
Merged
Conversation
Derived origins did nothing for hosted production projects -- the projects the feature exists for. Verified against a probe project whose page references two external image origins: preview served both in `img-src`, production served neither, from the same release and the same source. `getAllSourceFiles` answers `[]` whenever its own file list is cold, warming it asynchronously afterwards. Every pod is cold for a content version on the first request after a release, so the first request derived nothing -- and that emptiness was written to the per-content-version cache, which is never revisited. The warm file list that arrived a moment later was never read, and the release served the bare floor for the life of the pod. Preview only appeared to work because its file list happened to be warm by the time it was asked. An empty read is not an answer. Files read with no origins found is immutable for a content version and still cached; nothing read is a race and is retried. This reverses a deliberate earlier decision, pinned by a test, that cached the empty result so a broken adapter would not be re-read per request. That saving could not distinguish a broken adapter from a cold one, and the cold case is the common one -- it was paid for with the feature not working at all. The cost coming back is small by construction: the empty path is a cache lookup that schedules a warmup rather than a source read, and concurrent callers still collapse onto one attempt through the in-flight map.
|
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 (2)
📝 WalkthroughWalkthroughThe derived CSP cache no longer caches empty or failed source reads. Later requests can retry source loading. Tests cover retries, valid empty derivations, retained origins, and updated failed-read load counts. ChangesDerived CSP cache behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 8, 2026
Merged
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
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.
Derived origins have never worked in production. #3465 was promoted as what closes the loop for the ~100 projects #3417 broke. It closes it in preview only.
How it was found. I created a throwaway project (
vf-csp-probe) that declares nosecurity.cspand whose page referenceshttps://images.unsplash.comandhttps://cdn.jsdelivr.net, then deployed the same release to both environments:img-src'self' images.veryfront.com cdn.veryfront.com data:cdn.jsdelivr.netimages.unsplash.com'self' images.veryfront.com cdn.veryfront.com data:Same release, same source, different answer.
Root cause.
getAllSourceFilesreturns[]whenever its own file list is cold, scheduling an asynchronous warmup. Every pod is cold for a content version on the first request after a release. That first request therefore derived nothing — andderiveOncewrote that emptiness into the per-content-version cache, which is by design never revisited, since the input is supposed to be immutable. The warm file list that arrived moments later was never consulted, and the release served the bare floor for the life of the pod. Preview only appeared to work because its file list happened to be warm by the time anything asked.The extractor itself is fine — given those files it returns exactly those origins. The release genuinely contains the source. The whole failure was one line of caching.
Fix. An empty read is not an answer. Files read with no origins found stays cached (immutable for the content version); nothing read is a race and is retried.
This reverses a deliberate decision, pinned by the test
caches the empty result too, so a broken adapter is not retried per request. That saving could not tell a broken adapter from a cold one, and the cold case is the common one — it was paid for with the feature not working at all. The cost coming back is small by construction: the empty path is a cache lookup that schedules a warmup, not a source read, and concurrent callers still collapse onto one attempt through the in-flight map. I rewrote that test to state the new contract and why it changed rather than deleting it.Consequence for the rollout:
VERYFRONT_CSP_ENFORCEmust not be flipped until this ships. The derived origins that make enforcement survivable are not being served in production today, so enforcing now would reproduce #3417 exactly.The three new tests fail against
mainand pass with the fix; I checked that explicitly. Lint, typecheck, fmt, docs and the full unit suite (3777) are green.Summary by CodeRabbit