fix(router): avoid repeated App path decoding#2556
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(router): avoid repeated App path decoding
I traced the full request path for App Router across dev (index.ts), Node prod (prod-server.ts), and the generated RSC entry (which the Workers path also delegates to). This is a correct and well-targeted fix for a real double-decode / middleware-bypass class of bug. Verdict: approve with a couple of non-blocking observations.
What the fix does (and why it's sound)
The change has two complementary parts:
-
Idempotent segment decode (
routing/utils.ts) — broadeningPATH_DELIMITER_REGEXfrom%(2f|23|3f|5c)to%[0-9a-f]{2}makesdecodeRouteSegment/decodeRouteSegmentStrictre-encode every residual%XXescape afterdecodeURIComponent, not just delimiters. I verified this makes the function fully idempotent across a wide input set (normal UTF-8,%2F, double-encoded%2561, traversal shapes..%252fadmin, malformed%C3/%2G, bare%):decode(x) === decode(decode(x))for all of them. This is the crux — even code paths that still decode twice (e.g.findIntercept, the un-migratedmatchRoutecalls inapp-server-action-execution.ts) now produce stable results. -
matchNormalizedRoutefast path (app-rsc-route-matching.ts+ handler) — whencleanPathnameis still the request-boundary-normalized value, route matching skips the redundant re-decode viasplitPathSegments. ThecleanPathnameIsNormalizedflag is correctly flipped tofalseon every path that mutatescleanPathnameto an un-normalized value: middleware rewrite (gated onmiddlewareResult.rewritten, which — verified inapp-middleware.ts— is the only condition under whichcleanPathnamechanges), and each beforeFiles/afterFiles/fallback/action rewrite. Because the boundary value is guaranteed decoded-once (vianormalizePathnameForRouteMatchStrictinnormalizeRscRequest), and decode is now idempotent,matchNormalizedRoute(skip decode) andmatchRoute(decode once) are provably equivalent on normalized input.
Dev/prod/worker parity — verified
- prod-server: reverting the RSC handler call from
normalizedUrlback torawUrlis the right move. Static-asset lookup and the/__vinext/prerender gate still use the decodedpathname, but the RSC handler now decodes exactly once insidenormalizeRscRequestinstead of receiving an already-decoded path and decoding again. The prerender-secret gate stays consistent because%2Fsurvives decoding in both prod-server and the handler. - dev (
index.ts): passingpathname(decoded once at line 4731) as the newnormalizedPathnamearg preventsexecuteMiddleware→resolveMiddlewarePathnamefrom decodingrequest.urla second time. This is the actual dev-side bug being closed — previously/%2561dmincould decode twice to/adminand hit a middleware matcher it should not. - workers: inherits the fix via the built RSC entry; covered by the new
cloudflare-workers/ssr.spec.tscases.
Security scenario is legit
The /%2561dmin case is a genuine middleware-bypass fix: Next.js matches middleware against the decode-once pathname (/%61dmin != /admin, so middleware does not block), and route matching must not decode again to /admin. The traversal cases (..%252fadmin, /api/health/..%252fadmin) correctly stay 404 because %252f remains encoded through decode+normalizePath, so .. can't traverse to /admin. I confirmed all three assertions by simulation.
I ran tests/app-rsc-route-matching.test.ts (27 pass), app-router-malformed-url + app-router-middleware-next-request (20 pass), entry-templates (41 pass), and tsc --noEmit (clean).
Non-blocking observations
-
app-server-action-execution.tsstill callsoptions.matchRoute(options.cleanPathname)(lines 1284, 1420) with an already-normalizedcleanPathname, i.e. it double-decodes. This is now harmless thanks to the idempotency fix, but it's an inconsistency with the handler'smatchCleanPathnameoptimization. Not a regression (pre-existing), and out of scope — but worth a follow-up so the "match a normalized path" contract is applied uniformly. A code comment noting these intentionally rely on decode idempotency would help future readers. -
The
matchNormalizedRouteperf win is entirely dependent on decode idempotency for correctness. Since the idempotency fix alone would restore correctness, the fast path is a pure optimization. Consider a one-line comment at thematchCleanPathnamedefinition stating that skipping decode is safe because the boundary value is already strict-normalized (so callers don't accidentally route an encoded path through it after a future refactor). -
api/healthroute doesn't exist in the fixture, so/api/health/..%252fadminis really just exercising "unknown route → 404". That's fine for the traversal assertion, but if the intent was to prove traversal can't reach an existing sibling route, pointing the second traversal case at a real route (or the existing/api/hello) would make the test stronger.
Overall: clean, minimal, correct, and the idempotency insight makes the whole thing robust rather than relying on catching every call site. Nice work.
|
Review posted to PR #2556. Summary of my analysis: Verdict: Approve (no blocking issues)This is a correct, well-targeted fix for a double-decode / middleware-bypass bug class in App Router routing. Core mechanism (verified sound):
Parity verified across dev ( Security assertions checked — Tests I ran locally: route-matching (27), malformed-url + middleware-next-request (20), entry-templates (41), and Non-blocking notes flagged: (1) |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(router): avoid repeated App path decoding
I independently re-verified this fix from the ground up: the decode regex change, the fast-path flag lifecycle, dev/prod/worker parity, and the security scenarios. Verdict: approve. The fix is correct, minimal, and closes a real double-decode / middleware-bypass bug class. A couple of non-blocking robustness notes below.
The core mechanism is sound
1. Idempotent segment decode (routing/utils.ts:172). Broadening PATH_DELIMITER_REGEX from %(2f|23|3f|5c) to %[0-9a-f]{2} re-encodes every residual %XX escape after decodeURIComponent, not just delimiters. I confirmed decode(x) === decode(decode(x)) across normal UTF-8 (caf%C3%A9, %E2%9C%93), delimiters (%2F), double-encoded (%2561), percent literals (100%25), and plus/space. The net effect on a double-encoded segment is identity: %2561dmin → (decode) %61dmin → (re-encode) %2561dmin.
2. Full-pipeline correctness (this is the actual bug being fixed). The decode-once-at-boundary + decode-again-at-param pipeline is what produced the bypass. I simulated the end-to-end path:
| Input | OLD (buggy) | NEW (fixed) |
|---|---|---|
/%2561dmin |
/%61dmin → param decode → admin |
/%2561dmin → param decode → %61dmin |
/%2564ashboard |
/%64ashboard → dashboard |
→ %64ashboard |
/%61dmin (single) |
/admin ✓ |
/admin ✓ |
/%64ashboard (single) |
/dashboard ✓ |
/dashboard ✓ |
The OLD pipeline turned /%2561dmin into admin — a genuine middleware bypass. NEW keeps single-encoded paths decoding correctly while stopping double-encoded paths from collapsing to the target. This matches Next.js semantics: native URL.pathname does not decode, so Next.js middleware sees the raw double-encoded path; vinext decodes exactly once for matching and preserves residuals.
Parity verified across all three runtimes
- prod-server (
prod-server.ts:1432): reverting the RSC handler URL fromnormalizedUrlback torawUrlis correct — static-asset lookup still uses decodedpathname, but the RSC handler now decodes exactly once insidenormalizeRscRequestinstead of receiving a pre-decoded path and decoding again. - dev (
index.ts:4962): passing the once-decodedpathnameasnormalizedPathnameto the Pages runner'srunMiddlewarepreventsresolveMiddlewarePathnamefrom decodingrequest.urla second time. This is the dev-side bug. App Router middleware was already safe (it passesnormalizedPathname: cleanPathnameinapp-middleware.ts:249). - workers: inherits via the built RSC entry; covered by the new
cloudflare-workers/ssr.spec.tscases.
Fast-path flag lifecycle is complete
matchNormalizedRoute (skips the redundant decodeRouteSegment via splitPathSegments) is a pure optimization gated by cleanPathnameIsNormalized. I checked all six cleanPathname = assignments in app-rsc-handler.ts (642, 685, 721, 746, 963, 1009) — every un-normalizing mutation clears the flag. The middleware case (642) gates the reset on middlewareResult.rewritten, and I verified in app-middleware.ts:214-232 that cleanPathname is only reassigned when rewritten = true, so the invariant holds. Since the boundary value is strict-normalized and decode is idempotent, matchNormalizedRoute (decode-once) and matchRoute (decode-twice) are provably equivalent on normalized input — I confirmed identical param results for a%61, caf%C3%A9, a%2561, 50%2525, %2F, a%2Fb.
Verification run
tests/app-rsc-route-matching.test.ts+tests/shims.test.ts: 1212 passtsc --noEmitonpackages/vinext: clean
Non-blocking notes
-
Fragile cross-module invariant at
app-rsc-handler.ts:642-643. Line 642 unconditionally assignscleanPathname = middlewareResult.cleanPathname, but the flag reset on 643 is gated onrewritten. This is correct only becauseapplyAppMiddlewaremaintains "cleanPathname changes ⟺ rewritten." If a future refactor normalized/mutatedcleanPathnamewithout settingrewritten, the fast path would silently route an un-normalized path throughmatchNormalizedRoute. Consider a value-based guard (if (cleanPathname !== normalized.cleanPathname) cleanPathnameIsNormalized = false) or a comment documenting the dependency. -
app-server-action-execution.tsstill double-decodes viaoptions.matchRoute(options.cleanPathname)on an already-normalized path. Now harmless thanks to idempotency, but inconsistent with the handler'smatchCleanPathnameoptimization. Worth a follow-up + a comment noting these intentionally rely on decode idempotency. -
Traversal e2e tests only prove "unknown → 404."
/foo/..%252fadminand/api/health/..%252fadmindon't map to real traversal sources, so they exercise "unknown route → 404" rather than "traversal can't reach an existing sibling." Thenot.toContain("Protected admin content")assertion is the real security check and it holds; pointing one case at an existing route would make the intent sharper.
Clean, minimal, correct — the idempotency insight makes the whole thing robust rather than relying on catching every call site. Nice work.
|
Review posted to PR #2556. SummaryVerdict: Approve — correct, minimal fix for a real double-decode / middleware-bypass bug class. What I verified independently
Non-blocking notes flagged
No blocking issues found. |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(router): avoid repeated App path decoding
I reviewed the current state of the branch (the implementation has evolved considerably from the earlier PATH_DELIMITER_REGEX-broadening approach described in prior comments — it now uses a dedicated raw-matching path). I traced the full request pipeline across dev (index.ts), Node prod (prod-server.ts), the generated RSC entry (Workers inherits it), and the route matcher. Verdict: approve. The fix is correct and well-scoped. A few non-blocking notes below.
What the fix actually does now
Rather than making decodeRouteSegment idempotent, this version introduces a parallel raw matching path:
-
matchRequestRoute+trieMatchRaw/matchRoutePatternRaw(app-rsc-route-matching.ts,route-trie.ts,route-pattern.ts) — route matching runs against the encodedrequestCleanPathnamesplit withsplitPathSegments(no decode). Params are then finalized with a match-once contract:routeHandlerroutes getdecodeMatchedParams(decode once), page routes getcanonicalizeAppPageParams(decode→re-encode, i.e. kept encoded in canonical form). This matches Next.js, where route handlers receive decoded params but pageparamsretain encoding. -
requestCleanPathname(app-rsc-request-normalization.ts) — the original encoded pathname with only basePath +.rscstripped, threaded through so the RSC handler can route on the un-decoded path.matchCleanPathname()uses it only whilecleanPathnameIsRequestPathnameholds; the flag is cleared on everycleanPathnamemutation (middleware rewrite gated onrewritten, and each beforeFiles/afterFiles/fallback/action rewrite). I checked all six assignment sites — coverage is complete. -
Middleware sees the encoded path (
middleware-runtime.ts) —createNextRequestnow buildsNextRequestfrom rawurl.pathnameinstead ofnormalizedPathname, while the matcher (matchPathname) still uses the decodednormalizedPathname. That is exactly Next.js semantics: the matcher decides whether middleware runs (decoded), butrequest.nextUrl.pathnamereflects the encoded request. The new!hasBasePath(requestPathname, basePath)guard also correctly avoids double-prefixing basePath.
Parity — verified
- prod-server: reverting the RSC-handler URL from
normalizedUrlback torawUrlis correct — static-asset lookup still uses decodedpathname, but the handler now decodes exactly once insidenormalizeRscRequest. - dev (
index.ts): the newconfigureServermiddleware capturesreq.__vinextOriginalEncodedUrlbefore Vite normalizes, andmiddlewareUrlis threaded torunMiddleware(..., pathname). This closes the dev-side re-decode. - workers: inherits via the built RSC entry; covered by the new
cloudflare-workers/ssr.spec.tscases.
Security scenarios check out
The interesting interaction: /%2561dmin — the matcher decodes to /admin so the /admin middleware matcher fires, but the middleware body reads request.nextUrl.pathname === "/%61dmin" (encoded), so pathname === "/admin" is false → no block → falls through to matchRequestRoute("/%2561dmin") → 404. Internally consistent, and matches Next.js. I also confirmed the raw path is stricter on traversal: requestCleanPathname is not normalizePath-collapsed, so .. stays a literal segment and cannot resolve to a sibling route (e.g. /foo/..%252fadmin → segments ["foo","..%252fadmin"] → 404). //-collapsing via splitPathSegments is harmless because the protocol-relative guard runs first and the normalized cleanPathname collapses identically.
Verification run (locally)
tests/app-rsc-route-matching.test.ts(27),app-rsc-handler.test.ts(123),app-page-request.test.ts(29),prerender-route-params.test.ts(24) — all passtests/shims.test.ts(1186) +static-image-emission.test.ts(5) — all passtests/middleware-runtime.test.ts+app-router-middleware-next-request.test.ts+middleware-runtime-trailing-slash.test.ts(45) — all passvp checkon all changed source files — format/lint/types clean
Non-blocking notes
-
Dead parameter:
createNextRequest'snormalizedPathname(middleware-runtime.ts:227). SincemwPathnamenow derives fromurl.pathname, thenormalizedPathnameargument is no longer read insidecreateNextRequest(only referenced in a stale comment at :238). It still passes lint because of the comment/call-site, but it is genuinely unused now. Consider dropping the parameter (and updating the comment) so the function signature doesn't imply it participates inmwPathnameconstruction. -
middlewareUrlmay retain a stripped.htmlsuffix in dev (index.ts:4718).middlewareUrlis seeded fromreq.__vinextOriginalEncodedUrl(captured raw inconfigureServer), so it is not subject to the/index.htmland.htmlstripping applied tourljust above (lines 4711–4715). The_next/databranch explicitly resyncsmiddlewareUrl = url, but the.htmlcase doesn't, so a/about.htmlrequest would hand middleware anextUrl.pathnameending in.html. This is an unlikely path for App Router middleware, but it is an asymmetry with the non-encodedurland worth a resync (or a comment explaining why it's intentional). -
Traversal e2e cases still target non-existent routes.
/foo/..%252fadminand/api/health/..%252fadmindon't map to real traversal sources, so they mostly assert "unknown route → 404". Thenot.toContain("Protected admin content")check is the real security assertion and it holds; pointing one case at an existing sibling would sharpen intent.
Overall: correct, minimal, good test coverage across all three runtimes, and the raw-match-with-canonicalization approach cleanly separates "decode for matcher gating" from "route on encoded path." Nice work.
|
Review posted to PR #2556. Verdict: Approve (no blocking issues)The implementation has evolved from the earlier What I verified:
Non-blocking notes flagged:
|
1 participant
Summary
Test coverage