Skip to content

Commit

Permalink
Prune headers that don't belong in cache metadata
Browse files Browse the repository at this point in the history
There is no justifiable reason to put the `authorization` header in the
cache metadata.  Even if we can assume that the cache should not ever be
exposed or leaked, it's a security footgun that is unnecessary.

Some other fields probably should not be cached as well.  We may want to
re-enable storing the npm or Cloudflare session IDs at some point for
debugging, but for now, these are also just excessive.
  • Loading branch information
isaacs committed Dec 5, 2020
1 parent 2662302 commit f59c2d9
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 6 deletions.
29 changes: 27 additions & 2 deletions cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,20 @@ const MinipassPipeline = require('minipass-pipeline')

const MAX_MEM_SIZE = 5 * 1024 * 1024 // 5MB

// some headers should never be stored in the cache, either because
// they're a security footgun to leave lying around, or because we
// just don't need them taking up space.
// set to undefined so they're omitted from the JSON.stringify
const pruneHeaders = {
authorization: undefined,
'npm-session': undefined,
'set-cookie': undefined,
'cf-ray': undefined,
'cf-cache-status': undefined,
'cf-request-id': undefined,
'x-fetch-attempts': undefined
}

function cacheKey (req) {
const parsed = new url.URL(req.url)
return `make-fetch-happen:request-cache:${
Expand All @@ -35,6 +49,11 @@ module.exports = class Cache {
this.Promise = (opts && opts.Promise) || Promise
}

static get pruneHeaders () {
// exposed for testing, not modifiable
return { ...pruneHeaders }
}

// Returns a Promise that resolves to the response associated with the first
// matching request in the Cache object.
match (req, opts) {
Expand Down Expand Up @@ -109,8 +128,14 @@ module.exports = class Cache {
algorithms: opts.algorithms,
metadata: {
url: req.url,
reqHeaders: req.headers.raw(),
resHeaders: response.headers.raw()
reqHeaders: {
...req.headers.raw(),
...pruneHeaders
},
resHeaders: {
...response.headers.raw(),
...pruneHeaders
}
},
size,
memoize: fitInMemory && opts.memoize
Expand Down
20 changes: 16 additions & 4 deletions test/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,14 @@ test('put method', (t) => {
algorithms: undefined,
metadata: {
url: `${HOST}/put-test`,
reqHeaders: req.headers.raw(),
resHeaders: initialResponse.headers.raw()
reqHeaders: {
...req.headers.raw(),
...Cache.pruneHeaders
},
resHeaders: {
...initialResponse.headers.raw(),
...Cache.pruneHeaders
}
},
size: CONTENT.length,
memoize: undefined
Expand Down Expand Up @@ -160,8 +166,14 @@ test('put method', (t) => {
algorithms: undefined,
metadata: {
url: `${HOST}/put-test`,
reqHeaders: req.headers.raw(),
resHeaders: initialResponse.headers.raw()
reqHeaders: {
...req.headers.raw(),
...Cache.pruneHeaders
},
resHeaders: {
...initialResponse.headers.raw(),
...Cache.pruneHeaders
}
},
size: null,
memoize: false
Expand Down

0 comments on commit f59c2d9

Please sign in to comment.