Update handling for unnamed params and named patterns in custom-routes

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,
@@ -143,12 +143,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,})/)
@@ -172,6 +175,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
 

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 }

packages/next/next-server/server/router.ts

@@ -38,18 +38,26 @@ export const prepareDestination = (destination: string, params: Params) => {
   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]
-    if (
-      typeof val === 'string' &&
-      val.startsWith(':') &&
-      params[val.substr(1)]
-    ) {
-      destQuery[key] = params[val.substr(1)]
+    if (typeof val === 'string' && val.startsWith(':')) {
+      // remove any special chars for consistency /:path* /:path+
+      const paramName = val.substr(1).replace(/(\*|\+|\?)/g, '')
+
+      if (params[paramName]) {
+        destQuery[key] = params[paramName]
+      }
     }
   })
 

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',
@@ -79,6 +79,10 @@ module.exports = {
           source: '/:path/post-321',
           destination: '/with-params',
         },
+        {
+          source: '/unnamed-params/nested/(.*)/:test/(.*)',
+          destination: '/with-params',
+        },
       ]
     },
     async redirects() {
@@ -150,7 +154,7 @@ module.exports = {
         },
         {
           source: '/unnamed/(first|second)/(.*)',
-          destination: '/:1/:2',
+          destination: '/got-unnamed',
           permanent: false,
         },
         {
@@ -163,6 +167,11 @@ module.exports = {
           destination: '/thank-you-next',
           permanent: false,
         },
+        {
+          source: '/docs/:first(integrations|now-cli)/v2:second(.*)',
+          destination: '/:first/:second',
+          permanent: false,
+        },
       ]
     },
 

test/integration/custom-routes/test/index.test.js

@@ -269,7 +269,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 () => {
@@ -302,7 +302,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' },
     })
   })
 
@@ -311,6 +311,33 @@ const runTests = (isDev = false) => {
     expect(JSON.parse(data)).toEqual({ query: { name: 'hello' } })
   })
 
+  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) {
     it('should output routes-manifest successfully', async () => {
       const manifest = await fs.readJSON(
@@ -412,7 +439,7 @@ const runTests = (isDev = false) => {
             statusCode: 307,
           },
           {
-            destination: '/:1/:2',
+            destination: '/got-unnamed',
             regex: normalizeRegEx(
               '^\\/unnamed(?:\\/(first|second))(?:\\/(.*))$'
             ),
@@ -431,6 +458,14 @@ 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,
+          },
         ],
         headers: [
           {
@@ -564,9 +599,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?name=:name',
@@ -578,6 +613,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: [
           {

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\``
     )