Plain text 404 page for non language urls (github#37752)

Peter Bengtsson · web-flow · commit 5fc846c0ea7f · 2023-06-12T20:19:48.000Z
diff --git a/middleware/find-page.js b/middleware/find-page.js
@@ -3,9 +3,8 @@ import { existsSync } from 'fs'
 
 import { ROOT } from '../lib/constants.js'
 import Page from '../lib/page.js'
-import { languageKeys } from '../lib/languages.js'
+import { languagePrefixPathRegex } from '../lib/languages.js'
 
-const languagePrefixRegex = new RegExp(`^/(${languageKeys.join('|')})(/|$)`)
 const englishPrefixRegex = /^\/en(\/|$)/
 const CONTENT_ROOT = path.join(ROOT, 'content')
 
@@ -18,7 +17,7 @@ export default async function findPage(
   { isDev = process.env.NODE_ENV === 'development', contentRoot = CONTENT_ROOT } = {}
 ) {
   // Filter out things like `/will/redirect` or `/_next/data/...`
-  if (!languagePrefixRegex.test(req.pagePath)) {
+  if (!languagePrefixPathRegex.test(req.pagePath)) {
     return next()
   }
 
@@ -58,8 +57,8 @@ export default async function findPage(
 }
 
 async function rereadByPath(uri, contentRoot, currentVersion) {
-  const languageCode = uri.match(languagePrefixRegex)[1]
-  const withoutLanguage = uri.replace(languagePrefixRegex, '/')
+  const languageCode = uri.match(languagePrefixPathRegex)[1]
+  const withoutLanguage = uri.replace(languagePrefixPathRegex, '/')
   const withoutVersion = withoutLanguage.replace(`/${currentVersion}`, '')
   // TODO: Support loading translations the same way.
   // NOTE: No one is going to test translations like this in development
diff --git a/middleware/next.js b/middleware/next.js
@@ -8,14 +8,6 @@ export const nextHandleRequest = nextApp.getRequestHandler()
 await nextApp.prepare()
 
 function renderPageWithNext(req, res, next) {
-  // We currently don't use next/image for any images.
-  // We don't even have `sharp` installed.
-  // This could change in the future but right now can just 404 on these
-  // so we don't have to deal with any other errors.
-  if (req.path.startsWith('/_next/image')) {
-    return next(404)
-  }
-
   const isNextDataRequest = req.path.startsWith('/_next') && !req.path.startsWith('/_next/data')
 
   if (isNextDataRequest) {
diff --git a/middleware/render-page.js b/middleware/render-page.js
@@ -4,6 +4,7 @@ import FailBot from '#src/observability/lib/failbot.js'
 import patterns from '../lib/patterns.js'
 import getMiniTocItems from '../lib/get-mini-toc-items.js'
 import Page from '../lib/page.js'
+import { pathLanguagePrefixed } from '../lib/languages.js'
 import statsd from '#src/observability/lib/statsd.js'
 import { allVersions } from '../lib/all-versions.js'
 import { isConnectionDropped } from './halt-on-dropped-connection.js'
@@ -55,6 +56,14 @@ export default async function renderPage(req, res) {
       )
     }
 
+    if (!pathLanguagePrefixed(req.path)) {
+      defaultCacheControl(res)
+      return res.status(404).type('text').send('Not found')
+    }
+
+    // The rest is "unhandled" requests where we don't have the page
+    // but the URL looks like a real page.
+
     statsd.increment(STATSD_KEY_404, 1, [
       `url:${req.url}`,
       `ip:${req.ip}`,
diff --git a/src/shielding/middleware/handle-invalid-paths.js b/src/shielding/middleware/handle-invalid-paths.js
@@ -31,6 +31,13 @@ function isJunkPath(path) {
     return true
   }
 
+  // We currently don't use next/image for any images.
+  // This could change in the future but right now can just 404 on these
+  // so we don't have to deal with any other errors.
+  if (path.startsWith('/_next/image')) {
+    return true
+  }
+
   return false
 }
 
diff --git a/src/shielding/tests/shielding.js b/src/shielding/tests/shielding.js
@@ -74,3 +74,26 @@ describe('rate limiting', () => {
     expect(res.headers['ratelimit-remaining']).toBeUndefined()
   })
 })
+
+describe('404 pages and their content-type', () => {
+  const exampleNonLanguage404s = [
+    '/_next/image/foo',
+    '/wp-content/themes/seotheme/db.php?u',
+    '/enterprise/3.1/_next/static/chunks/616-910d0397bafa52e0.js',
+    '/~root',
+  ]
+  test.each(exampleNonLanguage404s)(
+    'non-language 404 response is plain text and cacheable: %s',
+    async (pathname) => {
+      const res = await get(pathname)
+      expect(res.statusCode).toBe(404)
+      expect(res.headers['content-type']).toMatch('text/plain')
+      expect(res.headers['cache-control']).toMatch('public')
+    }
+  )
+  test('valid language prefix 404 response is HTML', async () => {
+    const res = await get('/en/something-that-doesnt-existent')
+    expect(res.statusCode).toBe(404)
+    expect(res.headers['content-type']).toMatch('text/html')
+  })
+})
diff --git a/tests/rendering-fixtures/footer.js b/tests/rendering-fixtures/footer.js
@@ -15,7 +15,9 @@ describe('footer', () => {
     })
 
     test('leads to support on 404 pages', async () => {
-      const $ = await getDOM('/delicious-snacks/donuts.php', { allow404: true })
+      // Important to use the prefix /en/ on the failing URL or else
+      // it will render a very basic plain text 404 response.
+      const $ = await getDOM('/en/delicious-snacks/donuts.php', { allow404: true })
       expect($('a#support').attr('href')).toBe('https://support.github.com')
     })
   })
diff --git a/tests/rendering/server.js b/tests/rendering/server.js
@@ -156,7 +156,9 @@ describe('server', () => {
   })
 
   test('renders a 404 page', async () => {
-    const $ = await getDOM('/not-a-real-page', { allow404: true })
+    // Important to use the prefix /en/ on the failing URL or else
+    // it will render a very basic plain text 404 response.
+    const $ = await getDOM('/en/not-a-real-page', { allow404: true })
     expect($('h1').first().text()).toBe('Ooops!')
     expect($.text().includes("It looks like this page doesn't exist.")).toBe(true)
     expect(
diff --git a/tests/routing/next.js b/tests/routing/next.js
@@ -8,7 +8,7 @@ describe('bad requests', () => {
   test('any _next/image request should 404', async () => {
     const res = await get('/_next/image?what=ever')
     expect(res.statusCode).toBe(404)
-    expect(res.headers['content-type']).toMatch('text/html')
+    expect(res.headers['content-type']).toMatch('text/plain')
   })
 
   test('any _next.* request should 404', async () => {

Original file line number	Diff line number	Diff line change
`@@ -31,6 +31,13 @@ function isJunkPath(path) {`
`31`	`31`	`return true`
`32`	`32`	`}`
`33`	`33`
	`34`	`+ // We currently don't use next/image for any images.`
	`35`	`+ // This could change in the future but right now can just 404 on these`
	`36`	`+ // so we don't have to deal with any other errors.`
	`37`	`+ if (path.startsWith('/_next/image')) {`
	`38`	`+ return true`
	`39`	`+ }`
	`40`	`+`
`34`	`41`	`return false`
`35`	`42`	`}`
`36`	`43`