Skip to content

fix(caching/ppr): audit remediation — redirect guard, host key, base64 bound, SSR capture hardening, +docs/diagnostics - #685

Merged
ivogt merged 8 commits into
mainfrom
advisor/caching-ppr-fixes
Jul 5, 2026
Merged

ivogt merged 8 commits into
mainfrom
advisor/caching-ppr-fixes

Conversation

@ivogt

@ivogt ivogt commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Problem statement

An audit of the caching / cache-tags / revalidation, PPR + render-pipeline, and PPR×action subsystems surfaced a set of correctness, robustness, and coverage gaps. This PR lands the eight that were safe to fix now; each is independent and file-disjoint. Deferred/held items (a loader-HIT optimization that proved unsafe as a skip, two investigate-first hypotheses, and DRY consolidations) are tracked in #684.

Every finding was adversarially re-verified against the current code before a fix was written, and the one behavior-sensitive claim (SSR abort discrimination) was confirmed empirically against real react-dom/static.edge.

What's included

Fix Consumer-observable problem Change
Shell-HIT redirect guard A PPR shell HIT on a route that flipped to redirecting mid-TTL emitted <script>location.replace(<raw match.redirect>)</script> into a committed 200 body — bypassing the 3xx guardOutgoingRedirect chokepoint, so a caller-influenced target was an open-redirect. Route the target through the shared resolveSameOriginRedirect (extracted as resolveShellHitRedirectTarget); cross-origin/unsafe neutralizes to the basename root.
Document-cache host key On one function serving multiple domains, VercelCacheStore/MemorySegmentCacheStore served tenant A's cached /pricing HTML to tenant B (no host in the key; only CFCacheStore compensates internally). Prefix url.host on the default document-cache key, mirroring cache-scope.getCacheKeyBase.
Vercel base64 bound A large cached Response/PPR-shell body could throw RangeError inside putResponse/putShell (32768-arg spread) and degrade silently to no-cache. Delegate encode+decode to the import-pure, chunk-capped cf-base64.ts (byte-identical, no cold-start).
CF no-TTL marker warning With tagInvalidationTtl unset (default), high-cardinality tags accumulate permanent KV markers with no reaper — a silent, growing cost. Warn once per namespace at the invalidateTags batch entry, pointing at the sizing rule.
cacheTag() docs Four skills + two design docs taught cacheTag() as "use cache"-only, after the code shipped a render-callable form (#648) — consumer agents couldn't discover the canonical PPR-shell invalidation lever. Correct all six; fix the self-contradicting out-of-scope bullet.
Navigation-loading prod e2e Loading-state coverage ran dev-only; check:e2e-parity is warn-only, so the gap never failed CI — a prod-only regression in the partial-update commit/transition selection could ship green. Add (production) siblings via extracted spec functions; 10 production tests pass locally.
Untagged-bake + action-drop diagnostics Bake-lane data frozen into a shell with no tag is un-evictable except by TTL (an action refresh touches the client only) — silently; and an action-mode missing-segments refetch dropped with zero diagnostic. Dev warning once-per-shell-key for the untagged bake; debugLog on the intentional action drop.
SSR capture hardening captureShellHTML masked any AbortError-named throw (a component's own fetch/AbortController cancellation) as a retryable null no-shell, hiding real failures; maxWaitMs didn't bound the bootstrap load; a reader leaked on the mid-read abort path. Discriminate the deliberate abort by object identity (verified against real react-dom); arm the deadline before the bootstrap load; add try/finally reader cleanup.

Before vs. after (representative — the two consumer-facing ones)

Render-callable cacheTag() — the docs now show the shipped form:

// A plain server component makes the PPR shell tag-invalidatable with NO
// "use cache" in its tree — the tag rides ctx._requestTags onto the shell entry.
function CampaignBanner() {
  cacheTag("campaign:spring");
  return <aside>Spring sale</aside>;
}
// revalidateTag("campaign:spring") then evicts that shell.

Multi-tenant document cache — before, getResponse("/pricing:html") collided across hosts; after, the default key is a.example/pricing:html vs b.example/pricing:html. A consumer supplying a custom keyGenerator still owns their own namespacing (JSDoc warns to include host).

Semantics table (behavior that branches)

Case Before After
Shell-HIT redirect, cross-origin target inline location.replace(external) neutralized to basename root
Shell-HIT redirect, same-origin target passthrough passthrough (unchanged)
Document cache, same host+path HIT HIT (unchanged)
Document cache, different host, same path wrong-tenant HIT isolated MISS
SSR capture, our own abort null no-shell null no-shell (unchanged; by identity)
SSR capture, genuine AbortError-named throw masked → null propagates to reportCacheError
CF invalidateTags, no tagInvalidationTtl silent unbounded markers one warning/namespace (behavior unchanged)

Test plan

  • pnpm run typecheck (turbo, 16/16 packages) — exit 0
  • pnpm run test:unit:all (unit + rsc, every package incl. cloudflare-basic / vite-rsc-demo dogfood suites) — all pass; router package 319 files / 4979 tests
  • pnpm run lint (oxlint --deny-warnings) — 0/0
  • pnpm run format — clean
  • pnpm check:e2e-bucketing — OK (lefthook-verified on every commit)
  • Navigation-loading (production) project — 10/10 pass locally
  • New tests pin each fix (all proven red-before-green):
    • shell-hit-redirect-guard.test.ts — cross-origin→root, protocol-relative→off-host, same-origin passthrough (reverting the helper → 4 failed)
    • document-cache.test.ts — cross-host isolation MISS (reverting the host prefix → served the wrong tenant's body)
    • vercel-cache-store.test.ts — >32 KB high-byte body round-trips without overflow
    • cf-cache-store-tags.test.ts — warns exactly once per namespace; no warn when TTL set
    • shell-capture-untagged-bake.test.ts — warn once per key; no warn when tagged / ppr.tags / hole-only / production
    • shell-handlers.test.tsx — a genuine AbortError-named throw propagates (was masked); hung bootstrap degrades bounded

Notes

  • SSR abort discrimination was validated against real react-dom/static.edge: our own controller.abort(reason) resolves (holes postpone / trivial prelude → sanity gate), and a genuine fatal throw rejects with the thrown error (!== abortReason → propagates). The modified abort itself rejects fake exercises a defensive branch; the load-bearing red→green is the genuine-throw propagation.
  • Document-cache and CF no-TTL are the only two with an operational footprint: the host-key change cold-starts document-cache entries once on deploy; the CF warning is diagnostic-only.
  • Follow-ups (DRY consolidations, the deferred shell-redirect e2e, held loader-HIT redesign, two investigate plans, SSR-03/04) are tracked in Follow-ups: caching/PPR/render-pipeline audit (DRY consolidations, deferred coverage, held/investigate items) #684.
  • Held out of this PR: the hole-free loader-HIT skip (unsafe — executeLoaderData has unconditional telemetry/tag/interceptor side effects the HIT path relies on; needs a capture-time-replay redesign).

ivogt added 8 commits July 5, 2026 13:27
…ects

The shell-HIT tail's degradation branch emitted <script>location.replace(raw
match.redirect)</script> into an already-committed 200 body, bypassing the 3xx
guardOutgoingRedirect chokepoint. Route the target through the shared
resolveSameOriginRedirect (extracted as resolveShellHitRedirectTarget) so a
cross-origin/unsafe target neutralizes to the basename root, matching every
other redirect exit.
…gn docs

cacheTag() shipped a second, render-callable form (#648): called during a
request render outside a "use cache" function it records onto the document/
shell artifact. Four skills and two canonical design docs still taught it as
"use cache"-only; the ppr-shell-resume out-of-scope bullet contradicted the
shipped auto-collection. Correct all six, plus notes on the no-TTL marker cost
and that action invalidation does not evict the server shell.
VercelCacheStore and MemorySegmentCacheStore key the response cache by the raw
string with no host prefix (only CFCacheStore adds host internally), so on one
function serving multiple domains the default document-cache key bled tenant
A's full response to tenant B. Prefix url.host on the default-key branch,
mirroring cache-scope.getCacheKeyBase; the keyGenerator branch is left to the
consumer with a JSDoc warning.
…helpers

The Vercel store spread up to 32768 bytes into String.fromCharCode, 4x closer
to the JS argument-count ceiling than the CF encoder's deliberate 8192-.apply;
a large body could throw RangeError inside putResponse/putShell and degrade
silently to no-cache. Delegate both encode and decode to the import-pure
cf-base64.ts (byte-identical output, no cache cold-start).
…arker expiry

With tagInvalidationTtl unset (the default) every invalidated tag writes a
permanent KV marker; high-cardinality tags accumulate KV keys unboundedly with
no reaper. Emit a one-time-per-namespace warning at the invalidateTags batch
entry point pointing at the sizing rule, mirroring the existing floor-warning
idiom. No default-behavior change.
navigation-loading.test.ts ran both describes dev-only, and check:e2e-parity is
warn-only so the gap never failed CI. Extract each body into a spec function and
add a (production) mode:"build" sibling, mirroring shell-cache.test.ts. The 10
production tests pass locally.
…refetch drop

Two silent PPR/action edges now emit signal without changing behavior. (A) When
a bake-lane loader freezes data into the shell but the capture recorded zero
tags, that data is un-invalidatable except by TTL (an action refresh touches the
client only); warn once per shell key in dev. (B) The action-mode missing-
segments branch in partial-update was a bare return with no diagnostic; log the
intentional drop.
…oses reader leak

captureShellHTML swallowed any AbortError-named throw into a null no-shell
degrade because it aborts before awaiting, so signal.aborted was always true;
discriminate the deliberate abort by identity (error === abortReason), verified
against real react-dom/static.edge. Also arm the maxWaitMs deadline before the
bootstrap load so a hung load degrades bounded, and add try/finally reader
cleanup on the mid-read error path.
@pkg-pr-new

pkg-pr-new Bot commented Jul 5, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@rangojs/router@685

commit: c927fe4

@ivogt
ivogt merged commit 7ef0e37 into main Jul 5, 2026
34 checks passed
@ivogt
ivogt deleted the advisor/caching-ppr-fixes branch July 5, 2026 12:41
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