fix: enforce segment boundaries for basePath stripping and redirect prefixing#393
Conversation
commit: |
|
/bigbonk review por favor |
|
/bigbonk review |
There was a problem hiding this comment.
Review: basePath segment-boundary fix
This is a well-executed fix for a real correctness bug. The core issue — startsWith(basePath) matching /application when basePath is /app — is a classic boundary-matching mistake, and the fix (requiring exact match or basePath + "/" prefix) is the right approach.
What I like:
-
Centralized helpers. Extracting
hasBasePath/stripBasePathintoutils/base-path.tseliminates the duplicated inline implementations innavigation.ts,router.ts, andrequest-pipeline.ts. The shared module is clean and well-documented. -
Consistent application. The fix is applied across all code paths that perform basePath matching: dev server redirects (
index.ts), prod server stripping + redirects (prod-server.ts), App Router RSC entry (app-rsc-entry.ts), Cloudflare worker template (deploy.ts), and both client-side shims (navigation.ts,router.ts). This follows the AGENTS.md guidance about checking dev/prod parity. -
The redirect destination fix is a bonus. The old code also had a bug where
redirect.destination.startsWith(basePath)would skip basePath prefixing for destinations like/application/about, plus it would incorrectly try to prefix external URLs. Adding theisExternalUrlcheck alongsidehasBasePathfixes both cases. -
Test coverage is thorough. Unit tests for
hasBasePathandstripBasePathcover exact match, descendant, shared-prefix, and nested basePath cases. The HTTP integration tests exercise the actual routing pipeline with a collision route (/lication/about) and redirect destination prefixing. Snapshot updates and template assertions round it out. -
The worker template duplication is justified.
deploy.tsgenerates a self-contained worker entry as a template string, so it can't import fromutils/base-path.ts. Duplicating the two small functions there is the right call.
I have two minor observations (noted inline) but neither blocks merging. Approved.
| */ | ||
| export function hasBasePath(pathname: string, basePath: string): boolean { | ||
| if (!basePath) return false; | ||
| return pathname === basePath || pathname.startsWith(basePath + "/"); |
There was a problem hiding this comment.
Nit: this could also be written as:
return pathname === basePath || pathname.startsWith(basePath + "/");which is what you have — just noting it's clean and correct. The only edge case worth thinking about is a basePath of "/", but Next.js disallows that (basePath must not be /), so this is fine.
| } | ||
| return pathname; | ||
| } | ||
| export { hasBasePath, stripBasePath }; |
There was a problem hiding this comment.
Minor: the JSDoc block above (lines 39-49) still describes stripBasePath as a local function with @param tags, but it's now a re-export. This is cosmetic — the docs aren't wrong per se, but they could be misleading since the function is defined in utils/base-path.ts now. Consider simplifying this to just a re-export comment, or removing the duplicate docs entirely since the canonical docs live in base-path.ts.
Non-blocking.
|
Review submitted and approved. Here's a summary: Verdict: Approved This PR fixes a real correctness bug where Key strengths:
Two non-blocking inline comments were left:
|
This fixes incorrect
basePathhandling when a path only shares a string prefix with the configured base path, instead of matching on a path-segment boundary.Before this change, vinext treated any pathname beginning with the
basePathstring as being inside the base path. WithbasePath: "/app", that meant:/app/aboutcorrectly stripped to/about/appcorrectly stripped to//application/aboutincorrectly stripped as well/app2incorrectly stripped as wellThat was not limited to request-path stripping. Redirect destination handling had the same bug: a destination like
/application/aboutwas incorrectly treated as already being under/app, so vinext would emit/application/aboutinstead of/app/application/about.In the server flow, the stripped value was fed back into URL/request handling, so
/application/aboutcould normalize to"/lication/about"and potentially collide with a real in-app route. In other words, a request outside the configured base path could be misrouted into an unrelated page if the suffix happened to exist.The fix centralizes basePath handling into shared helpers:
hasBasePath()only matches when the pathname is exactly equal to the base path, or starts withbasePath + "/"stripBasePath()only strips in those casesImplementation details:
Test coverage added:
basePathcoverage such as/docs/v2vs/docs/v20/application/aboutmust not resolve to/app/lication/about/app/redircorrectly redirects to/app/application/aboutValidation run:
pnpm test tests/features.test.ts -t "basePath HTTP routing"pnpm test tests/request-pipeline.test.ts tests/deploy.test.ts tests/app-router.test.ts -t "basePath"pnpm test tests/entry-templates.test.ts -upnpm run typecheck