Repository navigation
fix(app-router): honor basePath opt-out rewrites - #2362
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: honor basePath opt-out rewrites (PR #2362)
The core fix is sound: threading the real hadBasePath through normalizeRscRequest and into basePathState correctly drives shouldEvaluateRule (config-matchers.ts:521-523), and the filesystemRouteEligible gate properly prevents unrelated opt-out rules from exposing direct App routes outside basePath. The rewrite path matches Next.js v16.2.6 behavior.
However, this PR newly enables basePath: false config redirects to match outside-basePath requests, and the App Router redirect path mishandles the destination — a real Next.js parity bug and a dev/prod divergence from the Pages Router.
Blocking: basePath: false config redirects get the basePath wrongly prepended
packages/vinext/src/server/app-rsc-handler.ts:562-565
if (redirect) {
const destination = sanitizeDestination(
redirectDestinationWithBasePath(redirect.destination, options.basePath),
);redirectDestinationWithBasePath (app-rsc-handler.ts:353-358) only skips the basePath prefix for external/already-prefixed destinations — it does not consider hadBasePath:
function redirectDestinationWithBasePath(destination: string, basePath: string): string {
if (!basePath || isExternalUrl(destination) || hasBasePath(destination, basePath)) {
return destination;
}
return basePath + destination;
}Before this PR, outside-basePath requests were 404'd by normalizeRscRequest before reaching the redirect block, so a basePath: false redirect never matched here (this is exactly what the now-stale TODO at lines 349-352 documents). After this PR, with hadBasePath correctly threaded, matchRedirect (config-matchers.ts:992 via shouldEvaluateRule) will match a basePath: false redirect on an outside-basePath request — and the destination then gets the basePath wrongly prepended.
Repro (basePath /docs):
- config:
redirects: [{ source: "/outside", destination: "/landing", basePath: false }] - request:
GET /outside - vinext App Router emits
Location: /docs/landing - Next.js (and vinext Pages Router) emit
Location: /landing
The Pages Router pipeline already guards this correctly at packages/vinext/src/server/pages-request-pipeline.ts:286-294:
const dest = sanitizeDestination(
basePath &&
hadBasePath &&
!isExternalUrl(redirect.destination) &&
!hasBasePath(redirect.destination, basePath)
? basePath + redirect.destination
: redirect.destination,
);The App Router handler needs the same hadBasePath guard. hadBasePath is already in scope (destructured at line 502), so pass it into redirectDestinationWithBasePath and skip the prefix when false. The stale TODO at lines 349-352 should be removed/updated since its premise ("normalizeRscRequest 404s out-of-basePath requests before they reach this code") is no longer true.
Please also add a test mirroring the existing rewrite test: a basePath: false config redirect on an outside-basePath request should redirect to the un-prefixed destination.
Non-blocking: trailing-slash redirect can prepend basePath on opt-out requests
packages/vinext/src/server/request-pipeline.ts:313-316
return new Response(null, {
status: 308,
headers: { Location: basePath + encodedPathname + search },
});normalizeTrailingSlash runs at app-rsc-handler.ts:535 (before config rewrites/redirects) and unconditionally prepends basePath. With trailingSlash: true configured, an outside-basePath request that needs slash normalization (now allowed through when an opt-out rule exists) would be 308-redirected into the basePath, short-circuiting the opt-out rewrite. Narrower edge case (requires trailingSlash: true + a normalizable outside path), but it's the same hadBasePath parity gap. Worth a follow-up if not addressed here.
Nits
- The
hasBasePathOptOutRulescan (app-rsc-handler.ts:485-491) runs on every request by spreading five config arrays into a new array. Fine for correctness; consider hoisting to a precomputed boolean at handler-construction time since the config is static per build.
|
Posted the review on PR #2362. SummaryThe core fix is correct — threading the real I found one blocking issue:
Plus one non-blocking parity gap (trailing-slash redirect prepending basePath on opt-out requests) and a minor perf nit (the per-request opt-out scan could be precomputed). |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: honor basePath opt-out rewrites (PR #2362)
Both issues from the previous review are resolved in commit 3c428cb.
Blocking issue — fixed ✅
redirectDestinationWithBasePath now takes hadBasePath and skips the prefix when false (app-rsc-handler.ts:349-362), and the redirect call site threads it through (app-rsc-handler.ts:569). The stale TODO(#1333) comment is removed. The new test does not prepend basePath to opt-out redirects outside basePath asserts Location: /landing (un-prefixed) for a basePath: false config redirect under basePath /docs, matching Next.js and the Pages Router pipeline. Confirmed correct.
Non-blocking trailing-slash gap — fixed ✅
normalizeTrailingSlash is now called with hadBasePath ? options.basePath : "" (app-rsc-handler.ts:540-545), so an outside-basePath request needing slash normalization no longer gets pulled into the basePath. The new test does not prepend basePath when normalizing trailing slashes outside basePath asserts Location: /outside/ (un-prefixed), which is correct.
Verification
vp test run tests/app-rsc-handler.test.ts tests/app-rsc-request-normalization.test.ts --maxWorkers=1— 150 passed.shouldEvaluateRule(config-matchers.ts:521-523) is now correctly driven by the realhadBasePath:basePath: falserules only match outside basePath, default rules only match inside. ThefilesystemRouteEligiblegate (app-rsc-handler.ts:629) correctly keeps direct App routes from leaking outside basePath unless a rewrite/middleware brings the request into scope.- The
/__vinext/prerender bypass yieldshadBasePath = false, but prerender endpoints return early (line 537) beforebasePathStateis used, so rule matching is unaffected.
Remaining nit (non-blocking, unchanged)
The hasBasePathOptOutRule scan (app-rsc-handler.ts:490-496) still spreads five config arrays into a new array on every request. Config is static per build, so this could be hoisted into the createAppRscHandler closure. Purely a micro-optimization; fine to leave or follow up.
LGTM — the parity fix is correct and well-tested.
|
Posted the re-review on PR #2362. SummaryBoth issues flagged in the prior review have been resolved in the new commit
Verification:
Only remaining item is the pre-existing non-blocking perf nit (per-request opt-out scan could be hoisted to handler construction). LGTM. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: PR #2362 — new commit 92aeb52 (post-3c428cb)
The two earlier reviews stopped at 3c428cb. A third commit landed afterward — 92aeb52 "harden basePath opt-out routing" — which the prior re-review did not cover. The basePath-prefix fixes (redirect destination + trailing-slash) and the filesystemRouteEligible gate remain correct. 92aeb52 is mostly good (it adds the missing action/progressive-action gates and threads hadBasePath into App middleware so request.nextUrl.basePath is empty outside basePath — both verified by new tests). But it changes one shared flag in a way that has an unverified side effect.
Non-blocking (correctness): didMiddlewareRewrite now over-triggers allowRscDocumentFallback
packages/vinext/src/server/app-rsc-handler.ts:622
didMiddlewareRewrite = middlewareResult.rewritten;92aeb52 changed this from the previous pathname comparison (cleanPathname !== normalized.cleanPathname) to the new rewritten flag returned by applyAppMiddleware (app-middleware.ts:288,299). rewritten is set whenever middleware emits any x-middleware-rewrite, including a query-only rewrite that leaves the pathname unchanged.
That change is correct and necessary for the filesystemRouteEligible use at app-rsc-handler.ts:631 — the new test "allows query-only middleware rewrites to make outside-basePath routes eligible" depends on it. The problem is that the same flag is reused for a second, unrelated concern:
packages/vinext/src/server/app-rsc-handler.ts:814
allowRscDocumentFallback: didMiddlewareRewrite,allowRscDocumentFallback gates whether an RSC request may be served a Pages-Router HTML document instead of a flight payload (app-pages-bridge.ts:102: if (isRscRequest && !allowRscDocumentFallback) return null;). Its purpose is "middleware rewrote this RSC navigation to a Pages route" — i.e. a path/route change. With the new semantics, a query-only middleware rewrite (same path, same App route, App route doesn't match) now also flips allowRscDocumentFallback to true, so in hybrid app+pages mode an RSC navigation can now be handed a Pages HTML document where before 92aeb52 it would not.
Why this slipped through: the only test for this flag ("allows middleware-rewritten RSC requests to hand off to Pages HTML", tests/app-rsc-handler.test.ts:894) uses a path-changing rewrite (/docs/source → /docs/pages), so it passes under both the old and new semantics. There is no test asserting a query-only middleware rewrite does not enable the document fallback, so the widening is unverified.
Recommendation: decouple the two concerns. Keep filesystemRouteEligible driven by middlewareResult.rewritten, but compute the RSC document-fallback eligibility from an actual path change, e.g.:
const middlewareChangedPath = cleanPathname !== normalized.cleanPathname;
// ...
allowRscDocumentFallback: middlewareChangedPath,and add a regression test: an RSC request + query-only middleware rewrite + no App match should not return Pages HTML. Please confirm against Next.js whether a query-only middleware rewrite is expected to enable the App→Pages document handoff; if it is, this can stay as-is with a test that pins the intended behavior. Without that confirmation this is a silent behavioral change limited to hybrid app+pages deployments.
Non-blocking parity nit: out-of-basePath App miss renders the styled app 404; Pages Router returns a bare 404
When an opt-out rule exists and an out-of-basePath request matches no rule, filesystemRouteEligible stays false, match is null, and the handler falls through to renderNotFound (app-rsc-handler.ts:949), rendering the App Router not-found page. The Pages Router pipeline instead returns a bare new Response("This page could not be found", { status: 404 }) for the same out-of-basePath-and-no-rewrite case (pages-request-pipeline.ts:502-510). Both return 404, but the body differs. Worth aligning (or confirming Next.js renders the app 404 here) for dev/prod + App/Pages parity per AGENTS.md.
Pre-existing perf nit (unchanged, non-blocking)
The hasBasePathOptOutRule scan (app-rsc-handler.ts:491-497) spreads five static config arrays into a new array on every request. The config is fixed per build, so this should be hoisted into the createAppRscHandler closure once rather than recomputed per request (AGENTS.md: "hoist immutable ... to module scope", "avoid repeated hot-path work").
Verified good
redirectDestinationWithBasePath+hadBasePathguard, trailing-slashhadBasePath ? basePath : "", and the redirect/slash tests — correct, match Pages Router and Next.js v16.2.6.hadBasePaththreaded into App middleware (app-middleware.ts:243,app-rsc-entry.ts) sorequest.nextUrl.basePathis""outside basePath — covered by "passes outside-basePath state to middleware".filesystemRouteEligiblegating of image/metadata/public-file/action/route-match — prevents direct App routes leaking outside basePath; covered by the new "does not expose direct App routes" / "does not dispatch server actions" tests.tests/app-rsc-handler.test.ts tests/app-rsc-request-normalization.test.ts tests/entry-templates.test.ts— 187 passed locally.
|
Posted the review on PR #2362. SummaryI reviewed the latest commit Non-blocking (correctness):
Plus two non-blocking items:
Verified good: the |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: PR #2362 — new commits 52fd5f3 + e7da120 (post-92aeb52)
The prior reviews stopped at 92aeb52. Two commits landed afterward — 52fd5f3 ("reject untouched basePath misses") and e7da120 ("scope pages fallback rewrites") — and they resolve both outstanding items from my last review. I re-verified the redirect/trailing-slash hadBasePath guards and the filesystemRouteEligible gate; all still correct.
Prior blocking concern (allowRscDocumentFallback over-trigger) — fixed ✅
92aeb52 had reused the broadened didMiddlewareRewrite flag for both filesystemRouteEligible and allowRscDocumentFallback, which meant a query-only middleware rewrite could wrongly enable the App→Pages HTML document handoff. e7da120 decouples them exactly as recommended (app-rsc-handler.ts:626):
didMiddlewareRewrite = middlewareResult.rewritten;
didMiddlewareRewritePathname = cleanPathname !== normalized.cleanPathname;filesystemRouteEligiblestays driven bydidMiddlewareRewrite(the broad flag — query-only rewrites still make outside-basePath routes eligible).allowRscDocumentFallbacknow usesdidMiddlewareRewritePathname(:818), an actual path change.
The new regression test "does not hand query-only middleware-rewritten RSC requests to Pages HTML" (tests/app-rsc-handler.test.ts:920) pins the intended behavior: a query-only x-middleware-rewrite yields allowRscDocumentFallback: false and a 404 with non-HTML content-type. This is exactly the missing coverage I flagged. Confirmed correct.
Prior parity nit (out-of-basePath App miss rendered styled 404) — fixed ✅
52fd5f3 adds an explicit pre-render gate (app-rsc-handler.ts:924-929):
if (!filesystemRouteEligible) {
options.clearRequestContext();
const headers = new Headers();
mergeMiddlewareResponseHeaders(headers, middlewareContext.headers);
return notFoundResponse({ headers });
}Since filesystemRouteEligible = hadBasePath || didMiddlewareRewrite (:635), this only fires outside basePath when nothing made the request eligible. Inside-basePath misses keep hadBasePath === true, skip this block, and still reach renderNotFound for the styled app 404. The new test "returns a plain 404 when middleware leaves an outside-basePath request unchanged" asserts renderNotFound is not called. Good — the bare-404 body matches the Pages Router / Next.js plain-text 404.
The missing-server-action and progressive-action paths are now also gated on filesystemRouteEligible (:746, :806), so outside-basePath POSTs no longer dispatch actions. Covered by the existing "does not dispatch server actions" test.
canHandleOutsideBasePath widening — verified parity ✅
52fd5f3 widens the normalization gate to admit outside-basePath requests whenever middleware exists (app-rsc-handler.ts:491-499), not just when a basePath: false config rule exists. I checked this against the Pages Router pipeline, which runs middleware unconditionally and passes the original (un-stripped) URL through for hadBasePath === false (pages-request-pipeline.ts:329, wrapper at :160-181). So App↔Pages parity holds: a middleware-only app with a basePath now lets middleware act on outside-basePath requests in both routers. New test "allows middleware-only apps to handle requests outside basePath" confirms a middleware redirect fires for /outside.
Non-blocking nit: out-of-basePath 404 Content-Type differs between routers
notFoundResponse({ headers }) (app-rsc-handler.ts:928) sets no Content-Type, while the Pages Router's equivalent out-of-basePath 404 sets Content-Type: text/html; charset=utf-8 (pages-request-pipeline.ts:505-508). Same status (404) and same body ("This page could not be found"), so this is cosmetic, but it's a small App/Pages divergence on the same code path. Worth aligning (or confirming which Content-Type Next.js emits for an out-of-basePath miss) for dev/prod + App/Pages parity per AGENTS.md. No local .nextjs-ref was available to confirm the authoritative value.
Pre-existing perf nit (unchanged, non-blocking)
The opt-out rule scan (app-rsc-handler.ts:491-499) still spreads five static config arrays into a new array on every request. Config is fixed per build, so this should be hoisted into the createAppRscHandler closure (AGENTS.md: "hoist immutable ... to module scope", "avoid repeated hot-path work"). Now slightly cheaper since it short-circuits on Boolean(options.runMiddleware) first, but still recomputed per request when no middleware is configured.
Verified good
redirectDestinationWithBasePath+hadBasePathguard and trailing-slashhadBasePath ? basePath : ""— unchanged, still correct.forwarded.rewriteUrlpath setsrewrittenindependently ofdidMiddlewareRewritePathname; the handler recomputes the pathname-change flag fromcleanPathname !== normalized.cleanPathname, so the forwarded path can't mis-trigger the document fallback./__vinext/prerender bypass yieldshadBasePath = falsebut returns early before rule matching / the new 404 block — no impact.afterFiles/fallbackrewrites correctly flipfilesystemRouteEligible = truebefore the!filesystemRouteEligible404 gate, so a fallback rewrite that brings an outside request into scope still renders.
Verification run
vp test run tests/app-rsc-handler.test.ts tests/app-rsc-request-normalization.test.ts --maxWorkers=1— 156 passed.vp checkonapp-rsc-handler.ts+app-middleware.ts— no format/lint/type errors.
LGTM. The two blocking/non-blocking items from the prior review are resolved and well-tested. Only the Content-Type parity nit and the pre-existing perf nit remain, both non-blocking.
|
Posted the re-review on PR #2362. SummaryI reviewed the two commits that landed after the prior reviews (which stopped at
Two non-blocking items remain:
Verification: 156 tests pass across the two relevant files; |
Summary
basePath: falseNext.js parity
Fixes the remaining failure in
test/e2e/app-dir/app-basepath/index.test.ts:should redirect externally when encountering absolute URLs on the same host outside the basePathReference: https://github.com/vercel/next.js/blob/v16.2.6/test/e2e/app-dir/app-basepath/index.test.ts
Validation
vp test run tests/app-rsc-request-normalization.test.ts tests/app-rsc-handler.test.ts -t "outside-basePath|outside basePath|basePath \+ \.rsc" --maxWorkers=1— 5 passedvp check --fixon the four changed filesvp run vinext#buildtest/e2e/app-dir/app-basepath/index.test.tswith one worker — 13/13 passedExcludes cacheComponents, PPR, resume, fallback-shell, and external endpoint failures.