From 6616a371e8a733f58a9e222f9e223319ef2abb00 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Tue, 10 Mar 2020 15:09:35 -0500 Subject: [PATCH] Update handling for patterns in custom routes (#10523) * Update handling for unnamed params and named patterns in custom-routes * Update query handling to match Now --- packages/next/lib/check-custom-routes.ts | 35 ++++++- .../next/next-server/server/lib/path-match.ts | 12 +-- .../next/next-server/server/next-server.ts | 3 +- packages/next/next-server/server/router.ts | 50 +++++++--- test/integration/custom-routes/next.config.js | 20 +++- .../custom-routes/test/index.test.js | 99 +++++++++++++++++-- .../invalid-custom-routes/test/index.test.js | 19 ++++ 7 files changed, 202 insertions(+), 36 deletions(-) diff --git a/packages/next/lib/check-custom-routes.ts b/packages/next/lib/check-custom-routes.ts index c3a538901323883..d51747c90335239 100644 --- a/packages/next/lib/check-custom-routes.ts +++ b/packages/next/lib/check-custom-routes.ts @@ -1,4 +1,4 @@ -import { match as regexpMatch } from 'path-to-regexp' +import * as pathToRegexp from 'path-to-regexp' import { PERMANENT_REDIRECT_STATUS, TEMPORARY_REDIRECT_STATUS, @@ -150,12 +150,15 @@ export default function checkCustomRoutes( invalidParts.push(...result.invalidParts) } + let sourceTokens: pathToRegexp.Token[] | undefined + if (typeof route.source === 'string' && route.source.startsWith('/')) { // only show parse error if we didn't already show error // for not being a string try { // Make sure we can parse the source properly - regexpMatch(route.source) + sourceTokens = pathToRegexp.parse(route.source) + pathToRegexp.tokensToRegexp(sourceTokens) } catch (err) { // If there is an error show our err.sh but still show original error or a formatted one if we can const errMatches = err.message.match(/at (\d{0,})/) @@ -179,6 +182,34 @@ export default function checkCustomRoutes( } } + // make sure no unnamed patterns are attempted to be used in the + // destination as this can cause confusion and is not allowed + if (typeof (route as Rewrite).destination === 'string') { + if ( + (route as Rewrite).destination.startsWith('/') && + Array.isArray(sourceTokens) + ) { + const unnamedInDest = new Set() + + for (const token of sourceTokens) { + if (typeof token === 'object' && typeof token.name === 'number') { + const unnamedIndex = `:${token.name}` + if ((route as Rewrite).destination.includes(unnamedIndex)) { + unnamedInDest.add(unnamedIndex) + } + } + } + + if (unnamedInDest.size > 0) { + invalidParts.push( + `\`destination\` has unnamed params ${[...unnamedInDest].join( + ', ' + )}` + ) + } + } + } + const hasInvalidKeys = invalidKeys.length > 0 const hasInvalidParts = invalidParts.length > 0 diff --git a/packages/next/next-server/server/lib/path-match.ts b/packages/next/next-server/server/lib/path-match.ts index 199268b13381baa..81e1fb5fe048bec 100644 --- a/packages/next/next-server/server/lib/path-match.ts +++ b/packages/next/next-server/server/lib/path-match.ts @@ -25,19 +25,13 @@ export default (customRoute = false) => { } if (customRoute) { - const newParams: { [k: string]: string } = {} for (const key of keys) { - // unnamed matches should always be a number while named - // should be a string + // unnamed params should be removed as they + // are not allowed to be used in the destination if (typeof key.name === 'number') { - newParams[key.name + 1 + ''] = (res.params as any)[key.name + ''] - delete (res.params as any)[key.name + ''] + delete (res.params as any)[key.name] } } - res.params = { - ...res.params, - ...newParams, - } } return { ...params, ...res.params } diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 294c6abf3eaa028..f9ecd2ed57deadf 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -488,7 +488,8 @@ export default class Server { fn: async (_req, res, params, _parsedUrl) => { const { parsedDestination } = prepareDestination( route.destination, - params + params, + true ) const updatedDestination = formatUrl(parsedDestination) diff --git a/packages/next/next-server/server/router.ts b/packages/next/next-server/server/router.ts index 73eced0696bdd3c..c738a23bab7fab3 100644 --- a/packages/next/next-server/server/router.ts +++ b/packages/next/next-server/server/router.ts @@ -1,7 +1,6 @@ import { IncomingMessage, ServerResponse } from 'http' import { parse as parseUrl, UrlWithParsedQuery } from 'url' import { compile as compilePathToRegex } from 'path-to-regexp' -import { stringify as stringifyQs } from 'querystring' import pathMatch from './lib/path-match' export const route = pathMatch() @@ -34,24 +33,53 @@ export type DynamicRoutes = Array<{ page: string; match: RouteMatch }> export type PageChecker = (pathname: string) => Promise -export const prepareDestination = (destination: string, params: Params) => { +export const prepareDestination = ( + destination: string, + params: Params, + isRedirect?: boolean +) => { const parsedDestination = parseUrl(destination, true) const destQuery = parsedDestination.query let destinationCompiler = compilePathToRegex( - `${parsedDestination.pathname!}${parsedDestination.hash || ''}` + `${parsedDestination.pathname!}${parsedDestination.hash || ''}`, + // we don't validate while compiling the destination since we should + // have already validated before we got to this point and validating + // breaks compiling destinations with named pattern params from the source + // e.g. /something:hello(.*) -> /another/:hello is broken with validation + // since compile validation is meant for reversing and not for inserting + // params from a separate path-regex into another + { validate: false } ) let newUrl - Object.keys(destQuery).forEach(key => { - const val = destQuery[key] + // update any params in query values + for (const [key, strOrArray] of Object.entries(destQuery)) { + let value = Array.isArray(strOrArray) ? strOrArray[0] : strOrArray + if (value) { + const queryCompiler = compilePathToRegex(value, { validate: false }) + value = queryCompiler(params) + } + destQuery[key] = value + } + + // add params to query + for (const [name, value] of Object.entries(params)) { if ( - typeof val === 'string' && - val.startsWith(':') && - params[val.substr(1)] + isRedirect && + new RegExp(`:${name}(?!\\w)`).test( + parsedDestination.pathname + (parsedDestination.hash || '') + ) ) { - destQuery[key] = params[val.substr(1)] + // Don't add segment to query if used in destination + // and it's a redirect so that we don't pollute the query + // with unwanted values + continue } - }) + + if (!(name in destQuery)) { + destQuery[name] = Array.isArray(value) ? value.join('/') : value + } + } try { newUrl = encodeURI(destinationCompiler(params)) @@ -59,8 +87,8 @@ export const prepareDestination = (destination: string, params: Params) => { const [pathname, hash] = newUrl.split('#') parsedDestination.pathname = pathname parsedDestination.hash = `${hash ? '#' : ''}${hash || ''}` - parsedDestination.search = stringifyQs(parsedDestination.query) parsedDestination.path = `${pathname}${parsedDestination.search}` + delete parsedDestination.search } catch (err) { if (err.message.match(/Expected .*? to not repeat, but got an array/)) { throw new Error( diff --git a/test/integration/custom-routes/next.config.js b/test/integration/custom-routes/next.config.js index 3ff2dca5755d5a6..ae854925d8dddd6 100644 --- a/test/integration/custom-routes/next.config.js +++ b/test/integration/custom-routes/next.config.js @@ -68,8 +68,8 @@ module.exports = { destination: '/api/hello', }, { - source: '/api-hello-regex/(.*)', - destination: '/api/hello?name=:1', + source: '/api-hello-regex/:first(.*)', + destination: '/api/hello?name=:first*', }, { source: '/api-hello-param/:name', @@ -83,6 +83,10 @@ module.exports = { source: '/:path/post-321', destination: '/with-params', }, + { + source: '/unnamed-params/nested/(.*)/:test/(.*)', + destination: '/with-params', + }, ] }, async redirects() { @@ -159,7 +163,7 @@ module.exports = { }, { source: '/unnamed/(first|second)/(.*)', - destination: '/:1/:2', + destination: '/got-unnamed', permanent: false, }, { @@ -172,6 +176,16 @@ module.exports = { destination: '/thank-you-next', permanent: false, }, + { + source: '/docs/:first(integrations|now-cli)/v2:second(.*)', + destination: '/:first/:second', + permanent: false, + }, + { + source: '/catchall-redirect/:path*', + destination: '/somewhere', + permanent: false, + }, ] }, diff --git a/test/integration/custom-routes/test/index.test.js b/test/integration/custom-routes/test/index.test.js index a3436c75ec52068..d574b1f46002e75 100644 --- a/test/integration/custom-routes/test/index.test.js +++ b/test/integration/custom-routes/test/index.test.js @@ -104,19 +104,39 @@ const runTests = (isDev = false) => { redirect: 'manual', } ) - const { pathname, hash } = url.parse(res.headers.get('location')) + const { pathname, hash, query } = url.parse( + res.headers.get('location'), + true + ) expect(res.status).toBe(301) expect(pathname).toBe('/docs/v2/network/status-codes') expect(hash).toBe('#500') + expect(query).toEqual({}) }) it('should redirect successfully with provided statusCode', async () => { const res = await fetchViaHTTP(appPort, '/redirect2', undefined, { redirect: 'manual', }) - const { pathname } = url.parse(res.headers.get('location')) + const { pathname, query } = url.parse(res.headers.get('location'), true) expect(res.status).toBe(301) expect(pathname).toBe('/') + expect(query).toEqual({}) + }) + + it('should redirect successfully with catchall', async () => { + const res = await fetchViaHTTP( + appPort, + '/catchall-redirect/hello/world', + undefined, + { + redirect: 'manual', + } + ) + const { pathname, query } = url.parse(res.headers.get('location'), true) + expect(res.status).toBe(307) + expect(pathname).toBe('/somewhere') + expect(query).toEqual({ path: 'hello/world' }) }) it('should server static files through a rewrite', async () => { @@ -158,7 +178,12 @@ const runTests = (isDev = false) => { const { pathname, query } = url.parse(res.headers.get('location'), true) expect(res.status).toBe(307) expect(pathname).toBe('/with-params') - expect(query).toEqual({ first: 'hello', second: 'world' }) + expect(query).toEqual({ + first: 'hello', + second: 'world', + name: 'world', + section: 'hello', + }) }) it('should overwrite param values correctly', async () => { @@ -260,7 +285,7 @@ const runTests = (isDev = false) => { it('should support proxying to external resource', async () => { const res = await fetchViaHTTP(appPort, '/proxy-me/first') expect(res.status).toBe(200) - expect([...externalServerHits]).toEqual(['/first']) + expect([...externalServerHits]).toEqual(['/first?path=first']) expect(await res.text()).toContain('hi from external') }) @@ -270,7 +295,7 @@ const runTests = (isDev = false) => { }) const { pathname } = url.parse(res.headers.get('location') || '') expect(res.status).toBe(307) - expect(pathname).toBe('/first/final') + expect(pathname).toBe('/got-unnamed') }) it('should support named like unnamed parameters correctly', async () => { @@ -303,7 +328,7 @@ const runTests = (isDev = false) => { it('should handle api rewrite with un-named param successfully', async () => { const data = await renderViaHTTP(appPort, '/api-hello-regex/hello/world') expect(JSON.parse(data)).toEqual({ - query: { '1': 'hello/world', name: 'hello/world' }, + query: { name: 'hello/world', first: 'hello/world' }, }) }) @@ -324,10 +349,41 @@ const runTests = (isDev = false) => { } ) - const { pathname, hostname } = url.parse(res.headers.get('location') || '') + const { pathname, hostname, query } = url.parse( + res.headers.get('location') || '', + true + ) expect(res.status).toBe(307) expect(pathname).toBe(encodeURI('/\\google.com/about')) expect(hostname).not.toBe('google.com') + expect(query).toEqual({}) + }) + + it('should handle unnamed parameters with multi-match successfully', async () => { + const html = await renderViaHTTP( + appPort, + '/unnamed-params/nested/first/second/hello/world' + ) + const params = JSON.parse( + cheerio + .load(html)('p') + .text() + ) + expect(params).toEqual({ test: 'hello' }) + }) + + it('should handle named regex parameters with multi-match successfully', async () => { + const res = await fetchViaHTTP( + appPort, + '/docs/integrations/v2-some/thing', + undefined, + { + redirect: 'manual', + } + ) + const { pathname } = url.parse(res.headers.get('location') || '') + expect(res.status).toBe(307) + expect(pathname).toBe('/integrations/-some/thing') }) if (!isDev) { @@ -439,7 +495,7 @@ const runTests = (isDev = false) => { statusCode: 307, }, { - destination: '/:1/:2', + destination: '/got-unnamed', regex: normalizeRegEx( '^\\/unnamed(?:\\/(first|second))(?:\\/(.*))$' ), @@ -458,6 +514,22 @@ const runTests = (isDev = false) => { source: '/redirect-override', statusCode: 307, }, + { + destination: '/:first/:second', + regex: normalizeRegEx( + '^\\/docs(?:\\/(integrations|now-cli))\\/v2(.*)$' + ), + source: '/docs/:first(integrations|now-cli)/v2:second(.*)', + statusCode: 307, + }, + { + destination: '/somewhere', + regex: normalizeRegEx( + '^\\/catchall-redirect(?:\\/((?:[^\\/]+?)(?:\\/(?:[^\\/]+?))*))?$' + ), + source: '/catchall-redirect/:path*', + statusCode: 307, + }, ], headers: [ { @@ -591,9 +663,9 @@ const runTests = (isDev = false) => { source: '/api-hello', }, { - destination: '/api/hello?name=:1', + destination: '/api/hello?name=:first*', regex: normalizeRegEx('^\\/api-hello-regex(?:\\/(.*))$'), - source: '/api-hello-regex/(.*)', + source: '/api-hello-regex/:first(.*)', }, { destination: '/api/hello?hello=:name', @@ -610,6 +682,13 @@ const runTests = (isDev = false) => { regex: normalizeRegEx('^(?:\\/([^\\/]+?))\\/post-321$'), source: '/:path/post-321', }, + { + destination: '/with-params', + regex: normalizeRegEx( + '^\\/unnamed-params\\/nested(?:\\/(.*))(?:\\/([^\\/]+?))(?:\\/(.*))$' + ), + source: '/unnamed-params/nested/(.*)/:test/(.*)', + }, ], dynamicRoutes: [ { diff --git a/test/integration/invalid-custom-routes/test/index.test.js b/test/integration/invalid-custom-routes/test/index.test.js index 92b0b166309e9d5..c9d731be9d71e26 100644 --- a/test/integration/invalid-custom-routes/test/index.test.js +++ b/test/integration/invalid-custom-routes/test/index.test.js @@ -59,6 +59,12 @@ const runTests = () => { destination: '/another', permanent: 'yes', }, + { + // unnamed in destination + source: '/hello/world/(.*)', + destination: '/:0', + permanent: true, + }, // invalid objects null, 'string', @@ -99,6 +105,10 @@ const runTests = () => { `\`permanent\` is not set to \`true\` or \`false\` for route {"source":"/hello","destination":"/another","permanent":"yes"}` ) + expect(stderr).toContain( + `\`destination\` has unnamed params :0 for route {"source":"/hello/world/(.*)","destination":"/:0","permanent":true}` + ) + expect(stderr).toContain( `The route null is not a valid object with \`source\` and \`destination\`` ) @@ -142,6 +152,11 @@ const runTests = () => { source: '/feedback/(?!general)', destination: '/feedback/general', }, + { + // unnamed in destination + source: '/hello/world/(.*)', + destination: '/:0', + }, // invalid objects null, 'string', @@ -174,6 +189,10 @@ const runTests = () => { `Error parsing \`/feedback/(?!general)\` https://err.sh/zeit/next.js/invalid-route-source` ) + expect(stderr).toContain( + `\`destination\` has unnamed params :0 for route {"source":"/hello/world/(.*)","destination":"/:0"}` + ) + expect(stderr).toContain( `The route null is not a valid object with \`source\` and \`destination\`` )