fix(vercel): correct ISR route rewrite query preservation and decoding#4409
fix(vercel): correct ISR route rewrite query preservation and decoding#4409pi0x wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReworks ChangesISR route rewrite fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
commit: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/vercel-isr.test.ts (1)
26-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a case for conflicting values between
matchesandreqParams.Current tests cover disjoint keys (
langpresent in one source but not the other) but not a case where both sources define the same key with different values, which would exercise the "first occurrence wins" precedence noted in Line 22 ofisr.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/vercel-isr.test.ts` around lines 26 - 39, Add a regression test for conflicting query values coming from both isrRouteRewrite inputs so the precedence in isr.ts is covered. Extend test/unit/vercel-isr.test.ts near isrRouteRewrite to include a case where x-now-route-matches and the request URL both provide the same key with different values, and assert the function keeps the first occurrence wins behavior. Use the existing isrRouteRewrite helper in the new test to verify the merged output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/unit/vercel-isr.test.ts`:
- Around line 26-39: Add a regression test for conflicting query values coming
from both isrRouteRewrite inputs so the precedence in isr.ts is covered. Extend
test/unit/vercel-isr.test.ts near isrRouteRewrite to include a case where
x-now-route-matches and the request URL both provide the same key with different
values, and assert the function keeps the first occurrence wins behavior. Use
the existing isrRouteRewrite helper in the new test to verify the merged output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df4c7e94-dcb9-4ceb-987a-948a76d7efa2
📒 Files selected for processing (2)
src/presets/vercel/runtime/isr.tstest/unit/vercel-isr.test.ts
…tches is present The `x-now-route-matches` branch of `isrRouteRewrite` returned an empty query string, discarding every `allowQuery`/`passQuery` param (e.g. `?lang`). The function then rendered the bare path and the ISR cache stored that document under the query-keyed entry (e.g. English HTML served at `?lang=es`). The header-less branch already preserved the query. Rebuild the query from both `x-now-route-matches` and the rewritten request URL, skipping the ISR routing param and Vercel's numeric regex-capture groups. Closes #4408 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2ef7ebd to
08e44a2
Compare
The previous rewrite merged `x-now-route-matches` into the preserved query. That header carries route regex capture groups (named like `slug`, numeric like `0`), not user query, so they leaked into the render and polluted the shared ISR cache entry. `allowQuery` params are forwarded by Vercel onto the rewritten request URL, so source the query solely from there and use the header only to read `__isr_route`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`URLSearchParams` already percent-decodes the ISR routing param once, so the extra `decodeURIComponent` decoded it a second time. This over-decoded encoded slugs (e.g. `a%2520b` rendered/cached as `a%20b`) and threw `URIError: URI malformed` on a literal `%` (e.g. `/posts/100%off`), crashing the function with a 400. Pre-existing in the header-less branch. Verified on a live Vercel deployment: `/posts/100%25off` returned 400 before and 200 after, and encoded paths now match non-ISR behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes #4408
This PR fixes two bugs in the Vercel ISR route rewrite (
isrRouteRewrite), both verified end-to-end on a live Vercel deployment.Bug 1 — dropped query string (#4408)
isrRouteRewritehandled its two branches asymmetrically. Thex-now-route-matchesbranch — the path taken for Vercel prerender-function invocations — returned an empty query string, discarding everyallowQuery/passQueryparam:The header-less branch already preserved the query (deleting only
__isr_route). So with a rule like{ isr: { allowQuery: ["lang"], passQuery: true } }, Vercel correctly keys the cache by?langand forwards the query to the function, but the runtime threw it away — SSR rendered the bare path and the ISR cache stored that document under the query-keyed entry (e.g. English HTML served/cached at?lang=es).Fix
Source the preserved query solely from the rewritten request URL (dropping only
__isr_route), and usex-now-route-matchesonly to read the ISR routing param.allowQuery/passQueryparams are forwarded by Vercel onto the rewritten request URL, so the request URL is the authoritative source.x-now-route-matchesis intentionally not merged into the query: it carries the route regex capture groups — named (e.g.slugfor/posts/:slug, generated bynormalizeRouteSrc) as well as numeric (0,1, …) — which are routing internals, not user query. Merging them would leak?slug=…into the render and pollute the shared ISR cache entry.This keeps both branches consistent and, as a side effect, preserves repeated (array-style) query params and never lets a header value override a real request-URL param.
Bug 2 — route param decoded twice (crash on
%)URLSearchParams.get()already percent-decodes the ISR routing param once. The extradecodeURIComponent(isrURL)decoded it a second time:%(e.g./posts/100%off) → after the first decode the string contains a bare%, and the seconddecodeURIComponentthrowsURIError: URI malformed→ the function crashes with a 400 (FUNCTION_INVOCATION_FAILED).a%2520b) → silently over-decoded toa%20b, so ISR renders and caches under the wrong path.This is masked for plain-ASCII paths (the once-decoded value has no
%, so the second decode is a no-op), which is why it went unnoticed. It is pre-existing — the original header-less branch already double-decoded — and independent of #4408.Fix
Drop the redundant
decodeURIComponent; theURLSearchParamsdecode is exactly the right amount.Testing
Added
test/unit/vercel-isr.test.ts(12 cases). Query-preservation cases fail onmain; the double-decode cases fail onmainwithURIError: URI malformed. Coverage:__isr_routeand preserve other params0) and named (slug) regex-capture groups are not leaked%, encoded sequences, both branches)End-to-end (live Vercel)
Built the
playground/app with an ISR route and deployed viavercel deploy --prebuilt:/?lang=es→ renders Hola and caches under the?lang=esentry (/still renders Hello) — confirms Bug 1 fixed and no cross-entry cache pollution. Non-allowQueryparams (?foo=bar) are correctly stripped./posts/100%25off→ 200 with the after fix (was 400 before), and encoded paths (a%2520b,caf%C3%A9,hello%20world) now match non-ISR behaviour — confirms Bug 2 fixed.Credit to the reporter of #4408 for the diagnosis and original patch.