From 7c66d4456f6a5b8f617b3ba4ad855a3caa93d871 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <peterbe@github.com>
Date: Mon, 6 Dec 2021 08:00:34 -0500
Subject: [PATCH] static assets should not use csrf (Set-Cookie) (#23357)

* static assets should not use csrf (Set-Cookie)

Part of #1316

* move setFastlySurrogateKey up too
---
 middleware/index.js       | 59 ++++++++++++++++++++++-----------------
 tests/rendering/server.js | 19 +++++++++++--
 2 files changed, 50 insertions(+), 28 deletions(-)

diff --git a/middleware/index.js b/middleware/index.js
index 202260a3e8..fcfcdec24a 100644
--- a/middleware/index.js
+++ b/middleware/index.js
@@ -81,6 +81,39 @@ export default function (app) {
     app.use(datadog)
   }
 
+  // Must appear before static assets and all other requests
+  // otherwise we won't be able to benefit from that functionality
+  // for static assets as well.
+  app.use(setFastlySurrogateKey)
+
+  // Must come before `csrf` otherwise you get a Set-Cookie on successful
+  // asset requests. And it can come before `rateLimit` because if it's a
+  // 200 OK, the rate limiting won't matter anyway.
+  // archivedEnterpriseVersionsAssets must come before static/assets
+  app.use(
+    asyncMiddleware(
+      instrument(archivedEnterpriseVersionsAssets, './archived-enterprise-versions-assets')
+    )
+  )
+  app.use(
+    '/assets',
+    express.static('assets', {
+      index: false,
+      etag: false,
+      lastModified: false,
+      maxAge: '1 day', // Relatively short in case we update images
+    })
+  )
+  app.use(
+    '/public',
+    express.static('data/graphql', {
+      index: false,
+      etag: false,
+      lastModified: false,
+      maxAge: '7 days', // A bit longer since releases are more sparse
+    })
+  )
+
   // *** 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
@@ -110,7 +143,6 @@ export default function (app) {
   app.set('etag', false) // We will manage our own ETags if desired
   app.use(compression())
   app.use(disableCachingOnSafari)
-  app.use(setFastlySurrogateKey)
   app.use(catchBadAcceptLanguage)
 
   // *** Config and context for redirects ***
@@ -136,31 +168,6 @@ export default function (app) {
   app.use(haltOnDroppedConnection)
 
   // *** Rendering, 2xx responses ***
-  // I largely ordered these by use frequency
-  // archivedEnterpriseVersionsAssets must come before static/assets
-  app.use(
-    asyncMiddleware(
-      instrument(archivedEnterpriseVersionsAssets, './archived-enterprise-versions-assets')
-    )
-  )
-  app.use(
-    '/assets',
-    express.static('assets', {
-      index: false,
-      etag: false,
-      lastModified: false,
-      maxAge: '1 day', // Relatively short in case we update images
-    })
-  )
-  app.use(
-    '/public',
-    express.static('data/graphql', {
-      index: false,
-      etag: false,
-      lastModified: false,
-      maxAge: '7 days', // A bit longer since releases are more sparse
-    })
-  )
   app.use('/events', asyncMiddleware(instrument(events, './events')))
   app.use('/search', asyncMiddleware(instrument(search, './search')))
 
diff --git a/tests/rendering/server.js b/tests/rendering/server.js
index 10dc5bb645..d71a99eb3a 100644
--- a/tests/rendering/server.js
+++ b/tests/rendering/server.js
@@ -956,11 +956,26 @@ describe('?json query param for context debugging', () => {
 
 describe('static routes', () => {
   it('serves content from the /assets directory', async () => {
-    expect((await get('/assets/images/site/be-social.gif')).statusCode).toBe(200)
+    const res = await get('/assets/images/site/be-social.gif')
+    expect(res.statusCode).toBe(200)
+    expect(res.headers['cache-control']).toContain('public')
+    expect(res.headers['cache-control']).toMatch(/max-age=\d+/)
+    // Because static assets shouldn't use CSRF and thus shouldn't
+    // be setting a cookie.
+    expect(res.headers['set-cookie']).toBeUndefined()
+    // The "Surrogate-Key" header is set so we can do smart invalidation
+    // in the Fastly CDN. This needs to be available for static assets too.
+    expect(res.headers['surrogate-key']).toBeTruthy()
   })
 
   it('serves schema files from the /data/graphql directory at /public', async () => {
-    expect((await get('/public/schema.docs.graphql')).statusCode).toBe(200)
+    const res = await get('/public/schema.docs.graphql')
+    expect(res.statusCode).toBe(200)
+    expect(res.headers['cache-control']).toContain('public')
+    expect(res.headers['cache-control']).toMatch(/max-age=\d+/)
+    // Because static assets shouldn't use CSRF and thus shouldn't
+    // be setting a cookie.
+    expect(res.headers['set-cookie']).toBeUndefined()
     expect(
       (await get(`/public/ghes-${enterpriseServerReleases.latest}/schema.docs-enterprise.graphql`))
         .statusCode