✨(api) add support for CORS#629
Conversation
Use FastAPI's CORS middleware, and add configurable allowed origins.
MYilFun00
left a comment
There was a problem hiding this comment.
Request changes
Thanks for the CORS implementation — the overall approach looks good (custom Pydantic validator, RUNSERVER_CORS_ALLOW_ORIGINS setting, and .env.dist documentation).
However, I found a few blocking issues during local review that should be addressed before merging.
🔴 Blocking issues
1. tests/api/test_cors.py (L20) — Missing comma causes silent string concatenation
"http:/another.wrong.format" "https://trailing-slash.com/",Python implicitly concatenates adjacent string literals, so https://trailing-slash.com/ is never tested independently.
Suggested fix:
Add the missing comma between both strings.
2. src/ralph/api/__init__.py — allow_headers should be a list of header names
Current implementation:
allow_headers=["Authorization,User-Agent,..."]allow_headers expects a list where each header is a separate string. Supplying a single comma-separated value is not compliant with the expected format and may cause preflight requests to fail with stricter CORS clients.
Suggested fix:
allow_headers=[
"Authorization",
"User-Agent",
"Keep-Alive",
"Content-Type",
"X-Experience-API-Version",
],3. src/ralph/conf.py (L191) — Multiline f-string is incompatible with Python < 3.12
The current multiline f-string raises a SyntaxError on Python 3.9–3.11.
Since the project supports Python >= 3.9 and CI runs against these versions, this needs to be rewritten.
Suggested fix:
port_suffix = f":{explicit_port}" if explicit_port is not None else ""
origin = f"{url.scheme}://{url.host}{port_suffix}"4. src/ralph/conf.py (L187) — Path validation rejects valid origins
With Pydantic v2, AnyHttpUrl.path is always '/' for valid origins.
The current condition:
if url.path is not None:evaluates to True for every valid origin, causing them all to be rejected once issue #3 is fixed.
Suggested fix:
if url.path not in ("", "/"):🟡 Non-blocking suggestions
These are not blockers but would improve the implementation:
- Only register the CORS middleware when
RUNSERVER_CORS_ALLOW_ORIGINSis non-empty. - Consider adding
DELETEtoallow_methods(xAPI compatibility). - Make the validation error message more explicit when
origin != str(value).
Overall, the implementation is heading in the right direction, but the four blocking issues above should be resolved before merging.
Purpose
Closes #628 .
Proposal
CORSMiddlewareto the FastAPI's server with appropriate config.Add preflight requests testsI don't think it is possible withpytestas the CORS settings are loaded when the server starts and can't be changed afterwards.