Skip to content

Commit 7b7bf3d

Browse files
jeswrjeswr
authored
fix: enforce must-revalidate and proxy-revalidate over max-stale and stale-if-error (#5511)
1 parent afd7194 commit 7b7bf3d

2 files changed

Lines changed: 250 additions & 4 deletions

File tree

‎lib/interceptor/cache.js‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,16 +57,33 @@ function needsRevalidation (result, cacheControlDirectives, { headers = {} }) {
5757
return false
5858
}
5959

60+
/**
61+
* must-revalidate (proxy-revalidate for shared caches) forbids serving a stale
62+
* response without successful validation, overriding max-stale and stale-if-error.
63+
* https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.2
64+
* https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.8
65+
* @param {import('../../types/cache-interceptor.d.ts').default.GetResult} result
66+
* @param {'shared' | 'private'} cacheType
67+
* @returns {boolean}
68+
*/
69+
function forbidsServingStale (result, cacheType) {
70+
return Boolean(
71+
result.cacheControlDirectives?.['must-revalidate'] ||
72+
(cacheType === 'shared' && result.cacheControlDirectives?.['proxy-revalidate'])
73+
)
74+
}
75+
6076
/**
6177
* @param {import('../../types/cache-interceptor.d.ts').default.GetResult} result
6278
* @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives | undefined} cacheControlDirectives
79+
* @param {'shared' | 'private'} cacheType
6380
* @returns {boolean}
6481
*/
65-
function isStale (result, cacheControlDirectives) {
82+
function isStale (result, cacheControlDirectives, cacheType) {
6683
const now = Date.now()
6784
if (now > result.staleAt) {
6885
// Response is stale
69-
if (cacheControlDirectives?.['max-stale']) {
86+
if (cacheControlDirectives?.['max-stale'] && !forbidsServingStale(result, cacheType)) {
7087
// There's a threshold where we can serve stale responses, let's see if
7188
// we're in it
7289
// https://www.rfc-editor.org/rfc/rfc9111.html#name-max-stale
@@ -286,7 +303,7 @@ function handleResult (
286303
return dispatch(opts, handler)
287304
}
288305

289-
const stale = isStale(result, reqCacheControl)
306+
const stale = isStale(result, reqCacheControl, globalOpts.type)
290307
const revalidate = needsRevalidation(result, reqCacheControl, opts)
291308

292309
// Check if the response is stale
@@ -345,7 +362,7 @@ function handleResult (
345362

346363
let withinStaleIfErrorThreshold = false
347364
const staleIfErrorExpiry = result.cacheControlDirectives['stale-if-error'] ?? reqCacheControl?.['stale-if-error']
348-
if (staleIfErrorExpiry) {
365+
if (staleIfErrorExpiry && !forbidsServingStale(result, globalOpts.type)) {
349366
withinStaleIfErrorThreshold = now < (result.staleAt + (staleIfErrorExpiry * 1000))
350367
}
351368

‎test/interceptors/cache.js‎

Lines changed: 229 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -749,6 +749,67 @@ describe('Cache Interceptor', () => {
749749
}
750750
})
751751

752+
test('must-revalidate excludes the response from stale-if-error', async () => {
753+
const clock = FakeTimers.install({
754+
toFake: ['Date']
755+
})
756+
757+
let requestsToOrigin = 0
758+
const server = createServer({ joinDuplicateHeaders: true }, (_, res) => {
759+
res.setHeader('date', 0)
760+
761+
requestsToOrigin++
762+
if (requestsToOrigin === 1) {
763+
// First request
764+
res.setHeader('cache-control', 'public, s-maxage=10, stale-if-error=20, must-revalidate')
765+
res.end('asd')
766+
} else {
767+
res.statusCode = 500
768+
res.end('')
769+
}
770+
}).listen(0)
771+
772+
const client = new Client(`http://localhost:${server.address().port}`)
773+
.compose(interceptors.cache())
774+
775+
after(async () => {
776+
clock.uninstall()
777+
server.close()
778+
await client.close()
779+
})
780+
781+
await once(server, 'listening')
782+
783+
/**
784+
* @type {import('../../types/dispatcher').default.RequestOptions}
785+
*/
786+
const request = {
787+
origin: 'localhost',
788+
method: 'GET',
789+
path: '/'
790+
}
791+
792+
// Send first request. This will hit the origin and succeed
793+
{
794+
const response = await client.request(request)
795+
equal(requestsToOrigin, 1)
796+
equal(response.statusCode, 200)
797+
equal(await response.body.text(), 'asd')
798+
}
799+
800+
clock.tick(15000)
801+
802+
// Send second request. The response is stale and revalidation fails.
803+
// Despite being within the stale-if-error threshold, must-revalidate
804+
// forbids serving it without successful validation (RFC 5861 §4,
805+
// RFC 9111 §5.2.2.2), so we should see the error.
806+
{
807+
const response = await client.request(request)
808+
equal(requestsToOrigin, 2)
809+
equal(response.statusCode, 500)
810+
}
811+
})
812+
752813
describe('Client-side directives', () => {
753814
test('max-age', async () => {
754815
const clock = FakeTimers.install({
@@ -884,6 +945,174 @@ describe('Cache Interceptor', () => {
884945
equal(revalidationRequests, 1)
885946
})
886947

948+
test('max-stale doesn\'t allow serving a stale response with must-revalidate', async () => {
949+
const clock = FakeTimers.install({
950+
toFake: ['Date']
951+
})
952+
953+
let requestsToOrigin = 0
954+
let revalidationRequests = 0
955+
const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
956+
res.setHeader('date', 0)
957+
958+
if (req.headers['if-none-match']) {
959+
revalidationRequests++
960+
if (req.headers['if-none-match'] !== '"asd123"') {
961+
res.statusCode = 500
962+
res.end('received incorrect etag')
963+
return
964+
}
965+
966+
res.statusCode = 304
967+
res.end()
968+
} else {
969+
requestsToOrigin++
970+
res.setHeader('cache-control', 'public, s-maxage=1, must-revalidate')
971+
res.setHeader('etag', '"asd123"')
972+
res.end('asd')
973+
}
974+
}).listen(0)
975+
976+
const client = new Client(`http://localhost:${server.address().port}`)
977+
.compose(interceptors.cache())
978+
979+
after(async () => {
980+
server.close()
981+
await client.close()
982+
clock.uninstall()
983+
})
984+
985+
await once(server, 'listening')
986+
987+
/**
988+
* @type {import('../../types/dispatcher').default.RequestOptions}
989+
*/
990+
const request = {
991+
origin: 'localhost',
992+
method: 'GET',
993+
path: '/'
994+
}
995+
996+
// Prime the cache
997+
{
998+
const response = await client.request(request)
999+
strictEqual(await response.body.text(), 'asd')
1000+
equal(requestsToOrigin, 1)
1001+
equal(revalidationRequests, 0)
1002+
}
1003+
1004+
clock.tick(1500)
1005+
1006+
// The response is now stale. max-stale would normally allow serving it
1007+
// as-is, but must-revalidate forbids using a stale response without
1008+
// successful validation (RFC 9111 §5.2.2.2)
1009+
{
1010+
const response = await client.request({
1011+
...request,
1012+
headers: {
1013+
'cache-control': 'max-stale=600'
1014+
}
1015+
})
1016+
strictEqual(response.statusCode, 200)
1017+
strictEqual(await response.body.text(), 'asd')
1018+
equal(requestsToOrigin, 1)
1019+
equal(revalidationRequests, 1)
1020+
}
1021+
})
1022+
1023+
test('max-stale doesn\'t allow a shared cache to serve a stale response with proxy-revalidate', async () => {
1024+
const clock = FakeTimers.install({
1025+
toFake: ['Date']
1026+
})
1027+
1028+
let revalidationRequests = 0
1029+
const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
1030+
res.setHeader('date', 0)
1031+
1032+
if (req.headers['if-none-match']) {
1033+
revalidationRequests++
1034+
if (req.headers['if-none-match'] !== '"asd123"') {
1035+
res.statusCode = 500
1036+
res.end('received incorrect etag')
1037+
return
1038+
}
1039+
1040+
res.statusCode = 304
1041+
res.end()
1042+
} else {
1043+
res.setHeader('cache-control', 'public, max-age=1, proxy-revalidate')
1044+
res.setHeader('etag', '"asd123"')
1045+
res.end('asd')
1046+
}
1047+
}).listen(0)
1048+
1049+
const origin = `http://localhost:${server.address().port}`
1050+
const sharedClient = new Client(origin)
1051+
.compose(interceptors.cache({ type: 'shared' }))
1052+
const privateClient = new Client(origin)
1053+
.compose(interceptors.cache({ type: 'private' }))
1054+
1055+
after(async () => {
1056+
server.close()
1057+
await sharedClient.close()
1058+
await privateClient.close()
1059+
clock.uninstall()
1060+
})
1061+
1062+
await once(server, 'listening')
1063+
1064+
/**
1065+
* @type {import('../../types/dispatcher').default.RequestOptions}
1066+
*/
1067+
const request = {
1068+
origin: 'localhost',
1069+
method: 'GET',
1070+
path: '/'
1071+
}
1072+
1073+
// Prime both caches
1074+
{
1075+
const response = await sharedClient.request(request)
1076+
strictEqual(await response.body.text(), 'asd')
1077+
}
1078+
{
1079+
const response = await privateClient.request(request)
1080+
strictEqual(await response.body.text(), 'asd')
1081+
}
1082+
equal(revalidationRequests, 0)
1083+
1084+
clock.tick(1500)
1085+
1086+
// proxy-revalidate has the same semantics as must-revalidate for shared
1087+
// caches (RFC 9111 §5.2.2.8), so the shared cache needs to revalidate
1088+
// despite the request's max-stale
1089+
{
1090+
const response = await sharedClient.request({
1091+
...request,
1092+
headers: {
1093+
'cache-control': 'max-stale=600'
1094+
}
1095+
})
1096+
strictEqual(response.statusCode, 200)
1097+
strictEqual(await response.body.text(), 'asd')
1098+
equal(revalidationRequests, 1)
1099+
}
1100+
1101+
// proxy-revalidate doesn't apply to private caches, so max-stale allows
1102+
// serving the stale response without validation
1103+
{
1104+
const response = await privateClient.request({
1105+
...request,
1106+
headers: {
1107+
'cache-control': 'max-stale=600'
1108+
}
1109+
})
1110+
strictEqual(response.statusCode, 200)
1111+
strictEqual(await response.body.text(), 'asd')
1112+
equal(revalidationRequests, 1)
1113+
}
1114+
})
1115+
8871116
test('min-fresh', async () => {
8881117
const clock = FakeTimers.install({
8891118
toFake: ['Date']

0 commit comments

Comments
 (0)