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

Repeated slash redirect #15171

Closed
wants to merge 22 commits into from
Closed
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
2 changes: 1 addition & 1 deletion packages/next/build/index.ts
Expand Up @@ -1275,7 +1275,7 @@ export default async function build(
hasReportWebVitals: namedExports?.includes('reportWebVitals') ?? false,
rewritesCount: rewrites.length,
headersCount: headers.length,
redirectsCount: redirects.length - 1, // reduce one for trailing slash
redirectsCount: redirects.length - 2, // subtract two for internal redirects
})
)

Expand Down
4 changes: 4 additions & 0 deletions packages/next/client/normalize-trailing-slash.ts
Expand Up @@ -20,3 +20,7 @@ export const normalizePathTrailingSlash = process.env.__NEXT_TRAILING_SLASH
}
}
: removePathTrailingSlash

export function normalizePathSlashes(path: string): string {
return normalizePathTrailingSlash(path.replace(/\/\/+/g, '/'))
}
8 changes: 8 additions & 0 deletions packages/next/lib/load-custom-routes.ts
Expand Up @@ -510,6 +510,14 @@ export default async function loadCustomRoutes(
}
}

// multiple slashes redirect
redirects.unshift({
source: '/:before*/{(/+)}:after(|.*/[^/.]+|[^/.]+)',
destination: '/:before*/:after',
permanent: true,
Janpot marked this conversation as resolved.
Show resolved Hide resolved
locale: config.i18n ? false : undefined,
} as Redirect)

return {
headers,
rewrites,
Expand Down
8 changes: 4 additions & 4 deletions packages/next/next-server/lib/router/router.ts
Expand Up @@ -4,7 +4,7 @@ import { ParsedUrlQuery } from 'querystring'
import { ComponentType } from 'react'
import { UrlObject } from 'url'
import {
normalizePathTrailingSlash,
normalizePathSlashes,
removePathTrailingSlash,
} from '../../../client/normalize-trailing-slash'
import { GoodPageCache, StyleSheetTuple } from '../../../client/page-loader'
Expand Down Expand Up @@ -80,7 +80,7 @@ function buildCancellationError() {
function addPathPrefix(path: string, prefix?: string) {
return prefix && path.startsWith('/')
? path === '/'
? normalizePathTrailingSlash(prefix)
? normalizePathSlashes(prefix)
: `${prefix}${pathNoQueryHash(path) === '/' ? path.substring(1) : path}`
: path
}
Expand Down Expand Up @@ -270,8 +270,8 @@ export function resolveHref(
return (resolveAs ? [urlAsString] : urlAsString) as string
}
try {
const finalUrl = new URL(urlAsString, base)
finalUrl.pathname = normalizePathTrailingSlash(finalUrl.pathname)
const finalUrl = new URL(urlAsString.replace(/^\/+/, '/'), base)
finalUrl.pathname = normalizePathSlashes(finalUrl.pathname)
let interpolatedAs = ''

if (
Expand Down
2 changes: 1 addition & 1 deletion test/integration/build-output/test/index.test.js
Expand Up @@ -104,7 +104,7 @@ describe('Build Output', () => {
expect(parseFloat(err404FirstLoad)).toBeCloseTo(67, 1)
expect(err404FirstLoad.endsWith('kB')).toBe(true)

expect(parseFloat(sharedByAll)).toBeCloseTo(63.5, 1)
expect(parseFloat(sharedByAll)).toBeCloseTo(63.6, 1)
expect(sharedByAll.endsWith('kB')).toBe(true)

if (_appSize.endsWith('kB')) {
Expand Down
8 changes: 8 additions & 0 deletions test/integration/custom-routes/test/index.test.js
Expand Up @@ -637,6 +637,14 @@ const runTests = (isDev = false) => {
basePath: '',
dataRoutes: [],
redirects: [
{
regex: normalizeRegEx(
'^(?:\\/((?:[^\\/]+?)(?:\\/(?:[^\\/]+?))*))?\\/(\\/+)(|.*\\/[^\\/.]+|[^\\/.]+)$'
),
source: '/:before*/{(/+)}:after(|.*/[^/.]+|[^/.]+)',
destination: '/:before*/:after',
statusCode: 308,
},
{
destination: '/:path+',
regex: normalizeRegEx(
Expand Down
3 changes: 3 additions & 0 deletions test/integration/repeated-slash-redirect/next.config.js
@@ -0,0 +1,3 @@
module.exports = {
// target: 'serverless',
}
@@ -0,0 +1,7 @@
export async function getServerSideProps({ query: { slug } }) {
return { props: { slug: slug || [] } }
}

export default function Page({ slug }) {
return <div id="page">{slug.join('/')}</div>
}
25 changes: 25 additions & 0 deletions test/integration/repeated-slash-redirect/pages/linker.js
@@ -0,0 +1,25 @@
import Link from 'next/link'
import { useRouter } from 'next/router'

export async function getServerSideProps({ query }) {
return {
props: { href: query.href || '/' },
}
}

export default function Linker({ href }) {
const router = useRouter()
const pushRoute = () => {
router.push(href)
}
return (
<div>
<Link href="/[[...slug]]" as={href}>
<a id="link">link to {href}</a>
</Link>
<button id="route-pusher" onClick={pushRoute}>
push route {href}
</button>
</div>
)
}
110 changes: 110 additions & 0 deletions test/integration/repeated-slash-redirect/test/index.test.js
@@ -0,0 +1,110 @@
/* eslint-env jest */

import cheerio from 'cheerio'
import fs from 'fs-extra'
import { join } from 'path'
import {
launchApp,
killApp,
findPort,
nextBuild,
nextStart,
fetchViaHTTP,
renderViaHTTP,
} from 'next-test-utils'
import webdriver from 'next-webdriver'

jest.setTimeout(1000 * 60 * 2)

let appDir = join(__dirname, '..')
const nextConfigPath = join(appDir, 'next.config.js')
let nextConfigContent
let appPort
let app

function getPathWithNormalizedQuery(url) {
const parsed = new URL(url, 'http://n')
if (parsed.search) {
parsed.search = `?${parsed.searchParams}`
}
return parsed.pathname + parsed.search
}

const runTests = (isDev = false) => {
const cases = [
['//hello/world', '/hello/world'],
['/hello//world', '/hello/world'],
Janpot marked this conversation as resolved.
Show resolved Hide resolved
['/hello/world//', '/hello/world'],
['/hello///world', '/hello/world'],
['/hello///world////foo', '/hello/world/foo'],
['/hello//world?foo=bar//baz', '/hello/world?foo=bar//baz'],
]

it.each(cases)('it should redirect %s to %s', async (from, to) => {
const res = await fetchViaHTTP(appPort, from)
expect(getPathWithNormalizedQuery(res.url)).toBe(
getPathWithNormalizedQuery(to)
)
})

it.each(cases)('it should rewrite href %s to %s', async (from, to) => {
const content = await renderViaHTTP(appPort, `/linker?href=${from}`)
const $ = cheerio.load(content)
expect($('#link').attr('href')).toBe(to)
})

it.each(cases)('it should navigate a href %s to %s', async (from, to) => {
const browser = await webdriver(appPort, `/linker?href=${from}`)
await browser.elementByCss('#link').click()
await browser.waitForElementByCss('#page')
const href = await browser.eval('window.location.href')
expect(getPathWithNormalizedQuery(href)).toBe(
getPathWithNormalizedQuery(to)
)
})
}

describe('Repeated trailing slashes', () => {
describe('dev mode', () => {
beforeAll(async () => {
appPort = await findPort()
app = await launchApp(appDir, appPort)
})
afterAll(() => killApp(app))
runTests(true)
})

describe('server mode', () => {
beforeAll(async () => {
await nextBuild(appDir, [], {
stdout: true,
})
appPort = await findPort()
app = await nextStart(appDir, appPort)
})
afterAll(() => killApp(app))
runTests()
})

describe('serverless mode', () => {
beforeAll(async () => {
nextConfigContent = await fs.readFile(nextConfigPath, 'utf8')
await fs.writeFile(
nextConfigPath,
nextConfigContent.replace(/\/\/ target/, 'target'),
'utf8'
)
await nextBuild(appDir, [], {
stdout: true,
})
appPort = await findPort()
app = await nextStart(appDir, appPort)
})
afterAll(async () => {
await fs.writeFile(nextConfigPath, nextConfigContent, 'utf8')
await killApp(app)
})

runTests()
})
})