fix(cache): handleCacheHeaders ignores multi-value If-None-Match header (RFC 7232 §3.2)#1395
Conversation
RFC 7232 §3.2 defines If-None-Match as a comma-separated list of entity-tags. The previous strict equality check (`ifNonMatch === opts.etag`) only matched when the header contained exactly one ETag — any client sending a list (e.g. `If-None-Match: "v1", "v2"`) would never receive a 304 Not Modified response even when its cached ETag was present. Fix: split the header on commas, trim whitespace from each token, and match if any token equals opts.etag.
📝 WalkthroughWalkthroughThe PR updates cache header handling to comply with RFC 7232 by parsing ChangesETag RFC 7232 Compliance
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/utils/cache.ts (1)
36-38: 💤 Low valueEdge case: ETags containing commas won't parse correctly.
The simple
split(",")approach doesn't respect quote boundaries. Per RFC 7232 §2.3, entity-tags can contain commas within the quoted opaque-tag (e.g.,"a,b"), so anIf-None-Match: "a,b"header would incorrectly split into["\"a", "b\""]and fail to match an etag of"a,b".However, commas within ETags are extremely rare in practice, and this change is a significant improvement over the current strict-equality check. A fully compliant parser would need to respect the entity-tag structure rather than split on all commas.
Additionally, RFC 7232 §3.2 specifies that
If-None-Match: *should match any existing representation, but this case isn't handled (neither before nor after this change).💡 Optional: Proper list parsing (if comma-in-ETag support is needed)
A more robust implementation would parse respecting quotes:
- if (ifNonMatch && ifNonMatch.split(",").some((token) => token.trim() === opts.etag)) { + if (ifNonMatch) { + // Handle "*" wildcard + if (ifNonMatch.trim() === "*") { + cacheMatched = true; + } else { + // Parse list respecting entity-tag structure (quoted strings) + const tokens = ifNonMatch.match(/(?:W\/)?"[^"]*"/g) || []; + if (tokens.some((token) => token.trim() === opts.etag)) { + cacheMatched = true; + } + } + }This regex-based approach extracts entity-tags (including optional weak prefix and quoted content) without splitting on commas inside quotes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/cache.ts` around lines 36 - 38, The current check using ifNonMatch.split(",").some(...) wrongly splits ETags that contain commas and also doesn't handle the special If-None-Match: * case; update the logic in the function that reads ifNonMatch and compares to opts.etag to (1) treat a lone "*" as an unconditional match (return true) and (2) parse the header into entity-tags without naively splitting on every comma — use a regex or a simple stateful parser that respects optional weak prefixes (W/) and quoted opaque-tags so tokens like "\"a,b\"" remain intact before comparing to opts.etag. Ensure you update the branch that currently uses ifNonMatch.split(",").some((token) => token.trim() === opts.etag) to use the new parser and comparison.test/utils.test.ts (1)
563-578: ⚡ Quick winConsider additional test cases for comma-separated ETags.
The test validates the core fix, but additional cases would improve coverage:
- Etag as the first item in the list:
"v2", "v1"- No spaces after comma:
"v1","v2"- Non-matching list returning 200:
"v1", "v3"with etag"v2"- Single etag (regression):
"v2"should still work📝 Suggested additional test cases
it("returns 304 when etag is first in if-none-match list", async () => { t.app.use((event) => { handleCacheHeaders(event, { etag: '"v1"', }); return "ok"; }); const res = await t.fetch("/", { headers: { "if-none-match": '"v1", "v2"', }, }); expect(res.status).toBe(304); }); it("returns 304 when if-none-match has no spaces after commas", async () => { t.app.use((event) => { handleCacheHeaders(event, { etag: '"v2"', }); return "ok"; }); const res = await t.fetch("/", { headers: { "if-none-match": '"v1","v2"', }, }); expect(res.status).toBe(304); }); it("returns 200 when if-none-match list does not contain etag", async () => { t.app.use((event) => { handleCacheHeaders(event, { etag: '"v3"', }); return "ok"; }); const res = await t.fetch("/", { headers: { "if-none-match": '"v1", "v2"', }, }); expect(res.status).toBe(200); expect(await res.text()).toBe("ok"); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/utils.test.ts` around lines 563 - 578, Add the missing edge-case tests for comma-separated If-None-Match handling by extending the existing spec that calls handleCacheHeaders: add tests verifying (1) etag present as the first item in the list (e.g., '"v2", "v1"'), (2) no spaces after commas (e.g., '"v1","v2"'), (3) non-matching lists return 200 (e.g., header '"v1", "v3"' with etag '"v2"'), and (4) single-etag regression (header '"v2"'); ensure each test mounts handleCacheHeaders in the same pattern and asserts the expected status and body where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/utils/cache.ts`:
- Around line 36-38: The current check using ifNonMatch.split(",").some(...)
wrongly splits ETags that contain commas and also doesn't handle the special
If-None-Match: * case; update the logic in the function that reads ifNonMatch
and compares to opts.etag to (1) treat a lone "*" as an unconditional match
(return true) and (2) parse the header into entity-tags without naively
splitting on every comma — use a regex or a simple stateful parser that respects
optional weak prefixes (W/) and quoted opaque-tags so tokens like "\"a,b\""
remain intact before comparing to opts.etag. Ensure you update the branch that
currently uses ifNonMatch.split(",").some((token) => token.trim() === opts.etag)
to use the new parser and comparison.
In `@test/utils.test.ts`:
- Around line 563-578: Add the missing edge-case tests for comma-separated
If-None-Match handling by extending the existing spec that calls
handleCacheHeaders: add tests verifying (1) etag present as the first item in
the list (e.g., '"v2", "v1"'), (2) no spaces after commas (e.g., '"v1","v2"'),
(3) non-matching lists return 200 (e.g., header '"v1", "v3"' with etag '"v2"'),
and (4) single-etag regression (header '"v2"'); ensure each test mounts
handleCacheHeaders in the same pattern and asserts the expected status and body
where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d268842-2b67-4971-af65-ad9dbebbdd56
📒 Files selected for processing (2)
src/utils/cache.tstest/utils.test.ts
1 participant
Bug
handleCacheHeaderscomparesIf-None-Matchusing strict equality against the server's ETag, but RFC 7232 §3.2 definesIf-None-Matchas a comma-separated list of entity-tags.Current code
A client that sends
If-None-Match: "v1", "v2"(e.g. after caching two responses from the same URL, which browsers may do) will never get a 304 even when the server's ETag is"v2"— the full header string"v1", "v2"!=="v2". The server always returns 200, defeating the conditional-GET caching contract entirely.Fix
Parse the list and check whether any token matches.
Test
Added a case where
If-None-Matchis a comma-separated list containing the server's ETag. Before this fix: 200. After: 304.Full suite: 1134 tests pass, 14 skipped, 0 failures (both
nodeandwebmatrix).Summary by CodeRabbit
Bug Fixes
If-None-Matchheader, improving cache validation accuracy.Tests