From c478460c56e1a0a137e3172a1276f17647a63bb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrg=C3=BCn=20Day=C4=B1o=C4=9Flu?= Date: Sat, 1 Feb 2025 13:16:07 +0100 Subject: [PATCH] fix: handle missing vary header values Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 2 +- lib/util/cache.js | 17 ++++---- test/issue-3959.js | 55 ++++++++++++++++++++++++++ test/types/cache-interceptor.test-d.ts | 39 ++++++++++++++++++ test/utils/cache.js | 41 +++++++++++++++++++ types/cache-interceptor.d.ts | 4 +- 6 files changed, 147 insertions(+), 11 deletions(-) create mode 100644 test/issue-3959.js create mode 100644 test/utils/cache.js diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index dd5ac00b5a3..c4d0eb8a949 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -79,7 +79,7 @@ class MemoryCacheStore { const entry = this.#entries.get(topLevelKey)?.find((entry) => ( entry.deleteAt > now && entry.method === key.method && - (entry.vary == null || Object.keys(entry.vary).every(headerName => entry.vary[headerName] === key.headers?.[headerName])) + (entry.vary == null || Object.keys(entry.vary).every(headerName => entry.vary[headerName] === key.headers[headerName])) )) return entry == null diff --git a/lib/util/cache.js b/lib/util/cache.js index 35c53512b2a..80f8b259d5b 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -26,10 +26,14 @@ function makeCacheKey (opts) { if (typeof key !== 'string' || typeof val !== 'string') { throw new Error('opts.headers is not a valid header map') } - headers[key] = val + headers[key.toLowerCase()] = val } } else if (typeof opts.headers === 'object') { - headers = opts.headers + headers = {} + + for (const key of Object.keys(opts.headers)) { + headers[key.toLowerCase()] = opts.headers[key] + } } else { throw new Error('opts.headers is not an object') } @@ -260,19 +264,16 @@ function parseVaryHeader (varyHeader, headers) { return headers } - const output = /** @type {Record} */ ({}) + const output = /** @type {Record} */ ({}) const varyingHeaders = typeof varyHeader === 'string' ? varyHeader.split(',') : varyHeader + for (const header of varyingHeaders) { const trimmedHeader = header.trim().toLowerCase() - if (headers[trimmedHeader]) { - output[trimmedHeader] = headers[trimmedHeader] - } else { - return undefined - } + output[trimmedHeader] = headers[trimmedHeader] } return output diff --git a/test/issue-3959.js b/test/issue-3959.js new file mode 100644 index 00000000000..6506f0f92fc --- /dev/null +++ b/test/issue-3959.js @@ -0,0 +1,55 @@ +const { describe, test, after } = require('node:test') +const assert = require('node:assert') +const { createServer } = require('node:http') +const MemoryCacheStore = require('../lib/cache/memory-cache-store.js') +const { request, Agent, setGlobalDispatcher } = require('..') +const { interceptors } = require('..') + +describe('Cache with Vary headers', () => { + test('should cache response when Vary header exists but request header is missing', async () => { + let requestCount = 0 + const server = createServer((req, res) => { + requestCount++ + res.setHeader('Vary', 'Accept-Encoding') + res.setHeader('Cache-Control', 'max-age=60') + res.end(`Request count: ${requestCount}`) + }) + + await new Promise(resolve => server.listen(0, resolve)) + const port = server.address().port + const url = `http://localhost:${port}` + + const store = new MemoryCacheStore() + const agent = new Agent() + setGlobalDispatcher( + agent.compose( + interceptors.cache({ + store, + cacheByDefault: 1000, + methods: ['GET'] + }) + ) + ) + + const res1 = await request(url) + const body1 = await res1.body.text() + assert.strictEqual(body1, 'Request count: 1') + assert.strictEqual(requestCount, 1) + + const res2 = await request(url) + const body2 = await res2.body.text() + assert.strictEqual(body2, 'Request count: 1') + assert.strictEqual(requestCount, 1) + + const res3 = await request(url, { + headers: { + 'Accept-Encoding': 'gzip' + } + }) + const body3 = await res3.body.text() + assert.strictEqual(body3, 'Request count: 2') + assert.strictEqual(requestCount, 2) + + after(() => server.close()) + }) +}) diff --git a/test/types/cache-interceptor.test-d.ts b/test/types/cache-interceptor.test-d.ts index 6fb66955de5..58d70d79e1a 100644 --- a/test/types/cache-interceptor.test-d.ts +++ b/test/types/cache-interceptor.test-d.ts @@ -78,6 +78,45 @@ expectNotAssignable({ deleteAt: '' }) +expectAssignable({ + statusCode: 200, + statusMessage: 'OK', + headers: {}, + vary: { + 'accept-encoding': undefined, + authorization: 'example-value' + }, + cachedAt: 0, + staleAt: 0, + deleteAt: 0 +}) + +expectAssignable({ + statusCode: 200, + statusMessage: 'OK', + headers: {}, + vary: { + 'accept-encoding': undefined, + authorization: undefined + }, + cachedAt: 0, + staleAt: 0, + deleteAt: 0 +}) + +expectNotAssignable({ + statusCode: 200, + statusMessage: 'OK', + headers: {}, + vary: { + 'accept-encoding': false, + authorization: 'example-value' + }, + cachedAt: 0, + staleAt: 0, + deleteAt: 0 +}) + expectAssignable({}) expectAssignable({ maxSize: 0 diff --git a/test/utils/cache.js b/test/utils/cache.js new file mode 100644 index 00000000000..8bf830c735e --- /dev/null +++ b/test/utils/cache.js @@ -0,0 +1,41 @@ +'use strict' + +const { describe, test } = require('node:test') +const assert = require('node:assert') +const { parseVaryHeader } = require('../../lib/util/cache.js') + +describe('parseVaryHeader', () => { + test('handles missing headers with undefined', () => { + const result = parseVaryHeader('Accept-Encoding, Authorization', {}) + assert.deepStrictEqual(result, { + 'accept-encoding': undefined, + authorization: undefined + }) + }) + + test('handles mix of present and missing headers', () => { + const result = parseVaryHeader('Accept-Encoding, Authorization', { + authorization: 'example-value' + }) + assert.deepStrictEqual(result, { + 'accept-encoding': undefined, + authorization: 'example-value' + }) + }) + + test('handles array input', () => { + const result = parseVaryHeader(['Accept-Encoding', 'Authorization'], { + 'accept-encoding': 'gzip' + }) + assert.deepStrictEqual(result, { + 'accept-encoding': 'gzip', + authorization: undefined + }) + }) + + test('preserves existing * behavior', () => { + const headers = { accept: 'text/html' } + const result = parseVaryHeader('*', headers) + assert.deepStrictEqual(result, headers) + }) +}) diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 1713ca7e747..ecbfd067167 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -70,7 +70,7 @@ declare namespace CacheHandler { statusCode: number statusMessage: string headers: Record - vary?: Record + vary?: Record etag?: string cacheControlDirectives?: CacheControlDirectives cachedAt: number @@ -88,7 +88,7 @@ declare namespace CacheHandler { statusCode: number statusMessage: string headers: Record - vary?: Record + vary?: Record etag?: string body?: Readable | Iterable | AsyncIterable | Buffer | Iterable | AsyncIterable | string cacheControlDirectives: CacheControlDirectives,