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 0 improved · 0 regressed · 6 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".
| readStreamAsText(streamForClient), | ||
| ]); | ||
| const headers = new Headers(response.headers); | ||
| applyProvenCdnHeaders(headers, renderedCacheControl, dynamicUsed, options.getPageTags(), { |
There was a problem hiding this comment.
When a streamed render tightens its policy through a late cacheLife() or cacheable fetch—for example, a route-level revalidate of 60 seconds resolving to 10—resolveAppPageCacheWritePolicy() computes the effective lifetime, but the outcome retains only dynamicUsed. This call therefore sends the pre-stream renderedCacheControl to the CDN, advertising 60 seconds rather than 10; if the resolved policy is invalid or non-cacheable, it can still advertise the original cacheable policy. Return the resolved policy with the write outcome and build the final HTML/RSC headers from that policy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 92d9d45.
Verified the two cases: a cacheLife() resolving revalidate: 60 → 10 was advertising 60s to the edge, and one resolving to revalidate: 0 still advertised the original cacheable policy while the origin correctly skipped its write — so the edge would cache a page the framework had decided not to cache.
The write outcome now carries the resolved policy and the final headers derive from it, with a null policy meaning no-store. One thing worth noting for anyone reading the fix: revalidate = false reaches the finalizer as Infinity (via resolveAppPageCacheWriteRevalidateSeconds), which is a valid policy rather than an absent one, so that case maps to STATIC_CACHE_CONTROL instead of falling into the no-store branch — otherwise this would have silently broken force-static edge caching.
Two behaviour tests added, both confirmed to fail against the previous implementation.
| const [{ dynamicUsed }, html] = await Promise.all([ | ||
| cachePromise, | ||
| readStreamAsText(streamForClient), | ||
| ]); |
There was a problem hiding this comment.
For a large App Router response under an adapter requiring a proven policy, this drains both tee branches into independent full strings: cachePromise already holds cachedHtml, while readStreamAsText(streamForClient) decodes and joins a second copy before any response is returned. The duplicated body and decoder chunk arrays can exhaust a Worker's memory on sufficiently large HTML responses; include the buffered HTML in the cache outcome and reuse it for the returned response instead of buffering the client branch again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 92d9d45.
Rather than threading the buffered HTML through the outcome, the proven path now skips the tee altogether — cdnRequiresProvenCachePolicy() is known up front, so it branches before response.body.tee() and reads the body once, reusing that single string for both the cache write and the client response. That drops the second copy and the second decoder chunk array entirely instead of just deduplicating the result.
The streaming path is untouched and still tees. Extracted the HTML cache write into a shared helper so the two paths do not duplicate the write logic.
… 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
1 participant
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.