From ee987512b34c4431661fd78e0252358f699c3685 Mon Sep 17 00:00:00 2001 From: Jesse Wright Date: Sat, 4 Jul 2026 16:32:25 +0000 Subject: [PATCH 1/3] fix: enforce must-revalidate and proxy-revalidate over max-stale and stale-if-error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 9111 §5.2.2.2: a cache MUST NOT use a stale response with must-revalidate to satisfy any request without successful validation — this takes precedence over the request's max-stale grace (§5.2.1.2) and excludes the response from stale-if-error (RFC 5861 §4). proxy-revalidate (§5.2.2.8) has the same semantics for shared caches (the interceptor's default type) and was parsed but never enforced. Before: store `200 Cache-Control: max-age=1, must-revalidate` (+ ETag), wait past staleness, request with `Cache-Control: max-stale=600` — the stale response is served straight from cache with zero origin contact. After: a conditional request is sent to the origin and the cached response is only reused on successful validation. isStale() now skips the max-stale grace window and handleResult() skips the stale-if-error threshold when the stored response carries must-revalidate (or proxy-revalidate on a shared cache). Fixes #5508 Co-Authored-By: Claude Fable 5 --- lib/interceptor/cache.js | 26 ++++- test/interceptors/cache.js | 229 +++++++++++++++++++++++++++++++++++++ 2 files changed, 251 insertions(+), 4 deletions(-) diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index fe56baa82c9..1ef3cc7aef1 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -57,16 +57,34 @@ function needsRevalidation (result, cacheControlDirectives, { headers = {} }) { return false } +/** + * A response with the must-revalidate directive (or proxy-revalidate for + * shared caches) can't be used once stale without successful validation, + * taking precedence over max-stale and stale-if-error + * https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.2 + * https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.8 + * @param {import('../../types/cache-interceptor.d.ts').default.GetResult} result + * @param {'shared' | 'private'} cacheType + * @returns {boolean} + */ +function forbidsServingStale (result, cacheType) { + return Boolean( + result.cacheControlDirectives?.['must-revalidate'] || + (cacheType === 'shared' && result.cacheControlDirectives?.['proxy-revalidate']) + ) +} + /** * @param {import('../../types/cache-interceptor.d.ts').default.GetResult} result * @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives | undefined} cacheControlDirectives + * @param {'shared' | 'private'} cacheType * @returns {boolean} */ -function isStale (result, cacheControlDirectives) { +function isStale (result, cacheControlDirectives, cacheType) { const now = Date.now() if (now > result.staleAt) { // Response is stale - if (cacheControlDirectives?.['max-stale']) { + if (cacheControlDirectives?.['max-stale'] && !forbidsServingStale(result, cacheType)) { // There's a threshold where we can serve stale responses, let's see if // we're in it // https://www.rfc-editor.org/rfc/rfc9111.html#name-max-stale @@ -286,7 +304,7 @@ function handleResult ( return dispatch(opts, handler) } - const stale = isStale(result, reqCacheControl) + const stale = isStale(result, reqCacheControl, globalOpts.type) const revalidate = needsRevalidation(result, reqCacheControl, opts) // Check if the response is stale @@ -345,7 +363,7 @@ function handleResult ( let withinStaleIfErrorThreshold = false const staleIfErrorExpiry = result.cacheControlDirectives['stale-if-error'] ?? reqCacheControl?.['stale-if-error'] - if (staleIfErrorExpiry) { + if (staleIfErrorExpiry && !forbidsServingStale(result, globalOpts.type)) { withinStaleIfErrorThreshold = now < (result.staleAt + (staleIfErrorExpiry * 1000)) } diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js index e31b1e307d0..d429f8004de 100644 --- a/test/interceptors/cache.js +++ b/test/interceptors/cache.js @@ -749,6 +749,67 @@ describe('Cache Interceptor', () => { } }) + test('must-revalidate excludes the response from stale-if-error', async () => { + const clock = FakeTimers.install({ + toFake: ['Date'] + }) + + let requestsToOrigin = 0 + const server = createServer({ joinDuplicateHeaders: true }, (_, res) => { + res.setHeader('date', 0) + + requestsToOrigin++ + if (requestsToOrigin === 1) { + // First request + res.setHeader('cache-control', 'public, s-maxage=10, stale-if-error=20, must-revalidate') + res.end('asd') + } else { + res.statusCode = 500 + res.end('') + } + }).listen(0) + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + clock.uninstall() + server.close() + await client.close() + }) + + await once(server, 'listening') + + /** + * @type {import('../../types/dispatcher').default.RequestOptions} + */ + const request = { + origin: 'localhost', + method: 'GET', + path: '/' + } + + // Send first request. This will hit the origin and succeed + { + const response = await client.request(request) + equal(requestsToOrigin, 1) + equal(response.statusCode, 200) + equal(await response.body.text(), 'asd') + } + + clock.tick(15000) + + // Send second request. The response is stale and revalidation fails. + // Despite being within the stale-if-error threshold, must-revalidate + // forbids serving it without successful validation (RFC 5861 §4, + // RFC 9111 §5.2.2.2), so we should see the error. + { + const response = await client.request(request) + equal(requestsToOrigin, 2) + equal(response.statusCode, 500) + } + }) + describe('Client-side directives', () => { test('max-age', async () => { const clock = FakeTimers.install({ @@ -884,6 +945,174 @@ describe('Cache Interceptor', () => { equal(revalidationRequests, 1) }) + test('max-stale doesn\'t allow serving a stale response with must-revalidate', async () => { + const clock = FakeTimers.install({ + toFake: ['Date'] + }) + + let requestsToOrigin = 0 + let revalidationRequests = 0 + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + res.setHeader('date', 0) + + if (req.headers['if-none-match']) { + revalidationRequests++ + if (req.headers['if-none-match'] !== '"asd123"') { + res.statusCode = 500 + res.end('received incorrect etag') + return + } + + res.statusCode = 304 + res.end() + } else { + requestsToOrigin++ + res.setHeader('cache-control', 'public, s-maxage=1, must-revalidate') + res.setHeader('etag', '"asd123"') + res.end('asd') + } + }).listen(0) + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + server.close() + await client.close() + clock.uninstall() + }) + + await once(server, 'listening') + + /** + * @type {import('../../types/dispatcher').default.RequestOptions} + */ + const request = { + origin: 'localhost', + method: 'GET', + path: '/' + } + + // Prime the cache + { + const response = await client.request(request) + strictEqual(await response.body.text(), 'asd') + equal(requestsToOrigin, 1) + equal(revalidationRequests, 0) + } + + clock.tick(1500) + + // The response is now stale. max-stale would normally allow serving it + // as-is, but must-revalidate forbids using a stale response without + // successful validation (RFC 9111 §5.2.2.2) + { + const response = await client.request({ + ...request, + headers: { + 'cache-control': 'max-stale=600' + } + }) + strictEqual(response.statusCode, 200) + strictEqual(await response.body.text(), 'asd') + equal(requestsToOrigin, 1) + equal(revalidationRequests, 1) + } + }) + + test('max-stale doesn\'t allow a shared cache to serve a stale response with proxy-revalidate', async () => { + const clock = FakeTimers.install({ + toFake: ['Date'] + }) + + let revalidationRequests = 0 + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + res.setHeader('date', 0) + + if (req.headers['if-none-match']) { + revalidationRequests++ + if (req.headers['if-none-match'] !== '"asd123"') { + res.statusCode = 500 + res.end('received incorrect etag') + return + } + + res.statusCode = 304 + res.end() + } else { + res.setHeader('cache-control', 'public, max-age=1, proxy-revalidate') + res.setHeader('etag', '"asd123"') + res.end('asd') + } + }).listen(0) + + const origin = `http://localhost:${server.address().port}` + const sharedClient = new Client(origin) + .compose(interceptors.cache({ type: 'shared' })) + const privateClient = new Client(origin) + .compose(interceptors.cache({ type: 'private' })) + + after(async () => { + server.close() + await sharedClient.close() + await privateClient.close() + clock.uninstall() + }) + + await once(server, 'listening') + + /** + * @type {import('../../types/dispatcher').default.RequestOptions} + */ + const request = { + origin: 'localhost', + method: 'GET', + path: '/' + } + + // Prime both caches + { + const response = await sharedClient.request(request) + strictEqual(await response.body.text(), 'asd') + } + { + const response = await privateClient.request(request) + strictEqual(await response.body.text(), 'asd') + } + equal(revalidationRequests, 0) + + clock.tick(1500) + + // proxy-revalidate has the same semantics as must-revalidate for shared + // caches (RFC 9111 §5.2.2.8), so the shared cache needs to revalidate + // despite the request's max-stale + { + const response = await sharedClient.request({ + ...request, + headers: { + 'cache-control': 'max-stale=600' + } + }) + strictEqual(response.statusCode, 200) + strictEqual(await response.body.text(), 'asd') + equal(revalidationRequests, 1) + } + + // proxy-revalidate doesn't apply to private caches, so max-stale allows + // serving the stale response without validation + { + const response = await privateClient.request({ + ...request, + headers: { + 'cache-control': 'max-stale=600' + } + }) + strictEqual(response.statusCode, 200) + strictEqual(await response.body.text(), 'asd') + equal(revalidationRequests, 1) + } + }) + test('min-fresh', async () => { const clock = FakeTimers.install({ toFake: ['Date'] From 4550ecf1746cd12846a3e9df3f1f7be12c61ea19 Mon Sep 17 00:00:00 2001 From: Jesse Wright <63333554+jeswr@users.noreply.github.com> Date: Sat, 4 Jul 2026 19:52:26 +0000 Subject: [PATCH 2/3] chore: re-trigger CI (unrelated flaky h2 TLS test on Node 26/ubuntu) 'Should end h2 zero-length request bodies with headers' failed with ERR_OSSL_ASN1_ILLEGAL_PADDING from a runtime-generated self-signed cert (@metcoder95/https-pem). The plain Node 26/ubuntu job passed on the same commit and the test passes locally on this branch and on main; this PR only touches lib/interceptor/cache.js. Co-Authored-By: Claude Fable 5 From c39128091c84d39cb9cf1d71dea5f891c6621783 Mon Sep 17 00:00:00 2001 From: Jesse Wright <63333554+jeswr@users.noreply.github.com> Date: Sat, 4 Jul 2026 21:03:55 +0000 Subject: [PATCH 3/3] docs: shorten cache interceptor comments Addresses review feedback by tightening the forbidsServingStale JSDoc to match undici's terse comment style; RFC references retained. Co-Authored-By: Claude Fable 5 --- lib/interceptor/cache.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 1ef3cc7aef1..5d497fca561 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -58,9 +58,8 @@ function needsRevalidation (result, cacheControlDirectives, { headers = {} }) { } /** - * A response with the must-revalidate directive (or proxy-revalidate for - * shared caches) can't be used once stale without successful validation, - * taking precedence over max-stale and stale-if-error + * must-revalidate (proxy-revalidate for shared caches) forbids serving a stale + * response without successful validation, overriding max-stale and stale-if-error. * https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.2 * https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.8 * @param {import('../../types/cache-interceptor.d.ts').default.GetResult} result