fix(security): derive CSP origins on release-backed content - #3487
Merged
Conversation
Production logs from #3482 named it on the first try. Preview derived from "environment:preview" with fileCount 2 and originCount 2; production read no sources at all from "release:<id>", every request, for every hosted project. The failure was specific to release-backed content, not to derivation. Two causes, both visible in that one log line. `getAllSourceFiles` answers empty on a cache miss and schedules the fetch in the background. That is right for callers who can proceed without the list, but nothing else populates it for a release-backed context, so the read was empty on every request for the life of the process. It now waits for the fetch it just started -- opt-in, because making every caller wait also pulls CSS pregeneration into the request, which a test correctly caught. The cache key contained the literal "[object Promise]": the multi-project wrapper's `getSourceSnapshotVersion` is async and was being template-stringified. Every content version of every project collapsed to one key, so a push to a preview would have served the previous derivation. This is the third attempt at this bug. #3474 fixed real negative caching and #3484 fixed a real missing initialization; neither was the cause. The difference this time is that the answer came from production logs rather than from me reasoning about which layer looked suspicious -- which is what #3482 was for.
📝 WalkthroughWalkthroughThe adapters now return warmup file data and support optional warmup waits. Runtime CSP derivation awaits snapshot versions and waits for release-backed sources. Tests verify reloading when the snapshot version changes. ChangesSource warmup and CSP derivation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectRuntimeContext
participant MultiProjectFSAdapter
participant VeryfrontFSAdapter
participant FileListCache
ProjectRuntimeContext->>ProjectRuntimeContext: await source snapshot version
ProjectRuntimeContext->>MultiProjectFSAdapter: getAllSourceFiles({waitForWarmup: true})
MultiProjectFSAdapter->>VeryfrontFSAdapter: forward warmup option
VeryfrontFSAdapter->>FileListCache: fetch or retry source files
FileListCache-->>VeryfrontFSAdapter: warmed source files
VeryfrontFSAdapter-->>ProjectRuntimeContext: return source files
ProjectRuntimeContext-->>ProjectRuntimeContext: derive CSP with snapshot-aware key
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 30cefedf07
ℹ️ 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".
Two review findings, both real. The warmup fetched the files and then the reread depended on them having been stored. Caching can be disabled outright, and a backend write can fail, in which case the fetch succeeded and the caller still saw nothing -- the same empty derivation on every request that this PR exists to fix. The warmup now resolves with what it fetched and the caller takes that, falling back to the cache only when it has nothing. The snapshot test asserted nothing: it looped over an array that was never populated, and it used two release ids, whose differing prefixes made the key distinct even with the promise stringified. So it would have passed against the bug. Replaced with a fixed release identity and a moving snapshot, which is the only shape that can see it, and confirmed it fails when the await is removed. That is the third vacuous assertion found in this area, after the dead literals in the skill runtime tests and the camel-case payload that exercised its own bug without seeing it. This one was mine.
![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 `@src/platform/adapters/fs/veryfront/adapter.ts`:
- Around line 711-720: Update getAllSourceFiles so failures from
this.cache.setAsync are caught and logged separately while preserving and
returning the successfully fetched files. Keep the existing warmup/read-failure
handling that returns null, and add a focused test covering a rejecting cache
write and verifying the fetched files are still returned.
- Around line 1076-1091: Bind the warmup awaited in the file-list read path to
the captured cacheKey instead of using the mutable singleton
this.fileListWarmupPromise. Update warmup scheduling and consumption around the
relevant file-list methods to store per-key promises or use a key-tagged handle,
and only apply fetched files when the key matches; otherwise read the requested
key’s cache. Add a test covering an interleaved context switch and verifying CSP
origins use the original source snapshot.
🪄 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: 4813ed3d-f68f-4fa7-a7e7-de2272ac657e
📒 Files selected for processing (4)
src/platform/adapters/fs/veryfront/adapter.tssrc/platform/adapters/fs/veryfront/multi-project-adapter.tssrc/server/runtime-handler/derive-project-csp.test.tssrc/server/runtime-handler/project-runtime-context.ts
Comment on lines
+711
to
+720
|
|
||
| return files; | ||
| } catch (error) { | ||
| logger.warn("File list warmup failed", { | ||
| reason, | ||
| cacheKey: effectiveCacheKey, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
|
|
||
| return null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve fetched files when the cache write fails.
If this.cache.setAsync rejects on Line 696, this catch returns null after a successful fetch. getAllSourceFiles then cannot use the fetched files and can only retry the failed cache read. CSP derivation remains empty in the cache-write-failure case that this change must support.
Catch and log cache-write failures separately. Continue with files. Add a focused test with a rejecting cache write.
🤖 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/platform/adapters/fs/veryfront/adapter.ts` around lines 711 - 720, Update
getAllSourceFiles so failures from this.cache.setAsync are caught and logged
separately while preserving and returning the successfully fetched files. Keep
the existing warmup/read-failure handling that returns null, and add a focused
test covering a rejecting cache write and verifying the fetched files are still
returned.
Comment on lines
+1076
to
+1091
| let files = cached?.files; | ||
|
|
||
| // A miss schedules a warmup and returns immediately, which is right for | ||
| // callers that can proceed without the list. This one cannot: nothing else | ||
| // populates it for a release-backed context, so returning early meant the | ||
| // list was empty on every request for the life of the process. Wait for the | ||
| // fetch this read just started, then look again. | ||
| if (options.waitForWarmup && cacheKey && !files?.length && this.fileListWarmupPromise) { | ||
| // Take what the fetch returned rather than re-reading the cache: with | ||
| // caching disabled, or a failed backend write, the cache keeps nothing | ||
| // and correctness would depend on a write that never happened. | ||
| const fetched = await this.fileListWarmupPromise; | ||
| files = fetched?.length | ||
| ? fetched | ||
| : await this.cache.getAsync<{ path: string; content?: string }[]>(cacheKey); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Bind the waited warmup to cacheKey.
cacheKey is captured before this block, but this.fileListWarmupPromise is a mutable singleton. If another content context starts a warmup before Line 1087, this call can await that other warmup and return its files for the original cache key. This can derive CSP origins from the wrong source snapshot.
Store warmups by cache key, or return a key-tagged warmup handle from scheduling and consume it only when its key matches cacheKey. Add an interleaved context-switch test.
🤖 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/platform/adapters/fs/veryfront/adapter.ts` around lines 1076 - 1091, Bind
the warmup awaited in the file-list read path to the captured cacheKey instead
of using the mutable singleton this.fileListWarmupPromise. Update warmup
scheduling and consumption around the relevant file-list methods to store
per-key promises or use a key-tagged handle, and only apply fetched files when
the key matches; otherwise read the requested key’s cache. Add a test covering
an interleaved context switch and verifying CSP origins use the original source
snapshot.
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
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 diagnostics from #3482 named this on the first request after deploy:
Never once a success for a
release:context — same for codersociety and tomcode. The failure is specific to release-backed content, not to derivation.Cause 1 — the read never waits.
getAllSourceFilesanswers empty on a cache miss and schedules the fetch in the background. Correct for callers that can proceed without the list; fatal here, because nothing else populates it for a release-backed context, so the read was empty on every request for the life of the process. It now waits for the fetch it just started.Made opt-in rather than changing it for everyone: waiting unconditionally also pulls CSS pregeneration into the request. A test (
does not pregenerate CSS during branch cache warmup) caught that when I first did it the blunt way, so only CSP derivation opts in.Cause 2 — the cache key contained
[object Promise]. The multi-project wrapper'sgetSourceSnapshotVersionis async and was being template-stringified. Every content version of every project collapsed to one key, so a push to a preview would have served the previous derivation. Now awaited, and the test wrapper is async like the real one.This is the third attempt. #3474 fixed real negative caching, #3484 fixed a real missing initialization, and I claimed each would close this out. Neither was the cause. What changed is where the answer came from: production logs, not me reasoning about which layer looked suspicious. That is what #3482 was for, and it worked.
Gate green by exit code: lint,
lint:test-typecheck, typecheck, fmt, docs, full unit suite.Verification is still one curl after promotion —
vf-csp-probe.production.veryfront.comgainingimages.unsplash.comandcdn.jsdelivr.netinimg-src— and if it fails again the logs will say why without another guess.Summary by CodeRabbit