Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cache: make vary headers case-insensitive #3990

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 18 additions & 14 deletions lib/handler/cache-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ const util = require('../core/util')
const {
parseCacheControlHeader,
parseVaryHeader,
isEtagUsable
isEtagUsable,
makeHeaderNamesLowercase
} = require('../util/cache')
const { parseHttpDate } = require('../util/date.js')

Expand Down Expand Up @@ -111,11 +112,13 @@ class CacheHandler {
return downstreamOnHeaders()
}

const cacheControlHeader = resHeaders['cache-control']
const heuristicallyCacheable = resHeaders['last-modified'] && HEURISTICALLY_CACHEABLE_STATUS_CODES.includes(statusCode)
const resHeadersLowercase = makeHeaderNamesLowercase(resHeaders)

const cacheControlHeader = resHeadersLowercase['cache-control']
const heuristicallyCacheable = resHeadersLowercase['last-modified'] && HEURISTICALLY_CACHEABLE_STATUS_CODES.includes(statusCode)
if (
!cacheControlHeader &&
!resHeaders['expires'] &&
!resHeadersLowercase.expires &&
!heuristicallyCacheable &&
!this.#cacheByDefault
) {
Expand All @@ -125,23 +128,23 @@ class CacheHandler {
}

const cacheControlDirectives = cacheControlHeader ? parseCacheControlHeader(cacheControlHeader) : {}
if (!canCacheResponse(this.#cacheType, statusCode, resHeaders, cacheControlDirectives)) {
if (!canCacheResponse(this.#cacheType, statusCode, resHeadersLowercase, cacheControlDirectives)) {
return downstreamOnHeaders()
}

const now = Date.now()
const resAge = resHeaders.age ? getAge(resHeaders.age) : undefined
const resAge = resHeadersLowercase.age ? getAge(resHeadersLowercase.age) : undefined
if (resAge && resAge >= MAX_RESPONSE_AGE) {
// Response considered stale
return downstreamOnHeaders()
}

const resDate = typeof resHeaders.date === 'string'
? parseHttpDate(resHeaders.date)
const resDate = typeof resHeadersLowercase.date === 'string'
? parseHttpDate(resHeadersLowercase.date)
: undefined

const staleAt =
determineStaleAt(this.#cacheType, now, resAge, resHeaders, resDate, cacheControlDirectives) ??
determineStaleAt(this.#cacheType, now, resAge, resHeadersLowercase, resDate, cacheControlDirectives) ??
this.#cacheByDefault
if (staleAt === undefined || (resAge && resAge > staleAt)) {
return downstreamOnHeaders()
Expand All @@ -155,16 +158,17 @@ class CacheHandler {
}

let varyDirectives
if (this.#cacheKey.headers && resHeaders.vary) {
varyDirectives = parseVaryHeader(resHeaders.vary, this.#cacheKey.headers)
if (this.#cacheKey.headers && resHeadersLowercase.vary) {
varyDirectives = parseVaryHeader(resHeadersLowercase.vary, this.#cacheKey.headers)

if (!varyDirectives) {
// Parse error
return downstreamOnHeaders()
}
}

const deleteAt = determineDeleteAt(baseTime, cacheControlDirectives, absoluteStaleAt)
const strippedHeaders = stripNecessaryHeaders(resHeaders, cacheControlDirectives)
const strippedHeaders = stripNecessaryHeaders(resHeadersLowercase, cacheControlDirectives)

/**
* @type {import('../../types/cache-interceptor.d.ts').default.CacheValue}
Expand All @@ -180,8 +184,8 @@ class CacheHandler {
deleteAt
}

if (typeof resHeaders.etag === 'string' && isEtagUsable(resHeaders.etag)) {
value.etag = resHeaders.etag
if (typeof resHeadersLowercase.etag === 'string' && isEtagUsable(resHeadersLowercase.etag)) {
value.etag = resHeadersLowercase.etag
}

this.#writeStream = this.#store.createWriteStream(this.#cacheKey, value)
Expand Down
19 changes: 17 additions & 2 deletions lib/util/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ function makeCacheKey (opts) {
origin: opts.origin.toString(),
method: opts.method,
path: opts.path,
headers
headers: makeHeaderNamesLowercase(headers)
}
}

Expand Down Expand Up @@ -347,6 +347,20 @@ function assertCacheMethods (methods, name = 'CacheMethods') {
}
}

/**
* @param {import('../../types/header.d.ts').IncomingHttpHeaders} headers
* @returns {import('../../types/header.d.ts').IncomingHttpHeaders}
*/
function makeHeaderNamesLowercase (headers) {
const lowercased = {}

for (const header of Object.keys(headers)) {
lowercased[header.toLowerCase()] = headers[header]
}

return lowercased
}

module.exports = {
makeCacheKey,
assertCacheKey,
Expand All @@ -355,5 +369,6 @@ module.exports = {
parseVaryHeader,
isEtagUsable,
assertCacheMethods,
assertCacheStore
assertCacheStore,
makeHeaderNamesLowercase
}
45 changes: 45 additions & 0 deletions test/interceptors/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,51 @@ describe('Cache Interceptor', () => {
}
})

test('vary directives are case-insensitive', async () => {
let requestsToOrigin = 0
const server = createServer((_, res) => {
requestsToOrigin++

res.setHeader('date', 0)
res.setHeader('cache-control', 'max-age=5000')
res.setHeader('vary', 'FoO, bar, bAZ')

res.end('asd')
}).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')

strictEqual(requestsToOrigin, 0)

/**
* @type {import('../../types/dispatcher').default.RequestOptions}
*/
const request = {
origin: 'localhost',
method: 'GET',
path: '/',
headers: {
Foo: '1',
BAr: 'abc',
BAZ: '789'
}
}

await client.request(request)
equal(requestsToOrigin, 1)

await client.request(request)
equal(requestsToOrigin, 1)
})

test('stale responses are revalidated before deleteAt (if-modified-since)', async () => {
const clock = FakeTimers.install({
shouldClearNativeTimers: true
Expand Down
Loading