Skip to content

fix(cors): use explicit header allowlist fallback instead of wildcard#4965

Open
abcxff wants to merge 1 commit intoabc-2.2.1-patchfrom
05-04-fix_cors_use_explicit_header_allowlist_fallback_instead_of_wildcard
Open

fix(cors): use explicit header allowlist fallback instead of wildcard#4965
abcxff wants to merge 1 commit intoabc-2.2.1-patchfrom
05-04-fix_cors_use_explicit_header_allowlist_fallback_instead_of_wildcard

Conversation

@abcxff
Copy link
Copy Markdown
Contributor

@abcxff abcxff commented May 4, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@railway-app
Copy link
Copy Markdown

railway-app Bot commented May 4, 2026 •
edited
Loading

🚅 Deployed to the rivet-pr-4965 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud 😴 Sleeping (View Logs) Web May 4, 2026 at 10:30 pm
frontend-inspector 😴 Sleeping (View Logs) Web May 4, 2026 at 10:27 pm
website 😴 Sleeping (View Logs) Web May 4, 2026 at 10:23 pm
mcp-hub ✅ Success (View Logs) Web May 4, 2026 at 10:15 pm
ladle ❌ Build Failed (View Logs) Web May 4, 2026 at 10:14 pm
kitchen-sink ❌ Build Failed (View Logs) Web May 4, 2026 at 10:14 pm

@abcxff Graphite App
Copy link
Copy Markdown
Contributor Author

abcxff commented May 4, 2026 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@abcxff abcxff mentioned this pull request May 4, 2026
Open
@abcxff abcxff marked this pull request as ready for review May 4, 2026 22:15
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 4, 2026

Code Review: fix(cors): use explicit header allowlist fallback instead of wildcard

Summary

This PR makes two targeted changes:

  1. actor-router-consts.ts: Adds "Authorization" to the ALLOWED_PUBLIC_HEADERS allowlist.
  2. cors.ts: Replaces the "*" wildcard fallback in access-control-allow-headers with the explicit DEFAULT_ALLOWED_HEADERS list derived from ALLOWED_PUBLIC_HEADERS.

Both changes are correct and address a real issue.

Security: Wildcard Fallback Fix (Positive)

The original code used "*" as the fallback value for access-control-allow-headers when no Access-Control-Request-Headers was present in a preflight request:

// Before
const requestHeaders =
    c.req.header("access-control-request-headers") || "*";

Returning access-control-allow-headers: * when access-control-allow-credentials: true is also set is a CORS spec violation. Per the Fetch spec (https://fetch.spec.whatwg.org/#cors-preflight-fetch), a wildcard in Access-Control-Allow-Headers is not treated as a wildcard when credentials mode is include — browsers will reject such a preflight. The fix correctly supplies a concrete fallback header list, which is both spec-compliant and safer.

Authorization Header Addition (Positive)

Adding "Authorization" to ALLOWED_PUBLIC_HEADERS is necessary. The Authorization header is already in active use throughout the codebase for:

  • Inspector endpoints (src/inspector/utils.ts, src/actor/router.ts) — Bearer token auth.
  • Remote manager driver (src/remote-manager-driver/api-utils.ts) — Bearer token for engine API calls.
  • Driver test suite (src/driver-test-suite/tests/actor-inspector.ts) — All inspector HTTP tests send Authorization: Bearer ....

Without this header in the allowlist, browser preflight requests for any cross-origin call with an Authorization header would be rejected. This was a silent breakage for any cross-origin actor or inspector client.

Minor Observations

?? vs || operator change — The switch from || to ?? is strictly better here. Since c.req.header(...) returns string | undefined, using ?? (nullish coalescing) only falls back on undefined/null, whereas || would also fall back on an empty string "". An empty string is a valid (if unusual) header value from a client, so ?? is the more correct operator.

DEFAULT_ALLOWED_HEADERS module-level constant — Pre-computing ALLOWED_PUBLIC_HEADERS.join(", ") once at module load time rather than on every preflight request is a small but good efficiency choice.

access-control-expose-headers: * still present — Both the preflight and actual-request paths set access-control-expose-headers: *. With access-control-allow-credentials: true, this wildcard also does not work as expected in credentialed requests (same spec restriction). This is a pre-existing issue not introduced by this PR, but worth a follow-up to replace with an explicit expose list (mirroring what is done here for allow-headers).

No test coverage — The PR description has unchecked test boxes and does not describe how this was verified. Given the security-sensitive nature of CORS handling, a unit or integration test covering the preflight fallback path (no Access-Control-Request-Headers present) and the Authorization header echo path would strengthen confidence. There is an existing actor-inspector.ts driver test suite that could be extended.

Verdict

The changes are correct, minimal, and fix a real CORS compliance/security gap. Approving with the recommendation to:

  1. Follow up on access-control-expose-headers: * + credentials inconsistency.
  2. Add a CORS preflight test covering the no-Access-Control-Request-Headers fallback case.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 4, 2026 •
edited
Loading

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@4965

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@4965

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@4965

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@4965

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@4965

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@4965

@rivetkit/sqlite-vfs

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sqlite-vfs@4965

@rivetkit/traces

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/traces@4965

@rivetkit/workflow-engine

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/workflow-engine@4965

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@4965

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@4965

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@4965

commit: a556a39

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@abcxff