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"