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

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

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) {

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}`,

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
 }
 

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')
+  })
+})

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')
     })
   })

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(

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 () => {