faster 404 for missing static assets (#25124)

middleware/handle-errors.js

@@ -1,5 +1,4 @@
 import FailBot from '../lib/failbot.js'
-import loadSiteData from '../lib/site-data.js'
 import { nextApp } from './next.js'
 
 function shouldLogException(error) {
@@ -33,7 +32,10 @@ export default async function handleError(error, req, res, next) {
   // anywhere. So this is why we log it additionally.
   // Note, not using console.error() because it's arguably handled.
   // Some tests might actually expect a 500 error.
-  if (process.env.NODE_ENV === 'test') {
+  if (
+    process.env.NODE_ENV === 'test' &&
+    !(req.path.startsWith('/assets') || req.path.startsWith('/_next/static'))
+  ) {
     console.warn('An error occurrred in some middleware handler', error)
   }
 
@@ -47,14 +49,11 @@ export default async function handleError(error, req, res, next) {
       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 }
+      req.context = {}
     }
     // display error on the page in development and staging, but not in production
-    if (req.context && process.env.HEROKU_PRODUCTION_APP !== 'true') {
+    if (process.env.HEROKU_PRODUCTION_APP !== 'true') {
       req.context.error = error
     }
 

middleware/index.js

@@ -1,3 +1,6 @@
+import fs from 'fs'
+import path from 'path'
+
 import express from 'express'
 import instrument from '../lib/instrument-middleware.js'
 import haltOnDroppedConnection from './halt-on-dropped-connection.js'
@@ -138,6 +141,11 @@ export default function (app) {
       // Can be aggressive because images inside the content get unique
       // URLs with a cache busting prefix.
       maxAge: '7 days',
+      immutable: process.env.NODE_ENV !== 'development',
+      // This means, that if you request a file that starts with /assets/
+      // any file doesn't exist, don't bother (NextJS) rendering a
+      // pretty HTML error page.
+      fallthrough: false,
     })
   )
   app.use(
@@ -146,9 +154,33 @@ export default function (app) {
       index: false,
       etag: false,
       maxAge: '7 days', // A bit longer since releases are more sparse
+      // See note about about use of 'fallthrough'
+      fallthrough: false,
     })
   )
 
+  // In development, let NextJS on-the-fly serve the static assets.
+  // But in production, don't let NextJS handle any static assets
+  // because they are costly to generate (the 404 HTML page)
+  // and it also means that a CSRF cookie has to be generated.
+  if (process.env.NODE_ENV !== 'development') {
+    const assetDir = path.join('.next', 'static')
+    if (!fs.existsSync(assetDir))
+      throw new Error(`${assetDir} directory has not been generated. Run 'npm run build' first.`)
+
+    app.use(
+      '/_next/static/',
+      express.static(assetDir, {
+        index: false,
+        etag: false,
+        maxAge: '365 days',
+        immutable: true,
+        // See note about about use of 'fallthrough'
+        fallthrough: false,
+      })
+    )
+  }
+
   // *** Early exits ***
   // Don't use the proxy's IP, use the requester's for rate limiting
   // See https://expressjs.com/en/guide/behind-proxies.html

tests/rendering/static-assets.js

@@ -0,0 +1,66 @@
+import fs from 'fs'
+import path from 'path'
+
+import { expect } from '@jest/globals'
+
+import { SURROGATE_ENUMS } from '../../middleware/set-fastly-surrogate-key.js'
+import { get } from '../helpers/supertest.js'
+
+function getNextStaticAsset(directory) {
+  const root = path.join('.next', 'static', directory)
+  const files = fs.readdirSync(root)
+  if (!files.length) throw new Error(`Can't find any files in ${root}`)
+  return path.join(root, files[0])
+}
+
+function checkCachingHeaders(res, defaultSurrogateKey = false) {
+  expect(res.headers['set-cookie']).toBeUndefined()
+  expect(res.headers['cache-control']).toContain('public')
+  const maxAgeSeconds = parseInt(res.header['cache-control'].match(/max-age=(\d+)/)[1], 10)
+  // Let's not be too specific in the tests, just as long as it's testing
+  // that it's a reasonably large number of seconds.
+  expect(maxAgeSeconds).toBeGreaterThanOrEqual(60 * 60)
+  // Because it doesn't have have a unique URL
+  expect(res.headers['surrogate-key']).toBe(
+    defaultSurrogateKey ? SURROGATE_ENUMS.DEFAULT : SURROGATE_ENUMS.MANUAL
+  )
+}
+describe('static assets', () => {
+  it('should serve /assets/cb-* with optimal headers', async () => {
+    const res = await get('/assets/cb-1234/images/site/logo.png')
+    expect(res.statusCode).toBe(200)
+    checkCachingHeaders(res)
+  })
+
+  it('should serve /assets/ with optimal headers', async () => {
+    const res = await get('/assets/images/site/logo.png')
+    expect(res.statusCode).toBe(200)
+    checkCachingHeaders(res, true)
+  })
+
+  it('should serve /_next/static/ with optimal headers', async () => {
+    // This picks the first one found. We just need it to be anything
+    // that actually resolves.
+    const filePath = getNextStaticAsset('css')
+    const asURL = '/' + filePath.replace('.next', '_next').split(path.sep).join('/')
+    const res = await get(asURL)
+    expect(res.statusCode).toBe(200)
+    checkCachingHeaders(res)
+  })
+
+  it('should 404 on /assets/cb-* with plain text', async () => {
+    const res = await get('/assets/cb-1234/never/heard/of.png')
+    expect(res.statusCode).toBe(404)
+    expect(res.header['content-type']).toContain('text/plain')
+  })
+  it('should 404 on /assets/ with plain text', async () => {
+    const res = await get('/assets/never/heard/of.png')
+    expect(res.statusCode).toBe(404)
+    expect(res.header['content-type']).toContain('text/plain')
+  })
+  it('should 404 on /_next/static/ with plain text', async () => {
+    const res = await get('/_next/static/never/heard/of.css')
+    expect(res.statusCode).toBe(404)
+    expect(res.header['content-type']).toContain('text/plain')
+  })
+})