Skip to content

Commit 55cff1c

Browse files
authored
fix: prevent CORS header duplication on middleware early-return responses (#1347)
* fix: prevent CORS header duplication on middleware early-return responses 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
1 parent a54ff96 commit 55cff1c

4 files changed

Lines changed: 225 additions & 1 deletion

File tree

integration_tests/cors_app.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
"""
2+
Standalone Robyn app with ALLOW_CORS enabled, used by CORS integration tests.
3+
Runs on a separate port (8083) so it doesn't conflict with base_routes.py.
4+
"""
5+
6+
import os
7+
8+
from robyn import ALLOW_CORS, Robyn
9+
10+
app = Robyn(__file__)
11+
12+
ALLOWED_ORIGINS = ["http://localhost:3000", "https://frontend.example.com"]
13+
ALLOW_CORS(app, origins=ALLOWED_ORIGINS)
14+
15+
16+
@app.get("/")
17+
def index():
18+
return "OK"
19+
20+
21+
@app.post("/data")
22+
def post_data(request):
23+
return "created"
24+
25+
26+
@app.get("/custom-header")
27+
def custom_header(request):
28+
from robyn import Response
29+
30+
return Response(
31+
status_code=200,
32+
headers={"x-custom": "hello"},
33+
description="custom",
34+
)
35+
36+
37+
if __name__ == "__main__":
38+
port = int(os.getenv("ROBYN_PORT", "8083"))
39+
app.start(port=port, _check_port=False)
Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
"""
2+
Integration tests for CORS preflight (OPTIONS) handling.
3+
4+
Regression test for https://github.com/sparckles/robyn/issues/1346
5+
These tests spin up a real Robyn server with ALLOW_CORS enabled and make
6+
actual HTTP requests — no TestClient.
7+
"""
8+
9+
import os
10+
import pathlib
11+
import signal
12+
import socket
13+
import subprocess
14+
import time
15+
16+
import pytest
17+
import requests
18+
19+
CORS_PORT = 8083
20+
CORS_HOST = "127.0.0.1"
21+
CORS_BASE_URL = f"http://{CORS_HOST}:{CORS_PORT}"
22+
REQUEST_TIMEOUT = 5
23+
24+
ALLOWED_ORIGIN = "http://localhost:3000"
25+
DISALLOWED_ORIGIN = "http://evil.example.com"
26+
27+
28+
def _start_cors_server():
29+
app_path = os.path.join(pathlib.Path(__file__).parent.resolve(), "cors_app.py")
30+
env = os.environ.copy()
31+
env["ROBYN_HOST"] = CORS_HOST
32+
env["ROBYN_PORT"] = str(CORS_PORT)
33+
34+
process = subprocess.Popen(
35+
["python3", app_path],
36+
env=env,
37+
preexec_fn=os.setsid,
38+
)
39+
40+
timeout = 15
41+
start = time.time()
42+
while True:
43+
if process.poll() is not None:
44+
raise RuntimeError(f"CORS server exited early with code {process.returncode}")
45+
if time.time() - start > timeout:
46+
os.killpg(os.getpgid(process.pid), signal.SIGKILL)
47+
raise ConnectionError(f"CORS server didn't start on {CORS_HOST}:{CORS_PORT}")
48+
try:
49+
sock = socket.create_connection((CORS_HOST, CORS_PORT), timeout=2)
50+
sock.close()
51+
break
52+
except Exception:
53+
time.sleep(0.5)
54+
55+
time.sleep(1)
56+
return process
57+
58+
59+
@pytest.fixture(scope="module")
60+
def cors_server():
61+
process = _start_cors_server()
62+
yield
63+
try:
64+
os.killpg(os.getpgid(process.pid), signal.SIGKILL)
65+
except ProcessLookupError:
66+
pass
67+
68+
69+
# ---------------------------------------------------------------------------
70+
# Preflight (OPTIONS) tests
71+
# ---------------------------------------------------------------------------
72+
73+
74+
def test_options_preflight_returns_204_with_cors_headers(cors_server):
75+
"""Browser sends OPTIONS preflight; server must return 204 with all CORS headers."""
76+
resp = requests.options(
77+
f"{CORS_BASE_URL}/data",
78+
headers={
79+
"Origin": ALLOWED_ORIGIN,
80+
"Access-Control-Request-Method": "POST",
81+
"Access-Control-Request-Headers": "Content-Type, Authorization",
82+
},
83+
timeout=REQUEST_TIMEOUT,
84+
)
85+
assert resp.status_code == 204
86+
assert resp.headers["Access-Control-Allow-Origin"] == ALLOWED_ORIGIN
87+
assert "POST" in resp.headers["Access-Control-Allow-Methods"]
88+
assert resp.headers["Access-Control-Allow-Credentials"] == "true"
89+
90+
91+
def test_options_preflight_no_duplicate_allow_origin(cors_server):
92+
"""
93+
Regression: ensure Access-Control-Allow-Origin appears exactly once.
94+
Duplicate values cause browsers to reject the preflight (Fetch spec).
95+
"""
96+
resp = requests.options(
97+
f"{CORS_BASE_URL}/",
98+
headers={
99+
"Origin": ALLOWED_ORIGIN,
100+
"Access-Control-Request-Method": "GET",
101+
},
102+
timeout=REQUEST_TIMEOUT,
103+
)
104+
raw_headers = resp.raw.headers if hasattr(resp.raw, "headers") else resp.headers
105+
106+
origin_values = raw_headers.getall("Access-Control-Allow-Origin", None) if hasattr(raw_headers, "getall") else None
107+
108+
if origin_values is not None:
109+
assert len(origin_values) == 1, f"Access-Control-Allow-Origin appeared {len(origin_values)} times: {origin_values}"
110+
111+
assert resp.headers["Access-Control-Allow-Origin"] == ALLOWED_ORIGIN
112+
113+
114+
def test_options_preflight_disallowed_origin_returns_403(cors_server):
115+
"""Origins not in the allowed list should be rejected."""
116+
resp = requests.options(
117+
f"{CORS_BASE_URL}/data",
118+
headers={
119+
"Origin": DISALLOWED_ORIGIN,
120+
"Access-Control-Request-Method": "POST",
121+
},
122+
timeout=REQUEST_TIMEOUT,
123+
)
124+
assert resp.status_code == 403
125+
126+
127+
# ---------------------------------------------------------------------------
128+
# Normal request tests (non-preflight)
129+
# ---------------------------------------------------------------------------
130+
131+
132+
def test_get_with_allowed_origin_has_cors_headers(cors_server):
133+
"""Normal GET from an allowed origin should carry CORS response headers."""
134+
resp = requests.get(
135+
f"{CORS_BASE_URL}/",
136+
headers={"Origin": ALLOWED_ORIGIN},
137+
timeout=REQUEST_TIMEOUT,
138+
)
139+
assert resp.status_code == 200
140+
allow_origin = resp.headers.get("Access-Control-Allow-Origin")
141+
assert allow_origin is not None
142+
assert "Access-Control-Allow-Methods" in resp.headers
143+
144+
145+
def test_get_without_origin_still_has_global_cors_headers(cors_server):
146+
"""Requests without Origin (e.g. curl, Postman) should still get global CORS headers."""
147+
resp = requests.get(f"{CORS_BASE_URL}/", timeout=REQUEST_TIMEOUT)
148+
assert resp.status_code == 200
149+
assert "Access-Control-Allow-Methods" in resp.headers
150+
151+
152+
def test_post_with_allowed_origin(cors_server):
153+
"""POST from allowed origin should succeed with CORS headers."""
154+
resp = requests.post(
155+
f"{CORS_BASE_URL}/data",
156+
headers={
157+
"Origin": ALLOWED_ORIGIN,
158+
"Content-Type": "application/json",
159+
},
160+
data="{}",
161+
timeout=REQUEST_TIMEOUT,
162+
)
163+
assert resp.status_code == 200
164+
165+
166+
def test_custom_response_headers_not_clobbered_by_globals(cors_server):
167+
"""Route-level headers set by the handler should not be overwritten by globals."""
168+
resp = requests.get(
169+
f"{CORS_BASE_URL}/custom-header",
170+
headers={"Origin": ALLOWED_ORIGIN},
171+
timeout=REQUEST_TIMEOUT,
172+
)
173+
assert resp.status_code == 200
174+
assert resp.headers.get("x-custom") == "hello"
175+
assert "Access-Control-Allow-Methods" in resp.headers

src/server.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -575,7 +575,7 @@ async fn index(
575575
.is_some_and(|paths| paths.contains(&request.url.path));
576576

577577
if !is_excluded {
578-
response.headers_mut().extend(&global_response_headers);
578+
response.headers_mut().set_missing(&global_response_headers);
579579
}
580580

581581
// After middleware

src/types/headers.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,16 @@ impl Headers {
177177
}
178178
}
179179

180+
/// Merge headers from `headers` into `self`, but only for keys not already present.
181+
/// This gives middleware-set headers precedence over global defaults,
182+
/// preventing duplicate `Access-Control-Allow-Origin` (and similar) violations.
183+
pub fn set_missing(&mut self, headers: &Headers) {
184+
for iter in headers.headers.iter() {
185+
let (key, values) = iter.pair();
186+
self.headers.entry(key.clone()).or_insert_with(|| values.clone());
187+
}
188+
}
189+
180190
pub fn from_actix_headers(req_headers: &HeaderMap) -> Self {
181191
let headers = Headers::default();
182192

0 commit comments

Comments
 (0)