fix(vercel): harden ISR URL restoration against carrier variants - #4641
nikolas-sapa wants to merge 1 commit into
Conversation
The ISR fallback function recovered the original URL from a single carrier: the `__isr_route` group of the `x-now-route-matches` header, and — only when the header was absent entirely — the destination query. Two gaps remain in that contract: 1. A header serialization without the named group (e.g. positional-only groups) shadows the query fallback, which the rewrite populates with the identical value. The internal `-isr` route renders, and the 404 is stored by Vercel as a valid ISR outcome — cache poisoning until expiration. 2. A bare invocation (no header, no query) renders the internal route the same way. Consult the destination query whenever the header carrier yields no `__isr_route` (strictly additive: working carrier shapes are unaffected), and respond 503 when no carrier resolves for an internal `-isr` path — a 5xx is never stored by the ISR cache, so the miss stays transient and any cached copy keeps serving with `x-vercel-cache: STALE`. Design per the analysis in nitrojs#4446; both gaps reproduce against the fixture build, and the new tests pin them. Fixes nitrojs#4446
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@nikolas-sapa is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
1 participant
🔗 Linked issue
Resolves #4446
❓ Type of change
📚 Description
This implements the fix design prioritized in #4446 — full credit to the reporter for the analysis, the carrier-shape taxonomy and the fail-safe reasoning; the code follows their proposal (items 1 and 2).
The ISR fallback function recovered the original URL from a single carrier: the
__isr_routegroup ofx-now-route-matches, and — only when the header was absent entirely — the destination query. Two gaps remain:__isr_route(e.g. positional-only serialization like1=schedule): the header branch ran and found nothing, and theif/elseshadowed the query fallback — so the rewrite's identical query value was never consulted. The internal-isrroute rendered a 404, which Vercel stores as a valid ISR outcome and replays withx-vercel-cache: HIT— cache poisoning.Changes to
presets/vercel/runtime/vercel.ts:__isr_route— strictly additive, since the rewrite writes the identical value into the dest query. Every carrier shape that works today resolves through the exact same code path as before.-isrpath, respond 503 instead of rendering the internal route. As measured in the issue on a live deployment: 404/410 are stored by the ISR cache, 401/5xx are never stored and any existing good copy keeps serving (x-vercel-cache: STALE) — turning an unrecognized future carrier shape into a transient miss instead of permanent poisoning.One trade-off, explicit in the issue: a user route literally named
…-isrhit bare would now get a 503 from this function. That is the accepted cost of the fail-safe, and external requests to such paths land on separate cache keys anyway.Testing. Four runtime tests against the built fixture function (the harness invokes
functions/__fallback.funcdirectly): header carrier (unchanged), query carrier (unchanged), header-without-__isr_routefalling back to the query (fails onmainwith a 404 for the internal path), and bare invocation returning 503 for both the page and_payload.jsonvariants. Reverting the runtime change turns exactly the two bug-pinning tests red while the two working-path tests stay green — pinning rather than describing. Full file: 55 passed. eslint / prettier /tsc --noEmitclean.The v3
mainequivalent (presets/vercel/runtime/isr.ts, via #3851) has the same gaps — happy to port this once the v2 approach is agreed.📝 Checklist