From e6ee4cb208bdbbd4040e7646978f46e6dcbde140 Mon Sep 17 00:00:00 2001 From: nazarli-shabnam Date: Fri, 10 Jul 2026 13:12:25 +0400 Subject: [PATCH] fix(security): trusted-proxy-aware client IP resolution for rate limiting Both rate limiters derived "client IP" incorrectly: - login_rate_limit.py trusted a client-supplied X-Forwarded-For header unconditionally, with no trusted-proxy allowlist. Any client could reset its own login-brute-force bucket on every request just by sending a fresh XFF value, defeating the 10-attempts/15-minute limit entirely on any deployment where the header isn't stripped/overwritten at the edge. - rate_limit.py did the opposite: it ignored X-Forwarded-For entirely and always used request.client.host, which -- once actually behind a reverse proxy/load balancer in production -- is the proxy's own IP for every request, collapsing the general 120 req/min limit into one shared bucket for every real client. New app/core/client_ip.py::get_client_ip() is now shared by both: - Only honors X-Forwarded-For when the immediate TCP peer is in the new TRUSTED_PROXY_CIDRS setting (comma-separated CIDRs, empty by default -- so XFF is never trusted out of the box). - Walks the header right-to-left and returns the first hop that isn't itself a trusted proxy, so an attacker can't defeat this by prepending a fake IP before the real proxy's hop. Added tests/test_client_ip.py covering: XFF ignored with no trusted proxies configured, XFF ignored from an untrusted peer, XFF honored from a trusted peer, correct hop selection through chained trusted proxies, and that a spoofed prefix hop from an attacker connecting directly to a trusted proxy is still correctly bypassed in favor of their real IP. Verified against a live docker-compose stack: with uvicorn's own loopback proxy-trust explicitly disabled (--forwarded-allow-ips=""), hammering /api/auth/login with a different spoofed X-Forwarded-For on every request no longer resets the bucket -- the 11th attempt within the window correctly returns 429, and Redis shows a single login_rl:127.0.0.1 key rather than 12 separate spoofed-IP keys. Fixes #26 --- api/app/app/config.py | 7 +++ api/app/app/core/client_ip.py | 63 +++++++++++++++++++++ api/app/app/core/login_rate_limit.py | 12 +--- api/app/app/core/rate_limit.py | 3 +- api/app/tests/test_client_ip.py | 85 ++++++++++++++++++++++++++++ 5 files changed, 159 insertions(+), 11 deletions(-) create mode 100644 api/app/app/core/client_ip.py create mode 100644 api/app/tests/test_client_ip.py diff --git a/api/app/app/config.py b/api/app/app/config.py index dd046e6..ce1648b 100644 --- a/api/app/app/config.py +++ b/api/app/app/config.py @@ -39,6 +39,13 @@ class Settings(BaseSettings): frontend_url: str = "http://localhost:5173" + # Comma-separated CIDRs (e.g. "10.0.0.0/8,172.16.0.0/12") of reverse + # proxies/load balancers allowed to set X-Forwarded-For. Left empty by + # default: a client-supplied XFF is never trusted unless the direct + # TCP peer is in this list, so rate limiting can't be bypassed by + # sending an arbitrary XFF header directly to an exposed API. + trusted_proxy_cidrs: str = "" + email_backend: Literal["console", "smtp"] = "console" smtp_host: str = "" smtp_port: int = 587 diff --git a/api/app/app/core/client_ip.py b/api/app/app/core/client_ip.py new file mode 100644 index 0000000..b35cb6c --- /dev/null +++ b/api/app/app/core/client_ip.py @@ -0,0 +1,63 @@ +"""Trusted-proxy-aware client IP resolution, shared by every rate limiter. + +X-Forwarded-For is attacker-controlled on any request that reaches this +process directly (no proxy in front, or the proxy isn't configured to +strip/overwrite it). Trusting it unconditionally lets a client reset its +own rate-limit bucket on every request just by sending a fresh header +value. We only honor it when the immediate TCP peer is a configured +trusted proxy, and even then only take the right-most hop that isn't +itself one of those proxies. +""" + +import ipaddress + +from fastapi import Request + +from app.config import settings + +_IPNetwork = ipaddress.IPv4Network | ipaddress.IPv6Network + + +def _trusted_networks() -> tuple[_IPNetwork, ...]: + # Not cached: settings.trusted_proxy_cidrs can change between tests, + # and parsing a short comma list is cheap enough to redo per call. + nets = [] + for raw in settings.trusted_proxy_cidrs.split(","): + raw = raw.strip() + if not raw: + continue + try: + nets.append(ipaddress.ip_network(raw, strict=False)) + except ValueError: + continue + return tuple(nets) + + +def _is_trusted(ip: str) -> bool: + try: + addr = ipaddress.ip_address(ip) + except ValueError: + return False + return any(addr in net for net in _trusted_networks()) + + +def get_client_ip(request: Request) -> str: + peer = request.client.host if request.client else "unknown" + + if not _trusted_networks() or not _is_trusted(peer): + # No trusted proxies configured, or this request didn't come + # from one: never honor a client-supplied X-Forwarded-For. + return peer + + xff = request.headers.get("x-forwarded-for") + if not xff: + return peer + + hops = [h.strip() for h in xff.split(",") if h.strip()] + # Walk right-to-left (closest to us first) and return the first hop + # that isn't itself one of our trusted proxies -- that's the real + # client, since each trusted proxy only ever appends its own address. + for hop in reversed(hops): + if not _is_trusted(hop): + return hop + return peer diff --git a/api/app/app/core/login_rate_limit.py b/api/app/app/core/login_rate_limit.py index d27b416..d9a41e2 100644 --- a/api/app/app/core/login_rate_limit.py +++ b/api/app/app/core/login_rate_limit.py @@ -2,6 +2,7 @@ from fastapi import HTTPException, Request, status +from app.core.client_ip import get_client_ip from app.core.redis import get_redis logger = logging.getLogger(__name__) @@ -11,17 +12,8 @@ LOGIN_RL_PREFIX = "login_rl" -def _client_ip(request: Request) -> str: - xff = request.headers.get("x-forwarded-for") - if xff: - first = xff.split(",", 1)[0].strip() - if first: - return first - return request.client.host if request.client else "unknown" - - def enforce_login_rate_limit(request: Request) -> None: - ip = _client_ip(request) + ip = get_client_ip(request) key = f"{LOGIN_RL_PREFIX}:{ip}" try: redis = get_redis() diff --git a/api/app/app/core/rate_limit.py b/api/app/app/core/rate_limit.py index 60bee80..a6aeb8e 100644 --- a/api/app/app/core/rate_limit.py +++ b/api/app/app/core/rate_limit.py @@ -5,6 +5,7 @@ from fastapi import Request, Response from starlette.middleware.base import BaseHTTPMiddleware +from app.core.client_ip import get_client_ip from app.core.redis import get_redis logger = logging.getLogger(__name__) @@ -31,7 +32,7 @@ async def dispatch( ) -> Response: try: redis = get_redis() - client_ip = request.client.host if request.client else "unknown" + client_ip = get_client_ip(request) key = f"{self.key_prefix}:{client_ip}" pipe = redis.pipeline() diff --git a/api/app/tests/test_client_ip.py b/api/app/tests/test_client_ip.py new file mode 100644 index 0000000..1d8687a --- /dev/null +++ b/api/app/tests/test_client_ip.py @@ -0,0 +1,85 @@ +"""get_client_ip must never trust X-Forwarded-For from a client that +isn't a configured trusted proxy -- otherwise rate limiting is trivially +bypassed by sending a fresh header value on every request.""" + +from typing import Any, cast + +import pytest +from fastapi import Request + +from app.config import settings +from app.core.client_ip import get_client_ip + + +class FakeClient: + def __init__(self, host: str) -> None: + self.host = host + + +class FakeRequest: + def __init__(self, peer: str, headers: dict[str, str] | None = None) -> None: + self.client = FakeClient(peer) + self.headers = headers or {} + + +def _req(peer: str, headers: dict[str, str] | None = None) -> Request: + return cast(Request, FakeRequest(peer, headers)) + + +@pytest.fixture(autouse=True) +def _reset_trusted_proxies(monkeypatch: Any) -> Any: + monkeypatch.setattr(settings, "trusted_proxy_cidrs", "") + yield + + +def test_xff_ignored_when_no_trusted_proxies_configured() -> None: + req = _req("203.0.113.9", {"x-forwarded-for": "1.2.3.4"}) + assert get_client_ip(req) == "203.0.113.9" + + +def test_xff_ignored_when_peer_is_not_a_trusted_proxy(monkeypatch: Any) -> None: + monkeypatch.setattr(settings, "trusted_proxy_cidrs", "10.0.0.0/8") + # Direct, untrusted client spoofing the header -- must be ignored. + req = _req("203.0.113.9", {"x-forwarded-for": "9.9.9.9"}) + assert get_client_ip(req) == "203.0.113.9" + + +def test_xff_honored_when_peer_is_a_trusted_proxy(monkeypatch: Any) -> None: + monkeypatch.setattr(settings, "trusted_proxy_cidrs", "10.0.0.0/8") + req = _req("10.0.0.5", {"x-forwarded-for": "198.51.100.7"}) + assert get_client_ip(req) == "198.51.100.7" + + +def test_xff_right_most_untrusted_hop_used_with_chained_proxies( + monkeypatch: Any, +) -> None: + monkeypatch.setattr(settings, "trusted_proxy_cidrs", "10.0.0.0/8") + # Real client, then two trusted proxy hops appended their own IPs. + # Walking right-to-left skips both trusted hops and lands on the + # real client at the left. + req = _req( + "10.0.0.5", + {"x-forwarded-for": "198.51.100.7, 10.0.0.1, 10.0.0.2"}, + ) + assert get_client_ip(req) == "198.51.100.7" + + +def test_attacker_cannot_spoof_a_fake_prefix_hop(monkeypatch: Any) -> None: + monkeypatch.setattr(settings, "trusted_proxy_cidrs", "10.0.0.0/8") + # An attacker connecting directly to our trusted reverse proxy sends + # X-Forwarded-For: 6.6.6.6 (trying to impersonate a different IP). + # A correctly configured proxy *appends* the real connecting IP + # rather than trusting/replacing the client's value, so the header + # we actually receive is "6.6.6.6, ". Walking + # right-to-left must land on the attacker's real IP, not their + # spoofed prefix. + req = _req("10.0.0.5", {"x-forwarded-for": "6.6.6.6, 198.51.100.42"}) + assert get_client_ip(req) == "198.51.100.42" + + +def test_no_xff_header_falls_back_to_peer_even_when_trusted( + monkeypatch: Any, +) -> None: + monkeypatch.setattr(settings, "trusted_proxy_cidrs", "10.0.0.0/8") + req = _req("10.0.0.5") + assert get_client_ip(req) == "10.0.0.5"