From 48faf1bc901f27d79dcd08c851fbad2243fd19b1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 15:12:28 +0000 Subject: [PATCH] feat(web): opt-in X-Forwarded-For keying for the rate limiter behind a trusted proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The in-memory limiter keyed on the client socket IP. Behind a reverse proxy — the documented deployment for network exposure — every request carries the proxy's IP, collapsing all clients into one shared bucket: one noisy client exhausts the limit for everyone. New opt-in setting MEDCHECK_TRUST_PROXY_HEADERS uses the first X-Forwarded-For hop as the bucket key. Off by default because the header is client-spoofable without a trusted proxy in front (and would then allow limiter bypass). SECURITY.md documents that the proxy's own limiter should remain the primary control in proxied deployments. Fixes #142 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01W4svt5QTs4WUMSy4HwiVt9 --- .env.example | 5 +++++ README.md | 1 + SECURITY.md | 7 +++++++ src/medcheck/core/config.py | 3 +++ src/medcheck/web/app.py | 19 +++++++++++++++++- tests/unit/test_web.py | 39 +++++++++++++++++++++++++++++++++++++ 6 files changed, 73 insertions(+), 1 deletion(-) diff --git a/.env.example b/.env.example index 96a52d9..191052d 100644 --- a/.env.example +++ b/.env.example @@ -34,6 +34,11 @@ MEDCHECK_PORT=8080 # Strongly recommended whenever MEDCHECK_HOST is not 127.0.0.1. MEDCHECK_API_KEY= +# Only when a trusted reverse proxy fronts the server: set to 1 so the rate +# limiter keys on the first X-Forwarded-For hop instead of the proxy's IP. +# Leave off otherwise — without a proxy the header is client-spoofable. +MEDCHECK_TRUST_PROXY_HEADERS= + # ============================================================================= # Default Settings # ============================================================================= diff --git a/README.md b/README.md index a24b2ec..e29db37 100644 --- a/README.md +++ b/README.md @@ -193,6 +193,7 @@ cp .env.example .env | `MEDCHECK_PORT` | `8080` | Bind port | | `MEDCHECK_API_KEY` | — | When set, `/api` requires an `X-API-Key` header | | `MEDCHECK_RATE_LIMIT` | `10` | `POST /api/analyze` requests per IP per minute (`0` = off) | +| `MEDCHECK_TRUST_PROXY_HEADERS` | off | Key the rate limiter on the first `X-Forwarded-For` hop (`1` — only behind a trusted reverse proxy) | | `MEDCHECK_MAX_VISION_IMAGES` | `12` | Max slice images sent to the LLM per analysis | | `MEDCHECK_MAX_DOWNLOAD_BYTES` | 2 GiB | Cap on portal exam-ZIP downloads | diff --git a/SECURITY.md b/SECURITY.md index 8ec4f4b..8ac9e06 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -57,6 +57,13 @@ MedCheck processes medical and health-related queries. Any vulnerability that co - **Network exposure is opt-in.** The web server binds to `127.0.0.1` by default. When exposing it on the network, set `MEDCHECK_API_KEY` so `/api` endpoints require an `X-API-Key` header. +- **Rate limiting behind a reverse proxy.** The built-in limiter keys on the + client socket IP; behind a proxy all clients would share the proxy's IP and + one bucket. Set `MEDCHECK_TRUST_PROXY_HEADERS=1` **only** when a trusted + proxy fronts the server, so the first `X-Forwarded-For` hop is used instead + (client-spoofable without a proxy). In proxied deployments, treat the + proxy's own rate limiting as the primary control and the app limiter as a + backstop — the in-process limiter is per-worker and not shared state. - **Generated reports contain PHI by default.** Reports (JSON/PDF/HTML) embed patient name, ID and birth date from the DICOM metadata unless `--deidentify` is passed, which replaces them with a stable pseudonym. diff --git a/src/medcheck/core/config.py b/src/medcheck/core/config.py index c9b7d42..5319f09 100644 --- a/src/medcheck/core/config.py +++ b/src/medcheck/core/config.py @@ -35,6 +35,9 @@ class Settings: host: str = field(default_factory=lambda: os.environ.get("MEDCHECK_HOST", "127.0.0.1")) port: int = field(default_factory=lambda: _env_int("MEDCHECK_PORT", 8080)) api_key: str | None = field(default_factory=lambda: os.environ.get("MEDCHECK_API_KEY")) + # Trust X-Forwarded-For from a fronting reverse proxy (rate-limit keying). + # Off by default: without a trusted proxy the header is client-spoofable. + trust_proxy_headers: bool = field(default_factory=lambda: _env_flag("MEDCHECK_TRUST_PROXY_HEADERS")) # Consent gate: patient-derived data is only sent to external cloud LLM APIs # when this is explicitly enabled (MEDCHECK_ALLOW_EXTERNAL_LLM=1). allow_external_llm: bool = field(default_factory=lambda: _env_flag("MEDCHECK_ALLOW_EXTERNAL_LLM")) diff --git a/src/medcheck/web/app.py b/src/medcheck/web/app.py index ec55910..2f1b432 100644 --- a/src/medcheck/web/app.py +++ b/src/medcheck/web/app.py @@ -155,6 +155,23 @@ def allow(self, key: str) -> bool: return True +def _client_key(request: Request, trust_proxy_headers: bool) -> str: + """Rate-limit bucket key for a request. + + Behind a reverse proxy every request arrives from the proxy's socket IP, + which would collapse all clients into one shared bucket. When the operator + explicitly declares the proxy trusted (MEDCHECK_TRUST_PROXY_HEADERS), use + the first hop of X-Forwarded-For instead. Never trust the header by + default — without a proxy in front it is trivially client-spoofable. + """ + if trust_proxy_headers: + forwarded = request.headers.get("x-forwarded-for", "") + first_hop = forwarded.split(",")[0].strip() + if first_hop: + return first_hop + return request.client.host if request.client else "unknown" + + def create_app(settings: Settings | None = None) -> FastAPI: settings = settings or Settings() app = FastAPI(title="MedCheck", version=__version__) @@ -167,7 +184,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: rate_limiter = _RateLimiter(limit=int(os.environ.get("MEDCHECK_RATE_LIMIT", "10"))) def enforce_rate_limit(request: Request) -> None: - client_ip = request.client.host if request.client else "unknown" + client_ip = _client_key(request, settings.trust_proxy_headers) if not rate_limiter.allow(client_ip): raise HTTPException( status_code=status.HTTP_429_TOO_MANY_REQUESTS, diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index b7bf310..fd3e376 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -178,6 +178,45 @@ def test_analyze_rate_limited(monkeypatch): assert statuses[4] == 429 +# --- Rate limiting behind a reverse proxy (issue #142) --- + + +def test_rate_limit_ignores_forwarded_header_by_default(monkeypatch): + # X-Forwarded-For is client-spoofable; without the trust flag it must NOT + # create fresh buckets (else the limiter is trivially bypassed). + monkeypatch.setenv("MEDCHECK_RATE_LIMIT", "2") + client = TestClient(create_app(Settings(api_key=None, trust_proxy_headers=False))) + statuses = [ + client.post("/api/analyze", json=_VALID_BODY, headers={"X-Forwarded-For": f"10.0.0.{i}"}).status_code + for i in range(4) + ] + assert statuses == [501, 501, 429, 429] + + +def test_rate_limit_keys_on_forwarded_hop_when_proxy_trusted(monkeypatch): + # With a trusted proxy, distinct clients (distinct first hops) must not + # share one bucket even though every request has the same socket IP. + monkeypatch.setenv("MEDCHECK_RATE_LIMIT", "2") + client = TestClient(create_app(Settings(api_key=None, trust_proxy_headers=True))) + for i in range(4): + resp = client.post("/api/analyze", json=_VALID_BODY, headers={"X-Forwarded-For": f"10.0.0.{i}, 172.16.0.1"}) + assert resp.status_code == 501 # each client has its own bucket + + # ...while one client hammering does get limited. + statuses = [ + client.post("/api/analyze", json=_VALID_BODY, headers={"X-Forwarded-For": "10.0.0.99"}).status_code + for _ in range(3) + ] + assert statuses == [501, 501, 429] + + +def test_rate_limit_trusted_proxy_missing_header_falls_back_to_socket_ip(monkeypatch): + monkeypatch.setenv("MEDCHECK_RATE_LIMIT", "2") + client = TestClient(create_app(Settings(api_key=None, trust_proxy_headers=True))) + statuses = [client.post("/api/analyze", json=_VALID_BODY).status_code for _ in range(3)] + assert statuses == [501, 501, 429] + + # --- Per-request cloud LLM consent (issue #63) ---