fix(middleware-runtime): normalize trailing slash on plain-URL redirect locations - #1750
Conversation
…ct locations
When middleware does NextResponse.redirect(new URL('/x', req.url)), the
Location header bypasses NextURL._applyTrailingSlash() and was emitted
verbatim even when trailingSlash config required normalization.
After relativizeLocation(), apply normalizeTrailingSlashPathname() to the
pathname of same-origin redirect Locations when trailingSlash is configured.
File-extension paths (e.g. /file.css) are correctly exempted by the existing
FILE_LIKE_PATHNAME_RE guard inside normalizeTrailingSlashPathname.
Adds 3 new tests covering the plain-URL redirect gap (trailingSlash: true,
trailingSlash: false, file-extension exemption).
Fixes cloudflare#1332
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.
Thanks for tackling #1332. The high-level direction is right: Next.js does apply its trailing-slash policy to middleware redirect Location headers. In packages/next/src/server/web/adapter.ts the redirect is re-wrapped in a NextURL before being relativized:
const redirectURL = new NextURL(redirect, { forceLocale: false, headers, nextConfig })
// ...
if (redirectURL.host === requestURL.host) {
response.headers.set('Location', getRelativeURL(redirectURL, requestURL))
}So normalizing plain-new URL() redirect Locations is the correct goal, and the same-origin guard matches Next.js's redirectURL.host === requestURL.host check.
However, this PR uses the wrong normalization function, and the result diverges from Next.js.
NextURL applies trailing-slash via Next.js's formatNextPathnameInfo (packages/next/src/shared/lib/router/utils/format-next-pathname-info.ts):
return !info.buildId && info.trailingSlash
? !pathname.endsWith('/') ? addPathSuffix(pathname, '/') : pathname
: removeTrailingSlash(pathname)That logic is a plain add-or-strip — it does not special-case /api, /.well-known, or file-like paths. vinext's own NextURL._applyTrailingSlash shim (packages/vinext/src/shims/server.ts:373) matches this exactly.
But normalizeTrailingSlashPathname (the function this PR reuses) is the request-side filesystem-routing normalizer. Per its own docstring it special-cases /api, /.well-known, .rsc, and file-like paths, returning null (no change) for them. Applying it to redirect Locations produces behavior that:
- Diverges from Next.js. A
trailingSlash: trueredirect to/file.cssshould become/file.css/(NextURL/formatNextPathnameInfojust appends/). This PR keeps it as/file.css. The new test "plain URL redirect to file path is not normalized" is asserting incorrect behavior. - Is inconsistent within vinext. A
NextResponse.redirect(request.nextUrl)to/file.cssalready goes through the shim's_applyTrailingSlashand yields/file.css/, while aNextResponse.redirect(new URL('/file.css', req.url))now yields/file.css. Same intent, two different results.
Suggested fix: normalize redirect Locations using the same rule NextURL/formatNextPathnameInfo uses (plain add-or-strip, only exempting root), not normalizeTrailingSlashPathname. This both matches Next.js and keeps NextURL-based and plain-URL redirects consistent. Consider extracting/sharing the _applyTrailingSlash logic so the shim and middleware-runtime stay in lockstep.
Secondary: normalizeTrailingSlashPathname is documented to take a basePath-stripped pathname, but loc.pathname here still includes basePath, so its /api check can misfire under a configured basePath. Switching to the formatNextPathnameInfo-equivalent rule sidesteps this too.
| try { | ||
| const loc = new URL(relativeLocation, options.request.url); | ||
| if (loc.origin === new URL(options.request.url).origin) { | ||
| const normalized = normalizeTrailingSlashPathname(loc.pathname, options.trailingSlash); |
There was a problem hiding this comment.
This is the core issue. normalizeTrailingSlashPathname is the request-side filesystem normalizer — it special-cases /api, /.well-known, .rsc, and file-like paths (returns null for them). Next.js's middleware redirect path instead uses formatNextPathnameInfo via NextURL, which is a plain add/strip with no such exemptions.
Result: a trailingSlash: true redirect to /file.css should become /file.css/ (matching Next.js + vinext's own NextURL._applyTrailingSlash shim at shims/server.ts:373), but this code leaves it as /file.css. That diverges from Next.js and is inconsistent with NextURL-based redirects in vinext.
Normalize with the same rule the shim uses instead. A minimal inline version (ideally share the shim's _applyTrailingSlash):
| const normalized = normalizeTrailingSlashPathname(loc.pathname, options.trailingSlash); | |
| const p = loc.pathname; | |
| let normalized: string | null = null; | |
| if (p !== "" && p !== "/") { | |
| if (options.trailingSlash) { | |
| normalized = p.endsWith("/") ? null : p + "/"; | |
| } else { | |
| normalized = p.endsWith("/") ? p.slice(0, -1) : null; | |
| } | |
| } |
| }); | ||
|
|
||
| expect(result.continue).toBe(false); | ||
| expect(result.redirectUrl).not.toBeUndefined(); |
There was a problem hiding this comment.
This test enshrines behavior that diverges from Next.js. NextURL applies trailing-slash via formatNextPathnameInfo, which appends / unconditionally for non-root paths (no file-extension exemption). So with trailingSlash: true, Next.js — and vinext's own NextURL._applyTrailingSlash for a request.nextUrl redirect — would emit /file.css/, not /file.css.
After switching to the formatNextPathnameInfo-equivalent rule, this expectation should be /file.css/. Worth adding a parallel test showing a NextURL-based redirect to /file.css produces the same result, to lock in consistency between the two redirect paths.
|
Review submitted successfully. Here's a summary of my findings: Review Summary: Changes RequestedThe PR's goal is correct — Next.js does apply its trailing-slash policy to middleware redirect The blocking issue: wrong normalization function. The PR reuses But Next.js's middleware redirect path uses This creates two problems:
Recommended fix: Use the same plain add/strip rule as I also flagged a secondary basePath concern ( |
…ailing slash Replaces normalizeTrailingSlashPathname (the filesystem-routing normalizer, which exempts /api, file-extension paths, and /.well-known) with the same plain add/strip rule used by NextURL._applyTrailingSlash and Next.js's formatNextPathnameInfo: only the root path is exempt. This makes plain-new-URL() redirects consistent with NextURL-based redirects and matches Next.js behavior, where a trailingSlash: true redirect to /file.css produces /file.css/ rather than keeping the path unchanged.
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Re-review: Looks good
The previous ask-bonk blocking concerns have been addressed. The implementation now uses the plain add/strip rule matching Next.js's formatNextPathnameInfo (and vinext's own NextURL._applyTrailingSlash shim at shims/server.ts:373-379) instead of the request-side normalizeTrailingSlashPathname. I verified each prior concern:
-
Divergence from Next.js (file paths) — Fixed. The inline logic at
middleware-runtime.ts:303-314only exempts root (""/"/") and otherwise does an unconditional add/strip. AtrailingSlash: trueredirect to/file.cssnow correctly yields/file.css/. The test at line 152 was updated to assert/file.css/. -
Internal consistency — Fixed and now locked in. The new
"NextURL redirect to file path ... (same result as plain URL)"test confirms both therequest.nextUrlpath (via the shim) and the plain-new URL()path produce identical/file.css/output. The inline rule is byte-for-byte equivalent to_applyTrailingSlash. -
basePath concern — Resolved. Since
normalizeTrailingSlashPathname(with its basePath-sensitive/apicheck) is no longer used, a configuredbasePathno longer causes misfires. The plain rule operates on the full path, matchingformatNextPathnameInfo.
Other checks I performed:
- Cross-origin redirects are untouched: the
loc.origin === new URL(options.request.url).originguard correctly skips absolute cross-origin Locations returned byrelativizeLocation. - Double-normalization is safe/idempotent: NextURL-based redirects already carry the slash, so
p.endsWith("/")short-circuits tonull(no change). - Malformed URLs fall through unchanged via the try/catch.
- All 8 tests in
middleware-runtime-trailing-slash.test.tspass;vp checkis clean on both changed files.
One non-blocking nit below.
Minor pre-existing note: the NextURL.trailingSlash docstring in shims/server.ts:301 says NextURL skips "non-file-like" pathnames, but _applyTrailingSlash actually has no file-like exemption — the comment is stale (not introduced by this PR, but worth a quick fix while nearby).
| let normalizedLocation = relativeLocation; | ||
| if (options.trailingSlash !== undefined) { | ||
| try { | ||
| const loc = new URL(relativeLocation, options.request.url); |
There was a problem hiding this comment.
Nit (non-blocking): new URL(options.request.url) is parsed twice — once implicitly as the base for relativeLocation on line 298 and again on line 299 for the origin check. Minor, but you could hoist a single const reqUrl = new URL(options.request.url) and reuse it for both the base and the origin comparison.
Also consider extracting the inline add/strip into a small shared helper so it stays in lockstep with NextURL._applyTrailingSlash (shims/server.ts:373). They're currently identical by hand; a shared function would prevent future drift.
|
Review submitted successfully (APPROVED). Here's a summary of my re-review of PR #1750: Re-review Summary: ApprovedThe PR was updated since the prior What changed: The implementation no longer uses Verified concerns:
Additional checks I performed (all pass):
Non-blocking nits posted: a minor double-parse of |
2 participants
Description
When middleware redirects via a plain
new URL('/x', req.url)instead ofrequest.nextUrl, theLocationheader bypassedNextURL._applyTrailingSlash()and was emitted verbatim — ignoring the user'strailingSlashconfig.Root cause: In
executeMiddleware,relativizeLocation()extracts the redirectLocationbut there was no trailing-slash normalization step for plain-URL redirects.NextURL-based redirects already work correctly becauseNextURLapplies the policy at stringify time.Fix: After
relativizeLocation(), applynormalizeTrailingSlashPathname()to same-origin redirectLocationpathnames whentrailingSlashis configured. File-extension paths (e.g./file.css) are correctly exempted by the existingFILE_LIKE_PATHNAME_REguard insidenormalizeTrailingSlashPathname.Related Issue
Closes #1332
Potential Risk & Impact
executeMiddlewareand only fires whentrailingSlashis explicitly set in config.Locationheaders are untouched (the same-origin guard ensures that).Locationvalues fall through unchanged via the try/catch.executeMiddleware, so parity is maintained without changes toprod-server.ts.How Has This Been Tested?
tests/middleware-runtime-trailing-slash.test.ts:trailingSlash: true)trailingSlash: false)/file.cssstays/file.css)middleware-runtime-trailing-slash.test.tspasstrailing-slash.test.tspassvp check(format + lint + typecheck) passes clean