fix(cache): security — do not edge-cache App Router responses before dynamic usage is proven - #2731
Conversation
…ing App Router responses An App Router page can only be proven non-dynamic after its stream drains: a Suspended server component may read cookies()/headers() long after the shell has flushed. The finalizers therefore stamp response cache headers with `pendingDynamicCheck: true` and, if the render turns out dynamic, skip the origin cache write. That correction only works for an origin-managed store. The Cloudflare CDN adapter keeps no origin store, so its headers alone decide whether the *shared* Workers Cache stores the page — and a header already sent cannot be retracted. A late-dynamic render could therefore leave the origin advertising `CDN-Cache-Control: public, max-age=...`, letting the edge store one user's personalized HTML or RSC payload and replay it to the next requester of the same URL (responses do not Vary on Cookie). Add `requiresProvenCachePolicy` to the CDN adapter contract. When the active adapter sets it, the App Router finalizers hold the response until the stream has proven the render's dynamic-ness, then emit a final policy: the real cacheable policy when proven static, `no-store` when dynamic. Cache-write errors fail closed. Origin-managed adapters keep the existing streaming behaviour unchanged.
|
@codex review |
commit: |
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![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: 6a487d0328
ℹ️ 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".
… the body Addresses review feedback on the proven-cache-policy path. The final headers were built from the `Cache-Control` computed before the stream drained. A `cacheLife()` or cacheable fetch resolving late can shorten that lifetime, or drop it entirely — so a route declaring 60s that resolved to 10s advertised 60s to the edge, and one that resolved to `revalidate: 0` still advertised the original cacheable policy even though the origin correctly skipped its write. The write outcome now carries the resolved policy and the final headers are derived from it, with no policy meaning no-store. The proven path also drained both tee branches into separate full strings, holding two copies of a large response plus both decoder chunk arrays. It now skips the tee entirely and reuses the single buffered body for both the cache write and the client response. Extracts the HTML cache write so the streaming and proven paths share it rather than duplicating the write logic.
…namic' into fix/cdn-shared-cache-unproven-dynamic
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db291e92b3
ℹ️ 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".
Under an adapter requiring a proven policy, the final headers were built solely from the render's resolved cache lifetime. Middleware owns singular response headers and is merged into the response before finalization, so a middleware-set "private" or "no-store" on an ISR page was replaced by a cacheable CDN policy once the render proved static, letting the shared edge store a response middleware had deliberately marked non-cacheable. The streaming path never had this gap because it feeds the response's existing Cache-Control to the adapter. Retain an existing non-shared-cacheable Cache-Control (no-store, no-cache, private) verbatim in the proven path, applying the most-restrictive-wins rule: either middleware or the render outcome alone can forbid edge caching. The adapter already clears its cacheable headers for such a policy. Both new tests fail without the guard.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c9b8efe93
ℹ️ 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".
…en policy The proven path rebuilt the edge policy from the render's resolved lifetime, so a middleware override such as "s-maxage=5" on a 60-second route was replaced by the route policy, and the edge served content 55 seconds staler than middleware asked for. The finalizer could not honor the override because middleware's value is indistinguishable from the provisional policy it must replace once both sit in the response headers. Thread the middleware Cache-Control from the render lifecycle into the finalizer. A proven-static render now advertises middleware's value verbatim, matching the ownership contract of mergeMiddlewareResponseHeaders; a dynamic render still forces no-store, since a personalized response must never reach a shared cache regardless of who asked for caching. The new test fails without the change.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # packages/vinext/src/server/app-page-cache-finalizer.ts
2 participants
Summary
Security fix. Under an edge-managed CDN cache adapter, an App Router response could be advertised to a shared CDN cache as cacheable before the render had proven it was non-dynamic. If the render turned out to be dynamic, that advertisement could not be withdrawn, so a personalized page could be stored by the edge and served to other users of the same URL.
Severity: high — cross-user disclosure of authenticated page content, reachable over ordinary public HTTP traffic on a default
vinext init --platform=cloudflaredeployment.Mechanism
An App Router page can only be proven non-dynamic once its stream drains — a Suspended server component may read
cookies()/headers()long after the shell has flushed.finalizeAppPageHtmlCacheResponse/finalizeAppPageRscCacheResponseaccount for this by stamping headers withpendingDynamicCheck: trueand, when the render turns out dynamic, skipping the origin cache write.Skipping the write is a sufficient correction only for an origin-managed store.
CloudflareCdnCacheAdapterkeeps no origin store (get()returnsnull,set()is a no-op), so its response headers alone decide whether the Workers Cache stores the page. OnceCDN-Cache-Control: public, max-age=…has left the origin there is nothing left to skip, and responses do notVaryonCookie, so the edge key is effectively the URL.Note this was a considered design decision, not an oversight — the
pendingDynamicCheckdoc comment explicitly contemplated edge adapters emitting edge-only cache headers in this state. The gap is that the design distinguished caches by location (browser vs. edge) and not by whether the store is shared between users.Change
Adds
requiresProvenCachePolicyto theCdnCacheAdaptercontract, set byCloudflareCdnCacheAdapter.When the active adapter declares it, the App Router finalizers hold the response until the stream has resolved the render's dynamic-ness, then emit a final policy:
no-storeAdapters that manage their own origin store are untouched and keep streaming with the existing
pendingDynamicCheckbehaviour, so this is opt-in per adapter and default (non-Cloudflare) behaviour is unchanged.Cost
Under the Workers Cache adapter, a cacheable App Router response is now buffered at the origin instead of streamed. This applies only to renders that reach the origin — i.e. cache misses — since the edge absorbs hits and stale-while-revalidate traffic. TTFB on a miss regresses; hit-path behaviour is unchanged.
An alternative that preserves streaming would be to gate on the existing render-observation / cache-proof machinery, emitting edge-cacheable headers only for a route with a stored negative request-API proof and streaming the first render as
no-store. That is a larger change; happy to go that route instead if preferred.Validation
tests/cloudflare-cdn-cache.test.tsassert the response contract for both outcomes: a late-dynamic render emits noCDN-Cache-Controlandno-store; a proven-static render still emits the full edge SWR policy. Verified that the first test fails onmainand the second passes both before and after — so the fix is not simply disabling edge caching.tsc --noEmitclean; existing call sites intests/app-page-cache.test.tsupdated to await the finalizers, which may now return a promise.Review path
packages/vinext/src/shims/cdn-cache.ts— the new contract and why it existspackages/cloudflare/src/cache/cdn-adapter.runtime.ts— one flagpackages/vinext/src/server/app-page-cache-finalizer.ts— the branch; the streaming path is unchanged, the proven path is newRisks
finalizeAppPage{Html,Rsc}CacheResponsenow returnResponse | Promise<Response>. Both production call sites are insiderenderAppPageLifecycle, which already returnsPromise<Response>, so no signature changes rippled outward — but this is worth a look for any out-of-tree callers.