fix: prevent CORS header duplication on middleware early-return responses#1347
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughGlobal response headers are now applied only when missing, preserving route-specific headers. A new Robyn CORS test app and integration tests (including OPTIONS preflight, allowed/disallowed origins, and header-duplication checks) were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
sansyrox
force-pushed
the
fix/cors-preflight-regression
branch
from
bad71e6 to
786fa05
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.rs (1)
143-145:⚠️ Potential issue | 🟠 MajorFix header merge semantics in const-route bake path to match dynamic path.
Line 578 correctly uses
set_missing()to preserve route-set headers over global defaults. However,ConstRouter::bake_global_headers()at line 187 insrc/routers/const_router.rsusesextend(), which will append global headers unconditionally. This creates inconsistent behavior: const-route responses may have duplicate headers (e.g., duplicateAccess-Control-Allow-Origin) while dynamic routes will not.Change line 187 from
combined.extend(extra_headers.iter().cloned());to useset_missing()semantics instead, ensuring both paths prefer route-set headers over global defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.rs` around lines 143 - 145, ConstRouter::bake_global_headers currently appends global headers unconditionally using combined.extend(extra_headers.iter().cloned()), which causes duplicate headers and differs from the dynamic path that uses set_missing semantics; update bake_global_headers to iterate extra_headers and only insert headers that are not already present (i.e., use the same set_missing logic as the dynamic route path) so route-set headers are preserved and global defaults are only applied when missing.
🧹 Nitpick comments (1)
integration_tests/test_cors_preflight.py (1)
146-156: Assert the POST CORS headers too.This currently passes even if POST responses lose
Access-Control-Allow-Origin, so it doesn't actually cover the non-preflight POST branch this PR is meant to protect.Possible fix
assert resp.status_code == 200 + assert resp.headers["Access-Control-Allow-Origin"] == ALLOWED_ORIGIN + assert "POST" in resp.headers["Access-Control-Allow-Methods"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration_tests/test_cors_preflight.py` around lines 146 - 156, The test_post_with_allowed_origin is only asserting a 200 status and misses verifying CORS response headers; update the test to also assert that resp.headers contains "Access-Control-Allow-Origin" equal to ALLOWED_ORIGIN (and any other expected CORS headers your server returns, e.g., "Access-Control-Allow-Credentials" if applicable) so the non-preflight POST branch is actually covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration_tests/test_cors_preflight.py`:
- Around line 74-81: The HTTP calls in integration_tests/test_cors_preflight.py
currently use requests without a timeout (e.g., the requests.options(...) call)
which can hang; update every requests call in this file (including the
requests.options call shown and the other requests.* calls later) to include a
small timeout parameter such as timeout=5 (pass as a named argument, e.g.,
requests.options(..., timeout=5)) so tests fail fast instead of hanging.
- Around line 100-107: The test currently skips the duplicate-header check when
raw_headers lacks getall; instead assert that raw_headers exposes getall (or
that resp.raw has headers supporting getall) before attempting to read duplicate
Access-Control-Allow-Origin values so the test fails rather than silently
passing; update the logic around raw_headers / origin_values (variables
resp.raw, raw_headers, origin_values and method getall) to assert
hasattr(raw_headers, "getall") (or assert origin_values is not None) prior to
the len(...) check and then proceed to assert exactly one occurrence and that
resp.headers["Access-Control-Allow-Origin"] == ALLOWED_ORIGIN.
- Around line 26-52: The readiness loop in _start_cors_server currently only
checks for a listening socket and can be fooled by an unrelated process; add a
liveness check by calling process.poll() inside the while loop and if it returns
a non-None value (process exited) kill the process group (os.killpg) and raise a
ConnectionError (including any available context), so the function fails fast
when the spawned cors_app.py dies before becoming ready. Ensure the poll check
runs before attempting socket.connect and that the cleanup uses the same process
variable and exit handling as the existing timeout branch.
---
Outside diff comments:
In `@src/server.rs`:
- Around line 143-145: ConstRouter::bake_global_headers currently appends global
headers unconditionally using combined.extend(extra_headers.iter().cloned()),
which causes duplicate headers and differs from the dynamic path that uses
set_missing semantics; update bake_global_headers to iterate extra_headers and
only insert headers that are not already present (i.e., use the same set_missing
logic as the dynamic route path) so route-set headers are preserved and global
defaults are only applied when missing.
---
Nitpick comments:
In `@integration_tests/test_cors_preflight.py`:
- Around line 146-156: The test_post_with_allowed_origin is only asserting a 200
status and misses verifying CORS response headers; update the test to also
assert that resp.headers contains "Access-Control-Allow-Origin" equal to
ALLOWED_ORIGIN (and any other expected CORS headers your server returns, e.g.,
"Access-Control-Allow-Credentials" if applicable) so the non-preflight POST
branch is actually covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 154ba304-2999-4bff-ab0e-cbb4fb7ec756
📒 Files selected for processing (4)
integration_tests/cors_app.pyintegration_tests/test_cors_preflight.pysrc/server.rssrc/types/headers.rs
…nses The 0.82.0 performance commit changed the response finalization path so that global response headers are unconditionally `extend`ed onto every response, including early-return responses from before-middleware. When ALLOW_CORS is used, this causes Access-Control-Allow-Origin (and other CORS headers) to appear twice — once from the middleware and once from the global defaults. Per the Fetch spec, browsers reject responses with duplicate Access-Control-Allow-Origin values, breaking preflight. Fix: replace `extend` (append) with `set_missing` (set-if-absent) when merging global response headers. This gives middleware-set headers precedence over global defaults, preventing duplication. Closes #1346 Made-with: Cursor
sansyrox
force-pushed
the
fix/cors-preflight-regression
branch
from
0854ade to
7988838
Compare
for more information, see https://pre-commit.ci
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.
Summary
ALLOW_CORSbreaks browser preflight (OPTIONS) requests in 0.82.0extendglobal headers onto every response, including early-return responses from before-middleware. WhenALLOW_CORSis used, the CORS middleware setsAccess-Control-Allow-Originon the preflight response, and then the global header merge duplicates it. Per the Fetch spec, browsers reject responses with multipleAccess-Control-Allow-Originvalues.Headers::set_missing()— a merge method that only inserts headers for keys not already present on the response — and uses it in place ofextendwhen applying global response headers. This gives middleware-set headers precedence over global defaults.Changes
src/types/headers.rsset_missing()methodsrc/server.rsextend()withset_missing()at the global header merge pointintegration_tests/cors_app.pyALLOW_CORSenabledintegration_tests/test_cors_preflight.pyTest plan
requestsAccess-Control-Allow-Originheader on preflightCloses #1346
Made with Cursor
Summary by CodeRabbit
Bug Fixes
Tests