Update handling for patterns in custom routes (vercel#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

@@ -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
 

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/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)
 

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,33 +33,62 @@ export type DynamicRoutes = Array<{ page: string; match: RouteMatch }>
 
 export type PageChecker = (pathname: string) => Promise<boolean>
 
-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))
 
     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(

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() {
@@ -155,7 +159,7 @@ module.exports = {
         },
         {
           source: '/unnamed/(first|second)/(.*)',
-          destination: '/:1/:2',
+          destination: '/got-unnamed',
           permanent: false,
         },
         {
@@ -168,6 +172,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,
+        },
       ]
     },