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

Ensure has segments are allowed in destination #23588

Merged
merged 2 commits into from Apr 1, 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
34 changes: 32 additions & 2 deletions packages/next/lib/load-custom-routes.ts
Expand Up @@ -8,6 +8,7 @@ import {
} from '../next-server/lib/constants'
import { execOnce } from '../next-server/lib/utils'
import * as Log from '../build/output/log'
import { getSafeParamName } from '../next-server/lib/router/utils/prepare-destination'

export type RouteHas =
| {
Expand Down Expand Up @@ -325,6 +326,34 @@ function checkCustomRoutes(
}
sourceTokens = tokens
}
const hasSegments = new Set<string>()

if (route.has) {
for (const hasItem of route.has) {
if (!hasItem.value && hasItem.key) {
hasSegments.add(hasItem.key)
}

if (hasItem.value) {
const matcher = new RegExp(`^${hasItem.value}$`)
const matches = matcher.exec('')

if (matches) {
if (matches.groups) {
Object.keys(matches.groups).forEach((groupKey) => {
const safeKey = getSafeParamName(groupKey)

if (safeKey && matches.groups![groupKey]) {
hasSegments.add(safeKey)
}
})
} else {
hasSegments.add(hasItem.key || 'host')
}
}
}
}
}

// make sure no unnamed patterns are attempted to be used in the
// destination as this can cause confusion and is not allowed
Expand Down Expand Up @@ -369,15 +398,16 @@ function checkCustomRoutes(
for (const token of destTokens!) {
if (
typeof token === 'object' &&
!sourceSegments.has(token.name)
!sourceSegments.has(token.name) &&
!hasSegments.has(token.name as string)
) {
invalidDestSegments.add(token.name)
}
}

if (invalidDestSegments.size) {
invalidParts.push(
`\`destination\` has segments not in \`source\` (${[
`\`destination\` has segments not in \`source\` or \`has\` (${[
...invalidDestSegments,
].join(', ')})`
)
Expand Down
Expand Up @@ -9,7 +9,7 @@ type Params = { [param: string]: any }

// ensure only a-zA-Z are used for param names for proper interpolating
// with path-to-regexp
const getSafeParamName = (paramName: string) => {
export const getSafeParamName = (paramName: string) => {
let newParamName = ''

for (let i = 0; i < paramName.length; i++) {
Expand Down
10 changes: 10 additions & 0 deletions test/integration/custom-routes/next.config.js
Expand Up @@ -149,6 +149,16 @@ module.exports = {
],
destination: '/with-params?host=1',
},
{
source: '/has-rewrite-5',
has: [
{
type: 'query',
key: 'hasParam',
},
],
destination: '/:hasParam',
},
],
beforeFiles: [
{
Expand Down
27 changes: 27 additions & 0 deletions test/integration/custom-routes/test/index.test.js
Expand Up @@ -692,6 +692,22 @@ const runTests = (isDev = false) => {
expect(res2.status).toBe(404)
})

it('should pass has segment for rewrite correctly', async () => {
const res1 = await fetchViaHTTP(appPort, '/has-rewrite-5')
expect(res1.status).toBe(404)

const res = await fetchViaHTTP(appPort, '/has-rewrite-5', {
hasParam: 'with-params',
})

expect(res.status).toBe(200)
const $ = cheerio.load(await res.text())

expect(JSON.parse($('#query').text())).toEqual({
hasParam: 'with-params',
})
})

it('should match has rewrite correctly before files', async () => {
const res1 = await fetchViaHTTP(appPort, '/hello')
expect(res1.status).toBe(200)
Expand Down Expand Up @@ -1509,6 +1525,17 @@ const runTests = (isDev = false) => {
regex: '^\\/has-rewrite-4$',
source: '/has-rewrite-4',
},
{
destination: '/:hasParam',
has: [
{
key: 'hasParam',
type: 'query',
},
],
regex: normalizeRegEx('^\\/has-rewrite-5$'),
source: '/has-rewrite-5',
},
],
fallback: [],
},
Expand Down
Expand Up @@ -542,7 +542,7 @@ const runTests = () => {
const stderr = await getStderr()

expect(stderr).toContain(
`\`destination\` has segments not in \`source\` (id) for route {"source":"/feedback/:type","destination":"/feedback/:id"}`
`\`destination\` has segments not in \`source\` or \`has\` (id) for route {"source":"/feedback/:type","destination":"/feedback/:id"}`
)
})
}
Expand Down