Skip to content

fix(app-router): isolate interception cache variants#2585

Open
james-elicx wants to merge 1 commit into
codex/security-cdn-late-dynamicfrom
codex/fix-interception-cache-key
Open

fix(app-router): isolate interception cache variants#2585
james-elicx wants to merge 1 commit into
codex/security-cdn-late-dynamicfrom
codex/fix-interception-cache-key

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • canonicalize interception source identity consistently across matching and caching
  • isolate malformed interception contexts from reusable response variants
  • keep HTML-generated Flight payloads in their direct cache namespace
  • make interception variant fingerprints unambiguous while preserving compatibility redirects

Validation

  • targeted cache, routing, and production server suites (508 passed)
  • production Chromium interception/client-cache E2E (10 passed)
  • Cloudflare Worker navigation/RSC E2E (4 passed)
  • vinext and Cloudflare adapter builds
  • changed-file format, lint, type, and diff checks

Dependency

Stacked on #2582 for bounded edge-cache eligibility verification.

@pkg-pr-new

pkg-pr-new Bot commented Jul 10, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2585
npm i https://pkg.pr.new/create-vinext-app@2585
npm i https://pkg.pr.new/vinext@2585

commit: 39d0c08

@github-actions

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@github-actions

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 39d0c08 against base 41821b6 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 126.6 KB 126.6 KB ⚫ -0.0%
Client entry size (gzip) vinext 120.6 KB 120.6 KB ⚫ -0.0%
Dev server cold start vinext 2.68 s 2.71 s ⚫ +1.0%
Production build time vinext 2.83 s 2.82 s ⚫ -0.2%
RSC entry closure size (gzip) vinext 99.6 KB 99.6 KB ⚫ +0.1%
Server bundle size (gzip) vinext 166.3 KB 166.3 KB ⚫ +0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Malformed source contexts no longer alias the direct variant. appIsrRscKey previously produced sourceVariant = null when the context failed to normalize, collapsing a forged context onto the direct RSC key. It now emits source-invalid:<fnv1a64(raw)>, keeping malformed contexts isolated (isr-cache.ts:417-422). The raw value is hashed, so the key stays bounded. Test flip from toBe(direct)not.toBe(direct) documents the intent.

  2. HTML document requests write their Flight by-product to the direct namespace. finalizeAppPageHtmlCacheResponse now passes null for 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.

  3. Cache-busting fingerprint is now injective. Switching createCacheBustingInput from comma-join to JSON.stringify removes the six-field/seven-field aliasing and the null vs 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 actual resolveInvalidRscCacheBustingRequest fallback behavior.

Things I checked that are fine

  • No import cycle: app-interception-context-header.tsrouting/utils + normalize-path only; app-page-render-identity.ts and app-rsc-route-matching.ts import from it; isr-cache.ts imports through app-page-render-identity.ts. Clean one-directional layering.
  • Matching/caching parity: findIntercept canonicalizes sourcePathname via 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 isInterceptionMatchedUrlPath both 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 interceptionContext finalizer option or the deleted normalizeHeaderValue helper.
  • Fixtures use revalidate = 1 to 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 to normalizeAppPageInterceptionProofPathname. 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.

@ask-bonk

ask-bonk Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

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 (normalizeInterceptionContextPathname) — the core consistency improvement.

What I verified:

  • isr-cache isolation (source-invalid:<hash>) — malformed contexts no longer alias the direct RSC variant
  • finalizer fix — HTML document requests write their Flight by-product to the direct namespace (null interception context), while the RSC-request write path still correctly preserves it
  • cache-busting fixJSON.stringify makes the fingerprint injective, closing the six/seven-field aliasing; older clients get a safe 307 redirect (comment update matches actual fallback behavior)
  • No import cycles, no dead references, clean layering
  • Matching/caching parity confirmed
  • Ran targeted unit + integration suites (275 tests passed, incl. all 3 new security regression tests) and vp check (clean)

Two non-blocking notes: the thin normalizeInterceptionContextForCacheKey wrapper could be inlined later, and the source-invalid: keyspace is attacker-header-driven but bounded by TTLs (worth a mental note if KV cardinality ever matters).

github run

@james-elicx
james-elicx marked this pull request as ready for review July 10, 2026 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant