Make sure routeKeys are PCRE compliant (#14987)

This adds additional checks against the routeKeys used to build the named regexes for dynamic routes to ensure they follow PCRE rules for named capture groups

x-ref: vercel/vercel#4813

packages/next/build/index.ts

@@ -17,6 +17,7 @@ import { fileExists } from '../lib/file-exists'
 import { findPagesDir } from '../lib/find-pages-dir'
 import loadCustomRoutes, {
   getRedirectStatus,
+  normalizeRouteRegex,
   Redirect,
   RouteType,
 } from '../lib/load-custom-routes'
@@ -273,7 +274,7 @@ export default async function build(dir: string, conf = null): Promise<void> {
             permanent: undefined,
           }
         : {}),
-      regex: routeRegex.source,
+      regex: normalizeRouteRegex(routeRegex.source),
     }
   }
 
@@ -291,7 +292,7 @@ export default async function build(dir: string, conf = null): Promise<void> {
         const routeRegex = getRouteRegex(page)
         return {
           page,
-          regex: routeRegex.re.source,
+          regex: normalizeRouteRegex(routeRegex.re.source),
           routeKeys: routeRegex.routeKeys,
           namedRegex: routeRegex.namedRegex,
         }
@@ -611,23 +612,24 @@ export default async function build(dir: string, conf = null): Promise<void> {
       if (isDynamicRoute(page)) {
         const routeRegex = getRouteRegex(dataRoute.replace(/\.json$/, ''))
 
-        dataRouteRegex = routeRegex.re.source.replace(
-          /\(\?:\\\/\)\?\$$/,
-          '\\.json$'
+        dataRouteRegex = normalizeRouteRegex(
+          routeRegex.re.source.replace(/\(\?:\\\/\)\?\$$/, '\\.json$')
         )
         namedDataRouteRegex = routeRegex.namedRegex!.replace(
           /\(\?:\/\)\?\$$/,
           '\\.json$'
         )
         routeKeys = routeRegex.routeKeys
       } else {
-        dataRouteRegex = new RegExp(
-          `^${path.posix.join(
-            '/_next/data',
-            escapeStringRegexp(buildId),
-            `${pagePath}.json`
-          )}$`
-        ).source
+        dataRouteRegex = normalizeRouteRegex(
+          new RegExp(
+            `^${path.posix.join(
+              '/_next/data',
+              escapeStringRegexp(buildId),
+              `${pagePath}.json`
+            )}$`
+          ).source
+        )
       }
 
       return {
@@ -887,14 +889,17 @@ export default async function build(dir: string, conf = null): Promise<void> {
       )
 
       finalDynamicRoutes[tbdRoute] = {
-        routeRegex: getRouteRegex(tbdRoute).re.source,
+        routeRegex: normalizeRouteRegex(getRouteRegex(tbdRoute).re.source),
         dataRoute,
         fallback: ssgFallbackPages.has(tbdRoute)
           ? `${normalizedRoute}.html`
           : false,
-        dataRouteRegex: getRouteRegex(
-          dataRoute.replace(/\.json$/, '')
-        ).re.source.replace(/\(\?:\\\/\)\?\$$/, '\\.json$'),
+        dataRouteRegex: normalizeRouteRegex(
+          getRouteRegex(dataRoute.replace(/\.json$/, '')).re.source.replace(
+            /\(\?:\\\/\)\?\$$/,
+            '\\.json$'
+          )
+        ),
       }
     })
     const prerenderManifest: PrerenderManifest = {

packages/next/lib/load-custom-routes.ts

@@ -29,6 +29,11 @@ export function getRedirectStatus(route: Redirect): number {
   )
 }
 
+export function normalizeRouteRegex(regex: string) {
+  // clean up un-necessary escaping from regex.source which turns / into \\/
+  return regex.replace(/\\\//g, '/')
+}
+
 function checkRedirect(
   route: Redirect
 ): { invalidParts: string[]; hadInvalidStatus: boolean } {

packages/next/next-server/lib/router/utils/route-regex.ts

@@ -51,6 +51,25 @@ export function getRouteRegex(
   // dead code eliminate for browser since it's only needed
   // while generating routes-manifest
   if (typeof window === 'undefined') {
+    let routeKeyCharCode = 97
+    let routeKeyCharLength = 1
+
+    // builds a minimal routeKey using only a-z and minimal number of characters
+    const getSafeRouteKey = () => {
+      let routeKey = ''
+
+      for (let i = 0; i < routeKeyCharLength; i++) {
+        routeKey += String.fromCharCode(routeKeyCharCode)
+        routeKeyCharCode++
+
+        if (routeKeyCharCode > 122) {
+          routeKeyCharLength++
+          routeKeyCharCode = 97
+        }
+      }
+      return routeKey
+    }
+
     const routeKeys: { [named: string]: string } = {}
 
     let namedParameterizedRoute = segments
@@ -59,7 +78,22 @@ export function getRouteRegex(
           const { key, optional, repeat } = parseParameter(segment.slice(1, -1))
           // replace any non-word characters since they can break
           // the named regex
-          const cleanedKey = key.replace(/\W/g, '')
+          let cleanedKey = key.replace(/\W/g, '')
+          let invalidKey = false
+
+          // check if the key is still invalid and fallback to using a known
+          // safe key
+          if (cleanedKey.length === 0 || cleanedKey.length > 30) {
+            invalidKey = true
+          }
+          if (!isNaN(parseInt(cleanedKey.substr(0, 1)))) {
+            invalidKey = true
+          }
+
+          if (invalidKey) {
+            cleanedKey = getSafeRouteKey()
+          }
+
           routeKeys[cleanedKey] = key
           return repeat
             ? optional

test/integration/dynamic-routing/pages/b/[123].js

@@ -0,0 +1,13 @@
+export const getServerSideProps = ({ params }) => {
+  console.log({ params })
+
+  return {
+    props: {
+      params,
+    },
+  }
+}
+
+export default function Page(props) {
+  return <p id="props">{JSON.stringify(props)}</p>
+}

test/integration/dynamic-routing/pages/c/[alongparamnameshouldbeallowedeventhoughweird].js

@@ -0,0 +1,13 @@
+export const getServerSideProps = ({ params }) => {
+  console.log({ params })
+
+  return {
+    props: {
+      params,
+    },
+  }
+}
+
+export default function Page(props) {
+  return <p id="props">{JSON.stringify(props)}</p>
+}

test/integration/dynamic-routing/test/index.test.js

@@ -552,6 +552,30 @@ function runTests(dev) {
         rewrites: [],
         redirects: expect.arrayContaining([]),
         dataRoutes: [
+          {
+            dataRouteRegex: `^\\/_next\\/data\\/${escapeRegex(
+              buildId
+            )}\\/b\\/([^\\/]+?)\\.json$`,
+            namedDataRouteRegex: `^/_next/data/${escapeRegex(
+              buildId
+            )}/b/(?<a>[^/]+?)\\.json$`,
+            page: '/b/[123]',
+            routeKeys: {
+              a: '123',
+            },
+          },
+          {
+            dataRouteRegex: `^\\/_next\\/data\\/${escapeRegex(
+              buildId
+            )}\\/c\\/([^\\/]+?)\\.json$`,
+            namedDataRouteRegex: `^/_next/data/${escapeRegex(
+              buildId
+            )}/c/(?<a>[^/]+?)\\.json$`,
+            page: '/c/[alongparamnameshouldbeallowedeventhoughweird]',
+            routeKeys: {
+              a: 'alongparamnameshouldbeallowedeventhoughweird',
+            },
+          },
           {
             namedDataRouteRegex: `^/_next/data/${escapeRegex(
               buildId
@@ -596,6 +620,14 @@ function runTests(dev) {
           },
         ],
         dynamicRoutes: [
+          {
+            namedRegex: '^/b/(?<a>[^/]+?)(?:/)?$',
+            page: '/b/[123]',
+            regex: normalizeRegEx('^\\/b\\/([^\\/]+?)(?:\\/)?$'),
+            routeKeys: {
+              a: '123',
+            },
+          },
           {
             namedRegex: `^/blog/(?<name>[^/]+?)/comment/(?<id>[^/]+?)(?:/)?$`,
             page: '/blog/[name]/comment/[id]',
@@ -607,6 +639,14 @@ function runTests(dev) {
               id: 'id',
             },
           },
+          {
+            namedRegex: '^/c/(?<a>[^/]+?)(?:/)?$',
+            page: '/c/[alongparamnameshouldbeallowedeventhoughweird]',
+            regex: normalizeRegEx('^\\/c\\/([^\\/]+?)(?:\\/)?$'),
+            routeKeys: {
+              a: 'alongparamnameshouldbeallowedeventhoughweird',
+            },
+          },
           {
             namedRegex: '^/catchall\\-dash/(?<helloworld>.+?)(?:/)?$',
             page: '/catchall-dash/[...hello-world]',