cache: fix excessive caching and some other lack of caching (#4335)

Co-authored-by: Frédéric Delaporte <fdelaporte@valap.com>

Author: fredericDelaporte

lib/handler/cache-handler.js

@@ -15,6 +15,15 @@ const HEURISTICALLY_CACHEABLE_STATUS_CODES = [
   200, 203, 204, 206, 300, 301, 308, 404, 405, 410, 414, 501
 ]
 
+// Status codes which semantic is not handled by the cache
+// https://datatracker.ietf.org/doc/html/rfc9111#section-3
+// This list should not grow beyond 206 and 304 unless the RFC is updated
+// by a newer one including more. Please introduce another list if
+// implementing caching of responses with the 'must-understand' directive.
+const NOT_UNDERSTOOD_STATUS_CODES = [
+  206, 304
+]
+
 const MAX_RESPONSE_AGE = 2147483647000
 
 /**
@@ -241,10 +250,19 @@ class CacheHandler {
  * @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives
  */
 function canCacheResponse (cacheType, statusCode, resHeaders, cacheControlDirectives) {
-  // Allow caching for status codes 200 and 307 (original behavior)
-  // Also allow caching for other status codes that are heuristically cacheable
-  // when they have explicit cache directives
-  if (statusCode !== 200 && statusCode !== 307 && !HEURISTICALLY_CACHEABLE_STATUS_CODES.includes(statusCode)) {
+  // Status code must be final and understood.
+  if (statusCode < 200 || NOT_UNDERSTOOD_STATUS_CODES.includes(statusCode)) {
+    return false
+  }
+  // Responses with neither status codes that are heuristically cacheable, nor "explicit enough" caching
+  // directives, are not cacheable. "Explicit enough": see https://www.rfc-editor.org/rfc/rfc9111.html#section-3
+  if (!HEURISTICALLY_CACHEABLE_STATUS_CODES.includes(statusCode) && !resHeaders['expires'] &&
+    !cacheControlDirectives.public &&
+    cacheControlDirectives['max-age'] === undefined &&
+    // RFC 9111: a private response directive, if the cache is not shared
+    !(cacheControlDirectives.private && cacheType === 'private') &&
+    !(cacheControlDirectives['s-maxage'] !== undefined && cacheType === 'shared')
+  ) {
     return false
   }
 

test/interceptors/cache.js

@@ -1437,8 +1437,11 @@ describe('Cache Interceptor', () => {
     })
   })
 
+  // Partial list.
   const cacheableStatusCodes = [
     { code: 204, body: '' },
+    { code: 302, body: 'Found' },
+    { code: 307, body: 'Temporary Redirect' },
     { code: 404, body: 'Not Found' },
     { code: 410, body: 'Gone' }
   ]
@@ -1489,13 +1492,71 @@ describe('Cache Interceptor', () => {
     })
   }
 
-  test('does not cache non-heuristically cacheable error status codes', async () => {
+  // Partial list.
+  const nonHeuristicallyCacheableStatusCodes = [
+    { code: 201, body: 'Created' },
+    { code: 307, body: 'Temporary Redirect' },
+    { code: 418, body: 'I am a teapot' }
+  ]
+
+  for (const { code, body } of nonHeuristicallyCacheableStatusCodes) {
+    test(`does not cache non-heuristically cacheable status ${code} without explicit directive`, async () => {
+      let requestsToOrigin = 0
+      const server = createServer({ joinDuplicateHeaders: true }, (_, res) => {
+        requestsToOrigin++
+        res.statusCode = code
+        // By default the response may have a date and last-modified header set to 'now',
+        // causing the cache to compute a 0 heuristic expiry, causing the test to not ascertain
+        // it is really not cached.
+        res.setHeader('date', '')
+        res.end(body)
+      }).listen(0)
+
+      const client = new Client(`http://localhost:${server.address().port}`)
+        .compose(interceptors.cache({ cacheByDefault: 60 }))
+
+      after(async () => {
+        server.close()
+        await client.close()
+      })
+
+      await once(server, 'listening')
+
+      equal(requestsToOrigin, 0)
+
+      const request = {
+        origin: 'localhost',
+        method: 'GET',
+        path: '/'
+      }
+
+      // First request should hit the origin
+      {
+        const res = await client.request(request)
+        equal(requestsToOrigin, 1)
+        equal(res.statusCode, code)
+        strictEqual(await res.body.text(), body)
+      }
+
+      // Second request should also hit the origin (not cached)
+      {
+        const res = await client.request(request)
+        equal(requestsToOrigin, 2) // Should be 2 (not cached)
+        equal(res.statusCode, code)
+        strictEqual(await res.body.text(), body)
+      }
+    })
+  }
+
+  test('discriminates caching of range requests, or does not cache them', async () => {
     let requestsToOrigin = 0
+    const body = 'Fake range request response'
+    const code = 206
     const server = createServer({ joinDuplicateHeaders: true }, (_, res) => {
       requestsToOrigin++
-      res.statusCode = 418 // I'm a teapot - not in heuristically cacheable list
+      res.statusCode = code
       res.setHeader('cache-control', 'public, max-age=60')
-      res.end('I am a teapot')
+      res.end(body)
     }).listen(0)
 
     const client = new Client(`http://localhost:${server.address().port}`)
@@ -1513,23 +1574,127 @@ describe('Cache Interceptor', () => {
     const request = {
       origin: 'localhost',
       method: 'GET',
-      path: '/'
+      path: '/',
+      headers: {
+        range: 'bytes=10-'
+      }
     }
 
     // First request should hit the origin
     {
       const res = await client.request(request)
       equal(requestsToOrigin, 1)
-      equal(res.statusCode, 418)
-      strictEqual(await res.body.text(), 'I am a teapot')
+      equal(res.statusCode, code)
+      strictEqual(await res.body.text(), body)
     }
 
-    // Second request should also hit the origin (not cached)
+    // Second request with different range should hit the origin too
+    request.headers.range = 'bytes=5-'
     {
       const res = await client.request(request)
-      equal(requestsToOrigin, 2) // Should be 2 (not cached)
-      equal(res.statusCode, 418)
-      strictEqual(await res.body.text(), 'I am a teapot')
+      equal(requestsToOrigin, 2)
+      equal(res.statusCode, code)
+      strictEqual(await res.body.text(), body)
+    }
+  })
+
+  test('discriminates caching of conditionnal requests (if-none-match), or does not cache them', async () => {
+    let requestsToOrigin = 0
+    const body = ''
+    const code = 304
+    const server = createServer({ joinDuplicateHeaders: true }, (_, res) => {
+      requestsToOrigin++
+      res.statusCode = code
+      res.setHeader('cache-control', 'public, max-age=60')
+      res.end(body)
+    }).listen(0)
+
+    const client = new Client(`http://localhost:${server.address().port}`)
+      .compose(interceptors.cache())
+
+    after(async () => {
+      server.close()
+      await client.close()
+    })
+
+    await once(server, 'listening')
+
+    equal(requestsToOrigin, 0)
+
+    const request = {
+      origin: 'localhost',
+      method: 'GET',
+      path: '/',
+      headers: {
+        'if-none-match': 'some-etag'
+      }
+    }
+
+    // First request should hit the origin
+    {
+      const res = await client.request(request)
+      equal(requestsToOrigin, 1)
+      equal(res.statusCode, code)
+      strictEqual(await res.body.text(), body)
+    }
+
+    // Second request with different etag should hit the origin too
+    request.headers['if-none-match'] = 'another-etag'
+    {
+      const res = await client.request(request)
+      equal(requestsToOrigin, 2)
+      equal(res.statusCode, code)
+      strictEqual(await res.body.text(), body)
+    }
+  })
+
+  test('discriminates caching of conditionnal requests (if-modified-since), or does not cache them', async () => {
+    let requestsToOrigin = 0
+    const body = ''
+    const code = 304
+    const server = createServer({ joinDuplicateHeaders: true }, (_, res) => {
+      requestsToOrigin++
+      res.statusCode = code
+      res.setHeader('cache-control', 'public, max-age=60')
+      res.end(body)
+    }).listen(0)
+
+    const client = new Client(`http://localhost:${server.address().port}`)
+      .compose(interceptors.cache())
+
+    after(async () => {
+      server.close()
+      await client.close()
+    })
+
+    await once(server, 'listening')
+
+    equal(requestsToOrigin, 0)
+
+    const request = {
+      origin: 'localhost',
+      method: 'GET',
+      path: '/',
+      headers: {
+        'if-modified-since': new Date().toUTCString()
+      }
+    }
+
+    // First request should hit the origin
+    {
+      const res = await client.request(request)
+      equal(requestsToOrigin, 1)
+      equal(res.statusCode, code)
+      strictEqual(await res.body.text(), body)
+    }
+
+    // Second request with different since should hit the origin too
+    request.headers['if-modified-since'] = new Date(0).toUTCString()
+    {
+      const res = await client.request(request)
+      equal(requestsToOrigin, 2)
+      equal(res.statusCode, code)
+      strictEqual(await res.body.text(), body)
     }
   })
 })