Upgrade Express from v4 to v5 (#56026)

package.json

@@ -269,7 +269,7 @@
     "dereference-json-schema": "^0.2.1",
     "dotenv": "^16.4.7",
     "escape-string-regexp": "5.0.0",
-    "express": "4.21.2",
+    "express": "^5.1.0",
     "fastest-levenshtein": "1.0.16",
     "file-type": "21.0.0",
     "flat": "^6.0.1",
@@ -349,7 +349,7 @@
     "@types/cookie": "0.6.0",
     "@types/cookie-parser": "1.4.8",
     "@types/event-to-promise": "^0.7.5",
-    "@types/express": "4.17.21",
+    "@types/express": "^5.0.3",
     "@types/imurmurhash": "^0.1.4",
     "@types/js-cookie": "^3.0.6",
     "@types/js-yaml": "^4.0.9",

src/frame/middleware/api.ts

@@ -65,10 +65,15 @@ router.get('/cookies', (req, res) => {
   const cookies = {
     isStaff: Boolean(req.cookies?.staffonly?.startsWith('yes')) || false,
   }
-  return res.json(cookies)
+  res.json(cookies)
 })
 
-router.get('*', (req, res) => {
+// Handle root /api requests
+router.get('/', (req, res) => {
+  res.status(404).json({ error: `${req.path} not found` })
+})
+
+router.get('/*path', (req, res) => {
   res.status(404).json({ error: `${req.path} not found` })
 })
 

src/frame/middleware/build-info.ts

@@ -8,7 +8,8 @@ export default function buildInfo(req: Request, res: Response) {
   res.type('text/plain')
   noCacheControl(res)
   if (!BUILD_SHA) {
-    return res.status(404).send('Not known')
+    res.status(404).send('Not known')
+    return
   }
-  return res.send(`${BUILD_SHA}`)
+  res.send(`${BUILD_SHA}`)
 }

src/frame/middleware/fast-head.ts

@@ -12,7 +12,8 @@ export default function fastHead(req: ExtendedRequest, res: Response, next: Next
     // this and allow the CDN to hold on to it.
     defaultCacheControl(res)
 
-    return res.status(200).send('')
+    res.status(200).send('')
+    return
   }
   next()
 }

src/frame/middleware/fastly-cache-test.ts

@@ -12,7 +12,7 @@ import crypto from 'crypto'
 
 const router = express.Router()
 
-router.get('/*', function (req, res) {
+router.get('/*path', function (req, res) {
   // If X-CacheTest-Error is set, simulate the site being down (regardless of URL)
   if (req.get('X-CacheTest-Error')) {
     res.status(parseInt(req.get('X-CacheTest-Error') as string)).end()

src/frame/middleware/index.ts

@@ -250,7 +250,7 @@ export default function (app: Express) {
 
   // Specifically deal with HEAD requests before doing the slower
   // full page rendering.
-  app.head('/*', fastHead)
+  app.head('/*path', fastHead)
 
   // *** Preparation for render-page: contextualizers ***
   app.use(asyncMiddleware(secretScanning))
@@ -282,7 +282,7 @@ export default function (app: Express) {
   app.use(haltOnDroppedConnection)
 
   // *** Rendering, must go almost last ***
-  app.get('/*', asyncMiddleware(renderPage))
+  app.get('/*path', asyncMiddleware(renderPage))
 
   // *** Error handling, must go last ***
   app.use(handleErrors)

src/frame/middleware/mock-va-portal.ts

@@ -58,7 +58,8 @@ export default function mockVaPortal(req: ExtendedRequest, res: Response, next:
 
   if (req.url.startsWith('/iframe/docs_va')) {
     res.removeHeader('content-security-policy')
-    return res.status(200).type('text/html').send(HTML)
+    res.status(200).type('text/html').send(HTML)
+    return
   }
 
   next()

src/frame/middleware/render-page.ts

@@ -108,7 +108,7 @@ export default async function renderPage(req: ExtendedRequest, res: Response) {
     // It's important to not use `src/pages/404.txt` (or `/404` as the path)
     // here because then it will set the wrong Cache-Control header.
     tempReq.url = '/_notfound'
-    tempReq.path = '/_notfound'
+    Object.defineProperty(tempReq, 'path', { value: '/_notfound', writable: true })
     tempReq.cookies = {}
     tempReq.headers = {}
     // By default, since the lookup for a `src/pages/*.tsx` file will work,

src/frame/middleware/robots.ts

@@ -23,8 +23,9 @@ export default function robots(req: ExtendedRequest, res: Response, next: NextFu
     req.hostname === 'docs.github.com' ||
     req.hostname === '127.0.0.1'
   ) {
-    return res.send(defaultResponse)
+    res.send(defaultResponse)
+    return
   }
 
-  return res.send(disallowAll)
+  res.send(disallowAll)
 }

src/observability/middleware/handle-errors.ts

@@ -1,4 +1,4 @@
-import type { NextFunction, Response } from 'express'
+import type { NextFunction, Response, ErrorRequestHandler } from 'express'
 
 import FailBot from '../lib/failbot.js'
 import { nextApp } from '@/frame/middleware/next.js'
@@ -53,7 +53,7 @@ function timedOut(req: ExtendedRequest) {
   statsd.increment('middleware.timeout', 1, incrementTags)
 }
 
-export default async function handleError(
+const handleError: ErrorRequestHandler = async function handleError(
   error: ErrorWithCode | number,
   req: ExtendedRequest,
   res: Response,
@@ -93,7 +93,8 @@ export default async function handleError(
       }
 
       // We MUST delegate to the default Express error handler
-      return next(error)
+      next(error)
+      return
     }
 
     if (!req.context) {
@@ -103,7 +104,8 @@ export default async function handleError(
     // Special handling for when a middleware calls `next(404)`
     if (error === 404) {
       // Note that if this fails, it will swallow that error.
-      return nextApp.render404(req, res)
+      nextApp.render404(req, res)
+      return
     }
     if (typeof error === 'number') {
       throw new Error("Don't use next(xxx) where xxx is any other number than 404")
@@ -117,7 +119,8 @@ export default async function handleError(
     // If the error contains a status code, just send that back. This is usually
     // from a middleware like `express.json()`.
     if (error.statusCode) {
-      return res.sendStatus(error.statusCode)
+      res.sendStatus(error.statusCode)
+      return
     }
 
     res.statusCode = 500
@@ -129,7 +132,8 @@ export default async function handleError(
     // it's easier to just see the full stack trace in the console
     // and in the client.
     if (process.env.NODE_ENV === 'development') {
-      return next(error)
+      next(error)
+      return
     } else {
       nextApp.renderError(error, req, res, req.path)
 
@@ -138,6 +142,9 @@ export default async function handleError(
     }
   } catch (error) {
     console.error('An error occurred in the error handling middleware!', error)
-    return next(error)
+    next(error)
+    return
   }
 }
+
+export default handleError

src/shielding/middleware/handle-invalid-headers.ts

@@ -22,7 +22,8 @@ export default function handleInvalidNextPaths(
     // The CDN will not cache if the status code is not a success
     // and not a 404.
 
-    return res.status(400).type('text').send('Invalid request headers')
+    res.status(400).type('text').send('Invalid request headers')
+    return
   }
 
   return next()

src/shielding/middleware/handle-invalid-nextjs-paths.ts

@@ -26,7 +26,8 @@ export default function handleInvalidNextPaths(
     const tags = [`path:${req.path}`]
     statsd.increment(STATSD_KEY, 1, tags)
 
-    return res.status(404).type('text').send('Not found')
+    res.status(404).type('text').send('Not found')
+    return
   }
 
   return next()

src/shielding/middleware/handle-invalid-paths.ts

@@ -80,7 +80,8 @@ export default function handleInvalidPaths(
     // they're not going to suddenly work in the next deployment.
     defaultCacheControl(res)
     res.setHeader('content-type', 'text/plain')
-    return res.status(404).send('Not found')
+    res.status(404).send('Not found')
+    return
   }
 
   if (req.path.endsWith('/index.md')) {
@@ -98,7 +99,8 @@ export default function handleInvalidPaths(
       .replace(/%2F/g, '/')
       .replace(/%40/g, '@')
     const newUrl = `/api/article/body?pathname=${encodedPath}`
-    return res.redirect(newUrl)
+    res.redirect(newUrl)
+    return
   }
   return next()
 }

src/shielding/middleware/handle-invalid-query-strings.ts

@@ -55,6 +55,30 @@ export default function handleInvalidQuerystrings(
   const { method, query, path } = req
   if (method === 'GET' || method === 'HEAD') {
     const originalKeys = Object.keys(query)
+
+    // Check for invalid query string patterns (square brackets, etc.)
+    const invalidKeys = originalKeys.filter((key) => {
+      // Check for square brackets which are invalid
+      return key.includes('[') || key.includes(']')
+    })
+
+    if (invalidKeys.length > 0) {
+      noCacheControl(res)
+      const invalidKey = invalidKeys[0].replace(/\[.*$/, '') // Get the base key name
+      res.status(400).send(`Invalid query string key (${invalidKey})`)
+
+      const tags = [
+        'response:400',
+        'reason:invalid-brackets',
+        `url:${req.url}`,
+        `path:${req.path}`,
+        `keys:${originalKeys.length}`,
+      ]
+      statsd.increment(STATSD_KEY, 1, tags)
+
+      return
+    }
+
     let keys = originalKeys.filter((key) => !RECOGNIZED_KEYS_BY_ANY.has(key))
     if (keys.length > 0) {
       // Before we judge the number of query strings, strip out all the ones

src/shielding/middleware/handle-old-next-data-paths.ts

@@ -36,7 +36,8 @@ export default function handleOldNextDataPaths(
     const requestBuildId = req.path.split('/')[3]
     if (requestBuildId !== getCurrentBuildID()) {
       errorCacheControl(res)
-      return res.status(404).type('text').send('build ID mismatch')
+      res.status(404).type('text').send('build ID mismatch')
+      return
     }
   }
   return next()

src/webhooks/middleware/webhooks.ts

@@ -12,10 +12,12 @@ const router = express.Router()
 //   /api/webhooks/v1?category=check_run&version=free-pro-team%40latest
 router.get('/v1', async function webhooks(req, res) {
   if (!req.query.category) {
-    return res.status(400).json({ error: "Missing 'category' in query string" })
+    res.status(400).json({ error: "Missing 'category' in query string" })
+    return
   }
   if (!req.query.version) {
-    return res.status(400).json({ error: "Missing 'version' in query string" })
+    res.status(400).json({ error: "Missing 'version' in query string" })
+    return
   }
 
   const webhookVersion = Object.values(allVersions).find(
@@ -24,7 +26,8 @@ router.get('/v1', async function webhooks(req, res) {
   const notFoundError = 'No webhook found for given category and version'
 
   if (!webhookVersion) {
-    return res.status(404).json({ error: notFoundError })
+    res.status(404).json({ error: notFoundError })
+    return
   }
 
   const webhook = await getWebhook(webhookVersion, req.query.category as string)
@@ -33,7 +36,7 @@ router.get('/v1', async function webhooks(req, res) {
     if (process.env.NODE_ENV !== 'development') {
       defaultCacheControl(res)
     }
-    return res.status(200).send(webhook)
+    res.status(200).send(webhook)
   } else {
     res.status(404).json({ error: notFoundError })
   }