fix(app-router): isolate interception cache variants#2585
Conversation
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 |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: isolate interception cache variants
I reviewed the full diff (source + tests + fixtures), ran the targeted unit suites (isr-cache, app-rsc-route-matching, app-rsc-request-normalization, app-rsc-cache-busting — 189 passed), the production/dev/CDN integration suites (86 passed including all three new security regression tests), and vp check on the seven changed source files (format/lint/type clean). This PR looks correct and well-tested. No blocking issues.
What the change does (and why it's sound)
Three related cache-poisoning vectors around interception variants are closed, and all three now route through a single canonicalizer (normalizeInterceptionContextPathname), which is the key consistency win:
-
Malformed source contexts no longer alias the direct variant.
appIsrRscKeypreviously producedsourceVariant = nullwhen the context failed to normalize, collapsing a forged context onto the direct RSC key. It now emitssource-invalid:<fnv1a64(raw)>, keeping malformed contexts isolated (isr-cache.ts:417-422). The raw value is hashed, so the key stays bounded. Test flip fromtoBe(direct)→not.toBe(direct)documents the intent. -
HTML document requests write their Flight by-product to the direct namespace.
finalizeAppPageHtmlCacheResponsenow passesnullfor the interception context (app-page-cache-finalizer.ts:157), and the HTML-triggered background regen does the same (app-page-cache.ts:437). Correctly, the RSC-request write path (scheduleAppPageRscCacheWrite,finalizer.ts:284) still preserves the interception context — so the HTML/RSC namespace split is deliberate and consistent across both write sites. -
Cache-busting fingerprint is now injective. Switching
createCacheBustingInputfrom comma-join toJSON.stringifyremoves the six-field/seven-field aliasing and thenullvs literal"0"collision (app-rsc-cache-busting.ts:191-195). Older clients minted under the comma format simply miss all accepted hashes and get a 307 to the canonical URL — the comment update from "requires accepting the previous hash during rollout" to "safely redirected" matches the actualresolveInvalidRscCacheBustingRequestfallback behavior.
Things I checked that are fine
- No import cycle:
app-interception-context-header.ts→routing/utils+normalize-pathonly;app-page-render-identity.tsandapp-rsc-route-matching.tsimport from it;isr-cache.tsimports throughapp-page-render-identity.ts. Clean one-directional layering. - Matching/caching parity:
findInterceptcanonicalizessourcePathnamevia the same helper before matching (app-rsc-route-matching.ts:141-145), so a source that matches a route is guaranteed to key identically in ISR. - Stricter proof pathname: the new helper applies
isInterceptionMatchedUrlPathboth before and after normalization, so shapes that only become invalid post-normalization (//, query, fragment) are now rejected — a net hardening over the prior single check. - No dead references left from the removed
interceptionContextfinalizer option or the deletednormalizeHeaderValuehelper. - Fixtures use
revalidate = 1to open the stale-regen window the production security test depends on; marker strings match assertions.
Minor (non-blocking) observations
normalizeInterceptionContextForCacheKey(isr-cache.ts:392) is now a thin one-line pass-through tonormalizeAppPageInterceptionProofPathname. Fine to keep for the named seam, but it could be inlined later.- The
source-invalid:prefix means every distinct malformed raw string gets its own cache entry. That's the correct isolation choice, but it is an unbounded-cardinality keyspace driven by an attacker-controlled header. Given ISR TTLs and that these entries never serve as the direct variant, the risk is bounded — worth a mental note if cache-entry cardinality ever becomes a KV cost concern.
Nice work — the regression tests (especially the seven-field aliasing test and the HTML-populates-RSC-variant test) capture exactly the vectors being closed.
|
Review posted to PR #2585. Summary of my findings: No blocking issues. The PR correctly closes three related interception-cache-poisoning vectors, and all three now route through a single canonicalizer ( What I verified:
Two non-blocking notes: the thin |
Summary
Validation
Dependency
Stacked on #2582 for bounded edge-cache eligibility verification.