Merge branch 'main' into ruby-guide-patch

middleware/archived-enterprise-versions-assets.js

@@ -15,12 +15,37 @@ const cacheControl = cacheControlFactory(60 * 60 * 24)
 // See also ./archived-enterprise-versions.js for non-CSS/JS paths
 
 export default async function archivedEnterpriseVersionsAssets(req, res, next) {
+  // Only match asset paths
+  // This can be true on /enterprise/2.22/_next/static/foo.css
+  // or /_next/static/foo.css
+  if (!patterns.assetPaths.test(req.path)) return next()
+
+  // We now know the URL is either /enterprise/2.22/_next/static/foo.css
+  // or the regular /_next/static/foo.css. But we're only going to
+  // bother looking it up on https://github.github.com/help-docs-archived-enterprise-versions
+  // if the URL has the enterprise bit in it, or if the path was
+  // /_next/static/foo.css *and* its Referrer had the enterprise
+  // bit in it.
+  if (
+    !(
+      patterns.getEnterpriseVersionNumber.test(req.path) ||
+      patterns.getEnterpriseServerNumber.test(req.path) ||
+      patterns.getEnterpriseVersionNumber.test(req.get('referrer')) ||
+      patterns.getEnterpriseServerNumber.test(req.get('referrer'))
+    )
+  ) {
+    return next()
+  }
+
+  // Now we know the URL is definitely not /_next/static/foo.css
+  // So it's probably /enterprise/2.22/_next/static/foo.css and we
+  // should see if we might find this in the proxied backend.
+  // But `isArchivedVersion()` will only return truthy if the
+  // Referrer header also indicates that the request for this static
+  // asset came from a page
   const { isArchived, requestedVersion } = isArchivedVersion(req)
   if (!isArchived) return next()
 
-  // Only match asset paths
-  if (!patterns.assetPaths.test(req.path)) return next()
-
   const assetPath = req.path.replace(`/enterprise/${requestedVersion}`, '')
   const proxyPath = path.join('/', requestedVersion, assetPath)
 
@@ -37,6 +62,28 @@ export default async function archivedEnterpriseVersionsAssets(req, res, next) {
     cacheControl(res)
     return res.send(r.body)
   } catch (err) {
-    return next(404)
+    // Primarly for the developers working on tests that mock
+    // requests. If you don't set up `nock` correctly, you might
+    // not realize that and think it failed for other reasons.
+    if (err.toString().includes('Nock: No match for request')) {
+      throw err
+    }
+
+    // It's important that we don't give up on this by returning a 404
+    // here. It's better to let this through in case the asset exists
+    // beyond the realm of archived enterprise versions.
+    // For example, image you load
+    // /enterprise-server@2.21/en/DOES/NOT/EXIST in your browser.
+    // Quickly, we discover that the proxying is failing because
+    // it didn't find a page called `/en/DOES/NOT/EXIST` over there.
+    // So, we proceed to render *our* 404 HTML page.
+    // Now, on that 404 page, it will reference static assets too.
+    // E.g. <link href="/_next/static/styles.css">
+    // These will thus be requested, with a Referrer header that
+    // forces us to give it a chance, but it'll find it can't find it
+    // but we mustn't return a 404 yet, because that
+    // /_next/static/styles.css will probably still succeed because the 404
+    // page is not that of the archived enterprise version.
+    return next()
   }
 }

middleware/handle-errors.js

@@ -41,21 +41,9 @@ export default async function handleError(error, req, res, next) {
     // By default, Fastly will cache 404 responses unless otherwise
     // told not to.
     // See https://docs.fastly.com/en/guides/how-caching-and-cdns-work#http-status-codes-cached-by-default
-    // Most of the time, that's a good thing! Especially, if bombarded
-    // for some static asset that we don't have.
-    // E.g. `ab -n 10000 https://docs.github.com/assets/doesnotexist.png`
-    // But due to potential timing issue related to how the servers start,
-    // what might happen is that a new insteance comes up that
-    // contains `<script src="/_next/static/foo.1234.css">` in the HTML.
-    // The browser then proceeds to request /_next/static/foo.1234.css
-    // but this time it could be unluckily routed to a different instance
-    // that hasn't yet been upgraded, so they get a 404. And the CDN will
-    // notice this and cache it.
-    // Setting a tiny cache gets us a good compromise. It protects us
-    // against most stamping herds on 404s (thank you CDN!) but it also
-    // clears itself if you get that unlucky timing issue with rolling
-    // instances in a new deployment.
-    // For more background see issue 1553.
+    // Let's cache our 404'ing assets conservatively.
+    // The Cache-Control is short, and let's use the default surrogate
+    // key just in case it was a mistake.
     cacheControl(res)
     // Undo the cookie setting that CSRF sets.
     res.removeHeader('set-cookie')

tests/rendering/events.js

@@ -31,6 +31,8 @@ describe('POST /events', () => {
     delete process.env.HYDRO_SECRET
     delete process.env.HYDRO_ENDPOINT
     csrfToken = ''
+
+    nock.cleanAll()
   })
 
   async function checkEvent(data, code) {

tests/rendering/static-assets.js

@@ -1,7 +1,8 @@
 import fs from 'fs'
 import path from 'path'
 
-import { expect } from '@jest/globals'
+import nock from 'nock'
+import { expect, jest } from '@jest/globals'
 
 import { SURROGATE_ENUMS } from '../../middleware/set-fastly-surrogate-key.js'
 import { get } from '../helpers/supertest.js'
@@ -69,3 +70,99 @@ describe('static assets', () => {
     checkCachingHeaders(res, true, 60)
   })
 })
+
+describe('archived enterprise static assets', () => {
+  // Sometimes static assets are proxied. The URL for the static asset
+  // might not indicate it's based on archived enterprise version.
+
+  jest.setTimeout(60 * 1000)
+
+  beforeAll(async () => {
+    // The first page load takes a long time so let's get it out of the way in
+    // advance to call out that problem specifically rather than misleadingly
+    // attributing it to the first test
+    // await get('/')
+
+    const sampleCSS = '/* nice CSS */'
+
+    nock('https://github.github.com')
+      .get('/help-docs-archived-enterprise-versions/2.21/_next/static/foo.css')
+      .reply(200, sampleCSS, {
+        'content-type': 'text/css',
+        'content-length': sampleCSS.length,
+      })
+    nock('https://github.github.com')
+      .get('/help-docs-archived-enterprise-versions/2.21/_next/static/only-on-proxy.css')
+      .reply(200, sampleCSS, {
+        'content-type': 'text/css',
+        'content-length': sampleCSS.length,
+      })
+    nock('https://github.github.com')
+      .get('/help-docs-archived-enterprise-versions/2.3/_next/static/only-on-2.3.css')
+      .reply(200, sampleCSS, {
+        'content-type': 'text/css',
+        'content-length': sampleCSS.length,
+      })
+    nock('https://github.github.com')
+      .get('/help-docs-archived-enterprise-versions/2.3/_next/static/fourofour.css')
+      .reply(404, 'Not found', {
+        'content-type': 'text/plain',
+      })
+    nock('https://github.github.com')
+      .get('/help-docs-archived-enterprise-versions/2.3/assets/images/site/logo.png')
+      .reply(404, 'Not found', {
+        'content-type': 'text/plain',
+      })
+  })
+
+  afterAll(() => nock.cleanAll())
+
+  it('should proxy if the static asset is prefixed', async () => {
+    const res = await get('/enterprise/2.21/_next/static/foo.css', {
+      headers: {
+        Referrer: '/enterprise/2.21',
+      },
+    })
+    expect(res.statusCode).toBe(200)
+    checkCachingHeaders(res, true, 60)
+  })
+  it('should proxy if the Referrer header indicates so', async () => {
+    const res = await get('/_next/static/only-on-proxy.css', {
+      headers: {
+        Referrer: '/enterprise/2.21',
+      },
+    })
+    expect(res.statusCode).toBe(200)
+    checkCachingHeaders(res, true, 60)
+  })
+  it('should proxy if the Referrer header indicates so', async () => {
+    const res = await get('/_next/static/only-on-2.3.css', {
+      headers: {
+        Referrer: '/en/enterprise-server@2.3/some/page',
+      },
+    })
+    expect(res.statusCode).toBe(200)
+    checkCachingHeaders(res, true, 60)
+  })
+  it('might still 404 even with the right referrer', async () => {
+    const res = await get('/_next/static/fourofour.css', {
+      headers: {
+        Referrer: '/en/enterprise-server@2.3/some/page',
+      },
+    })
+    expect(res.statusCode).toBe(404)
+    checkCachingHeaders(res, true, 60)
+  })
+
+  it('404 on the proxy but actually present here', async () => {
+    const res = await get('/assets/images/site/logo.png', {
+      headers: {
+        Referrer: '/en/enterprise-server@2.3/some/page',
+      },
+    })
+    // It tried to go via the proxy, but it wasn't there, but then it
+    // tried "our disk" and it's eventually there.
+    expect(res.statusCode).toBe(200)
+    checkCachingHeaders(res, true, 60)
+  })
+})