Believe forwarded client addresses only from trusted proxies

.env.example

@@ -13,6 +13,11 @@ SOURCE_URL=https://github.com/dabeckham/beckham-share
 SECRET_KEY=CHANGE_ME
 DB_PASSWORD=CHANGE_ME
 
+# Peers allowed to name the client via X-Forwarded-For / X-Real-IP: addresses,
+# CIDR ranges, or hostnames. Anything arriving from elsewhere is attributed to
+# the address it came from. Leave empty if the app is exposed directly.
+TRUSTED_PROXIES=idp-caddy
+
 # ── OIDC (Authentik) — filled in after scripts/setup-authentik.py runs ──────
 OIDC_DISCOVERY_URL=https://auth.beckham.ai/application/o/beckham-share/.well-known/openid-configuration
 OIDC_CLIENT_ID=

CHANGELOG.md

@@ -5,6 +5,17 @@ pre-1.0 scheme; dates are when the change reached `main`.
 
 ## Unreleased
 
+### Security
+- The client address behind the anonymous rate limit and the upload audit trail
+  is no longer take-your-word-for-it. Forwarded headers are honoured only from
+  peers listed in the new `TRUSTED_PROXIES` setting, and `X-Forwarded-For` is
+  read right to left so hops appended by the client are discarded. Previously
+  any caller that could open a socket to the app could name itself, which reset
+  the rate-limit budget and wrote a false address into the audit trail;
+  `TRUST_FORWARDED_FOR=false` did not prevent it, because the server was
+  rewriting the peer address before the application saw it. That setting is
+  replaced by `TRUSTED_PROXIES`.
+
 ### Added
 - Full reference documentation under `docs/`: architecture, configuration,
   operations runbook, security model, and API reference, with a docs index and

Dockerfile

@@ -23,5 +23,9 @@ EXPOSE 8000
 HEALTHCHECK --interval=30s --timeout=5s --start-period=10s \
     CMD curl -fsS http://localhost:8000/healthz || exit 1
 
-CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000", \
-     "--proxy-headers", "--forwarded-allow-ips", "*"]
+# No --proxy-headers: it would rewrite the peer address from X-Forwarded-For
+# before the app sees it, and with --forwarded-allow-ips "*" it did so for any
+# caller. Deciding whose forwarded headers to believe belongs in one place —
+# see TRUSTED_PROXIES and app/fingerprint.py — so uvicorn reports the socket
+# peer and the app does the rest.
+CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000"]

app/config.py

@@ -63,8 +63,14 @@ class Settings(BaseSettings):
     smtp_from: str = "Beckham Share <[email protected]>"
     smtp_use_tls: bool = True
 
-    # Whether to trust X-Forwarded-For (true when behind the Caddy/HAProxy front).
-    trust_forwarded_for: bool = True
+    # ── Reverse proxy ────────────────────────────────────────────────────
+    # Which peers are allowed to tell us who the client is. Comma-separated
+    # addresses, CIDR ranges, or resolvable hostnames; hostnames are looked up
+    # at runtime because container addresses are assigned by Docker. A request
+    # arriving from anywhere else is attributed to the address it actually came
+    # from, whatever X-Forwarded-For / X-Real-IP it carries. Leave empty to
+    # trust nothing. `idp-caddy` is the shared front this app sits behind.
+    trusted_proxies: str = "idp-caddy"
 
     @property
     def email_enabled(self) -> bool:

app/fingerprint.py

@@ -8,23 +8,83 @@
 from __future__ import annotations
 
 import hashlib
+import ipaddress
 import json
+import logging
+import socket
+import time
 
 from fastapi import Request
 from ua_parser import user_agent_parser
 
 from .config import settings
 
+log = logging.getLogger("beckham_share.fingerprint")
+
+# Hostnames in TRUSTED_PROXIES resolve to container addresses that Docker can
+# reassign, so the parsed list is rebuilt periodically. It is also rebuilt
+# whenever the setting itself changes, which keeps it honest under test.
+_TRUST_TTL_SECONDS = 60.0
+_trust_cache: tuple[str, float, list] = ("", -_TRUST_TTL_SECONDS, [])
+
+
+def _trusted_networks() -> list:
+    global _trust_cache
+    configured = settings.trusted_proxies
+    key, stamp, nets = _trust_cache
+    if key == configured and time.monotonic() - stamp < _TRUST_TTL_SECONDS:
+        return nets
+
+    nets = []
+    for entry in (e.strip() for e in configured.split(",")):
+        if not entry:
+            continue
+        try:
+            nets.append(ipaddress.ip_network(entry, strict=False))
+            continue
+        except ValueError:
+            pass  # not an address or range — try resolving it as a hostname
+        try:
+            for info in socket.getaddrinfo(entry, None):
+                nets.append(ipaddress.ip_network(info[4][0]))
+        except OSError:
+            log.warning("TRUSTED_PROXIES: cannot resolve %r; ignoring it", entry)
+    _trust_cache = (configured, time.monotonic(), nets)
+    return nets
+
+
+def _is_trusted(address: str | None) -> bool:
+    if not address:
+        return False
+    try:
+        parsed = ipaddress.ip_address(address)
+    except ValueError:
+        return False
+    return any(parsed in net for net in _trusted_networks())
+
 
 def client_ip(request: Request) -> str | None:
-    if settings.trust_forwarded_for:
-        xff = request.headers.get("x-forwarded-for")
-        if xff:
-            return xff.split(",")[0].strip()
-        xri = request.headers.get("x-real-ip")
-        if xri:
-            return xri.strip()
-    return request.client.host if request.client else None
+    """The client's address, believed only as far as the infrastructure vouches.
+
+    Proxy headers are honoured only when the request actually arrived from a
+    trusted proxy — otherwise anyone who can open a socket to the app could
+    claim any address, which would reset the anonymous rate-limit budget and
+    poison the audit trail. ``X-Forwarded-For`` is read right to left: trusted
+    hops are skipped and the first untrusted address is the closest one that a
+    trusted proxy actually observed. Anything further left was appended by the
+    client and means nothing.
+    """
+    peer = request.client.host if request.client else None
+    if not _is_trusted(peer):
+        return peer
+    for hop in reversed(request.headers.get("x-forwarded-for", "").split(",")):
+        hop = hop.strip()
+        if hop and not _is_trusted(hop):
+            return hop
+    real_ip = request.headers.get("x-real-ip")
+    if real_ip:
+        return real_ip.strip()
+    return peer
 
 
 def parse_ua(user_agent: str | None) -> dict[str, str | None]:

docs/CONFIGURATION.md

@@ -24,7 +24,7 @@ ignored. Booleans accept `true`/`false`/`1`/`0`.
 | Variable | Default | Purpose |
 |---|---|---|
 | `SECRET_KEY` | `dev-insecure-change-me` | Signs the session cookie. **Must** be a long random value in production — rotating it logs everyone out. |
-| `TRUST_FORWARDED_FOR` | `true` | Trust `X-Forwarded-For` / `X-Real-IP` for the client IP. Correct when behind the Caddy/HAProxy front; set `false` only if the app is exposed directly. |
+| `TRUSTED_PROXIES` | `idp-caddy` | Peers allowed to name the client through `X-Forwarded-For` / `X-Real-IP`. Comma-separated addresses, CIDR ranges, or hostnames (resolved at runtime, since container addresses are assigned by Docker). Requests from anywhere else are attributed to the address they arrived from, whatever headers they carry. Leave empty to trust nothing — correct when the app is exposed directly. |
 
 ## Storage & database
 

docs/SECURITY.md

@@ -41,7 +41,12 @@ fully buffered. Members have a separate, larger cap (`MAX_UPLOAD_BYTES`).
 - `ANON_UPLOADS_PER_DAY` (default 20) in the last 24 hours.
 
 The count is taken from the `upload_events` audit rows, keyed by **IP _or_
-fingerprint** (`OR`), so rotating just one signal does not reset the budget.
+fingerprint** (`OR`), so rotating just one signal does not reset the budget. The
+two keys carry different weight on purpose: the address is established by the
+infrastructure (see `TRUSTED_PROXIES` below), while the browser fingerprint is
+supplied by the client and can be changed at will. The fingerprint's job is to
+catch one device rotating addresses; it is an additional key, never a
+substitute for the address.
 Because it counts rows that already exist in the database, the limit holds across
 worker processes and restarts with no separate store. Over-budget requests get
 `429` with a `Retry-After` header.
@@ -57,9 +62,17 @@ Every upload writes an `upload_events` row for review — see below.
 
 `app/fingerprint.py` records, per upload:
 
-- **Client IP** — from `X-Forwarded-For` / `X-Real-IP` when `TRUST_FORWARDED_FOR`
-  is set (the real client IP is preserved through HAProxy's PROXY protocol),
-  otherwise the socket peer.
+- **Client IP** — the address the request arrived from, unless it arrived from a
+  peer listed in `TRUSTED_PROXIES`, in which case that peer's
+  `X-Forwarded-For` / `X-Real-IP` is believed instead. The real client IP is
+  preserved to the front through HAProxy's PROXY protocol.
+
+  `X-Forwarded-For` is read **right to left**: trusted hops are skipped and the
+  first untrusted address is the closest one a trusted proxy actually observed.
+  Reading left to right would return whatever the client itself put in the
+  header, because proxies append. This matters because the anonymous rate limit
+  is keyed on the address — an address the client can choose is an address that
+  resets the budget.
 - **User agent** — raw, plus a parsed `browser` / `os` / `device` breakdown for
   human-readable review.
 - **Accept-Language**.
@@ -106,7 +119,7 @@ Every upload writes an `upload_events` row for review — see below.
 | Public share page abused as a spam relay | Server-side email is members-only; anonymous uses `mailto:`. |
 | Oversized-upload resource exhaustion | Streaming write with an early abort + partial-file cleanup. |
 | Session forgery | Signed session cookie (`SECRET_KEY`); `Secure` + `SameSite=Lax`. |
-| Spoofed client IP | Real IP preserved via PROXY protocol; `X-Forwarded-For` trusted only behind the front. |
+| Spoofed client IP (to reset the rate limit or poison the audit trail) | Forwarded headers are honoured only from peers in `TRUSTED_PROXIES`, and the chain is read right to left so client-appended hops are discarded. Everything else is attributed to the address it arrived from. |
 
 ## 7. Operational guidance
 

tests/backend/test_client_ip.py

@@ -0,0 +1,84 @@
+"""Whose word we take for the client's address.
+
+The anonymous rate limit and the upload audit trail are both keyed on the
+client IP, so an address the client gets to choose is an address that resets
+the budget. These cover which peers may speak for a client, and how a
+forwarded chain is read once one of them does.
+"""
+import pytest
+from starlette.requests import Request
+
+from app import fingerprint
+from app.config import settings
+
+TRUSTED = "10.9.0.1"
+
+
+def _request(peer, **headers) -> Request:
+    raw = [(k.replace("_", "-").encode(), v.encode()) for k, v in headers.items()]
+    return Request({"type": "http", "headers": raw, "client": (peer, 51234) if peer else None})
+
+
+@pytest.fixture
+def behind_proxy(monkeypatch):
+    """Trust one address, the way the deployment trusts the shared front."""
+    monkeypatch.setattr(settings, "trusted_proxies", TRUSTED)
+
+
+def test_forwarded_header_from_an_untrusted_peer_is_ignored(behind_proxy):
+    # A neighbour on the container network can reach the app directly. It does
+    # not get to name the client.
+    req = _request("172.23.0.9", x_forwarded_for="203.0.113.5", x_real_ip="203.0.113.5")
+    assert fingerprint.client_ip(req) == "172.23.0.9"
+
+
+def test_forwarded_header_from_the_trusted_proxy_is_believed(behind_proxy):
+    req = _request(TRUSTED, x_forwarded_for="203.0.113.5")
+    assert fingerprint.client_ip(req) == "203.0.113.5"
+
+
+def test_client_supplied_hops_are_discarded(behind_proxy):
+    # The client sent its own X-Forwarded-For and the proxy appended the address
+    # it saw. Reading left to right would return the client's invention.
+    req = _request(TRUSTED, x_forwarded_for="198.51.100.1, 203.0.113.5")
+    assert fingerprint.client_ip(req) == "203.0.113.5"
+
+
+def test_trusted_hops_are_skipped(monkeypatch):
+    monkeypatch.setattr(settings, "trusted_proxies", "10.9.0.0/24")
+    req = _request("10.9.0.1", x_forwarded_for="203.0.113.5, 10.9.0.7")
+    assert fingerprint.client_ip(req) == "203.0.113.5"
+
+
+def test_real_ip_is_used_when_there_is_no_forwarded_chain(behind_proxy):
+    req = _request(TRUSTED, x_real_ip="203.0.113.5")
+    assert fingerprint.client_ip(req) == "203.0.113.5"
+
+
+def test_trusting_nothing_falls_back_to_the_socket_peer(monkeypatch):
+    monkeypatch.setattr(settings, "trusted_proxies", "")
+    req = _request(TRUSTED, x_forwarded_for="203.0.113.5")
+    assert fingerprint.client_ip(req) == TRUSTED
+
+
+def test_unresolvable_entries_do_not_grant_trust(monkeypatch):
+    monkeypatch.setattr(settings, "trusted_proxies", "no-such-host.invalid")
+    req = _request("172.23.0.9", x_forwarded_for="203.0.113.5")
+    assert fingerprint.client_ip(req) == "172.23.0.9"
+
+
+def test_rate_limit_survives_forwarded_header_rotation(client):
+    # conftest sets ANON_UPLOADS_PER_HOUR=3. The test client is not a trusted
+    # proxy, so rotating the header (and the browser fingerprint with it) must
+    # not buy a fresh budget.
+    for i in range(3):
+        r = client.post("/api/anon-upload",
+                        files={"file": ("a.txt", b"hi", "text/plain")},
+                        data={"fp": f"fp-{i}"},
+                        headers={"x-forwarded-for": f"203.0.113.{i}"})
+        assert r.status_code == 200
+    blocked = client.post("/api/anon-upload",
+                          files={"file": ("a.txt", b"hi", "text/plain")},
+                          data={"fp": "fp-fresh"},
+                          headers={"x-forwarded-for": "203.0.113.200"})
+    assert blocked.status_code == 429