From 776fb38b9b749e26cf4d686b2df0ac417e40e73b Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 12 Jul 2026 03:49:08 +0200 Subject: [PATCH] =?UTF-8?q?harden(auth):=20SA3=20review=20follow-ups=20?= =?UTF-8?q?=E2=80=94=20IPv6-mapped=20match=20+=20no-proxy-headers=20guardr?= =?UTF-8?q?ail?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-review hardening (both fail-safe, not bugs): - _client_ip normalizes IPs via ipaddress and unwraps IPv4-mapped IPv6, so a plain TRUSTED_PROXY_IPS=10.10.0.1 also matches a peer surfaced as ::ffff:10.10.0.1 (a Docker/IPv6 self-host footgun that would otherwise silently collapse everyone into one rate-limit bucket). - entrypoint.sh + _client_ip docstring: explicit warning never to add uvicorn --proxy-headers, which would rewrite request.client from the forgeable XFF and defeat the trust check. --- backend/app/auth.py | 20 ++++++++++++++++++-- backend/entrypoint.sh | 5 +++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/backend/app/auth.py b/backend/app/auth.py index 7a2f254..5874b65 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -1,3 +1,4 @@ +import ipaddress import logging import secrets from datetime import datetime, timedelta, timezone @@ -172,15 +173,30 @@ def get_or_create_demo_user(db: Session) -> User: return user +def _norm_ip(s: str) -> ipaddress.IPv4Address | ipaddress.IPv6Address | None: + """Parse an IP for comparison, unwrapping IPv4-mapped IPv6 (`::ffff:10.0.0.1` → `10.0.0.1`) so a + plain-IPv4 allowlist still matches a peer that surfaces in mapped form. None for a non-IP string.""" + try: + ip = ipaddress.ip_address(s.strip()) + except ValueError: + return None + return ip.ipv4_mapped if getattr(ip, "ipv4_mapped", None) else ip + + def _client_ip(request: Request) -> str: """Best-effort client IP for rate limiting. X-Forwarded-For is client-controlled, so we trust it ONLY when the request actually arrived from a configured reverse proxy (settings.trusted_proxy_ips — the address the proxy connects FROM, e.g. the VPS Caddy's WireGuard peer IP). Our proxy APPENDS the client it saw to XFF, so the RIGHTMOST entry is the real client and can't be forged by a client pre-seeding the header. Any other peer (someone hitting the app port directly, bypassing the - proxy) is untrusted — we use its real socket IP, so it can't spoof its rate-limit identity.""" + proxy) is untrusted — we use its real socket IP, so it can't spoof its rate-limit identity. + + SAFETY: this relies on `request.client` being the TRUE socket peer. uvicorn is started WITHOUT + `--proxy-headers` (see entrypoint.sh) precisely so it never rewrites `request.client` from XFF — + do NOT add that flag, or the trust check below would compare against a forgeable value.""" peer = request.client.host if request.client else "unknown" - if peer in settings.trusted_proxy_ip_set: + trusted = {_norm_ip(p) for p in settings.trusted_proxy_ip_set} - {None} + if _norm_ip(peer) in trusted: xff = request.headers.get("x-forwarded-for") parts = [p.strip() for p in (xff or "").split(",") if p.strip()] if parts: diff --git a/backend/entrypoint.sh b/backend/entrypoint.sh index 04ac8af..f7fc271 100644 --- a/backend/entrypoint.sh +++ b/backend/entrypoint.sh @@ -10,4 +10,9 @@ echo "Starting Siftlode API..." # timeout, so Caddy would reuse a connection uvicorn had just closed -> broken pipe / reset # on the next request -> 502. Non-idempotent POSTs (the player's progress/state writes, # fired every 5s) can't be auto-retried by the proxy, so those 502s reached the user. +# +# SECURITY: do NOT add --proxy-headers / --forwarded-allow-ips here. auth._client_ip trusts +# X-Forwarded-For only when the TRUE socket peer is a configured proxy (TRUSTED_PROXY_IPS); those +# flags would make uvicorn overwrite request.client from the forgeable XFF, defeating that check and +# reopening the rate-limit bypass (SA3). exec uvicorn app.main:app --host 0.0.0.0 --port 8000 --log-config log_config.json --timeout-keep-alive 75