diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 3c4044a653e..d48675c3760 100644 --- 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 index b03734aa886..63d56b6880b 100644 --- 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 deleted file mode 100644 index 40cad118bba..00000000000 --- a/test/cache-interceptor/interceptor.js +++ /dev/null @@ -1,240 +0,0 @@ -'use strict' - -const { describe, test, after } = require('node:test') -const { strictEqual, notEqual, fail } = require('node:assert') -const { createServer } = require('node:http') -const { once } = require('node:events') -const { Client, interceptors, cacheStores } = require('../../index') - -describe('Cache Interceptor', () => { - test('doesn\'t cache request w/ no cache-control header', async () => { - let requestsToOrigin = 0 - - const server = createServer((_, res) => { - requestsToOrigin++ - res.end('asd') - }).listen(0) - - after(() => server.close()) - await once(server, 'listening') - - const client = new Client(`http://localhost:${server.address().port}`) - .compose(interceptors.cache()) - - strictEqual(requestsToOrigin, 0) - - // Send initial request. This should reach the origin - let response = await client.request({ - origin: 'localhost', - method: 'GET', - path: '/' - }) - strictEqual(requestsToOrigin, 1) - strictEqual(await response.body.text(), 'asd') - - // Send second request that should be handled by cache - response = await client.request({ - origin: 'localhost', - method: 'GET', - path: '/' - }) - strictEqual(requestsToOrigin, 2) - strictEqual(await response.body.text(), 'asd') - }) - - test('caches request successfully', async () => { - let requestsToOrigin = 0 - - const server = createServer((_, res) => { - requestsToOrigin++ - res.setHeader('cache-control', 'public, s-maxage=10') - res.end('asd') - }).listen(0) - - after(() => server.close()) - await once(server, 'listening') - - const client = new Client(`http://localhost:${server.address().port}`) - .compose(interceptors.cache()) - - strictEqual(requestsToOrigin, 0) - - // Send initial request. This should reach the origin - let response = await client.request({ - origin: 'localhost', - method: 'GET', - path: '/' - }) - strictEqual(requestsToOrigin, 1) - strictEqual(await response.body.text(), 'asd') - - // Send second request that should be handled by cache - response = await client.request({ - origin: 'localhost', - method: 'GET', - path: '/' - }) - strictEqual(requestsToOrigin, 1) - strictEqual(await response.body.text(), 'asd') - strictEqual(response.headers.age, '0') - }) - - test('respects vary header', async () => { - let requestsToOrigin = 0 - - const server = createServer((req, res) => { - requestsToOrigin++ - res.setHeader('cache-control', 'public, s-maxage=10') - res.setHeader('vary', 'some-header, another-header') - - if (req.headers['some-header'] === 'abc123') { - res.end('asd') - } else { - res.end('dsa') - } - }).listen(0) - - after(() => server.close()) - await once(server, 'listening') - - const client = new Client(`http://localhost:${server.address().port}`) - .compose(interceptors.cache()) - - strictEqual(requestsToOrigin, 0) - - // Send initial request. This should reach the origin - let response = await client.request({ - origin: 'localhost', - method: 'GET', - path: '/', - headers: { - 'some-header': 'abc123', - 'another-header': '123abc' - } - }) - strictEqual(requestsToOrigin, 1) - strictEqual(await response.body.text(), 'asd') - - // Make another request with changed headers, this should miss - const secondResponse = await client.request({ - method: 'GET', - path: '/', - headers: { - 'some-header': 'qwerty', - 'another-header': 'asdfg' - } - }) - strictEqual(requestsToOrigin, 2) - strictEqual(await secondResponse.body.text(), 'dsa') - - // Resend the first request again which should still be cahced - response = await client.request({ - origin: 'localhost', - method: 'GET', - path: '/', - headers: { - 'some-header': 'abc123', - 'another-header': '123abc' - } - }) - strictEqual(requestsToOrigin, 2) - strictEqual(await response.body.text(), 'asd') - }) - - test('revalidates request when needed', async () => { - let requestsToOrigin = 0 - - const server = createServer((req, res) => { - res.setHeader('cache-control', 'public, s-maxage=1, stale-while-revalidate=10') - - requestsToOrigin++ - - if (requestsToOrigin > 1) { - notEqual(req.headers['if-modified-since'], undefined) - - if (requestsToOrigin === 3) { - res.end('asd123') - } else { - res.statusCode = 304 - res.end() - } - } else { - res.end('asd') - } - }).listen(0) - - after(() => server.close()) - await once(server, 'listening') - - const client = new Client(`http://localhost:${server.address().port}`) - .compose(interceptors.cache()) - - strictEqual(requestsToOrigin, 0) - - const request = { - origin: 'localhost', - method: 'GET', - path: '/' - } - - // Send initial request. This should reach the origin - let response = await client.request(request) - strictEqual(requestsToOrigin, 1) - strictEqual(await response.body.text(), 'asd') - - // Now we send two more requests. Both of these should reach the origin, - // but now with a conditional header asking if the resource has been - // updated. These need to be ran after the response is stale. - const completed = new Promise((resolve, reject) => { - setTimeout(async () => { - try { - // No update for the second request - response = await client.request(request) - strictEqual(requestsToOrigin, 2) - strictEqual(await response.body.text(), 'asd') - - // This should be updated, even though the value isn't expired. - response = await client.request(request) - strictEqual(requestsToOrigin, 3) - strictEqual(await response.body.text(), 'asd123') - - resolve() - } catch (e) { - reject(e) - } - }, 1500) - }) - await completed - }) - - test('respects cache store\'s isFull property', async () => { - const server = createServer((_, res) => { - res.end('asd') - }).listen(0) - - after(() => server.close()) - await once(server, 'listening') - - const store = new cacheStores.MemoryCacheStore() - Object.defineProperty(store, 'isFull', { - value: true - }) - - store.createWriteStream = (...args) => { - fail('shouln\'t have reached this') - } - - const client = new Client(`http://localhost:${server.address().port}`) - .compose(interceptors.cache({ store })) - - await client.request({ - origin: 'localhost', - method: 'GET', - path: '/', - headers: { - 'some-header': 'abc123', - 'another-header': '123abc' - } - }) - }) -}) diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js index 302fc734b4b..c99cb52279f 100644 --- 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) + } + }) })