fix(app-router): fixing cache request leaks#907
fix(app-router): fixing cache request leaks#907NathanDrake2406 wants to merge 1 commit intocloudflare:mainfrom
Conversation
commit: |
NathanDrake2406
force-pushed
the
nathan/cache-isolation-fixes
branch
2 times, most recently
from
88ec5af to
69e11ff
Compare
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid pass at three real cache-leak risks. The runtime-proxy work for force-static / error route handlers tracks Next.js's forceStaticRequestHandlers and requireStaticRequestHandlers closely (including the localhost:3000 quirk in cleanURL), and the forceStatic flag on HeadersContext is a clean way to short-circuit markDynamicUsage for stubbed APIs.
A few things worth addressing before merging:
1. The HTML cache-write skip plugs the internal ISR write but not the response cache headers. resolveAppPageHtmlResponsePolicy runs against the eager dynamicUsedDuringRender (currently app-page-render.ts:278) — i.e. before the HTML stream is consumed. When a lazy server component touches cookies() / headers() mid-stream, the new check in app-page-cache.ts:252 correctly skips the ISR write, but the response we already returned to the client carries Cache-Control: s-maxage=N, stale-while-revalidate and X-Vinext-Cache: MISS. CDN edges and shared HTTP caches will still cache the personalized HTML — same shape of leak the PR description targets. The fix needs to either swap the response's Cache-Control to no-store post-stream, or hold response headers until the cache decision is finalized.
2. No coverage for dynamic = "error" route handlers. The PR adds wrapRequireStaticNextUrl, the request.* throwing branch, and the accessError plumbing in configureAppRouteStaticGenerationContext, but every new test exercises force-static only. A test analogous to tests/app-route-handler-runtime.test.ts's force-static case — asserting request.headers, request.cookies, request.url, request.ip, request.geo, nextUrl.search, etc. throw with the route-pattern message — would lock in this behavior. Without it, regressions in the error path (e.g. accidentally falling through default) won't be caught.
3. runInRevalidationContext doesn't propagate accessError for dynamic = "error". In the route-handler regen block in app-rsc-entry.ts around 2151, __revalHeadCtx only carries forceStatic: handler.dynamic === "force-static". The route-handler path is mostly fine because runAppRouteHandler re-installs accessError via configureAppRouteStaticGenerationContext, but the page-revalidation block (line ~2308) builds an empty headers context with no accessError story at all. Worth either documenting that pages don't honor dynamic = "error" during regen, or threading an accessError in when isDynamicError is true.
4. Minor: forceStaticCookies is not sealed. Next.js wraps the empty cookies in RequestCookiesAdapter.seal(...) so set / delete throw. wrapRequest returns a bare new RequestCookies(new Headers()), so a force-static route handler that calls request.cookies.set(...) will silently succeed with no effect. Probably harmless given the rest of the response is static, but it's a behavioral divergence from upstream.
| return finalizeAppPageHtmlCacheResponse(isrResponse, { | ||
| capturedRscDataPromise: isrRscDataPromise, | ||
| cleanPathname: options.cleanPathname, | ||
| consumeDynamicUsage: options.consumeDynamicUsage, |
There was a problem hiding this comment.
Threading consumeDynamicUsage into finalizeAppPageHtmlCacheResponse correctly closes the internal-ISR-write side of the leak, but the response itself was already shaped by resolveAppPageHtmlResponsePolicy higher up in this function against the eager pre-stream dynamicUsedDuringRender. So when a lazy server component calls headers() / cookies() during stream consumption:
- the ISR write is now skipped (good),
- but the response we already streamed to the client still has
Cache-Control: s-maxage=N, stale-while-revalidateandX-Vinext-Cache: MISS, so CDN edges and shared HTTP caches will still cache the personalized HTML.
For full parity with the PR's stated goal, either rewrite the response headers to no-store from inside finalizeAppPageHtmlCacheResponse once the post-stream consumeDynamicUsage() flips, or buffer ISR-eligible HTML and decide headers + write together. The current state is a partial fix — internal cache is safe, response cache is still leaky.
| expect(tracked.request.nextUrl.search).toBe(""); | ||
| expect(tracked.request.nextUrl.searchParams.get("secret")).toBeNull(); | ||
| expect(tracked.didAccessDynamicRequest()).toBe(false); | ||
| expect(accesses).toEqual([]); |
There was a problem hiding this comment.
There's no symmetric coverage for requestMode: "error". The PR adds wrapRequireStaticNextUrl plus a throwing branch in requestHandler.get for request.headers / cookies / url / ip / geo / body methods, plus a staticGenerationErrorMessage carrying the route pattern — none of which are exercised by tests. A case mirroring this force-static test that asserts each of those access paths throws with Route /path with \dynamic = "error"` couldn't be rendered statically because it used a dynamic request API.` would lock the behavior in and protect against future fall-through-to-default regressions.
It would also be the right place to verify that request.clone() in error mode keeps throwing on access (the clone switch case re-wraps with wrapRequest, but a regression that returned target.clone directly would silently break the contract).
| case "text": | ||
| case "arrayBuffer": | ||
| case "formData": | ||
| return throwStaticGenerationError(`request.${String(prop)}`); |
There was a problem hiding this comment.
Two minor divergences from Next.js's requireStaticRequestHandlers worth a sanity check:
- Next.js does not include
ip/geoin the throwing list for require-static — they fall through to the default branch and return whatever the underlyingNextRequest.ip/georesolve to. vinext throws on them. This is fine (and arguably safer) given vinext keepsip/geoonNextRequestas a Cloudflare-compat surface, but a comment noting it's a deliberate divergence rather than a port mistake would help future readers. - Next.js throws
StaticGenBailoutError; this PR throws a plainError. vinext doesn't currently distinguish that error class anywhere, so it's a no-op for now, but if you ever wire up special handling for static-gen bailout (e.g. preventing the error from being treated as a 500 in the route-handler error reporter), a custom error class would be useful.
| forceStaticHeaders ??= new Headers(); | ||
| return forceStaticHeaders; | ||
| case "cookies": | ||
| forceStaticCookies ??= new RequestCookies(new Headers()); |
There was a problem hiding this comment.
Next.js wraps the empty cookies in RequestCookiesAdapter.seal(...) so request.cookies.set(...) / .delete(...) throw a ReadonlyRequestCookiesError rather than silently mutating an empty map. vinext returns a bare RequestCookies here, so a force-static route handler that calls request.cookies.set("x", "y") will succeed and leave the rest of the handler thinking it set a cookie. The response is static and no Set-Cookie is emitted, so it's mostly harmless, but it's a small behavioral divergence from upstream.
If you have a _sealCookies helper in shims/headers.ts, exposing it (or an equivalent in shims/server.ts) and reusing it here would line up the behavior.
| const __revalHeadCtx = { | ||
| headers: new Headers(), | ||
| cookies: new Map(), | ||
| forceStatic: handler.dynamic === "force-static", |
There was a problem hiding this comment.
For dynamic = "error" route handlers the regen context only carries forceStatic: false (since the comparison is strictly to "force-static"). The runAppRouteHandler call inside the regen will re-install accessError via configureAppRouteStaticGenerationContext, so foreground execution is correct.
The analogous revalidation block for pages (around line 2308 below) doesn't have an accessError path at all — it builds an empty __revalHeadCtx and runs the page render. For a dynamic = "error" page that ended up with a stale ISR entry (e.g. because the dynamic config changed since the entry was written), the regen would silently succeed with empty headers/cookies instead of erroring. Worth either documenting that pages don't honor dynamic = "error" during regen, or mirroring the route-handler approach and threading an accessError into the page regen headers context when isDynamicError is true.
| page, | ||
| }) => { | ||
| // Intercept the .rsc request for /about and return a 500 error. This | ||
| const targetPath = "/rsc-fetch-error-target"; |
There was a problem hiding this comment.
Unrelated to the cache-leak fix — would land cleaner as its own PR with a title that reflects the e2e test stabilization. Bundling it here makes it easy to revert one without the other if the dedicated-fixture approach turns out to interact poorly with prefetch in some browser. Not blocking.
|
Review submitted successfully at #907 (review). Summary of feedback posted:
|
NathanDrake2406
force-pushed
the
nathan/cache-isolation-fixes
branch
3 times, most recently
from
297988a to
1f97392
Compare
|
/bigbonk review |
1 similar comment
|
/bigbonk review |
NathanDrake2406
force-pushed
the
nathan/cache-isolation-fixes
branch
from
1f97392 to
b9dec59
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Solid follow-up on the prior review. The HTML-path leak that the previous bonk pass flagged is genuinely closed now: finalizeAppPageHtmlCacheResponse overrides Cache-Control on the streamed client response to no-store, must-revalidate before it gets returned, so shared caches no longer see the speculative s-maxage=N, stale-while-revalidate while the internal ISR write is still pending the post-stream consumeDynamicUsage() re-check. The new tests/app-page-cache.test.ts and tests/app-page-render.test.ts assertions pin that exactly. The dynamic = "error" route handler now has parity coverage with force-static (proxy throws on request.headers/cookies/url/ip/geo/body methods plus all nextUrl.* reads, and the clone path stays wrapped), the page-revalidation __revalHeadCtx propagates accessError for dynamic = "error", and the force-static route-handler cookies are now sealed via sealRequestCookies — items 2-4 from the prior review look addressed.
A few items still worth looking at before merge:
-
RSC response Cache-Control still leaks the post-stream dynamic decision. The HTML path now correctly rewrites
Cache-Controlon the response before returning, but the RSC path doesn't have the same treatment.buildAppPageRscResponseshapes the response withCache-Control: s-maxage=N, stale-while-revalidatebased on the eagerdynamicUsedDuringBuild, the response is returned to the client immediately, and only later doesscheduleAppPageRscCacheWritere-checkconsumeDynamicUsage()to skip the internal write. If a lazy server component touchescookies()/headers()while the RSC stream is being consumed, the internal ISR write is correctly skipped, but the response we already returned still tells shared caches to cache the personalized RSC payload fors-maxageseconds. RSC payloads are not as widely shared-cached as HTML, but for full parity with the HTML fix the same response-rewrite should apply: either tee the RSC stream and only emit headers after the post-stream check resolves, or downgradeCache-Controltono-storeon the response stream wrapper whenconsumeDynamicUsage()flips. Worth at least an explicit comment marking this as a known asymmetry rather than letting it look like the HTML fix accidentally drifted. -
ReadonlyRequestCookiesErroris duplicated.packages/vinext/src/shims/headers.tsalready definesReadonlyRequestCookiesErrorplus a_sealCookieshelper that wraps the localRequestCookies(theMap<string,string>-based one). The newsealRequestCookiesinpackages/vinext/src/shims/server.tsdefines a secondReadonlyRequestCookiesErrorwith the same message and wraps the otherRequestCookiesclass (theHeaders-based one). Same name, same message, two classes — a future change to the error message has to be made in both places or behavior diverges. Worth either consolidating into a single shared error type or at least a comment in each call site noting the deliberate duplication and why the twoRequestCookiesclasses can't be unified yet. -
Static-generation error message diverges from Next.js wording. The thrown message is
Route ${route} with \dynamic = "error"` couldn't be rendered statically because it used a dynamic request API.Next.js's equivalent (inpackages/next/src/server/request/utils.tsanderrors.jsonentry 543) interpolates the actual expression and appends a docs link:Route ${route} with `dynamic = "error"` couldn't be rendered statically because it used ${expression}. See more info here: https://nextjs.org/docs/app/building-your-application/rendering/static-and-dynamic#dynamic-rendering`. The vinext proxy already passes the expression name tothrowStaticGenerationError("request.headers")etc., so the underlying call site has the data — just the formatter discards it. Threading it into the message (and adding the docs URL) would line up with Next.js error reporting conventions and keep error-comparison tests portable. -
Unrelated e2e refactor still bundled. The
tests/e2e/app-router/rsc-fetch-errors.spec.tsrework (dedicated unlinked fixture pages,targetRscHits === 1exact assertion, real-server redirect chain instead of a route-fulfill chain) is a stabilization of pre-existing tests and isn't covered by the PR title. It's reasonable on its own merits, but co-locating it with the cache-leak fix makes a partial revert (e.g. if the dedicated-fixture approach interacts badly with prefetch in some browser) drag the cache-leak fix with it. Already noted in the previous bonk review; not blocking.
| chunks.push(decoder.decode()); | ||
|
|
||
| if (options.consumeDynamicUsage()) { | ||
| options.isrDebug?.("HTML cache write skipped (dynamic usage during render)", htmlKey); |
There was a problem hiding this comment.
RSC response Cache-Control is not rewritten the way finalizeAppPageHtmlCacheResponse rewrites the HTML response above. buildAppPageRscResponse set Cache-Control: s-maxage=${revalidateSeconds}, stale-while-revalidate based on the eager pre-stream dynamicUsedDuringBuild, returned the response to the client, and only later does scheduleAppPageRscCacheWrite re-check consumeDynamicUsage() to decide whether to skip the internal ISR write. When a lazy server component touches cookies() / headers() during RSC stream consumption:
- the ISR write is correctly skipped (good),
- but the RSC response that already streamed to the client still carries the cacheable
Cache-Control, so a shared CDN / HTTP cache will still cache the personalized RSC payload.
For parity with the HTML fix landed a few lines above (finalizeAppPageHtmlCacheResponse overrides Cache-Control to no-store, must-revalidate on the response wrapper), the same response-header rewrite should apply on the RSC side — either by teeing the RSC body and emitting no-store on the wrapper stream when the post-stream check flips, or by buffering the RSC response and finalizing headers + write together. As-is, the RSC path is a partial fix relative to the PR's stated goal.
| super( | ||
| "Cookies can only be modified in a Server Action or Route Handler. Read more: https://nextjs.org/docs/app/api-reference/functions/cookies#options", | ||
| ); | ||
| } |
There was a problem hiding this comment.
There's already a ReadonlyRequestCookiesError in packages/vinext/src/shims/headers.ts with the same message, plus a _sealCookies helper that wraps the RequestCookies defined in that file. This second ReadonlyRequestCookiesError here in shims/server.ts is identical in message but a separate class, and the sealRequestCookies helper below wraps the other RequestCookies class (the Headers-based one used by NextRequest). A future change to the error message would silently drift the two paths. Either consolidate into a single shared error type re-exported from one shim, or add a brief comment in both files noting the duplication is deliberate because the two RequestCookies classes haven't been unified yet.
| `Route ${route} with \`dynamic = "error"\` couldn't be rendered statically ` + | ||
| "because it used a dynamic request API." | ||
| ); | ||
| } |
There was a problem hiding this comment.
Next.js's analogous error formatter (see packages/next/src/server/request/utils.ts and errors.json entry 543) interpolates the specific ${expression} (e.g. request.headers, nextUrl.search) and appends See more info here: https://nextjs.org/docs/app/building-your-application/rendering/static-and-dynamic#dynamic-rendering. The proxy in app-route-handler-runtime.ts already passes the expression name to throwStaticGenerationError("request.headers") / throwStaticGenerationError("nextUrl.search") etc., so the data is available — just discarded by the current formatter, which uses a single generic "a dynamic request API" string for every access. Threading the expression in (and matching Next.js's wording + docs link) would line up with errors.json entry 543 and make the error comparable to upstream for users / tooling that pattern-match on it.
| } | |
| function getStaticGenerationErrorMessage(routePattern?: string, expression?: string): string { | |
| const route = routePattern ?? "unknown route"; | |
| const expr = expression ?? "a dynamic request API"; | |
| return ( | |
| `Route ${route} with \`dynamic = "error"\` couldn't be rendered statically ` + | |
| `because it used ${expr}. ` + | |
| "See more info here: https://nextjs.org/docs/app/building-your-application/rendering/static-and-dynamic#dynamic-rendering" | |
| ); | |
| } |
| // home page's visible Links, so a count of 0 means the test skipped the | ||
| // !ok guard path, while a count above 1 means hydration fell back to a | ||
| // post-reload .rsc fetch or entered a reload loop. | ||
| expect(targetRscHits).toBe(1); |
There was a problem hiding this comment.
This refactor (dedicated unlinked fixture target + tightened toBe(1) assertion + real-server redirect chain instead of a route.fulfill 307→500) is unrelated to the cache-leak fix in the PR title. Not blocking, but it would land cleaner as its own PR — if the dedicated-fixture approach turns out to interact poorly with browser prefetch heuristics later and needs reverting, bundling it here drags the cache-leak fix back too.
(Already raised in the previous bonk review — restating for visibility now that the PR has been re-pushed.)
|
All four inline comments and the top-level review have been posted successfully. SummaryPosted a review on PR #907 with one summary comment and four inline comments: Top-level review acknowledges that the prior bonk feedback has been mostly addressed (HTML response Cache-Control rewrite, Inline comments:
|
NathanDrake2406
force-pushed
the
nathan/cache-isolation-fixes
branch
from
b9dec59 to
c6aec62
Compare
NathanDrake2406
force-pushed
the
nathan/cache-isolation-fixes
branch
from
c6aec62 to
1419580
Compare
NathanDrake2406
changed the title
What
Fixes two App Router cache-safety issues around request-specific data:
dynamic = "force-static"/dynamic = "error"for route-handler request APIs by stubbing or throwing for directNextRequestfields andnext/headers.request.ip/request.geoas dynamic in auto mode.Why
Route handlers could previously expose request-specific IP/geo data through untracked fields, and HTML ISR could cache page output before lazy streamed Server Components had finished touching
cookies()/headers(). Both cases could put per-request output into shared cache.Validation
vp checkvp test run tests/app-route-handler-runtime.test.ts tests/app-route-handler-execution.test.ts tests/app-route-handler-cache.test.ts tests/app-route-handler-policy.test.ts tests/app-page-cache.test.ts tests/app-page-render.test.ts tests/app-page-response.test.ts tests/app-page-probe.test.ts tests/shims.test.tsvp test run tests/app-router.test.tsvp run vinext#build