Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix basePath replacing server-side and normalizeLocalePath() when path is empty string #30978

Merged
merged 4 commits into from Nov 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/next/server/dev/next-dev-server.ts
Expand Up @@ -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'
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion packages/next/server/next-server.ts
Expand Up @@ -67,6 +67,7 @@ import Router, {
DynamicRoutes,
PageChecker,
Params,
replaceBasePath,
route,
Route,
} from './router'
Expand Down Expand Up @@ -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)
}

Expand Down
25 changes: 19 additions & 6 deletions packages/next/server/router.ts
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -283,8 +296,8 @@ export default class Router {
const keepLocale = isCustomRoute

const currentPathnameNoBasePath = replaceBasePath(
this.basePath,
currentPathname
currentPathname,
this.basePath
)

if (!keepBasePath) {
Expand Down
3 changes: 2 additions & 1 deletion packages/next/server/web/next-url.ts
Expand Up @@ -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
Expand Down Expand Up @@ -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 = ''
Expand Down
5 changes: 4 additions & 1 deletion packages/next/shared/lib/i18n/normalize-locale-path.ts
Expand Up @@ -21,7 +21,10 @@ export function normalizeLocalePath(
const pathnameParts = pathname.split('/')

;(locales || []).some((locale) => {
if (pathnameParts[1].toLowerCase() === locale.toLowerCase()) {
if (
pathnameParts[1] &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you able to reproduce this? This might cause issues elsewhere in the server if the pathname is being passed as an empty string here. Splitting / should yield [ '', '' ] which is handled properly here

Copy link
Member Author

@styfle styfle Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only happens if pathname = '' which might mean the root cause is earlier in the code, before we get to this function. Because you're right, generally the assumption is that pathname is never an empty string, should be /.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we got a test case in #31423 so added that in and fixed the original error it uncovered.

pathnameParts[1].toLowerCase() === locale.toLowerCase()
) {
detectedLocale = locale
pathnameParts.splice(1, 1)
pathname = pathnameParts.join('/') || '/'
Expand Down
5 changes: 3 additions & 2 deletions packages/next/shared/lib/router/utils/parse-next-url.ts
Expand Up @@ -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 }
Expand All @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion 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: [
Expand Down
4 changes: 0 additions & 4 deletions test/integration/i18n-support-base-path/test/index.test.js
Expand Up @@ -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()
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
21 changes: 21 additions & 0 deletions test/integration/i18n-support/test/shared.js
Expand Up @@ -35,6 +35,27 @@ async function addDefaultLocaleCookie(browser) {
}

export function runTests(ctx) {
if (ctx.basePath) {
it.only('should handle basePath like pathname', async () => {
styfle marked this conversation as resolved.
Show resolved Hide resolved
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,
Expand Down