fix(cors): merge Vary headers when both origin and allow-headers emit vary#1396
Open
francisjohnjohnston-web wants to merge 2 commits into
Open
fix(cors): merge Vary headers when both origin and allow-headers emit vary#1396francisjohnjohnston-web wants to merge 2 commits into
francisjohnjohnston-web wants to merge 2 commits into
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.
…ribute When a specific origin allowlist is combined with wildcard allowHeaders, two header-builder functions each emit a `vary` key. Plain object spread overwrites the first: `createAllowHeaderHeaders` (vary: access-control-request-headers) silently drops `vary: origin` from `createOriginHeaders`. Result: a CDN caches a preflight response without `Vary: Origin`, then serves it to a different origin — correct CORS headers never reach that origin's browser. Fix: extract both header objects, collect their vary values, and join them before writing. All 38 existing CORS tests continue to pass; added the missing test case (specific origin + wildcard allowHeaders + request headers present). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds RFC 7232 compliance to conditional HTTP caching and fixes vary header composition in CORS preflight responses. ETag matching now treats the If-None-Match header as a comma-separated list of entity-tags instead of requiring exact equality, and CORS preflight headers explicitly merge vary values from multiple header sources. ChangesHTTP Cache and CORS Utility Improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/utils.test.ts (1)
563-579: ⚡ Quick winAdd regression cases for
If-None-Match: *and weak ETagsThis test is solid for comma-separated values; adding wildcard and weak-validator variants would better lock in RFC behavior and prevent future regressions.
🤖 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 - 579, Add two regression test cases alongside the existing "returns 304 when if-none-match is a comma-separated list containing the etag" test: one that sends If-None-Match: * and asserts 304 when handleCacheHeaders is called with a matching etag (use the same t.app.use and t.fetch pattern), and another that uses a weak ETag (e.g., W/"v2") in either the response etag or the If-None-Match header to assert the correct RFC-compliant behavior (weak validators should not match strong ETags; add assertions for both matching weak-to-weak and non-matching weak-to-strong scenarios). Ensure both tests invoke handleCacheHeaders and check res.status like the existing test.
🤖 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.
Inline comments:
In `@src/utils/cache.ts`:
- Around line 36-38: The current If-None-Match check only does a raw equality
against opts.etag and thus misses wildcard and weak validators; update the match
logic around ifNonMatch.split(...).some(...) to first treat a token of '*' as an
immediate match, and otherwise normalize both sides by trimming, removing an
optional weak prefix (leading "W/"), and stripping surrounding quotes before
comparing to opts.etag (also normalized similarly) so weak ETags match per RFC
7232 §3.2.
---
Nitpick comments:
In `@test/utils.test.ts`:
- Around line 563-579: Add two regression test cases alongside the existing
"returns 304 when if-none-match is a comma-separated list containing the etag"
test: one that sends If-None-Match: * and asserts 304 when handleCacheHeaders is
called with a matching etag (use the same t.app.use and t.fetch pattern), and
another that uses a weak ETag (e.g., W/"v2") in either the response etag or the
If-None-Match header to assert the correct RFC-compliant behavior (weak
validators should not match strong ETags; add assertions for both matching
weak-to-weak and non-matching weak-to-strong scenarios). Ensure both tests
invoke handleCacheHeaders and check res.status like the existing test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d26e58a9-7f56-4172-8964-1b89bde93cff
📒 Files selected for processing (4)
src/utils/cache.tssrc/utils/cors.tstest/unit/cors.test.tstest/utils.test.ts
Comment on lines
+36
to
+38
| // RFC 7232 §3.2: If-None-Match is a comma-separated list of entity-tags. | ||
| // A match occurs when any token in the list equals the ETag. | ||
| if (ifNonMatch && ifNonMatch.split(",").some((token) => token.trim() === opts.etag)) { |
There was a problem hiding this comment.
Handle wildcard and weak ETag validators in cache matching
Line 38 only does exact token equality, so valid If-None-Match forms like * and weak validators can miss and incorrectly return 200 instead of 304.
💡 Suggested fix
if (opts.etag) {
event.res.headers.set("etag", opts.etag);
const ifNonMatch = event.req.headers.get("if-none-match");
// RFC 7232 §3.2: If-None-Match is a comma-separated list of entity-tags.
// A match occurs when any token in the list equals the ETag.
- if (ifNonMatch && ifNonMatch.split(",").some((token) => token.trim() === opts.etag)) {
+ if (ifNonMatch) {
+ const normalize = (tag: string) => tag.trim().replace(/^W\//i, "");
+ if (
+ ifNonMatch.split(",").some((token) => {
+ const value = token.trim();
+ return value === "*" || normalize(value) === normalize(opts.etag);
+ })
+ ) {
+ cacheMatched = true;
+ }
+ }
- cacheMatched = true;
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // RFC 7232 §3.2: If-None-Match is a comma-separated list of entity-tags. | |
| // A match occurs when any token in the list equals the ETag. | |
| if (ifNonMatch && ifNonMatch.split(",").some((token) => token.trim() === opts.etag)) { | |
| if (opts.etag) { | |
| event.res.headers.set("etag", opts.etag); | |
| const ifNonMatch = event.req.headers.get("if-none-match"); | |
| // RFC 7232 §3.2: If-None-Match is a comma-separated list of entity-tags. | |
| // A match occurs when any token in the list equals the ETag. | |
| if (ifNonMatch) { | |
| const normalize = (tag: string) => tag.trim().replace(/^W\//i, ""); | |
| if ( | |
| ifNonMatch.split(",").some((token) => { | |
| const value = token.trim(); | |
| return value === "*" || normalize(value) === normalize(opts.etag); | |
| }) | |
| ) { | |
| cacheMatched = true; | |
| } | |
| } | |
| } |
🤖 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 If-None-Match check
only does a raw equality against opts.etag and thus misses wildcard and weak
validators; update the match logic around ifNonMatch.split(...).some(...) to
first treat a token of '*' as an immediate match, and otherwise normalize both
sides by trimming, removing an optional weak prefix (leading "W/"), and
stripping surrounding quotes before comparing to opts.etag (also normalized
similarly) so weak ETags match per RFC 7232 §3.2.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
appendCorsPreflightHeadersnow correctly emitsVary: origin, access-control-request-headerswhen bothcreateOriginHeadersandcreateAllowHeaderHeadersindependently contribute avarykey.Why
The headers object is built by spreading several creators:
Plain object spread means the second
varykey silently overwrites the first. When a server uses a specific origin allowlist (non-*) and hasaccess-control-request-headersin the preflight request, the response Vary header isaccess-control-request-headersalone —originis lost.Without
Vary: origin, a CDN may cache the preflight response for origin A and serve it to origin B. Origin B's browser never seesAccess-Control-Allow-Origin: Band blocks the actual request — a silent CORS failure that's hard to diagnose because it only appears with caching middleware in front.Fix
Extract the two objects that may produce
vary, collect their values, and join them before writing:Testing
Added the missing scenario to
test/unit/cors.test.ts: specific origin allowlist + wildcardallowHeaders+access-control-request-headerspresent. Test fails before the fix (varyis only"access-control-request-headers"), passes after.All 48 test files / 1152 tests remain green;
tsc --noEmitclean.Summary by CodeRabbit
Bug Fixes
varyheader values when multiple sources contribute.Tests