fix: resolve client IP from trusted proxy hops, not uvicorn's wildcard

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

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.

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.

backend/.gitignore

@@ -1,4 +1,5 @@
 venv/
+.venv*/
 __pycache__/
 *.pyc
 app.db

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"]

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

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:

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