Repository navigation
fix(caching/ppr): audit remediation — redirect guard, host key, base64 bound, SSR capture hardening, +docs/diagnostics - #685
Merged
Conversation
…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.
11 tasks
commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
<script>location.replace(<raw match.redirect>)</script>into a committed 200 body — bypassing the 3xxguardOutgoingRedirectchokepoint, so a caller-influenced target was an open-redirect.resolveSameOriginRedirect(extracted asresolveShellHitRedirectTarget); cross-origin/unsafe neutralizes to the basename root.VercelCacheStore/MemorySegmentCacheStoreserved tenant A's cached/pricingHTML to tenant B (no host in the key; onlyCFCacheStorecompensates internally).url.hoston the default document-cache key, mirroringcache-scope.getCacheKeyBase.Response/PPR-shell body could throwRangeErrorinsideputResponse/putShell(32768-arg spread) and degrade silently to no-cache.cf-base64.ts(byte-identical, no cold-start).tagInvalidationTtlunset (default), high-cardinality tags accumulate permanent KV markers with no reaper — a silent, growing cost.invalidateTagsbatch entry, pointing at the sizing rule.cacheTag()docscacheTag()as"use cache"-only, after the code shipped a render-callable form (#648) — consumer agents couldn't discover the canonical PPR-shell invalidation lever.check:e2e-parityis warn-only, so the gap never failed CI — a prod-only regression in thepartial-updatecommit/transition selection could ship green.(production)siblings via extracted spec functions; 10 production tests pass locally.debugLogon the intentional action drop.captureShellHTMLmasked anyAbortError-named throw (a component's ownfetch/AbortControllercancellation) as a retryablenullno-shell, hiding real failures;maxWaitMsdidn't bound the bootstrap load; a reader leaked on the mid-read abort path.Before vs. after (representative — the two consumer-facing ones)
Render-callable
cacheTag()— the docs now show the shipped form:Multi-tenant document cache — before,
getResponse("/pricing:html")collided across hosts; after, the default key isa.example/pricing:htmlvsb.example/pricing:html. A consumer supplying a customkeyGeneratorstill owns their own namespacing (JSDoc warns to include host).Semantics table (behavior that branches)
location.replace(external)AbortError-named throwreportCacheErrorinvalidateTags, notagInvalidationTtlTest plan
pnpm run typecheck(turbo, 16/16 packages) — exit 0pnpm run test:unit:all(unit + rsc, every package incl. cloudflare-basic / vite-rsc-demo dogfood suites) — all pass; router package 319 files / 4979 testspnpm run lint(oxlint--deny-warnings) — 0/0pnpm run format— cleanpnpm check:e2e-bucketing— OK (lefthook-verified on every commit)(production)project — 10/10 pass locallyshell-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 overflowcf-cache-store-tags.test.ts— warns exactly once per namespace; no warn when TTL setshell-capture-untagged-bake.test.ts— warn once per key; no warn when tagged /ppr.tags/ hole-only / productionshell-handlers.test.tsx— a genuineAbortError-named throw propagates (was masked); hung bootstrap degrades boundedNotes
react-dom/static.edge: our owncontroller.abort(reason)resolves (holes postpone / trivial prelude → sanity gate), and a genuine fatal throw rejects with the thrown error (!== abortReason→ propagates). The modifiedabort itself rejectsfake exercises a defensive branch; the load-bearing red→green is the genuine-throw propagation.executeLoaderDatahas unconditional telemetry/tag/interceptor side effects the HIT path relies on; needs a capture-time-replay redesign).