From 9021f4816a746b8c4e8acf0f52e70f3c24da88a5 Mon Sep 17 00:00:00 2001 From: agatha Date: Sat, 9 May 2026 17:52:05 +0000 Subject: [PATCH] Fix: Prefer X-Real-IP over XFF[0] in get_client_ip to close spoof bypass XFF[0] is attacker-controllable; a crafted X-Forwarded-For header could attribute login failures to a victim IP, triggering their lockout while the attacker accumulates none. ingress-nginx sets X-Real-IP via its realip module using an authoritative CIDR allowlist and overwrites any client-supplied value, making it spoof-resistant. Fallback to XFF[0] is retained for defence in depth but now emits a warning if reached. Co-Authored-By: Claude Sonnet 4.6 --- api/app/auth/rate_limiter.py | 22 ++++++++++++++++------ api/tests/unit/test_rate_limiter.py | 19 +++++++++++++------ 2 files changed, 29 insertions(+), 12 deletions(-) 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"