Skip to content

fix(cache): split repeated vary headers on commas#5582

Open
arshsmith1 wants to merge 1 commit into
nodejs:mainfrom
arshsmith1:vary-repeated-header-split
Open

fix(cache): split repeated vary headers on commas#5582
arshsmith1 wants to merge 1 commit into
nodejs:mainfrom
arshsmith1:vary-repeated-header-split

Conversation

@arshsmith1

Copy link
Copy Markdown

This relates to...

N/A

Rationale

parseVaryHeader only splits on commas when the Vary header arrives as a single string. An origin is free to send the header more than once, and parseHeaders then hands the cache handler an array, whose entries were used verbatim as field names. A response carrying Vary: accept-encoding, x-user-id followed by a second Vary: accept-language line is therefore stored under the field name accept-encoding, x-user-id, so x-user-id never takes part in the cache match and the next request with a different value is served the first client's body. The wildcard guard in canCacheResponse misses the same shape, since resHeaders.vary?.includes('*') on an array only matches an entry that is exactly *. With the default shared cache type both paths hand one caller a response that was stored for another.

Repro against interceptors.cache(), with a server that sets vary to ['accept-encoding, a', 'accept-language'] and echoes the a request header: the request with a: alice reaches the origin, and the following request with a: bob is answered from the cache with alice's body.

Changes

Split every element of an array-valued Vary header on commas, and share one wildcard check between the parser and canCacheResponse so a repeated header carrying * also blocks storage.

Features

N/A

Bug Fixes

  • Vary field names spread over repeated response headers are honored when matching cached entries.
  • Vary: * is detected when it arrives in a repeated header alongside other field names.

Breaking Changes and Deprecations

N/A

Status

Signed-off-by: arshiya tabasum <arshi@bugqore.com>
Comment thread lib/util/cache.js
function varyHeaderHasWildcard (varyHeader) {
return typeof varyHeader === 'string'
? varyHeader.includes('*')
: varyHeader.some(value => value.includes('*'))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please use a for(;;)/for..in loop? .some is slow

Comment thread lib/util/cache.js
const varyingHeaders = typeof varyHeader === 'string'
? varyHeader.split(',')
: varyHeader
: varyHeader.flatMap(value => value.split(','))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please avoid flatMap and actually implement a for(;;) or for..in loop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants