From 4db803c7ba09f0ded3760089edb3dc9be2264c20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vahdet=20Eren=20BOZY=C4=B0L?= Date: Wed, 22 Jul 2026 20:54:09 +0300 Subject: [PATCH] fix: resolve client IP from trusted proxy hops, not uvicorn's wildcard The image ran uvicorn with `--forwarded-allow-ips "*"`, on the reasoning that Railway/Render/Fly always have their proxy in front so trusting it is safe. The flag doesn't mean that. With `"*"`, uvicorn rewrites request.client from the LEFTMOST X-Forwarded-For entry -- the end of the chain furthest from the proxy, written by whoever sent the request. Since client_ip() keys on request.client.host, a different random address per request bought a fresh budget on every limiter in the app: login's 5/min brute-force protection, register, password reset, /translate, and the app-wide backstop. It also made the `ip=` field in every security log line attacker-authored, and -- because RateLimiter never evicts keys -- let a single caller grow the limiter's dict without bound. The header is now read by the app and counted from the RIGHT: each proxy appends the address of its own immediate peer, so with TRUSTED_PROXY_HOPS proxies in front, the real client is that many entries from the end and everything to its left is ignored. Entries must parse as IP addresses; an unparseable one, or a chain shorter than the configured hop count, falls back to the socket peer, which over-limits rather than under-limits. Parsed addresses are normalized, so respelling an IPv6 address doesn't buy a second budget. TRUSTED_PROXY_HOPS defaults to 0 (no proxy -- correct for local dev and docker compose, where the browser reaches the backend directly). BEHAVIOUR CHANGE: deployments behind a proxy must set TRUSTED_PROXY_HOPS=1 or rate limiting will count every visitor as one client. DEPLOYMENT.md explains how to count, which direction is safe to get wrong, and how to verify both failure modes after deploying -- the old guide asserted the Dockerfile "handles this", which was exactly the misreading that caused the bug. The uvicorn flags are gone from the image; nothing else needed them, since this app reads request.url.path only and builds no absolute URLs. Tests, and their honest limit: the resolution logic is pinned directly (which entry becomes the key, ports stripped, IPv6 normalized, both fallbacks), plus end-to-end checks that an unconfigured deployment ignores the header. Those end-to-end tests cannot reproduce the original bypass -- uvicorn's ProxyHeadersMiddleware is installed by the server, not by app.main:app, so TestClient never runs it and they would have passed before this fix too. The flag is guarded where it actually lives: a contract test that fails if any --forwarded-allow-ips returns to the Dockerfile CMD, and another that fails if DEPLOYMENT.md stops documenting the setting. 180 passed (was 168). Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 53 ++++++++ DEPLOYMENT.md | 39 +++++- backend/.env.example | 8 ++ backend/.gitignore | 1 + backend/Dockerfile | 21 ++-- backend/app/config.py | 24 +++- backend/app/services/rate_limiter.py | 85 ++++++++++++- backend/tests/test_deployment_contracts.py | 27 +++++ backend/tests/test_security.py | 133 +++++++++++++++++++++ 9 files changed, 376 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c2248e..da41d61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,59 @@ Turkish, then each given an English mirror at the same version number directly). New features starting from 0.0.4 are English-only going forward, one PATCH version per completed feature/topic. +## [0.1.4] — Security review follow-up (in progress) + +A fresh adversarial read of the codebase at v0.1.3, working through the +findings one at a time. This section grows as each is fixed. + +### Fixed + +- **Every per-IP rate limit could be bypassed with a forged header.** + The image ran uvicorn with `--forwarded-allow-ips "*"`, on the + reasoning that Railway/Render/Fly always have their proxy in front so + trusting it is safe. The flag doesn't mean that. With `"*"`, uvicorn + rewrites `request.client` from the **leftmost** `X-Forwarded-For` + entry — the end of the chain furthest from the proxy, written by + whoever sent the request. Since `client_ip()` keys on + `request.client.host`, a different random address per request bought a + fresh budget on *every* limiter in the app: login's 5/min brute-force + protection, register, password reset, `/translate`, and the app-wide + backstop. It also meant the `ip=` field in every security log line was + attacker-authored, and — because `RateLimiter` never evicts keys — that + a single caller could grow the limiter's dict without bound. + + The header is now read by the app itself (`client_ip()` in + `app/services/rate_limiter.py`) and counted from the **right**: each + proxy appends the address of its own immediate peer, so with + `TRUSTED_PROXY_HOPS` proxies in front, the real client is that many + entries from the end and everything to the left is ignored. Entries + must parse as IP addresses — an unparseable one, or a chain shorter + than the configured hop count, falls back to the socket peer, which + over-limits rather than under-limits. Parsed addresses are also + normalized, so respelling an IPv6 address doesn't buy a second budget. + + `TRUSTED_PROXY_HOPS` defaults to **0** (no proxy — correct for local + dev and `docker compose`, where the browser reaches the backend + directly). Deployments behind one proxy set 1; `DEPLOYMENT.md` explains + how to count, which direction is safe to get wrong, and how to verify + both failure modes after deploying — the old guide asserted the + Dockerfile "handles this", which was exactly the misreading that caused + the bug. The uvicorn flags are gone from the image; nothing else needed + them, since this app reads `request.url.path` only and builds no + absolute URLs. + + Test coverage, and its honest limit: the resolution logic is pinned + directly (which entry becomes the key, ports stripped, IPv6 normalized, + fallbacks), plus end-to-end checks that an unconfigured deployment + ignores the header on the login and `/translate` limiters. Those + end-to-end tests can't reproduce the *original* bypass, because + uvicorn's `ProxyHeadersMiddleware` is installed by the server rather + than by `app.main:app`, so `TestClient` never runs it — they would have + passed before the fix too. The flag is therefore guarded where it + actually lives: a deployment-contract test that fails if any + `--forwarded-allow-ips` returns to the Dockerfile's `CMD`, and another + that fails if `DEPLOYMENT.md` stops documenting the setting. + ## [0.1.3] — PWA support & content expansion Two roadmap items, and a quiet full circle: the Turkish course written diff --git a/DEPLOYMENT.md b/DEPLOYMENT.md index 82f1459..2469a4f 100644 --- a/DEPLOYMENT.md +++ b/DEPLOYMENT.md @@ -29,9 +29,35 @@ root directory at `backend/`. | `FRONTEND_BASE_URL` | your Vercel URL, e.g. `https://lingua-xyz.vercel.app` | Base for verification/reset links in emails. | | `CORS_ALLOWED_ORIGINS` | same Vercel URL (comma-separated if several) | Browser origins allowed to call the API. Setting it replaces the development defaults entirely — deployments get exactly what they ask for. | | `REDIS_URL` | provision the platform's Redis add-on and paste its URL | Optional -- the cache silently disables itself when unset. | +| `TRUSTED_PROXY_HOPS` | `1` on all three platforms | **Set this, or rate limiting counts every visitor as one client.** All three terminate TLS at their own proxy, so the app never sees the real client address on the socket — it has to read it out of `X-Forwarded-For`. See below. | | `USE_MOCK_TRANSLATION` | `true` for now | The real NLLB model needs ~3 GB and a beefier instance; keep the mock until that's sized. | | `SMTP_HOST` / `SMTP_PORT` / `SMTP_USERNAME` / `SMTP_PASSWORD` / `SMTP_FROM_ADDRESS` | your mail provider's values | Unset = mock email service: verification/reset emails are logged, not sent. Fine for a demo, not for real users. | +**Counting proxy hops correctly** (`TRUSTED_PROXY_HOPS`): set it to the +number of proxies **you** control between the internet and the container. +Each one appends the address it received the request from, so the app +takes the Nth entry from the *right* of `X-Forwarded-For` and ignores +everything to its left — that part arrived with the request and anyone +can write it. + +- Railway / Render / Fly, nothing else in front: **1**. +- A CDN (Cloudflare, CloudFront) in front of one of those: **2**. +- Reachable directly, no proxy: **0** (the default — the socket address + is already the client's). + +Getting it **too low** is the safe direction to be wrong in: the app +falls back to the peer address, so everyone shares one rate-limit bucket +and legitimate users start seeing 429s — visible, annoying, harmless. +Getting it **too high** is the dangerous one: the app starts reading +entries the caller wrote, and per-IP limits become bypassable. When +unsure, count low and check step 6 of the post-deploy checklist. + +> *Removed in v0.1.4:* the image no longer passes uvicorn +> `--forwarded-allow-ips "*"`. That made uvicorn believe the *leftmost* +> `X-Forwarded-For` entry — the one the caller writes — so a random header +> per request bought a fresh login/translate/global budget every time. If +> you carried that flag into your own start command, drop it. + **Postgres instead of SQLite** (recommended once real users exist): uncomment `psycopg2-binary` in `backend/requirements.txt`, set `DATABASE_URL` to the platform's Postgres URL. Migrations run at startup @@ -83,6 +109,13 @@ Idempotent, so re-running after a redeploy is safe. See 5. The app is installable: open the deployed frontend on a phone and check the browser offers "Add to home screen" (PWA manifest + service worker are served over HTTPS). -6. Rate limiting sees real client IPs: hit an auth endpoint 6x and check - the 429 logs a real address, not the proxy's (the Dockerfile's - `--proxy-headers` handles this on Railway/Render/Fly). +6. Rate limiting sees real client IPs — check **both** directions, since + each failure mode is invisible from the other side: + - Hit an auth endpoint 6x. The 6th returns 429, and the + `rate_limit_exceeded` log line shows a real client address rather + than the platform's internal proxy address. If it shows the proxy, + `TRUSTED_PROXY_HOPS` is too low. + - Repeat while sending a junk `X-Forwarded-For: 1.2.3.4` header that + changes every request. You must **still** get a 429 at the same + point. If the limit never trips, `TRUSTED_PROXY_HOPS` is too high + and the app is reading caller-supplied entries. diff --git a/backend/.env.example b/backend/.env.example index b392cb0..13c588d 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -33,6 +33,14 @@ TRANSLATION_CACHE_TTL_SECONDS=604800 API_RATE_LIMIT_PER_MINUTE=120 TRANSLATE_RATE_LIMIT_PER_MINUTE=30 +# How many reverse proxies YOU control sit in front of the app (v0.1.4). +# 0 (default) = none: the socket address is the client. Correct for local +# dev and docker-compose. Behind Railway/Render/Fly set 1; add 1 more for +# a CDN in front of that. Rate limiting keys on the address this resolves, +# so too high means per-IP limits can be bypassed with a forged +# X-Forwarded-For -- see DEPLOYMENT.md before changing it. +TRUSTED_PROXY_HOPS=0 + # CORS allowlist (comma-separated). Empty = development defaults, which # cover both http://localhost:5173 and http://127.0.0.1:5173 (different # origins to a browser). Set this explicitly in any deployment. diff --git a/backend/.gitignore b/backend/.gitignore index 5738dae..ff67080 100644 --- a/backend/.gitignore +++ b/backend/.gitignore @@ -1,4 +1,5 @@ venv/ +.venv*/ __pycache__/ *.pyc app.db diff --git a/backend/Dockerfile b/backend/Dockerfile index 87d720c..5f0811f 100644 --- a/backend/Dockerfile +++ b/backend/Dockerfile @@ -35,10 +35,17 @@ EXPOSE 8000 HEALTHCHECK --interval=30s --timeout=3s --start-period=15s --retries=3 \ CMD python -c "import urllib.request,sys; sys.exit(0 if urllib.request.urlopen('http://localhost:8000/health', timeout=2).status==200 else 1)" -# --proxy-headers so client_ip() (rate limiting, security logs) sees the -# real client address behind the TLS-terminating proxies of -# Railway/Render/Fly. --forwarded-allow-ips='*' trusts any upstream to -# set those headers -- correct on those platforms (their proxy is always -# in front) and harmless in local compose (nothing sends the headers); -# do NOT expose this container directly to the internet without a proxy. -CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000", "--proxy-headers", "--forwarded-allow-ips", "*"] +# No --proxy-headers / --forwarded-allow-ips (removed in v0.1.4). The +# `"*"` form this used to pass makes uvicorn rewrite request.client from +# the LEFTMOST X-Forwarded-For entry, which the client writes -- so every +# per-IP rate limit could be reset by varying a header, and security logs +# recorded whatever address the caller chose. The app now resolves the +# client address itself from TRUSTED_PROXY_HOPS, counting from the right +# (see app/services/rate_limiter.py: client_ip). Deployments behind +# exactly one proxy -- Railway, Render, Fly -- set TRUSTED_PROXY_HOPS=1; +# see DEPLOYMENT.md. +# +# Nothing else needed those flags: X-Forwarded-Proto only sets +# request.url.scheme, and this app reads request.url.path only -- it +# builds no absolute URLs (email links come from FRONTEND_BASE_URL). +CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000"] diff --git a/backend/app/config.py b/backend/app/config.py index 2a33599..685a726 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -5,7 +5,7 @@ class Settings(BaseSettings): model_config = SettingsConfigDict(env_file=".env", extra="ignore") app_name: str = "AI Translation and Language Learning Platform" - app_version: str = "0.1.3" + app_version: str = "0.1.4" database_url: str = "sqlite:///./app.db" secret_key: str = "change-this-for-development" @@ -48,6 +48,28 @@ class Settings(BaseSettings): api_rate_limit_per_minute: int = 120 translate_rate_limit_per_minute: int = 30 + # How many trusted reverse proxies sit in front of this app (v0.1.4). + # + # 0 (default): none. The TCP peer address *is* the client -- true for + # local dev, `docker compose up` (the browser calls :8000 directly), + # and any direct exposure. + # + # N > 0: the rightmost N entries of X-Forwarded-For were appended by + # proxies you control, so the real client is the Nth entry from the + # right; everything left of it was written by the client and is + # ignored. Railway/Render/Fly each put exactly one proxy in front -- + # set this to 1 there (see DEPLOYMENT.md). + # + # Why a hop count rather than a trusted-proxy IP allowlist: those + # platforms' proxy addresses are internal and not documented as + # stable, so an accurate allowlist can't actually be written. And the + # thing this replaces -- uvicorn's `--forwarded-allow-ips "*"` -- is + # worse than either: it trusts the *leftmost* X-Forwarded-For entry, + # which is entirely client-supplied, so anyone could hand themselves + # a fresh login/translate/global rate-limit budget on every single + # request just by varying a header. + trusted_proxy_hops: int = 0 + frontend_base_url: str = "http://localhost:5173" # CORS allowlist, comma-separated. Empty = development defaults (see diff --git a/backend/app/services/rate_limiter.py b/backend/app/services/rate_limiter.py index dd0ef1b..c46c6d0 100644 --- a/backend/app/services/rate_limiter.py +++ b/backend/app/services/rate_limiter.py @@ -16,9 +16,11 @@ saving a dozen lines of code. """ +import ipaddress from collections import defaultdict from datetime import datetime, timedelta, timezone from threading import Lock +from typing import Optional from fastapi import HTTPException, Request, status @@ -86,11 +88,86 @@ def clear_all(self) -> None: # inference and is therefore the most expensive thing an abuser can call. +def _parse_forwarded_host(value: str) -> Optional[str]: + """Extracts a bare IP address from a single X-Forwarded-For entry. + + Entries are usually bare addresses, but a proxy may append the source + port ("1.2.3.4:51234", "[2001:db8::1]:443"). Returns None for + anything that doesn't parse as an IP address -- see client_ip() for + why refusing to guess matters here. + + Returning the *parsed* address rather than the original string also + normalizes it, which stops one client from occupying many rate-limit + buckets just by respelling its own IPv6 address ("::1" and + "0:0:0:0:0:0:0:1" are the same host and must map to the same key). + """ + value = value.strip() + if not value: + return None + + if value.startswith("["): + # Bracketed IPv6, with or without a trailing ":port". + end = value.find("]") + if end == -1: + return None + candidate = value[1:end] + elif value.count(":") == 1: + # Exactly one colon means IPv4 with a port; a bare IPv6 address + # always has more, and never carries a port unbracketed. + candidate = value.rsplit(":", 1)[0] + else: + candidate = value + + try: + return str(ipaddress.ip_address(candidate)) + except ValueError: + return None + + def client_ip(request: Request) -> str: - """Best-effort per-client key for rate limiting. Behind a reverse - proxy this is the proxy's address unless forwarded headers are - configured (a deployment concern, noted for the v0.1.0 deploy guide).""" - return request.client.host if request.client else "unknown" + """Per-client key for rate limiting and security logging. + + Resolved here rather than by uvicorn's --proxy-headers (v0.1.4). + That flag was previously passed as `--forwarded-allow-ips "*"`, which + makes uvicorn rewrite `request.client` from the **leftmost** + X-Forwarded-For entry -- the one furthest from the proxy and written + entirely by whoever sent the request. Every per-IP budget in this + module (login 5/min, register, password reset, /translate, and the + app-wide backstop) was therefore bypassable by putting a different + random address in a header on each request, and the `ip=` field in + the security log was attacker-authored. + + The correct entry is counted from the *right*: each proxy appends the + address of its own immediate peer, so with `trusted_proxy_hops` + proxies in front, the last one to write was the outermost and the + real client sits `trusted_proxy_hops` from the end. Anything to the + left of it arrived with the request and is ignored. + + Falls back to the TCP peer address whenever the header can't be + trusted to mean what the configuration claims -- an unparseable entry, + or a chain shorter than the configured hop count (which means the + request did not come through the expected proxies). That fallback + lumps such requests together under the proxy's own address, i.e. it + over-limits rather than under-limits: the safe direction for a + misconfiguration to fail in. + """ + peer = request.client.host if request.client else "unknown" + + hops = settings.trusted_proxy_hops + if hops <= 0: + return peer + + # A request can carry several X-Forwarded-For headers; together they + # form one chain, in order. + chain = [ + entry + for header in request.headers.getlist("x-forwarded-for") + for entry in header.split(",") + ] + if len(chain) < hops: + return peer + + return _parse_forwarded_host(chain[-hops]) or peer def enforce_rate_limit(limiter: RateLimiter, key: str, endpoint: str) -> None: diff --git a/backend/tests/test_deployment_contracts.py b/backend/tests/test_deployment_contracts.py index d782bc6..0761570 100644 --- a/backend/tests/test_deployment_contracts.py +++ b/backend/tests/test_deployment_contracts.py @@ -72,6 +72,33 @@ def test_content_packs_are_present_and_discoverable(): assert available_packs(), "no content packs found" +# --- the image must not hand X-Forwarded-For back to uvicorn (v0.1.4) ------ + + +def test_docker_image_does_not_blanket_trust_forwarded_headers(): + """`--forwarded-allow-ips "*"` makes uvicorn overwrite request.client + with the leftmost X-Forwarded-For entry -- which the caller writes. + That turned every per-IP rate limit into a suggestion. The app reads + the chain itself now (TRUSTED_PROXY_HOPS); this guards against the + flag being reinstated as an apparently-innocent 'see real client IPs + behind the proxy' fix.""" + dockerfile = (BACKEND_DIR / "Dockerfile").read_text() + cmd = [line for line in dockerfile.splitlines() if line.startswith("CMD")] + assert cmd, "no CMD line found in the Dockerfile" + assert "--forwarded-allow-ips" not in cmd[0], ( + "uvicorn is trusting forwarded headers again -- per-IP rate limiting " + "is only as trustworthy as whatever this flag allows" + ) + + +def test_deployment_guide_documents_the_proxy_hop_setting(): + """The old guide told operators the Dockerfile's --proxy-headers + handled real client IPs for them. It no longer does, and a deployment + that misses this silently rate-limits every visitor as one client.""" + guide = (BACKEND_DIR.parent / "DEPLOYMENT.md").read_text(encoding="utf-8") + assert "TRUSTED_PROXY_HOPS" in guide + + def test_docker_image_ships_the_content_directory(): """DEPLOYMENT.md instructs operators to run scripts/import_content.py inside the backend container. If the image doesn't carry content/, the diff --git a/backend/tests/test_security.py b/backend/tests/test_security.py index 160be4d..7752a62 100644 --- a/backend/tests/test_security.py +++ b/backend/tests/test_security.py @@ -1,6 +1,10 @@ import re +from starlette.requests import Request + +from app.config import settings from app.services.email_service import get_email_service +from app.services.rate_limiter import client_ip def _register_and_login(client, username="secuser", email="secuser@example.com", password="password1234"): @@ -356,3 +360,132 @@ def test_auth_rate_limit_response_includes_retry_after(client): blocked = client.post("/auth/login", data={"username": "ghost", "password": "wrong-password"}) assert blocked.status_code == 429 assert "Retry-After" in blocked.headers + + +# --- v0.1.4: which address rate limiting actually keys on ------------------- +# +# The bug: the image ran uvicorn with `--forwarded-allow-ips "*"`, which +# rewrites request.client from the *leftmost* X-Forwarded-For entry -- a +# value the caller writes. Sending a different one per request handed the +# caller a fresh budget on every per-IP limiter in the deployed app. +# +# Note on what these can and can't prove: uvicorn's ProxyHeadersMiddleware +# is installed by the *server*, not by app.main:app, so TestClient never +# runs it and cannot reproduce the original bypass -- these tests would +# have passed before the fix too. The flag itself is guarded where it +# actually lives, in tests/test_deployment_contracts.py. What's pinned +# here is the app-side resolution that replaced it: which entry of the +# chain becomes the rate-limit key, and that an unconfigured deployment +# ignores the header completely. + + +PEER = "203.0.113.7" # the TCP peer -- in production, the proxy + + +def _request(peer=PEER, xff=None): + """A Starlette Request with just enough scope for client_ip(). + + `xff` may be a string (one header) or a list (several, as a client can + legally send). + """ + headers = [] + if xff is not None: + for value in [xff] if isinstance(xff, str) else xff: + headers.append((b"x-forwarded-for", value.encode())) + return Request( + {"type": "http", "method": "GET", "path": "/", "headers": headers, + "client": (peer, 443) if peer else None} + ) + + +def test_forwarded_header_is_ignored_without_configured_proxies(): + # The default. No proxy is declared, so the header carries no + # authority no matter what it says. + assert client_ip(_request(xff="1.2.3.4")) == PEER + + +def test_one_proxy_takes_the_entry_that_proxy_appended(monkeypatch): + # This is the whole fix: the proxy appends its own peer *last*, so + # the rightmost entry is the only trustworthy one. "9.9.9.9" here is + # the attacker's own invention, sent to be picked up as the client. + monkeypatch.setattr(settings, "trusted_proxy_hops", 1) + assert client_ip(_request(xff="9.9.9.9, 198.51.100.23")) == "198.51.100.23" + + +def test_two_proxies_count_further_in_from_the_right(monkeypatch): + # CDN -> load balancer -> app: the LB appended the CDN's address, the + # CDN appended the real client's. + monkeypatch.setattr(settings, "trusted_proxy_hops", 2) + chain = "9.9.9.9, 198.51.100.23, 192.0.2.60" + assert client_ip(_request(xff=chain)) == "198.51.100.23" + + +def test_chain_split_across_several_headers_is_one_chain(monkeypatch): + monkeypatch.setattr(settings, "trusted_proxy_hops", 1) + assert client_ip(_request(xff=["9.9.9.9", "198.51.100.23"])) == "198.51.100.23" + + +def test_chain_shorter_than_configured_hops_falls_back_to_peer(monkeypatch): + # The request didn't come through the proxies the config promises, so + # the header proves nothing. Falling back to the peer over-limits + # (everyone shares one bucket) instead of under-limiting. + monkeypatch.setattr(settings, "trusted_proxy_hops", 2) + assert client_ip(_request(xff="198.51.100.23")) == PEER + assert client_ip(_request(xff=None)) == PEER + + +def test_unparseable_entry_falls_back_to_peer(monkeypatch): + # Never let an arbitrary string become a rate-limit key or land in a + # security log line as `ip=`. + monkeypatch.setattr(settings, "trusted_proxy_hops", 1) + for junk in ("not-an-ip", "", "127.0.0.1.5", "