Improve server performance by skipping decode/re-encode (vercel#17323)

Prior to this pull request, Next.js would immediately decode all URLs sent to its server (via `path-match`).

This was rarely needed, and Next.js would typically re-encode the incoming request right away (see all the `encodeURIComponent`s removed in PR diff). This adds unnecessary performance overhead.

Long term, this will also help prevent weird encoding edge-cases like vercel#10004, vercel#10022, vercel#11371, et al.

---

No new tests are necessary for this change because we've extensively tested these edge cases with existing tests.
One test was updated to reflect that we skip decoding in a 404 scenario.

Let's see if all the existing tests pass!

packages/next/next-server/lib/router/utils/path-match.ts

@@ -2,13 +2,14 @@ import * as pathToRegexp from 'next/dist/compiled/path-to-regexp'
 
 export { pathToRegexp }
 
-export const matcherOptions = {
+export const matcherOptions: pathToRegexp.TokensToRegexpOptions &
+  pathToRegexp.ParseOptions = {
   sensitive: false,
   delimiter: '/',
-  decode: decodeParam,
 }
 
-export const customRouteMatcherOptions = {
+export const customRouteMatcherOptions: pathToRegexp.TokensToRegexpOptions &
+  pathToRegexp.ParseOptions = {
   ...matcherOptions,
   strict: true,
 }
@@ -21,11 +22,7 @@ export default (customRoute = false) => {
       keys,
       customRoute ? customRouteMatcherOptions : matcherOptions
     )
-    const matcher = pathToRegexp.regexpToFunction(
-      matcherRegex,
-      keys,
-      matcherOptions
-    )
+    const matcher = pathToRegexp.regexpToFunction(matcherRegex, keys)
 
     return (pathname: string | null | undefined, params?: any) => {
       const res = pathname == null ? false : matcher(pathname)
@@ -47,13 +44,3 @@ export default (customRoute = false) => {
     }
   }
 }
-
-function decodeParam(param: string) {
-  try {
-    return decodeURIComponent(param)
-  } catch (_) {
-    const err: Error & { code?: string } = new Error('failed to decode param')
-    err.code = 'DECODE_FAILED'
-    throw err
-  }
-}

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

@@ -4,7 +4,11 @@ import chalk from 'next/dist/compiled/chalk'
 import { IncomingMessage, ServerResponse } from 'http'
 import Proxy from 'next/dist/compiled/http-proxy'
 import { join, relative, resolve, sep } from 'path'
-import { parse as parseQs, ParsedUrlQuery } from 'querystring'
+import {
+  parse as parseQs,
+  stringify as stringifyQs,
+  ParsedUrlQuery,
+} from 'querystring'
 import { format as formatUrl, parse as parseUrl, UrlWithParsedQuery } from 'url'
 import { PrerenderManifest } from '../../build'
 import {
@@ -352,11 +356,7 @@ export default class Server {
             match: route('/static/:path*'),
             name: 'static catchall',
             fn: async (req, res, params, parsedUrl) => {
-              const p = join(
-                this.dir,
-                'static',
-                ...(params.path || []).map(encodeURIComponent)
-              )
+              const p = join(this.dir, 'static', ...params.path)
               await this.serveStatic(req, res, p, parsedUrl)
               return {
                 finished: true,
@@ -428,11 +428,7 @@ export default class Server {
 
           // re-create page's pathname
           const pathname = getRouteFromAssetPath(
-            `/${params.path
-              // we need to re-encode the params since they are decoded
-              // by path-match and we are re-building the URL
-              .map((param: string) => encodeURIComponent(param))
-              .join('/')}`,
+            `/${params.path.join('/')}`,
             '.json'
           )
 
@@ -559,6 +555,14 @@ export default class Server {
             false,
             getCustomRouteBasePath(redirectRoute)
           )
+
+          const { query } = parsedDestination
+          delete parsedDestination.query
+
+          parsedDestination.search = stringifyQs(query, undefined, undefined, {
+            encodeURIComponent: (str: string) => str,
+          } as any)
+
           const updatedDestination = formatUrl(parsedDestination)
 
           res.setHeader('Location', updatedDestination)
@@ -597,6 +601,15 @@ export default class Server {
 
           // external rewrite, proxy it
           if (parsedDestination.protocol) {
+            const { query } = parsedDestination
+            delete parsedDestination.query
+            parsedDestination.search = stringifyQs(
+              query,
+              undefined,
+              undefined,
+              { encodeURIComponent: (str) => str }
+            )
+
             const target = formatUrl(parsedDestination)
             const proxy = new Proxy({
               target,
@@ -775,7 +788,9 @@ export default class Server {
 
   protected generatePublicRoutes(): Route[] {
     const publicFiles = new Set(
-      recursiveReadDirSync(this.publicDir).map((p) => p.replace(/\\/g, '/'))
+      recursiveReadDirSync(this.publicDir).map((p) =>
+        encodeURI(p.replace(/\\/g, '/'))
+      )
     )
 
     return [
@@ -798,8 +813,7 @@ export default class Server {
             await this.serveStatic(
               req,
               res,
-              // we need to re-encode it since send decodes it
-              join(this.publicDir, ...pathParts.map(encodeURIComponent)),
+              join(this.publicDir, ...pathParts),
               parsedUrl
             )
             return {
@@ -1323,6 +1337,11 @@ export default class Server {
       }
     } catch (err) {
       this.logError(err)
+
+      if (err && err.code === 'DECODE_FAILED') {
+        res.statusCode = 400
+        return await this.renderErrorToHTML(err, req, res, pathname, query)
+      }
       res.statusCode = 500
       return await this.renderErrorToHTML(err, req, res, pathname, query)
     }

packages/next/server/hot-reloader.ts

@@ -215,7 +215,22 @@ export default class HotReloader {
         return {}
       }
 
-      const page = denormalizePagePath(`/${params.path.join('/')}`)
+      let decodedPagePath: string
+
+      try {
+        decodedPagePath = `/${params.path
+          .map((param) => decodeURIComponent(param))
+          .join('/')}`
+      } catch (_) {
+        const err: Error & { code?: string } = new Error(
+          'failed to decode param'
+        )
+        err.code = 'DECODE_FAILED'
+        throw err
+      }
+
+      const page = denormalizePagePath(decodedPagePath)
+
       if (page === '/_error' || BLOCKED_PAGES.indexOf(page) === -1) {
         try {
           await this.ensurePage(page)

packages/next/server/next-dev-server.ts

@@ -336,7 +336,17 @@ export default class DevServer extends Server {
     const path = `/${pathParts.join('/')}`
     // check for a public file, throwing error if there's a
     // conflicting page
-    if (await this.hasPublicFile(path)) {
+    let decodedPath: string
+
+    try {
+      decodedPath = decodeURIComponent(path)
+    } catch (_) {
+      const err: Error & { code?: string } = new Error('failed to decode param')
+      err.code = 'DECODE_FAILED'
+      throw err
+    }
+
+    if (await this.hasPublicFile(decodedPath)) {
       if (await this.hasPage(pathname!)) {
         const err = new Error(
           `A conflicting public file and page file was found for path ${pathname} https://err.sh/vercel/next.js/conflicting-public-file-page`
@@ -660,7 +670,7 @@ export default class DevServer extends Server {
     res: ServerResponse,
     pathParts: string[]
   ): Promise<void> {
-    const p = pathJoin(this.publicDir, ...pathParts.map(encodeURIComponent))
+    const p = pathJoin(this.publicDir, ...pathParts)
     return this.serveStatic(req, res, p)
   }
 

test/integration/production/pages/invalid-param/[slug].js

@@ -0,0 +1,13 @@
+import { useRouter } from 'next/router'
+
+export default function Page() {
+  return <p>hello {useRouter().query}</p>
+}
+
+export const getServerSideProps = () => {
+  return {
+    props: {
+      hello: 'world',
+    },
+  }
+}

test/integration/production/test/index.test.js

@@ -710,7 +710,7 @@ describe('Production Usage', () => {
   })
 
   it('should handle failed param decoding', async () => {
-    const html = await renderViaHTTP(appPort, '/%DE~%C7%1fY/')
+    const html = await renderViaHTTP(appPort, '/invalid-param/%DE~%C7%1fY/')
     expect(html).toMatch(/400/)
     expect(html).toMatch(/Bad Request/)
   })