diff --git a/NEWS.md b/NEWS.md index e2ee8dc5d..69aeed200 100644 --- a/NEWS.md +++ b/NEWS.md @@ -8,6 +8,8 @@ border-radius: 128px; ## v2.5.5 +- Features + - `csrf_trusted_origins` setting for CSRF 403s behind a reverse proxy. - Fixes - Online tagging matches saved for review by 2.5.3 no longer break the review list and scan status with "No module named 'numpy'". They are diff --git a/README.md b/README.md index d382facab..38abb37cb 100644 --- a/README.md +++ b/README.md @@ -471,6 +471,10 @@ present. # websockets = true # HTTP path prefix for codex (e.g. "/codex" for reverse proxy sub-path) # url_path_prefix = "" +# Public origins accepted for CSRF when a reverse proxy serves Codex under +# another scheme or host than Codex sees (e.g. https in front of http). +# First make sure the proxy sends X-Forwarded-Proto and X-Forwarded-Host. +# csrf_trusted_origins = ["https://comics.example.com"] # [logging] # Log level: TRACE, DEBUG, INFO, SUCCESS, WARNING, ERROR, CRITICAL @@ -569,6 +573,9 @@ Environment variables override values set in the TOML config file. Default true. - `GRANIAN_URL_PATH_PREFIX` HTTP path prefix for codex (e.g. "/codex" for reverse proxy sub-path). Defaults to "". +- `CODEX_CSRF_TRUSTED_ORIGINS` Comma-separated public origins (e.g. + `https://comics.example.com`) accepted for CSRF behind a reverse proxy. See + [CSRF 403 behind a proxy](#csrf-403-behind-a-proxy). Defaults to none. ##### Repair @@ -697,6 +704,27 @@ of the `/codex` location with `auth_request`, exempt the API the way Codex's own examples do (`location /codex/api { auth_request off; ... }`); the `^~` note above is what keeps the static assets reachable. +#### CSRF 403 behind a proxy + +If logging in or saving changes fails with a 403 "CSRF Failed" error behind a +reverse proxy, first make sure the proxy sends `X-Forwarded-Proto` and +`X-Forwarded-Host` (the nginx example above does). Codex compares them with the +browser's `Origin` header. Only if the proxy can't send them, or the error +persists, add the public origin the browser uses to `config/codex.toml`: + +```toml +[server] +csrf_trusted_origins = ["https://comics.example.com"] +``` + +or set `CODEX_CSRF_TRUSTED_ORIGINS="https://comics.example.com"` (comma +separated for more than one). An origin is `scheme://host[:port]` with no path; +`https://*.example.com` trusts every subdomain. + +Do not remove Django's `CsrfViewMiddleware` to work around the error. The API +enforces CSRF through Django REST Framework anyway, so removing it doesn't fix +the API, and it strips CSRF protection from Codex's other pages. + #### Preserving the Codex Server Header Codex identifies itself with a `Server: codex/` response header, which diff --git a/codex/settings/__init__.py b/codex/settings/__init__.py index cf7a3ff77..936f39bad 100644 --- a/codex/settings/__init__.py +++ b/codex/settings/__init__.py @@ -31,7 +31,9 @@ get_float, get_int, get_str, + get_str_list, load_codex_config, + normalize_csrf_trusted_origins, normalize_url_path_prefix, ) from codex.settings.logging import get_logging_settings @@ -535,6 +537,14 @@ def _get_secure_csp(features: FeatureFlags) -> dict[str, list[str]]: # Proxy headers USE_X_FORWARDED_HOST = True SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https") +# Public origins (e.g. "https://comics.example.com") a proxy serves Codex under +# when the scheme or host Django sees differs. DRF's SessionAuthentication +# enforces CSRF on the API too, so this is the fix, not removing the middleware. +CSRF_TRUSTED_ORIGINS = list( + normalize_csrf_trusted_origins( + get_str_list(CODEX_CONFIG, "server.csrf_trusted_origins") + ) +) # Debug INTERNAL_IPS = ("127.0.0.1",) diff --git a/codex/settings/codex.toml.default b/codex/settings/codex.toml.default index 7e0e38f43..6eb7c0c3b 100644 --- a/codex/settings/codex.toml.default +++ b/codex/settings/codex.toml.default @@ -15,6 +15,10 @@ # websockets = true # HTTP path prefix for codex (e.g. "/codex" for reverse proxy sub-path) # url_path_prefix = "" +# Public origins accepted for CSRF when a reverse proxy serves Codex under +# another scheme or host than Codex sees (e.g. https in front of http). +# First make sure the proxy sends X-Forwarded-Proto and X-Forwarded-Host. +# csrf_trusted_origins = ["https://comics.example.com"] # Worker threads that keep a warm database connection between requests. # Opening one costs a few milliseconds, mostly SQLite re-reading the # schema, so reusing them makes short requests noticeably cheaper. Each diff --git a/codex/settings/config.py b/codex/settings/config.py index f0258bfbb..6e6b102b3 100644 --- a/codex/settings/config.py +++ b/codex/settings/config.py @@ -2,7 +2,7 @@ import shutil import tomllib -from collections.abc import Mapping, MutableMapping +from collections.abc import Iterable, Mapping, MutableMapping from os import environ from pathlib import Path @@ -20,6 +20,7 @@ "GRANIAN_HTTP": "server.http", "GRANIAN_WEBSOCKETS": "server.websockets", "GRANIAN_URL_PATH_PREFIX": "server.url_path_prefix", + "CODEX_CSRF_TRUSTED_ORIGINS": "server.csrf_trusted_origins", # Logging "LOGLEVEL": "logging.loglevel", "CODEX_LOG_RETENTION": "logging.log_retention", @@ -150,6 +151,33 @@ def normalize_url_path_prefix(prefix: str) -> str: return f"/{prefix}" if prefix else "" +def normalize_csrf_trusted_origins(origins: Iterable[str]) -> tuple[str, ...]: + """ + Normalize origins for Django's ``CSRF_TRUSTED_ORIGINS``. + + Django matches the ``Origin`` header against each entry by exact string, + or by subdomain for a ``https://*.example.com`` wildcard. A trailing slash + never matches, so it is stripped. An entry without a scheme can never + match either (and fails system check 4_0.E001), so it is dropped with a + warning. Duplicates are removed, keeping the first. + """ + normalized: dict[str, None] = {} + for raw_origin in origins: + origin = raw_origin.rstrip("/") + if "://" not in origin: + reason = ( + f"Ignoring server.csrf_trusted_origins entry {raw_origin!r}:" + " an origin has the form scheme://host[:port], e.g." + " https://comics.example.com. If CSRF 403s persist behind a" + " proxy, first check that it sends X-Forwarded-Proto and" + " X-Forwarded-Host." + ) + logger.warning(reason) + continue + normalized[origin] = None + return tuple(normalized) + + def get_int(config: Mapping, keypath: str, default: int = 0) -> int: """Get an integer value from the config.""" val = _deep_get(config, keypath, default) @@ -172,3 +200,25 @@ def get_bool(config: Mapping, keypath: str, *, default: bool = False) -> bool: if isinstance(val, str): return val.lower() not in {"", "false", "0", "no"} return bool(val) + + +def get_str_list( + config: Mapping, keypath: str, default: tuple[str, ...] = () +) -> tuple[str, ...]: + """ + Get a list of strings from the config. + + TOML gives a native array; an env override gives one comma-separated + string. Items are stripped of whitespace and empty items are dropped. + """ + val = _deep_get(config, keypath) + match val: + case None: + return default + case str(): + items = val.split(",") + case list() | tuple(): + items = val + case _: + items = (val,) + return tuple(item for raw in items if (item := str(raw).strip())) diff --git a/tests/test_csrf_trusted_origins.py b/tests/test_csrf_trusted_origins.py new file mode 100644 index 000000000..23bc84bf4 --- /dev/null +++ b/tests/test_csrf_trusted_origins.py @@ -0,0 +1,159 @@ +""" +CSRF trusted origins. + +A reverse proxy that serves Codex under another scheme or host makes the +browser's ``Origin`` header differ from the origin Django sees, so every +unsafe API request fails the CSRF check with a 403. DRF's +``SessionAuthentication`` enforces CSRF itself, so removing +``CsrfViewMiddleware`` does not help. ``server.csrf_trusted_origins`` (or +``CODEX_CSRF_TRUSTED_ORIGINS``) is read by :func:`get_str_list` and cleaned by +:func:`normalize_csrf_trusted_origins` into Django's ``CSRF_TRUSTED_ORIGINS``. +""" + +from __future__ import annotations + +from contextlib import contextmanager +from http import HTTPStatus +from typing import TYPE_CHECKING, Final, override + +import pytest +from django.contrib.auth.models import User +from django.test import Client, TestCase, override_settings +from loguru import logger + +from codex.settings.config import ( + _apply_env_overrides, + get_str_list, + normalize_csrf_trusted_origins, +) +from codex.startup import init_admin_flags + +if TYPE_CHECKING: + from collections.abc import Generator, Mapping + + from django.http import HttpResponse + +_TEST_PASSWORD: Final = "test-pw-hush-S106" # noqa: S105 +_ENV_NAME: Final = "CODEX_CSRF_TRUSTED_ORIGINS" +_KEYPATH: Final = "server.csrf_trusted_origins" +_ORIGIN_A: Final = "https://a.example.com" +_ORIGIN_B: Final = "https://b.example.com" +_PUBLIC_ORIGIN: Final = "https://comics.example.com" + + +@contextmanager +def _capture_logs() -> Generator[list[str]]: + """Collect loguru WARNING+ messages emitted inside the ``with`` block.""" + records: list[str] = [] + sink_id = logger.add(records.append, level="WARNING", format="{message}") + try: + yield records + finally: + logger.remove(sink_id) + + +def _server(value) -> dict: + return {"server": {"csrf_trusted_origins": value}} + + +@pytest.mark.parametrize( + ("config", "expected"), + [ + (_server([_ORIGIN_A, _ORIGIN_B]), (_ORIGIN_A, _ORIGIN_B)), + (_server(f"{_ORIGIN_A},{_ORIGIN_B}"), (_ORIGIN_A, _ORIGIN_B)), + (_server(f" {_ORIGIN_A} , {_ORIGIN_B} ,"), (_ORIGIN_A, _ORIGIN_B)), + (_server(""), ()), + (_server([]), ()), + (_server(None), ()), + ({"server": {}}, ()), + ], +) +def test_get_str_list(config: Mapping, expected: tuple[str, ...]) -> None: + """A TOML array or a comma string becomes a tuple of stripped items.""" + assert get_str_list(config, _KEYPATH) == expected + + +@pytest.mark.parametrize( + ("raw", "expected"), + [ + ((f"{_ORIGIN_A}/",), (_ORIGIN_A,)), + (("https://*.example.com",), ("https://*.example.com",)), + ((_ORIGIN_B, _ORIGIN_A, f"{_ORIGIN_B}/"), (_ORIGIN_B, _ORIGIN_A)), + ], +) +def test_normalize_csrf_trusted_origins( + raw: tuple[str, ...], expected: tuple[str, ...] +) -> None: + """Trailing slashes go, wildcards stay, duplicates collapse in order.""" + assert normalize_csrf_trusted_origins(raw) == expected + + +def test_normalize_drops_schemeless_origin_with_warning() -> None: + """An entry without a scheme can never match, so it is dropped loudly.""" + with _capture_logs() as records: + result = normalize_csrf_trusted_origins(("comics.example.com", _ORIGIN_A)) + assert result == (_ORIGIN_A,) + assert len(records) == 1 + assert "'comics.example.com'" in records[0] + assert "scheme://host[:port]" in records[0] + + +def test_env_override_reaches_config(monkeypatch: pytest.MonkeyPatch) -> None: + """The env var lands as the raw string and splits into origins.""" + monkeypatch.setenv(_ENV_NAME, f"{_ORIGIN_A}, {_ORIGIN_B}") + config: dict = {} + _apply_env_overrides(config) + assert config["server"]["csrf_trusted_origins"] == f"{_ORIGIN_A}, {_ORIGIN_B}" + assert get_str_list(config, _KEYPATH) == (_ORIGIN_A, _ORIGIN_B) + + +def test_env_override_beats_toml(monkeypatch: pytest.MonkeyPatch) -> None: + """The env var replaces a TOML array, even when empty.""" + config = _server([_PUBLIC_ORIGIN]) + monkeypatch.setenv(_ENV_NAME, _ORIGIN_A) + _apply_env_overrides(config) + assert get_str_list(config, _KEYPATH) == (_ORIGIN_A,) + + monkeypatch.setenv(_ENV_NAME, "") + _apply_env_overrides(config) + assert get_str_list(config, _KEYPATH) == () + + +class CsrfTrustedOriginsTestCase(TestCase): + """A proxied browser passes the API's CSRF check only for a trusted origin.""" + + @override + def setUp(self) -> None: + """Log in a CSRF-enforcing client and fetch its token.""" + init_admin_flags() + User.objects.create_user(username="reader", password=_TEST_PASSWORD) + self.client = Client(enforce_csrf_checks=True) + self.client.login(username="reader", password=_TEST_PASSWORD) + response = self.client.get("/api/v4/auth/csrf") + self.token = response.json()["data"]["csrfToken"] # pyright: ignore[reportUninitializedInstanceVariable] + + def _put_timezone(self) -> HttpResponse: + # Django sees the internal http host; the browser saw the public + # https origin, as behind a proxy that forwards neither header. + return self.client.put( + "/api/v4/auth/timezone", + data={"timezone": "America/New_York"}, + content_type="application/json", + HTTP_HOST="codex.internal", + HTTP_ORIGIN=_PUBLIC_ORIGIN, + HTTP_X_CSRFTOKEN=self.token, + ) + + def test_untrusted_origin_is_rejected(self) -> None: + """Without the setting the mismatched origin fails CSRF.""" + response = self._put_timezone() + assert response.status_code == HTTPStatus.FORBIDDEN + body = response.content.decode() + assert "CSRF Failed" in body + assert "Origin checking failed" in body + + def test_trusted_origin_is_accepted(self) -> None: + """The same request passes once its origin is trusted.""" + with override_settings(CSRF_TRUSTED_ORIGINS=[_PUBLIC_ORIGIN]): + response = self._put_timezone() + assert response.status_code == HTTPStatus.NO_CONTENT