Fix basePath replacing server-side and normalizeLocalePath() when p…

…ath is empty string (#30978) This fixes our `basePath` detection/replacing server-side as we were incorrectly considering `/docss` a match for a `basePath` of `/docs` which caused us to have an unexpected value in the `normalizeLocalePath` function. - Fixes #22429 - Regression introduced in #17757 - Fixes: #31423 Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
vercel · Nov 15, 2021 · eb0bd63 · eb0bd63
1 parent 2d9ac39
commit eb0bd63
Show file tree

Hide file tree

Showing 9 changed files with 55 additions and 19 deletions.
diff --git a/packages/next/server/dev/next-dev-server.ts b/packages/next/server/dev/next-dev-server.ts
@@ -34,7 +34,7 @@ import Server, {
   FindComponentsResult,
 } from '../next-server'
 import { normalizePagePath } from '../normalize-page-path'
-import Router, { Params, route } from '../router'
+import Router, { hasBasePath, Params, replaceBasePath, route } from '../router'
 import { eventCliSession } from '../../telemetry/events'
 import { Telemetry } from '../../telemetry/storage'
 import { setGlobal } from '../../trace'
@@ -543,11 +543,11 @@ export default class DevServer extends Server {
     const { basePath } = this.nextConfig
     let originalPathname: string | null = null
 
-    if (basePath && parsedUrl.pathname?.startsWith(basePath)) {
+    if (basePath && hasBasePath(parsedUrl.pathname || '/', basePath)) {
       // strip basePath before handling dev bundles
       // If replace ends up replacing the full url it'll be `undefined`, meaning we have to default it to `/`
       originalPathname = parsedUrl.pathname
-      parsedUrl.pathname = parsedUrl.pathname!.slice(basePath.length) || '/'
+      parsedUrl.pathname = replaceBasePath(parsedUrl.pathname || '/', basePath)
     }
 
     const { pathname } = parsedUrl

diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts
@@ -67,6 +67,7 @@ import Router, {
   DynamicRoutes,
   PageChecker,
   Params,
+  replaceBasePath,
   route,
   Route,
 } from './router'
@@ -369,7 +370,7 @@ export default class Server {
     })
 
     if (url.basePath) {
-      req.url = req.url!.replace(this.nextConfig.basePath, '') || '/'
+      req.url = replaceBasePath(req.url!, this.nextConfig.basePath)
       addRequestMeta(req, '_nextHadBasePath', true)
     }
 

diff --git a/packages/next/server/router.ts b/packages/next/server/router.ts
@@ -44,9 +44,22 @@ export type PageChecker = (pathname: string) => Promise<boolean>
 
 const customRouteTypes = new Set(['rewrite', 'redirect', 'header'])
 
-function replaceBasePath(basePath: string, pathname: string) {
-  // If replace ends up replacing the full url it'll be `undefined`, meaning we have to default it to `/`
-  return pathname!.replace(basePath, '') || '/'
+export function hasBasePath(pathname: string, basePath: string): boolean {
+  return (
+    typeof pathname === 'string' &&
+    (pathname === basePath || pathname.startsWith(basePath + '/'))
+  )
+}
+
+export function replaceBasePath(pathname: string, basePath: string): string {
+  // ensure basePath is only stripped if it matches exactly
+  // and doesn't contain extra chars e.g. basePath /docs
+  // should replace for /docs, /docs/, /docs/a but not /docsss
+  if (hasBasePath(pathname, basePath)) {
+    pathname = pathname.substr(basePath.length)
+    if (!pathname.startsWith('/')) pathname = `/${pathname}`
+  }
+  return pathname
 }
 
 export default class Router {
@@ -142,7 +155,7 @@ export default class Router {
 
     const applyCheckTrue = async (checkParsedUrl: NextUrlWithParsedQuery) => {
       const originalFsPathname = checkParsedUrl.pathname
-      const fsPathname = replaceBasePath(this.basePath, originalFsPathname!)
+      const fsPathname = replaceBasePath(originalFsPathname!, this.basePath)
 
       for (const fsRoute of this.fsRoutes) {
         const fsParams = fsRoute.match(fsPathname)
@@ -283,8 +296,8 @@ export default class Router {
       const keepLocale = isCustomRoute
 
       const currentPathnameNoBasePath = replaceBasePath(
-        this.basePath,
-        currentPathname
+        currentPathname,
+        this.basePath
       )
 
       if (!keepBasePath) {

diff --git a/packages/next/server/web/next-url.ts b/packages/next/server/web/next-url.ts
@@ -2,6 +2,7 @@ import type { PathLocale } from '../../shared/lib/i18n/normalize-locale-path'
 import type { DomainLocale, I18NConfig } from '../config-shared'
 import { getLocaleMetadata } from '../../shared/lib/i18n/get-locale-metadata'
 import cookie from 'next/dist/compiled/cookie'
+import { replaceBasePath } from '../router'
 
 /**
  * TODO
@@ -48,7 +49,7 @@ export class NextURL extends URL {
     const { headers = {}, basePath, i18n } = this._options
 
     if (basePath && this._url.pathname.startsWith(basePath)) {
-      this._url.pathname = this._url.pathname.replace(basePath, '') || '/'
+      this._url.pathname = replaceBasePath(this._url.pathname, basePath)
       this._basePath = basePath
     } else {
       this._basePath = ''

diff --git a/packages/next/shared/lib/i18n/normalize-locale-path.ts b/packages/next/shared/lib/i18n/normalize-locale-path.ts
@@ -21,7 +21,10 @@ export function normalizeLocalePath(
   const pathnameParts = pathname.split('/')
 
   ;(locales || []).some((locale) => {
-    if (pathnameParts[1].toLowerCase() === locale.toLowerCase()) {
+    if (
+      pathnameParts[1] &&
+      pathnameParts[1].toLowerCase() === locale.toLowerCase()
+    ) {
       detectedLocale = locale
       pathnameParts.splice(1, 1)
       pathname = pathnameParts.join('/') || '/'

diff --git a/packages/next/shared/lib/router/utils/parse-next-url.ts b/packages/next/shared/lib/router/utils/parse-next-url.ts
@@ -4,6 +4,7 @@ import { parseUrl } from './parse-url'
 import type { NextConfig, DomainLocale } from '../../../../server/config-shared'
 import type { ParsedUrl } from './parse-url'
 import type { PathLocale } from '../../i18n/normalize-locale-path'
+import { hasBasePath, replaceBasePath } from '../../../../server/router'
 
 interface Params {
   headers?: { [key: string]: string | string[] | undefined }
@@ -15,8 +16,8 @@ export function parseNextUrl({ headers, nextConfig, url = '/' }: Params) {
   const urlParsed: ParsedNextUrl = parseUrl(url)
   const { basePath } = nextConfig
 
-  if (basePath && urlParsed.pathname.startsWith(basePath)) {
-    urlParsed.pathname = urlParsed.pathname.replace(basePath, '') || '/'
+  if (basePath && hasBasePath(urlParsed.pathname, basePath)) {
+    urlParsed.pathname = replaceBasePath(urlParsed.pathname, basePath)
     urlParsed.basePath = basePath
   }
 

diff --git a/test/integration/i18n-support-base-path/next.config.js b/test/integration/i18n-support-base-path/next.config.js
@@ -1,6 +1,6 @@
 module.exports = {
   // target: 'experimental-serverless-trace',
-  // basePath: '/docs',
+  basePath: '/docs',
   i18n: {
     // localeDetection: false,
     locales: [

diff --git a/test/integration/i18n-support-base-path/test/index.test.js b/test/integration/i18n-support-base-path/test/index.test.js
@@ -41,7 +41,6 @@ describe('i18n Support basePath', () => {
       isDev: true,
     }
     beforeAll(async () => {
-      nextConfig.replace('// basePath', 'basePath')
       nextConfig.replace(/__EXTERNAL_PORT__/g, ctx.externalPort)
       await fs.remove(join(appDir, '.next'))
       curCtx.appPort = await findPort()
@@ -57,7 +56,6 @@ describe('i18n Support basePath', () => {
 
   describe('production mode', () => {
     beforeAll(async () => {
-      nextConfig.replace('// basePath', 'basePath')
       nextConfig.replace(/__EXTERNAL_PORT__/g, ctx.externalPort)
       await fs.remove(join(appDir, '.next'))
       await nextBuild(appDir)
@@ -78,7 +76,6 @@ describe('i18n Support basePath', () => {
     beforeAll(async () => {
       await fs.remove(join(appDir, '.next'))
       nextConfig.replace('// target', 'target')
-      nextConfig.replace('// basePath', 'basePath')
       nextConfig.replace(/__EXTERNAL_PORT__/g, ctx.externalPort)
 
       await nextBuild(appDir)
@@ -194,7 +191,6 @@ describe('i18n Support basePath', () => {
   describe('with localeDetection disabled', () => {
     beforeAll(async () => {
       await fs.remove(join(appDir, '.next'))
-      nextConfig.replace('// basePath', 'basePath')
       nextConfig.replace('// localeDetection', 'localeDetection')
 
       await nextBuild(appDir)

diff --git a/test/integration/i18n-support/test/shared.js b/test/integration/i18n-support/test/shared.js
@@ -35,6 +35,27 @@ async function addDefaultLocaleCookie(browser) {
 }
 
 export function runTests(ctx) {
+  if (ctx.basePath) {
+    it.only('should handle basePath like pathname', async () => {
+      const { basePath } = ctx
+
+      for (const pathname of [
+        `${basePath}extra`,
+        `/en${basePath}`,
+        `${basePath}extra/en`,
+        `${basePath}en`,
+        `/en${basePath}`,
+      ]) {
+        console.error('checking', pathname)
+        const res = await fetchViaHTTP(ctx.appPort, pathname, undefined, {
+          redirect: 'manual',
+        })
+        expect(res.status).toBe(404)
+        expect(await res.text()).toContain('This page could not be found')
+      }
+    })
+  }
+
   it('should redirect external domain correctly', async () => {
     const res = await fetchViaHTTP(
       ctx.appPort,