✨(api) add support for CORS

[piptouque]

Purpose

Closes #628 .

Proposal

• Add CORSMiddleware to the FastAPI's server with appropriate config.
• Add a runserver setting for a list of allowed origins
• Add some tests for the above setting's validation
• Add preflight requests tests I don't think it is possible with pytest as the CORS settings are loaded when the server starts and can't be changed afterwards.

[MYilFun00]

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_ORIGINS is non-empty.
• Consider adding DELETE to allow_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.

[piptouque]

Thanks for the review @MYilFun00 , see the above comment for my changes.

[MYilFun00]

Thanks @piptouque. I re-tested the branch locally at 97f7c3c.

All four blocking points from my previous review are resolved:

• Point 1 — missing comma in ALLOW_ORIGINS_INVALID — fixed, https://trailing-slash.com/
  is now a separate entry and genuinely tested ✅
• Point 2 — allow_headers — now a proper list of individual header names ✅
• Point 3 — multiline f-string — gone; src/ralph/conf.py, src/ralph/api/__init__.py
  and tests/api/test_cors.py all parse cleanly under Python 3.10 ✅
• Point 4 — path validation — fixed, and your solution is better than the one I
  suggested; see below ✅

You also picked up two of the non-blocking suggestions: the middleware is now
registered only when RUNSERVER_CORS_ALLOW_ORIGINS is non-empty, and DELETE
was added to allow_methods. Thanks.

---

On point 4 — preserve_empty_path=True is the better fix

I had suggested if url.path not in ("", "/"). You went with
UrlConstraints(preserve_empty_path=True) and kept if url.path is not None,
which is more precise. Verified locally:

origin	url.path	result
https://my-allowed-origin.com	None	accepted
http://my-local-origin:8080	None	accepted
https://trailing-slash.com/	'/'	rejected

My version would have collapsed "no path at all" and "explicit root path /"
into the same case. Yours keeps them distinct, which is the correct semantics
here since the Origin header never carries a trailing slash. Good call.

I also ran the validator against a wider set of inputs than the test file covers,
and the behaviour is sound throughout:

ACCEPTE 'https://foo.com:443'     -> 'https://foo.com:443'      (explicit default port)
ACCEPTE 'http://foo.com:80'       -> 'http://foo.com:80'        (explicit default port)
ACCEPTE 'http://[::1]:8080'       -> 'http://[::1]:8080'        (IPv6)
ACCEPTE 'http://localhost:8080'   -> 'http://localhost:8080'
ACCEPTE 'https://sub.foo.com'     -> 'https://sub.foo.com'
REJETE  'https://FOO.com'            (uppercase host, clear message)
REJETE  'https://foo.com?a=1'        (query string)
REJETE  'null'                       (sandboxed-iframe origin)
REJETE  '*'                          (wildcard)

Rejecting null and * is the right default for a allow_credentials=True
setup — worth keeping as is.

---

🔴 One blocking item left: ci/circleci: lint

A single line, src/ralph/conf.py:194:

src/ralph/conf.py:194:89: E501 Line too long (105 > 88)
        raise ValueError(f"CORS AllowOrigin URL format incorrect. Expected: {origin}, got: {str(value)}")

black --check also flags the same file, for the single quotes on the line above:

-    port_suffix = ':' + str(explicit_port) if explicit_port is not None else ''
+    port_suffix = ":" + str(explicit_port) if explicit_port is not None else ""

One caveat worth flagging: running black alone will not make the lint job
pass. Black wraps the raise but the string still ends up at 91 chars, so
ruff check keeps failing on E501. Dropping the redundant str() (an f-string
already calls it) brings it under the limit:

    port_suffix = ":" + str(explicit_port) if explicit_port is not None else ""
    origin = f"{url.scheme}://{url.host}{port_suffix}"
    if origin != str(value):
        raise ValueError(
            f"CORS AllowOrigin URL format incorrect. Expected: {origin}, got: {value}"
        )

With that, locally: black --check → 312 files unchanged, ruff check . →
all checks passed, pytest tests/api/test_cors.py → 2 passed.

---

🟢 The other red checks are not your code — the branch just needs a rebase

check-changelog and test-python 3.9→3.12 are failing for a reason unrelated
to this PR. The branch is still based on 5e1558d, i.e. before #634 landed on
main. That PR fixed exactly these jobs:

4a5e1b0 💚(ci) fix check-changelog, mongo tests, tray and test-helm (#634)
 .circleci/config.yml                    | 24 +++++++++++++++++-------
 tests/backends/data/test_async_mongo.py | 22 +++++++++++-----------
 tests/backends/data/test_mongo.py       | 25 +++++++++++++++++--------

Your diff touches only .env.dist, CHANGELOG.md, src/ralph/api/__init__.py,
src/ralph/conf.py and tests/api/test_cors.py — nothing under
tests/backends/. So a rebase on current main should clear those four
test-python jobs and check-changelog on its own.

ci/circleci: package is pending rather than failing: it requires lint and
test, so it will start on its own once those are green. Nothing to do there.

---

🟡 Non-blocking: preflight requests are testable

About the struck-through item in your description — "I don't think it is possible
with pytest as the CORS settings are loaded when the server starts and can't be
changed afterwards" — you can reload the modules after setting the environment
variable. I wrote this locally against your branch and all three tests pass:

"""CORS preflight tests."""
import importlib
import json

import pytest
from fastapi.testclient import TestClient

ORIGIN = "https://my-allowed-origin.com"


def _build_app(monkeypatch, origins: list):
    monkeypatch.setenv("RALPH_RUNSERVER_CORS_ALLOW_ORIGINS", json.dumps(origins))
    import ralph.conf

    importlib.reload(ralph.conf)
    import ralph.api

    importlib.reload(ralph.api)
    return ralph.api.app


@pytest.fixture(autouse=True)
def _restore():
    """Reload modules with the original environment after each test."""
    yield
    import ralph.api
    import ralph.conf

    importlib.reload(ralph.conf)
    importlib.reload(ralph.api)


def test_preflight_allowed_origin(monkeypatch):
    app = _build_app(monkeypatch, [ORIGIN])
    response = TestClient(app).options(
        "/xAPI/statements/",
        headers={
            "Origin": ORIGIN,
            "Access-Control-Request-Method": "POST",
            "Access-Control-Request-Headers": "Authorization,X-Experience-API-Version",
        },
    )
    assert response.status_code == 200
    assert response.headers["access-control-allow-origin"] == ORIGIN
    assert "authorization" in response.headers["access-control-allow-headers"].lower()


def test_preflight_disallowed_origin(monkeypatch):
    app = _build_app(monkeypatch, [ORIGIN])
    response = TestClient(app).options(
        "/xAPI/statements/",
        headers={"Origin": "https://evil.com", "Access-Control-Request-Method": "POST"},
    )
    assert "access-control-allow-origin" not in response.headers


def test_no_middleware_when_setting_empty(monkeypatch):
    app = _build_app(monkeypatch, [])
    response = TestClient(app).options(
        "/xAPI/statements/",
        headers={"Origin": ORIGIN, "Access-Control-Request-Method": "POST"},
    )
    assert "access-control-allow-origin" not in response.headers

3 passed

The autouse fixture matters: without it the reloaded ralph.api leaks into
later tests in the same session. This is entirely optional for this PR, but since
the whole point of #628 is preflight support, having one test that actually
exercises the middleware — rather than only the setting validation — would be
worth it. Happy for it to land as a follow-up if you prefer.

Once the E501 is fixed and the branch is rebased, I'll re-review.