Ensure decode error in minimal mode responds with 400 not 500 (#31037)

This ensures we handle decode failures in minimal mode as a 400 response (bad request) instead of a 500 (server error). ## Bug - [ ] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md`
vercel · Nov 5, 2021 · fab345d · fab345d
1 parent b75b2f0
commit fab345d
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 36 deletions.
diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts
@@ -425,51 +425,61 @@ export default class Server {
         rewrites: combinedRewrites,
       })
 
-      utils.handleRewrites(req, parsedUrl)
-
-      // interpolate dynamic params and normalize URL if needed
-      if (pageIsDynamic) {
-        let params: ParsedUrlQuery | false = {}
+      try {
+        utils.handleRewrites(req, parsedUrl)
 
-        Object.assign(parsedUrl.query, query)
-        const paramsResult = utils.normalizeDynamicRouteParams(parsedUrl.query)
+        // interpolate dynamic params and normalize URL if needed
+        if (pageIsDynamic) {
+          let params: ParsedUrlQuery | false = {}
 
-        if (paramsResult.hasValidParams) {
-          params = paramsResult.params
-        } else if (req.headers['x-now-route-matches']) {
-          const opts: Record<string, string> = {}
-          params = utils.getParamsFromRouteMatches(
-            req,
-            opts,
-            (parsedUrl.query.__nextLocale as string | undefined) || ''
+          Object.assign(parsedUrl.query, query)
+          const paramsResult = utils.normalizeDynamicRouteParams(
+            parsedUrl.query
           )
 
-          if (opts.locale) {
-            parsedUrl.query.__nextLocale = opts.locale
+          if (paramsResult.hasValidParams) {
+            params = paramsResult.params
+          } else if (req.headers['x-now-route-matches']) {
+            const opts: Record<string, string> = {}
+            params = utils.getParamsFromRouteMatches(
+              req,
+              opts,
+              (parsedUrl.query.__nextLocale as string | undefined) || ''
+            )
+
+            if (opts.locale) {
+              parsedUrl.query.__nextLocale = opts.locale
+            }
+          } else {
+            params = utils.dynamicRouteMatcher!(matchedPathnameNoExt)
           }
-        } else {
-          params = utils.dynamicRouteMatcher!(matchedPathnameNoExt)
-        }
 
-        if (params) {
-          params = utils.normalizeDynamicRouteParams(params).params
+          if (params) {
+            params = utils.normalizeDynamicRouteParams(params).params
 
-          matchedPathname = utils.interpolateDynamicPath(
-            matchedPathname,
-            params
-          )
-          req.url = utils.interpolateDynamicPath(req.url!, params)
-        }
+            matchedPathname = utils.interpolateDynamicPath(
+              matchedPathname,
+              params
+            )
+            req.url = utils.interpolateDynamicPath(req.url!, params)
+          }
 
-        if (reqUrlIsDataUrl && matchedPathIsDataUrl) {
-          req.url = formatUrl({
-            ...parsedPath,
-            pathname: matchedPathname,
-          })
-        }
+          if (reqUrlIsDataUrl && matchedPathIsDataUrl) {
+            req.url = formatUrl({
+              ...parsedPath,
+              pathname: matchedPathname,
+            })
+          }
 
-        Object.assign(parsedUrl.query, params)
-        utils.normalizeVercelUrl(req, true)
+          Object.assign(parsedUrl.query, params)
+          utils.normalizeVercelUrl(req, true)
+        }
+      } catch (err) {
+        if (err instanceof DecodeError) {
+          res.statusCode = 400
+          return this.renderError(null, req, res, '/_error', {})
+        }
+        throw err
       }
 
       parsedUrl.pathname = `${basePath || ''}${

diff --git a/test/production/required-server-files.test.ts b/test/production/required-server-files.test.ts
@@ -39,6 +39,10 @@ describe('should set-up next', () => {
               source: '/some-catch-all/:path*',
               destination: '/',
             },
+            {
+              source: '/to-dynamic/:path',
+              destination: '/dynamic/:path',
+            },
           ]
         },
       },
@@ -553,6 +557,21 @@ describe('should set-up next', () => {
     })
   })
 
+  it('should handle bad request correctly with rewrite', async () => {
+    const res = await fetchViaHTTP(
+      appPort,
+      '/to-dynamic/%c0.%c0.',
+      '?path=%c0.%c0.',
+      {
+        headers: {
+          'x-matched-path': '/dynamic/[slug]',
+        },
+      }
+    )
+    expect(res.status).toBe(400)
+    expect(await res.text()).toContain('Bad Request')
+  })
+
   it('should bubble error correctly for gip page', async () => {
     errors = []
     const res = await fetchViaHTTP(appPort, '/errors/gip', { crash: '1' })