Try to prevent late/duplicate response errors (github#20077)

* Ensure all Express 'next' calls include 'return's * Add headersSent checks during archived versions flow * Return a 404 earlier if archived resource fetch fails * Short-circuit responses for archived stuff * Be more careful about responding to and short-circuiting after search requests * Fix tests
01753529836 · Jun 28, 2021 · 2dbae93 · 2dbae93
1 parent 930b266
commit 2dbae93
Show file tree

Hide file tree

Showing 8 changed files with 39 additions and 33 deletions.
diff --git a/middleware/archived-enterprise-versions-assets.js b/middleware/archived-enterprise-versions-assets.js
@@ -32,6 +32,6 @@ module.exports = async function archivedEnterpriseVersionsAssets (req, res, next
     res.set('cache-control', `public, max-age=${ONE_DAY}`)
     return res.send(r.body)
   } catch (err) {
-    return next()
+    return next(404)
   }
 }
diff --git a/middleware/archived-enterprise-versions.js b/middleware/archived-enterprise-versions.js
@@ -6,7 +6,7 @@ const versionSatisfiesRange = require('../lib/version-satisfies-range')
 const isArchivedVersion = require('../lib/is-archived-version')
 const got = require('got')
 const readJsonFile = require('../lib/read-json-file')
-const archvivedRedirects = readJsonFile('./lib/redirects/static/archived-redirects-from-213-to-217.json')
+const archivedRedirects = readJsonFile('./lib/redirects/static/archived-redirects-from-213-to-217.json')
 const archivedFrontmatterFallbacks = readJsonFile('./lib/redirects/static/archived-frontmatter-fallbacks.json')
 
 // This module handles requests for deprecated GitHub Enterprise versions
@@ -29,54 +29,49 @@ module.exports = async function archivedEnterpriseVersions (req, res, next) {
   // starting with 2.18, we updated the archival script to create a redirects.json file
   if (versionSatisfiesRange(requestedVersion, `>=${firstVersionDeprecatedOnNewSite}`) &&
     versionSatisfiesRange(requestedVersion, `<=${lastVersionWithoutArchivedRedirectsFile}`)) {
-    const redirect = archvivedRedirects[req.path]
+    const redirect = archivedRedirects[req.path]
     if (redirect && redirect !== req.path) {
       return res.redirect(301, redirect)
     }
   }
 
-  let reqPath = req.path
-  let isRedirect = false
   if (versionSatisfiesRange(requestedVersion, `>${lastVersionWithoutArchivedRedirectsFile}`)) {
     try {
-      const res = await got(getProxyPath('redirects.json', requestedVersion))
-      const redirectJson = JSON.parse(res.body)
+      const r = await got(getProxyPath('redirects.json', requestedVersion))
+      const redirectJson = JSON.parse(r.body)
 
+      // make redirects found via redirects.json redirect with a 301
       if (redirectJson[req.path]) {
-        isRedirect = true
+        res.set('x-robots-tag', 'noindex')
+        return res.redirect(301, redirectJson[req.path])
       }
-      reqPath = redirectJson[req.path] || req.path
     } catch (err) {
-      // nooop
+      // noop
     }
   }
 
   try {
-    const r = await got(getProxyPath(reqPath, requestedVersion))
-    res.set('content-type', r.headers['content-type'])
+    const r = await got(getProxyPath(req.path, requestedVersion))
     res.set('x-robots-tag', 'noindex')
 
-    // make redirects found via redirects.json return 301 instead of 200
-    if (isRedirect) {
-      res.status(301)
-      res.set('location', reqPath)
-    }
-
-    // make stubbed redirect files (which exist in versions <2.13) return 301 instead of 200
+    // make stubbed redirect files (which exist in versions <2.13) redirect with a 301
     const staticRedirect = r.body.match(patterns.staticRedirect)
     if (staticRedirect) {
-      res.status(301)
-      res.set('location', staticRedirect[1])
+      return res.redirect(301, staticRedirect[1])
     }
 
+    res.set('content-type', r.headers['content-type'])
     return res.send(r.body)
   } catch (err) {
     for (const fallbackRedirect of getFallbackRedirects(req, requestedVersion) || []) {
       try {
         await got(getProxyPath(fallbackRedirect, requestedVersion))
         return res.redirect(301, fallbackRedirect)
-      } catch (err) { } // noop
+      } catch (err) {
+        // noop
+      }
     }
+
     return next()
   }
 }

diff --git a/middleware/handle-next-data-path.js b/middleware/handle-next-data-path.js
@@ -15,5 +15,5 @@ module.exports = async function handleNextDataPath (req, res, next) {
     req.pagePath = req.path
   }
 
-  next()
+  return next()
 }
diff --git a/middleware/index.js b/middleware/index.js
@@ -160,6 +160,10 @@ module.exports = function (app) {
   }))
   app.use('/events', asyncMiddleware(instrument(events, './events')))
   app.use('/search', asyncMiddleware(instrument(search, './search')))
+
+  // Check for a dropped connection before proceeding (again)
+  app.use(haltOnDroppedConnection)
+
   app.use(asyncMiddleware(instrument(archivedEnterpriseVersions, './archived-enterprise-versions')))
   app.use(instrument(robots, './robots'))
   app.use(/(\/.*)?\/early-access$/, instrument(earlyAccessLinks, './contextualizers/early-access-links'))

diff --git a/middleware/is-next-request.js b/middleware/is-next-request.js
@@ -1,11 +1,11 @@
-const { FEATURE_NEXTJS } = process.env;
+const { FEATURE_NEXTJS } = process.env
 
-module.exports = function isNextRequest(req, res, next) {
-  req.renderWithNextjs = false;
+module.exports = function isNextRequest (req, res, next) {
+  req.renderWithNextjs = false
 
   if (FEATURE_NEXTJS) {
-    req.renderWithNextjs = true;
+    req.renderWithNextjs = true
   }
 
-  next();
-};
+  return next()
+}
diff --git a/middleware/next.js b/middleware/next.js
@@ -15,7 +15,7 @@ function renderPageWithNext (req, res, next) {
     return nextHandleRequest(req, res)
   }
 
-  next()
+  return next()
 }
 
 renderPageWithNext.nextHandleRequest = nextHandleRequest

diff --git a/middleware/search.js b/middleware/search.js
@@ -27,10 +27,17 @@ router.get('/', async function postSearch (req, res, next) {
     const results = process.env.AIRGAP || req.cookies.AIRGAP
       ? await loadLunrResults({ version, language, query: `${query} ${filters || ''}`, limit })
       : await loadAlgoliaResults({ version, language, query, filters, limit })
-    return res.status(200).json(results)
+
+    // Only reply if the headers have not been sent and the request was not aborted...
+    if (!res.headersSent && !req.aborted) {
+      return res.status(200).json(results)
+    }
   } catch (err) {
     console.error(err)
-    return res.status(400).json([])
+    // Only reply if the headers have not been sent and the request was not aborted...
+    if (!res.headersSent && !req.aborted) {
+      return res.status(400).json([])
+    }
   }
 })
 

diff --git a/tests/routing/redirects.js b/tests/routing/redirects.js
@@ -191,7 +191,7 @@ describe('redirects', () => {
     test('frontmatter redirect', async () => {
       const res = await get('/enterprise/2.12/user/articles/github-flavored-markdown')
       expect(res.statusCode).toBe(301)
-      expect(res.text).toContain('location=\'/enterprise/2.12/user/categories/writing-on-github/\'')
+      expect(res.headers.location).toBe('/enterprise/2.12/user/categories/writing-on-github/')
     })
   })
-Original file line number
+Diff line change
@@ Expand Up @@
         req.pagePath = req.path
       }
-      next()
+      return next()
     }