fix(basepath): enforce scoping on rewrites/redirects/routes#1397
Conversation
Sub-issues addressed under #1333: 1. Rewrites/redirects/headers were firing on requests outside the configured `basePath`. Threaded a `BasePathMatchState` through the matchers and prod-server / deploy worker / dev plugin so default rules (no `basePath: false` opt-out) only evaluate when the request was under basePath, and opt-out rules only evaluate outside it. 5. Pages Router requests that landed outside basePath fell through to internal route matching. After redirects and beforeFiles rewrites run, the request now 404s when it was outside basePath and no `basePath: false` rule rewrote it — matching Next.js's `resolve-routes.ts:304-309`. 4. With rewrites no longer firing on out-of-basepath requests, the downstream code that prepends basePath to destinations no longer sees a stripped-but-already-internal pathname, eliminating the `/docs/docs/...` doubling for the rewrite-driven cases. App Router uses the same gating; `basePath: false` rules don't yet fire there because `normalizeRscRequest` 404s out-of-basepath requests before the matchers see them — flagged as follow-up in the rule gating site. External rewrite proxying (sub-issue #2) and static asset doubling (sub-issue #3, partly addressed by #1337/#1383) are deferred. Refs #1333
commit: |
|
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped change. The shouldEvaluateRule gating logic correctly implements Next.js semantics from load-custom-routes.ts:530-580, the threading through all four request-handling paths (App Router, Pages Router prod, Cloudflare worker, dev server) is consistent, and the 404 gate for out-of-basePath requests matches resolve-routes.ts:304-309.
A few observations:
-
Test completeness (minor): The test suite covers
basePath: falserewrite under basePath → null (line 318), but the equivalent inverse case is missing for redirects and headers (basePath: falserule when request IS under basePath → should NOT match). The logic is shared viashouldEvaluateRuleso this isn't a correctness risk, but adding these two cases would round out the test matrix. -
App Router redirect destinations are safe today but fragile for the follow-up.
redirectDestinationWithBasePathinapp-rsc-handler.ts:343unconditionally prepends basePath. This works becausehadBasePathis hardcoded totrue— but when sub-issue 2 makeshadBasePathdynamic (to supportbasePath: falserules in App Router), the redirect destination path will need the samehadBasePath && ...guard thatprod-server.ts:1600anddeploy.ts:685have. Worth a// TODO(#1333):comment to flag it. -
The
configRewriteFired404 gate only checksbeforeFiles, notafterFiles/fallback. This is correct per Next.js semantics (the 404 fires before filesystem routing), but since it's non-obvious, the inline comment indeploy.tscould benefit from explicitly stating thatafterFilesandfallbackintentionally run after this gate, not before it.
All nits, the change is solid. CI fully green.
| basePath: "/docs", | ||
| hadBasePath: false, | ||
| }); | ||
| expect(result).toEqual({ destination: "/another-destination", permanent: false }); |
There was a problem hiding this comment.
Nit: consider adding inverse tests for basePath: false redirect and header when the request IS under basePath (should NOT match). You have this case for rewrites at line 318 ("basePath: false rewrite rule does NOT match when request is under basePath") but the analogous redirect and header tests are missing. Since shouldEvaluateRule is shared this is low-risk, but it rounds out the matrix.
| basePathState, | ||
| ); | ||
| if (redirect) { | ||
| const destination = sanitizeDestination( |
There was a problem hiding this comment.
When the sub-issue 2 follow-up makes hadBasePath dynamic (to support basePath: false rules in App Router), this redirectDestinationWithBasePath call will need the same hadBasePath && guard that prod-server.ts:1600 and deploy.ts:685 have. Consider adding a // TODO(#1333): here to flag it for the follow-up.
|
The review has been posted. Here's a summary: Review posted on PR #1397 — Approved with two inline nits. Verdict: APPROVE. The change is well-structured and correct. The core Two minor inline comments posted:
|
1 participant
Summary
Sub-issues addressed under #1333:
basePath, andbasePath: falseopt-out rules only fire when the request is outside it. Implemented by threading aBasePathMatchStatethroughmatchRewrite/matchRedirect/matchHeadersand every caller (Pages Routerprod-server, App Routerapp-rsc-handler, the Cloudflare worker entry indeploy.ts, and the dev plugin).prod-server.tsand the generated worker indeploy.ts) now returns 404 for out-of-basepath requests after rewrites have had a chance to fire. App Router already had this vianormalizeRscRequest. Matches Next.js'sresolve-routes.ts:304-309./rewrite-1style rules, then the redirect-destination code prepended basePath again. Fixed once Virtual module imports break esbuild dependency optimization when vinext is installed from npm #1 lands. Pure-asset doubling (if any) is tracked separately.Deferred (called out in the issue):
basePath: falserewrite proxying). The matcher gating now lets these rules fire on out-of-basepath requests, but App Router'snormalizeRscRequest404s before reaching them. Pages Router proxying viaproxyExternalRequestworks (it was already wired). Tracked as the App Router follow-up — flagged inline inapp-rsc-handler.ts._next/staticpaths should return plain text 404, not rendered HTML #1337 / fix: plain-text 404 for invalid _next/static paths #1383'sisNextStaticPathwork.References:
packages/next/src/lib/load-custom-routes.ts:530-580.packages/next/src/server/lib/router-utils/resolve-routes.ts:304-309.test/e2e/basepath/redirect-and-rewrite.test.ts.Refs #1333 (not Closes — sub-issues 2 and 3 remain).
Test plan
tests/nextjs-compat/basepath.test.tscovering each gating case (default +basePath: false, in + out of basePath, all three matchers, plus backward-compat without an explicit state arg). All 23 tests in that file pass.vp test run tests/shims.test.ts tests/next-config.test.ts tests/nextjs-compat/basepath.test.ts— 1065 pass.vp test run tests/app-router.test.ts— 318 pass.vp test run tests/pages-router.test.ts— 209 pass.vp test run tests/features.test.ts— 303 pass.vp check --fix— lint + types clean.test/e2e/basepath/*failures drop.