fix: unsafe methods not causing cache purge (#3739)

* fix: unsafe methods not causing cache purge Fixes bug introduced in #3733 where unsafe methods no longer make it to the cache handler and cause a cache purge for an origin. Also removes a duplicate test file. Signed-off-by: flakey5 <[email protected]> * Update cache.js * extend test to check that safe methods we're not caching don't purge the cache Signed-off-by: flakey5 <[email protected]> * code review Signed-off-by: flakey5 <[email protected]> --------- Signed-off-by: flakey5 <[email protected]>
nodejs · Oct 19, 2024 · 3030506 · 3030506
1 parent 8e025d1
commit 3030506
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 250 deletions.
diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js
@@ -16,11 +16,6 @@ class CacheHandler extends DecoratorHandler {
    */
   #store
 
-  /**
-   * @type {import('../../types/cache-interceptor.d.ts').default.CacheMethods}
-   */
-  #methods
-
   /**
    * @type {import('../../types/dispatcher.d.ts').default.RequestOptions}
    */
@@ -42,14 +37,13 @@ class CacheHandler extends DecoratorHandler {
    * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler
    */
   constructor (opts, requestOptions, handler) {
-    const { store, methods } = opts
+    const { store } = opts
 
     super(handler)
 
     this.#store = store
     this.#requestOptions = requestOptions
     this.#handler = handler
-    this.#methods = methods
   }
 
   /**
@@ -75,7 +69,7 @@ class CacheHandler extends DecoratorHandler {
     )
 
     if (
-      !this.#methods.includes(this.#requestOptions.method) &&
+      !util.safeHTTPMethods.includes(this.#requestOptions.method) &&
       statusCode >= 200 &&
       statusCode <= 399
     ) {

diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js
@@ -30,9 +30,11 @@ module.exports = (opts = {}) => {
     methods
   }
 
+  const safeMethodsToNotCache = util.safeHTTPMethods.filter(method => methods.includes(method) === false)
+
   return dispatch => {
     return (opts, handler) => {
-      if (!opts.origin || !methods.includes(opts.method)) {
+      if (!opts.origin || safeMethodsToNotCache.includes(opts.method)) {
         // Not a method we want to cache or we don't have the origin, skip
         return dispatch(opts, handler)
       }

diff --git a/test/cache-interceptor/interceptor.js b/test/cache-interceptor/interceptor.js
diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js
@@ -1,7 +1,7 @@
 'use strict'
 
 const { describe, test, after } = require('node:test')
-const { strictEqual, notEqual, fail } = require('node:assert')
+const { strictEqual, notEqual, fail, equal } = require('node:assert')
 const { createServer } = require('node:http')
 const { once } = require('node:events')
 const FakeTimers = require('@sinonjs/fake-timers')
@@ -250,4 +250,59 @@ describe('Cache Interceptor', () => {
       }
     })
   })
+
+  test('unsafe methods call the store\'s deleteByOrigin function', async () => {
+    const server = createServer((_, res) => {
+      res.end('asd')
+    }).listen(0)
+
+    after(() => server.close())
+    await once(server, 'listening')
+
+    let deleteByOriginCalled = false
+    const store = new cacheStores.MemoryCacheStore()
+
+    const originalDeleteByOrigin = store.deleteByOrigin.bind(store)
+    store.deleteByOrigin = (origin) => {
+      deleteByOriginCalled = true
+      originalDeleteByOrigin(origin)
+    }
+
+    const client = new Client(`http://localhost:${server.address().port}`)
+      .compose(interceptors.cache({
+        store,
+        methods: ['GET'] // explicitly only cache GET methods
+      }))
+
+    // Make sure safe methods that we want to cache don't cause a cache purge
+    await client.request({
+      origin: 'localhost',
+      method: 'GET',
+      path: '/'
+    })
+
+    equal(deleteByOriginCalled, false)
+
+    // Make sure other safe methods that we don't want to cache don't cause a cache purge
+    await client.request({
+      origin: 'localhost',
+      method: 'HEAD',
+      path: '/'
+    })
+
+    strictEqual(deleteByOriginCalled, false)
+
+    // Make sure the common unsafe methods cause cache purges
+    for (const method of ['POST', 'PUT', 'PATCH', 'DELETE']) {
+      deleteByOriginCalled = false
+
+      await client.request({
+        origin: 'localhost',
+        method,
+        path: '/'
+      })
+
+      equal(deleteByOriginCalled, true, method)
+    }
+  })
 })