Middleware overhaul! (github#18218)

* Middleware overhaul!

- Remove unnecessary 'async' keywords from middleware functions
- Ensure all middleware functions we create have names
- Wrap the method contents of all async middleware functions in a try-catch+next(error) pattern

* Use asyncMiddleware wrapper instead of try-catch+next(error) pattern

* Remove unnecessary try-catch+next(error) pattern from context middleware

Author: JamesMGreene

Diff for: middleware/abort.js

@@ -1,4 +1,4 @@
-module.exports = function (req, res, next) {
+module.exports = function abort (req, res, next) {
   // If the client aborts the connection, send an error
   req.once('aborted', () => {
     // NOTE: Node.js will also automatically set `req.aborted = true`

Diff for: middleware/archived-enterprise-versions-assets.js

@@ -11,7 +11,7 @@ const ONE_DAY = 24 * 60 * 60 // 1 day in seconds
 //
 // See also ./archived-enterprise-versions.js for non-CSS/JS paths
 
-module.exports = async (req, res, next) => {
+module.exports = async function archivedEnterpriseVersionsAssets (req, res, next) {
   const { isArchived, requestedVersion } = isArchivedVersion(req)
   if (!isArchived) return next()
 

Diff for: middleware/archived-enterprise-versions.js

@@ -11,7 +11,7 @@ const archivedFrontmatterFallbacks = require('../lib/redirects/static/archived-f
 // This module handles requests for deprecated GitHub Enterprise versions
 // by routing them to static content in help-docs-archived-enterprise-versions
 
-module.exports = async (req, res, next) => {
+module.exports = async function archivedEnterpriseVersions (req, res, next) {
   const { isArchived, requestedVersion } = isArchivedVersion(req)
   if (!isArchived) return next()
 

Diff for: middleware/block-robots.js

@@ -30,7 +30,7 @@ function blockIndex (path) {
   return pathRegExps.some(pathRe => pathRe.test(path))
 }
 
-const middleware = (req, res, next) => {
+const middleware = function blockRobots (req, res, next) {
   if (blockIndex(req.path)) res.set('x-robots-tag', 'noindex')
   return next()
 }

Diff for: middleware/breadcrumbs.js

@@ -3,7 +3,7 @@ const { getPathWithoutLanguage } = require('../lib/path-utils')
 const nonEnterpriseDefaultVersion = require('../lib/non-enterprise-default-version')
 const removeFPTFromPath = require('../lib/remove-fpt-from-path')
 
-module.exports = async (req, res, next) => {
+module.exports = async function breadcrumbs (req, res, next) {
   if (!req.context.page) return next()
   if (req.context.page.hidden) return next()
 

Diff for: middleware/categories-for-support-team.js

@@ -8,7 +8,7 @@ const dotcomDir = path.join(__dirname, '../content/github')
 const dotcomIndex = path.join(dotcomDir, 'index.md')
 const linkRegex = /{% (?:topic_)?link_in_list ?\/(.*?) ?%}/g
 
-module.exports = async (req, res, next) => {
+module.exports = async function categoriesForSupportTeam (req, res, next) {
   if (req.path !== '/categories.json') return next()
   const categories = await generateCategories()
   return res.json(categories)

Diff for: middleware/contextualizers/enterprise-release-notes.js

@@ -66,7 +66,7 @@ async function renderPatchNotes (patch, ctx) {
   return patch
 }
 
-module.exports = async (req, res, next) => {
+module.exports = async function enterpriseReleaseNotesContext (req, res, next) {
   // The `/release-notes` sub-path
   if (!req.path.endsWith('/release-notes')) return next()
 

Diff for: middleware/contextualizers/graphql.js

@@ -8,7 +8,7 @@ const explorerUrl = process.env.NODE_ENV === 'production'
   ? 'https://graphql.github.com/explorer'
   : 'http://localhost:3000'
 
-module.exports = async (req, res, next) => {
+module.exports = function graphqlContext (req, res, next) {
   const currentVersionObj = allVersions[req.context.currentVersion]
   // ignore requests to non-GraphQL reference paths
   // and to versions that don't exist

Diff for: middleware/contextualizers/rest.js

@@ -2,7 +2,7 @@ const path = require('path')
 const rest = require('../../lib/rest')
 const removeFPTFromPath = require('../../lib/remove-fpt-from-path')
 
-module.exports = async function (req, res, next) {
+module.exports = function restContext (req, res, next) {
   req.context.rest = rest
 
   // link to include in `Works with GitHub Apps` notes

Diff for: middleware/contextualizers/webhooks.js

@@ -4,7 +4,7 @@ const webhookPayloads = require(path.join(process.cwd(), 'lib/webhooks'))
 const nonEnterpriseDefaultVersion = require('../../lib/non-enterprise-default-version')
 const allVersions = require('../../lib/all-versions')
 
-module.exports = async (req, res, next) => {
+module.exports = function webhooksContext (req, res, next) {
   const currentVersionObj = allVersions[req.context.currentVersion]
   // ignore requests to non-webhook reference paths
   // and to versions that don't exist

Diff for: middleware/csp.js

@@ -7,7 +7,7 @@ const versionSatisfiesRange = require('../lib/version-satisfies-range')
 const AZURE_STORAGE_URL = 'githubdocs.azureedge.net'
 
 // module.exports = contentSecurityPolicy({
-module.exports = async (req, res, next) => {
+module.exports = function csp (req, res, next) {
   const csp = {
     directives: {
       defaultSrc: ["'none'"],

Diff for: middleware/detect-language.js

@@ -1,7 +1,7 @@
 const languageCodes = Object.keys(require('../lib/languages'))
 
 // determine language code from first part of URL, or default to English
-module.exports = async function detectLanguage (req, res, next) {
+module.exports = function detectLanguage (req, res, next) {
   // /en/articles/foo
   //  ^^
   const firstPartOfPath = req.path.split('/')[1]

Diff for: middleware/dev-toc.js

@@ -1,7 +1,7 @@
 const { liquid } = require('../lib/render-content')
 const layouts = require('../lib/layouts')
 
-module.exports = async (req, res, next) => {
+module.exports = async function devToc (req, res, next) {
   if (process.env.NODE_ENV !== 'development') return next()
   if (!req.path.endsWith('/dev-toc')) return next()
 

Diff for: middleware/disable-caching-on-safari.js

@@ -1,4 +1,4 @@
-module.exports = (req, res, next) => {
+module.exports = function disableCachingOnSafari (req, res, next) {
   const isSafari = /^((?!chrome|android).)*safari/i.test(req.headers['user-agent'])
   if (isSafari) {
     res.header('Last-Modified', (new Date()).toUTCString())

Diff for: middleware/early-access-breadcrumbs.js

@@ -5,7 +5,7 @@ const removeFPTFromPath = require('../lib/remove-fpt-from-path')
 
 // Early Access content doesn't conform to the same structure as other products, so we
 // can't derive breadcrumbs from the siteTree in the same way. Hence, this separate middleware.
-module.exports = async (req, res, next) => {
+module.exports = async function earlyAccessBreadcrumbs (req, res, next) {
   if (!req.context.page) return next()
   if (!req.context.page.hidden) return next()
 

Diff for: middleware/enterprise-server-releases.js

@@ -2,7 +2,7 @@ const { liquid } = require('../lib/render-content')
 const layouts = require('../lib/layouts')
 const getMiniTocItems = require('../lib/get-mini-toc-items')
 
-module.exports = async (req, res, next) => {
+module.exports = async function enterpriseServerReleases (req, res, next) {
   if (!req.path.endsWith('/enterprise-server-releases')) return next()
 
   const html = await liquid.parseAndRender(layouts['enterprise-server-releases'], req.context)

Diff for: middleware/events.js

@@ -10,7 +10,7 @@ const ajv = new Ajv()
 
 const router = express.Router()
 
-router.post('/', async (req, res, next) => {
+router.post('/', async function postEvents (req, res, next) {
   const isDev = process.env.NODE_ENV === 'development'
   const fields = omit(req.body, '_csrf')
 

Diff for: middleware/featured-links.js

@@ -1,7 +1,7 @@
 const getLinkData = require('../lib/get-link-data')
 
 // this middleware adds properties to the context object
-module.exports = async (req, res, next) => {
+module.exports = async function featuredLinks (req, res, next) {
   if (!req.context.page) return next()
 
   if (!(req.context.page.relativePath.endsWith('index.md') || req.context.page.layout === 'product-landing')) return next()

Diff for: middleware/handle-csrf-errors.js

@@ -1,4 +1,4 @@
-module.exports = async function handleCSRFError (error, req, res, next) {
+module.exports = function handleCSRFError (error, req, res, next) {
   // If the CSRF token is bad
   if (error.code === 'EBADCSRFTOKEN') {
     return res.sendStatus(403)

Diff for: middleware/handle-errors.js

@@ -28,47 +28,52 @@ async function logException (error, req) {
 }
 
 module.exports = async function handleError (error, req, res, next) {
-  // If the headers have already been sent or the request was aborted...
-  if (res.headersSent || req.aborted) {
-    // Report to Failbot
-    await logException(error, req)
+  try {
+    // If the headers have already been sent or the request was aborted...
+    if (res.headersSent || req.aborted) {
+      // Report to Failbot
+      await logException(error, req)
 
-    // We MUST delegate to the default Express error handler
-    return next(error)
-  }
+      // We MUST delegate to the default Express error handler
+      return next(error)
+    }
 
-  // if the error is thrown before req.context is created (say, in the Page class),
-  // set req.context.site here so we can pass data/ui.yml text to the 500 layout
-  if (!req.context) {
-    const site = await loadSiteData()
-    req.context = { site: site[req.language || 'en'].site }
-  }
+    // if the error is thrown before req.context is created (say, in the Page class),
+    // set req.context.site here so we can pass data/ui.yml text to the 500 layout
+    if (!req.context) {
+      const site = await loadSiteData()
+      req.context = { site: site[req.language || 'en'].site }
+    }
 
-  // display error on the page in development, but not in production
-  if (process.env.NODE_ENV !== 'production' && req.context) {
-    req.context.error = error
-  }
+    // display error on the page in development, but not in production
+    if (process.env.NODE_ENV !== 'production' && req.context) {
+      req.context.error = error
+    }
 
-  // Special handling for when a middleware calls `next(404)`
-  if (error === 404) {
-    return res
-      .status(404)
-      .send(await liquid.parseAndRender(layouts['error-404'], req.context))
-  }
+    // Special handling for when a middleware calls `next(404)`
+    if (error === 404) {
+      return res
+        .status(404)
+        .send(await liquid.parseAndRender(layouts['error-404'], req.context))
+    }
 
-  // If the error contains a status code, just send that back. This is usually
-  // from a middleware like `express.json()` or `csrf`.
-  if (error.statusCode || error.status) {
-    return res.sendStatus(error.statusCode || error.status)
-  }
+    // If the error contains a status code, just send that back. This is usually
+    // from a middleware like `express.json()` or `csrf`.
+    if (error.statusCode || error.status) {
+      return res.sendStatus(error.statusCode || error.status)
+    }
 
-  if (process.env.NODE_ENV !== 'test') {
-    console.error('500 error!', req.path)
-    console.error(error)
-  }
+    if (process.env.NODE_ENV !== 'test') {
+      console.error('500 error!', req.path)
+      console.error(error)
+    }
 
-  res.status(500).send(await liquid.parseAndRender(layouts['error-500'], req.context))
+    res.status(500).send(await liquid.parseAndRender(layouts['error-500'], req.context))
 
-  // Report to Failbot AFTER responding to the user
-  await logException(error, req)
+    // Report to Failbot AFTER responding to the user
+    await logException(error, req)
+  } catch (error) {
+    console.error('An error occurred in the error handling middleware!', error)
+    return next(error)
+  }
 }

Diff for: middleware/handle-invalid-paths.js

@@ -1,6 +1,6 @@
 const patterns = require('../lib/patterns')
 
-module.exports = (req, res, next) => {
+module.exports = function handleInvalidPaths (req, res, next) {
   // prevent open redirect vulnerability
   if (req.path.match(patterns.multipleSlashes)) {
     return next(404)
@@ -10,12 +10,13 @@ module.exports = (req, res, next) => {
   // for paths like /%7B%
   try {
     decodeURIComponent(req.path)
-    return next()
   } catch (err) {
     if (process.env.NODE_ENV !== 'test') {
       console.log('unable to decode path', req.path, err)
     }
 
     return res.sendStatus(400)
   }
+
+  return next()
 }

Diff for: middleware/index.js

@@ -64,15 +64,15 @@ module.exports = function (app) {
   app.use(instrument('./redirects/handle-redirects')) // Must come before contextualizers
 
   // *** Config and context for rendering ***
-  app.use(instrument('./find-page')) // Must come before archived-enterprise-versions, breadcrumbs, featured-links, products, render-page
+  app.use(asyncMiddleware(instrument('./find-page'))) // Must come before archived-enterprise-versions, breadcrumbs, featured-links, products, render-page
   app.use(instrument('./block-robots'))
 
   // Check for a dropped connection before proceeding
   app.use(haltOnDroppedConnection)
 
   // *** Rendering, 2xx responses ***
   // I largely ordered these by use frequency
-  app.use(instrument('./archived-enterprise-versions-assets')) // Must come before static/assets
+  app.use(asyncMiddleware(instrument('./archived-enterprise-versions-assets'))) // Must come before static/assets
   app.use('/dist', express.static('dist', {
     index: false,
     etag: false,
@@ -92,12 +92,12 @@ module.exports = function (app) {
     lastModified: false,
     maxAge: '7 days' // A bit longer since releases are more sparse
   }))
-  app.use('/events', instrument('./events'))
-  app.use('/search', instrument('./search'))
-  app.use(instrument('./archived-enterprise-versions'))
+  app.use('/events', asyncMiddleware(instrument('./events')))
+  app.use('/search', asyncMiddleware(instrument('./search')))
+  app.use(asyncMiddleware(instrument('./archived-enterprise-versions')))
   app.use(instrument('./robots'))
   app.use(/(\/.*)?\/early-access$/, instrument('./contextualizers/early-access-links'))
-  app.use(instrument('./categories-for-support-team'))
+  app.use(asyncMiddleware(instrument('./categories-for-support-team')))
   app.use(instrument('./loaderio-verification'))
   app.get('/_500', asyncMiddleware(instrument('./trigger-error')))
 
@@ -109,12 +109,12 @@ module.exports = function (app) {
   app.use(instrument('./contextualizers/graphql'))
   app.use(instrument('./contextualizers/rest'))
   app.use(instrument('./contextualizers/webhooks'))
-  app.use(instrument('./breadcrumbs'))
-  app.use(instrument('./early-access-breadcrumbs'))
-  app.use(instrument('./enterprise-server-releases'))
-  app.use(instrument('./dev-toc'))
-  app.use(instrument('./featured-links'))
-  app.use(instrument('./learning-track'))
+  app.use(asyncMiddleware(instrument('./breadcrumbs')))
+  app.use(asyncMiddleware(instrument('./early-access-breadcrumbs')))
+  app.use(asyncMiddleware(instrument('./enterprise-server-releases')))
+  app.use(asyncMiddleware(instrument('./dev-toc')))
+  app.use(asyncMiddleware(instrument('./featured-links')))
+  app.use(asyncMiddleware(instrument('./learning-track')))
 
   // *** Headers for pages only ***
   app.use(require('./set-fastly-cache-headers'))

Diff for: middleware/learning-track.js

@@ -1,7 +1,7 @@
 const { getPathWithoutLanguage, getPathWithoutVersion } = require('../lib/path-utils')
 const getLinkData = require('../lib/get-link-data')
 
-module.exports = async (req, res, next) => {
+module.exports = async function learningTrack (req, res, next) {
   const noTrack = () => {
     req.context.currentLearningTrack = {}
     return next()

Diff for: middleware/loaderio-verification.js

@@ -1,6 +1,6 @@
 // prove to loader.io that we own this site
 // by responding to requests like `/loaderio-12345/` with `loaderio-12345`
-module.exports = (req, res, next) => {
+module.exports = function loaderIoVerification (req, res, next) {
   if (!req.path.startsWith('/loaderio-')) return next()
   return res.send(req.path.replace(/\//g, ''))
 }

Diff for: middleware/record-redirect.js

@@ -1,6 +1,6 @@
 const { v4: uuidv4 } = require('uuid')
 
-module.exports = function (req, res, next) {
+module.exports = function recordRedirects (req, res, next) {
   if (!req.hydro.maySend()) return next()
 
   res.on('finish', async function recordRedirect () {

Diff for: middleware/redirects/external.js

@@ -1,7 +1,7 @@
 const externalSites = require('../../lib/redirects/external-sites')
 
 // blanket redirects to external websites
-module.exports = async function externalRedirects (req, res, next) {
+module.exports = function externalRedirects (req, res, next) {
   if (req.path in externalSites) {
     return res.redirect(301, externalSites[req.path])
   } else {

Diff for: middleware/redirects/handle-redirects.js

@@ -1,7 +1,7 @@
 const patterns = require('../../lib/patterns')
 const { URL } = require('url')
 
-module.exports = async function handleRedirects (req, res, next) {
+module.exports = function handleRedirects (req, res, next) {
   // never redirect assets
   if (patterns.assetPaths.test(req.path)) return next()
 

Diff for: middleware/redirects/help-to-docs.js

@@ -3,7 +3,7 @@ const patterns = require('../../lib/patterns')
 
 // redirect help.github.com requests to docs.github.com
 
-module.exports = async (req, res, next) => {
+module.exports = function helpToDocs (req, res, next) {
   if (req.hostname === 'help.github.com') {
     // prevent open redirect security vulnerability
     const path = req.originalUrl.replace(patterns.multipleSlashes, '/')

Diff for: middleware/redirects/language-code-redirects.js

@@ -5,7 +5,7 @@ const languages = require('../../lib/languages')
 // Examples:
 // /jp*    -> /ja*
 // /zh-TW* -> /cn*
-module.exports = async function languageCodeRedirects (req, res, next) {
+module.exports = function languageCodeRedirects (req, res, next) {
   for (const code in languages) {
     const language = languages[code]
     const redirectPatterns = language.redirectPatterns || []

Diff for: middleware/req-utils.js

@@ -1,6 +1,6 @@
 const Hydro = require('../lib/hydro')
 
-module.exports = (req, res, next) => {
+module.exports = function reqUtils (req, res, next) {
   req.hydro = new Hydro()
   return next()
 }