diff --git a/api/app/auth/rate_limiter.py b/api/app/auth/rate_limiter.py index fc7ba53..e4ad3d8 100644 --- a/api/app/auth/rate_limiter.py +++ b/api/app/auth/rate_limiter.py @@ -14,20 +14,30 @@ def get_client_ip( request: Request, trusted_networks: list[IPv4Network | IPv6Network], ) -> str: - """Return the resolved client IP, honouring X-Forwarded-For when the - TCP peer is a trusted upstream proxy. Falls back to the TCP peer address - when no trusted networks are configured or the peer is not in the list.""" + """Return the resolved client IP. + + Prefers X-Real-IP over X-Forwarded-For when the TCP peer is a trusted + proxy. ingress-nginx sets X-Real-IP via its realip module using an + authoritative CIDR allowlist; it overwrites any client-supplied value, so + it cannot be spoofed via XFF injection. XFF[0] is the fallback for paths + that lack nginx (none currently exist, but kept for defence in depth). + """ peer = request.client.host if request.client else "unknown" if trusted_networks and peer != "unknown": try: peer_addr = ipaddress.ip_address(peer) if any(peer_addr in net for net in trusted_networks): - xff = request.headers.get("X-Forwarded-For", "").split(",")[0].strip() - if xff: - return xff real_ip = request.headers.get("X-Real-IP", "").strip() if real_ip: return real_ip + # XFF[0] fallback — warn because this path should not be + # reached in production (nginx always sets X-Real-IP). + xff = request.headers.get("X-Forwarded-For", "").split(",")[0].strip() + if xff: + logger.warning( + "X-Real-IP absent from trusted peer %s; falling back to XFF[0]", peer + ) + return xff except ValueError: pass return peer diff --git a/api/tests/unit/test_rate_limiter.py b/api/tests/unit/test_rate_limiter.py index 033f9b8..b296b98 100644 --- a/api/tests/unit/test_rate_limiter.py +++ b/api/tests/unit/test_rate_limiter.py @@ -80,10 +80,17 @@ def test_get_client_ip_no_trusted_networks_returns_peer(): assert get_client_ip(req, []) == "203.0.113.1" -def test_get_client_ip_trusted_peer_uses_xff(): - req = make_request("10.0.0.1", {"X-Forwarded-For": "203.0.113.5"}) +def test_get_client_ip_trusted_peer_uses_real_ip(): + req = make_request("10.0.0.1", {"X-Real-IP": "203.0.113.9"}) nets = [ipaddress.ip_network("10.0.0.0/8")] - assert get_client_ip(req, nets) == "203.0.113.5" + assert get_client_ip(req, nets) == "203.0.113.9" + + +def test_get_client_ip_real_ip_wins_over_xff(): + # Regression: spoofed XFF must not override nginx-set X-Real-IP. + req = make_request("10.0.0.1", {"X-Real-IP": "203.0.113.9", "X-Forwarded-For": "1.2.3.4"}) + nets = [ipaddress.ip_network("10.0.0.0/8")] + assert get_client_ip(req, nets) == "203.0.113.9" def test_get_client_ip_untrusted_peer_ignores_xff(): @@ -92,7 +99,7 @@ def test_get_client_ip_untrusted_peer_ignores_xff(): assert get_client_ip(req, nets) == "8.8.8.8" -def test_get_client_ip_trusted_peer_falls_back_to_real_ip(): - req = make_request("10.0.0.1", {"X-Real-IP": "203.0.113.9"}) +def test_get_client_ip_trusted_peer_falls_back_to_xff_when_no_real_ip(): + req = make_request("10.0.0.1", {"X-Forwarded-For": "203.0.113.5"}) nets = [ipaddress.ip_network("10.0.0.0/8")] - assert get_client_ip(req, nets) == "203.0.113.9" + assert get_client_ip(req, nets) == "203.0.113.5"