fix(cors): correct vary handling and credentialed wildcard behavior by pi0x · Pull Request #1456 · h3js/h3

pi0x · 2026-07-11T13:15:13Z

Summary

Fixes several spec-level correctness issues in the CORS utils found during a review of src/utils/cors.ts / src/utils/internal/cors.ts.

`vary: origin` was missing when the origin is rejected (cache poisoning)

When origin is an allowlist/function, an allowed origin got access-control-allow-origin + vary: origin, but a rejected origin got neither. The response varies by request origin without declaring it, so a shared cache could store the ACAO-less response from a disallowed origin and serve it to an allowed one (or cache the reflected header and serve it to everyone). createOriginHeaders now always emits vary: origin when the origin option is not "*".

Wildcards are treated literally on credentialed requests

Per the Fetch spec, * in access-control-allow-methods / access-control-expose-headers is a literal token when credentials are included:

methods: "*" + credentials: true now reflects access-control-request-method back (with vary: access-control-request-method) instead of sending a literal * that browsers would reject — same technique already used for allowHeaders.
exposeHeaders: "*" + credentials: true now omits the useless header and logs a warning suggesting an explicit list.

`vary: access-control-request-headers` was missing when the header is absent

With wildcard allowHeaders, the preflight response reflects access-control-request-headers, but no vary was declared when the request omitted that header — same undeclared-variance class as the origin issue.

`origin: "null"` was inconsistent with `isCorsOriginAllowed`

createOriginHeaders unconditionally emitted access-control-allow-origin: null for any request origin, while isCorsOriginAllowed only matches a literal "null" origin. Both now agree: null is only reflected when the request origin is actually null.

The vary merge in appendCorsPreflightHeaders was generalized since three header groups can now emit vary independently.

Testing

Updated unit tests for the new vary behavior and added regression tests for each fix (test/unit/cors.test.ts, 44 tests)
Full suite passes: 1345 passed, no type errors

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved credentialed CORS preflight handling for methods: "*" by reflecting the requested method when available.
- Suppressed access-control-expose-headers for exposeHeaders: "*" when credentials are enabled.
- Corrected Vary header behavior for allowed and rejected origins, and for preflight responses (including combined variation across relevant dimensions).
- Deduplicated and clarified warnings for ineffective credentialed wildcard configurations.
Tests
- Expanded and reorganized CORS unit coverage for wildcard + credentials scenarios, origin edge cases, and updated expected Vary/header outputs.

- Emit `vary: origin` even when the request origin is rejected, so shared caches never serve a response cached for one origin to another - Always emit `vary: access-control-request-headers` when reflecting request headers with wildcard `allowHeaders` - Reflect the requested method (with `vary: access-control-request-method`) when `methods: "*"` is combined with `credentials: true`, since browsers treat a literal `*` as a method name on credentialed requests - Omit `access-control-expose-headers: *` and warn when combined with `credentials: true` (browsers treat it literally) - Handle `origin: "null"` consistently with `isCorsOriginAllowed` (only reflect `null` when the request origin is actually `null`) Co-Authored-By: Claude Fable 5 <[email protected]>

coderabbitai · 2026-07-11T13:15:34Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a9960abb-99c6-48e5-94db-d8474ee19511

📥 Commits

Reviewing files that changed from the base of the PR and between 2b5a7ce and 1482e17.

📒 Files selected for processing (2)

src/utils/internal/cors.ts
test/unit/cors.test.ts

🚧 Files skipped from review as they are similar to previous changes (2)

src/utils/internal/cors.ts
test/unit/cors.test.ts

📝 Walkthrough

Walkthrough

Changes

CORS behavior updates

Layer / File(s)	Summary
Credentialed header semantics `src/utils/cors.ts`, `src/utils/internal/cors.ts`, `test/unit/cors.test.ts`	Credentialed wildcard methods reflect the requested method, wildcard expose headers are omitted, warnings are deduplicated, rejected origins emit `Vary: origin`, and missing request headers emit their corresponding `Vary` value.
Preflight header assembly `src/utils/cors.ts`, `test/unit/cors.test.ts`	Preflight headers are assembled from ordered groups, with `Vary` collected across all groups and event-aware method generation tested.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

h3js/h3#1323: Related CORS warnings for credentialed wildcard configurations.
h3js/h3#1352: Related changes to CORS header appending.
h3js/h3#1396: Related Vary composition changes in preflight handling.

Suggested reviewers: pi0

Poem

A rabbit hops through headers bright,
Reflecting methods just right.
Origins vary, wildcards bow,
Preflight groups merge now.
CORS carrots bloom tonight! 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main CORS fixes around Vary handling and credentialed wildcard behavior.
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/cors

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

test/unit/cors.test.ts (1)

667-670: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the complete merged Vary value.

Default wildcard allowHeaders also contributes access-control-request-headers. The current partial checks would pass if that group were dropped, weakening coverage of the PR’s generalized merge.

Proposed assertion

-        const vary = eventMock.res.headers.get("vary") ?? "";
-        expect(vary).toContain("origin");
-        expect(vary).toContain("access-control-request-method");
+        expect(eventMock.res.headers.get("vary")).toEqual(
+          "origin, access-control-request-method, access-control-request-headers",
+        );

🤖 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/unit/cors.test.ts` around lines 667 - 670, Update the Vary-header
assertions in the CORS test to validate the complete merged value, including
access-control-request-headers alongside origin and
access-control-request-method. Assert the expected combined header value rather
than checking only individual substrings.

🤖 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/cors.ts`:
- Around line 35-37: Update the credentials guidance near the `methods`
documentation in `cors.ts` to clarify that `methods: "*"` is supported by
reflecting the requested method when credentials are enabled, rather than
advising credentialed callers never to use it. Keep the existing wildcard
behavior and surrounding documentation unchanged.

---

Nitpick comments:
In `@test/unit/cors.test.ts`:
- Around line 667-670: Update the Vary-header assertions in the CORS test to
validate the complete merged value, including access-control-request-headers
alongside origin and access-control-request-method. Assert the expected combined
header value rather than checking only individual substrings.

🪄 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: 84fdd227-0bd6-47f8-b0cb-b90968e54a96

📥 Commits

Reviewing files that changed from the base of the PR and between 3813e9b and a8775d2.

📒 Files selected for processing (3)

src/utils/cors.ts
src/utils/internal/cors.ts
test/unit/cors.test.ts

coderabbitai · 2026-07-11T13:19:32Z

+   * When `credentials` is enabled, browsers treat `"*"` as a literal method name — in that
+   * case the requested method is reflected back instead of sending a literal `*`.
+   *


📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Reconcile the wildcard-method documentation.

Lines 35-37 say methods: "*" is supported through reflection, while Line 65 says credentialed callers should not set methods to "*". Update the credentials documentation to distinguish this supported behavior.

🤖 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/cors.ts` around lines 35 - 37, Update the credentials guidance near the `methods` documentation in `cors.ts` to clarify that `methods: "*"` is supported by reflecting the requested method when credentials are enabled, rather than advising credentialed callers never to use it. Keep the existing wildcard behavior and surrounding documentation unchanged.

resolveCorsOptions runs on every request via handleCors, so the credential warnings previously re-logged per request. Route them through a warn-once helper and check the resolved options so a defaulted wildcard exposeHeaders under credentials is diagnosed consistently with origin. Co-Authored-By: Claude Opus 4.8 <[email protected]>

codecov · 2026-07-11T17:26:19Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

pi0x requested a review from pi0 as a code owner July 11, 2026 13:15

coderabbitai Bot reviewed Jul 11, 2026

View reviewed changes

pi0x force-pushed the fix/cors branch from 2b5a7ce to 1482e17 Compare July 11, 2026 17:25

pi0 approved these changes Jul 11, 2026

View reviewed changes

pi0 merged commit 303fd86 into main Jul 11, 2026
10 checks passed

pi0 deleted the fix/cors branch July 11, 2026 17:28

coderabbitai Bot mentioned this pull request Jul 14, 2026

fix(cors): set single-valued CORS headers instead of appending #1466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(cors): correct vary handling and credentialed wildcard behavior#1456

fix(cors): correct vary handling and credentialed wildcard behavior#1456
pi0 merged 2 commits into
mainfrom
fix/cors

pi0x commented Jul 11, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jul 11, 2026

Uh oh!

codecov Bot commented Jul 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

pi0x commented Jul 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

vary: origin was missing when the origin is rejected (cache poisoning)

Wildcards are treated literally on credentialed requests

vary: access-control-request-headers was missing when the header is absent

origin: "null" was inconsistent with isCorsOriginAllowed

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 11, 2026

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pi0x commented Jul 11, 2026 •

edited by coderabbitai Bot

Loading

`vary: origin` was missing when the origin is rejected (cache poisoning)

`vary: access-control-request-headers` was missing when the header is absent

`origin: "null"` was inconsistent with `isCorsOriginAllowed`

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading