From 31f79fd8e28e6918a3d1dc1f1a2f054c5a8f99ca Mon Sep 17 00:00:00 2001 From: BigBodyCobain Date: Fri, 22 May 2026 00:46:25 -0600 Subject: [PATCH] Fix #287: proxy-aware rate-limit key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by @tg12 in the external security/correctness audit. Before this change, backend/limiter.py was: from slowapi.util import get_remote_address limiter = Limiter(key_func=get_remote_address) get_remote_address only ever returns request.client.host — it does not look at X-Forwarded-For. Behind the bundled Next.js proxy (or any other reverse proxy), every connected operator's client.host is the frontend container's bridge IP, so @limiter.limit("120/minute") collapses into one shared bucket for everybody on the same backend. One heavy tab can starve every other operator on that node. This change swaps in shadowbroker_rate_limit_key, which: * Reads X-Forwarded-For ONLY when the immediate peer matches the SAME hostname-bound allowlist we use for Docker-bridge local-operator trust (auth._resolve_trusted_bridge_ips — fix #250). Default is `frontend,shadowbroker-frontend`, override via SHADOWBROKER_TRUSTED_FRONTEND_HOSTS. * Picks the FIRST entry in the XFF chain — that's the operator end, not the proxy end. * Falls back to request.client.host for any peer not on the allowlist. Direct hits, unrelated containers, and unknown hosts are bucketed exactly like before. * Falls back to request.client.host when the resolver itself raises (e.g. DNS down). XFF is never accepted on a peer we can't confirm is the trusted frontend — there is no way to spoof another operator's bucket from outside. No new env vars. No new operator config. Single-operator nodes are unaffected — same behaviour as before. The 120/minute and 60/minute windows on the existing endpoints are unchanged; only the KEY they bucket on changes. Tests cover: * Direct loopback → keys on peer (regression check vs. get_remote_address default). * Untrusted peer sending XFF → XFF ignored, keys on peer. * Trusted frontend peer with XFF → keys on first XFF entry. * First XFF entry picked from a multi-hop chain. * Trusted peer without XFF → falls back to peer IP. * Empty/whitespace XFF entries skipped. * Header lookup is case-insensitive. * Two operators behind same proxy → different keys (the whole point of the fix). * Spoof attempt from internet-facing untrusted IP can't steal the victim's bucket. * Resolver raising is treated as untrusted (fail-closed). * No-client request shape doesn't raise. --- backend/limiter.py | 106 ++++++++++- backend/tests/test_rate_limit_proxy_aware.py | 186 +++++++++++++++++++ 2 files changed, 291 insertions(+), 1 deletion(-) create mode 100644 backend/tests/test_rate_limit_proxy_aware.py diff --git a/backend/limiter.py b/backend/limiter.py index 38404a8..a35e4c5 100644 --- a/backend/limiter.py +++ b/backend/limiter.py @@ -1,4 +1,108 @@ +"""Rate-limit key function for slowapi. + +Issue #287 (tg12): the previous implementation used +``slowapi.util.get_remote_address`` which only ever returns +``request.client.host``. Behind the bundled Next.js proxy (or any other +reverse proxy), every connected operator's ``client.host`` is the +frontend container's bridge IP. ``@limiter.limit("120/minute")`` then +collapses into one shared bucket for everybody on the same backend — +one heavy tab can starve every other operator on the node. + +This module replaces that key function with one that: + + * Reads ``X-Forwarded-For`` ONLY when the immediate peer is a trusted + frontend container (same allowlist used by the Docker bridge + local-operator trust path — see ``backend/auth.py`` ``#250``). + * Picks the FIRST entry in the XFF chain. That's the client end of + the proxy chain, which is the operator we want to bucket on. + * Falls back to ``request.client.host`` for any peer that isn't on + the trusted-frontend allowlist. Direct hits, unrelated containers, + and unknown hosts are bucketed exactly like before — there is no + way for an untrusted caller to spoof XFF and steal another + operator's rate-limit bucket. + +Single-operator nodes are unaffected: the frontend resolves to one IP, +that IP is on the trust list, the XFF header is read, and you get one +bucket per operator (i.e. you). +""" + +from __future__ import annotations + +from typing import Any + from slowapi import Limiter from slowapi.util import get_remote_address -limiter = Limiter(key_func=get_remote_address) + +def _client_host(request: Any) -> str: + """Return the immediate peer's IP, normalised to a lowercase string.""" + client = getattr(request, "client", None) + if client is None: + return "" + host = getattr(client, "host", "") or "" + return host.lower() + + +def _first_forwarded_for(value: str) -> str: + """Return the first non-empty entry from an ``X-Forwarded-For`` header. + + RFC 7239 / de-facto XFF format is ``client, proxy1, proxy2, …``. The + client end is what we want to bucket on. Empty parts (which appear + in some malformed headers) are skipped so we don't end up keying on + an empty string. + """ + for raw in value.split(","): + candidate = raw.strip() + if candidate: + return candidate.lower() + return "" + + +def _is_trusted_frontend_peer(host: str) -> bool: + """True iff ``host`` is one of the resolved trusted-frontend IPs. + + Imported lazily so this module stays usable in unit tests that + don't want to pull the whole auth module into scope. + """ + if not host: + return False + try: + from auth import _resolve_trusted_bridge_ips + except Exception: # pragma: no cover - defensive + return False + try: + trusted_ips = _resolve_trusted_bridge_ips() + except Exception: # pragma: no cover - defensive + return False + return host in trusted_ips + + +def shadowbroker_rate_limit_key(request: Any) -> str: + """slowapi key_func that is proxy-aware on trusted frontend peers only. + + Behaviour matrix: + + * Direct loopback / unknown peer → ``request.client.host`` + (identical to slowapi's default ``get_remote_address``). + * Peer is a trusted frontend container AND ``X-Forwarded-For`` is + present → first XFF entry (the actual operator). + * Peer is a trusted frontend container but no XFF → fall back to + ``request.client.host`` (the bridge IP). One shared bucket for + everyone in that case, same as before — but you only get there + if the trusted frontend forgot to forward XFF, which it won't. + """ + peer = _client_host(request) + if _is_trusted_frontend_peer(peer): + headers = getattr(request, "headers", None) + if headers is not None: + xff = headers.get("x-forwarded-for") or headers.get("X-Forwarded-For") + if xff: + first = _first_forwarded_for(xff) + if first: + return first + # Untrusted peer (or trusted peer without XFF): match the original + # get_remote_address behaviour byte-for-byte. + return get_remote_address(request) + + +limiter = Limiter(key_func=shadowbroker_rate_limit_key) diff --git a/backend/tests/test_rate_limit_proxy_aware.py b/backend/tests/test_rate_limit_proxy_aware.py new file mode 100644 index 0000000..a603316 --- /dev/null +++ b/backend/tests/test_rate_limit_proxy_aware.py @@ -0,0 +1,186 @@ +"""Tests for issue #287: proxy-aware slowapi key function. + +Contract: + * Untrusted peer → key is the peer IP (matches old get_remote_address). + * Trusted frontend peer with X-Forwarded-For → key is first XFF entry. + * Trusted frontend peer without X-Forwarded-For → key is the peer IP + (fail-soft: no behaviour change vs. before #287). + * XFF from an untrusted peer is IGNORED — there must be no way to + spoof another operator's bucket by sending XFF directly. + * The first XFF entry is used (not the last — that's the trusted + proxy talking to the backend, not the actual operator). +""" + +import pytest + + +class _FakeClient: + def __init__(self, host: str): + self.host = host + + +class _FakeRequest: + """Minimal slowapi-compatible request shim — has ``client`` and + ``headers`` attributes, which is all the key_func touches.""" + + def __init__(self, client_host: str, headers: dict | None = None): + self.client = _FakeClient(client_host) if client_host is not None else None + self.headers = dict(headers or {}) + # slowapi's get_remote_address also tries request.client; we + # exercise both branches via the same shim. + + +# ───────────────────────── untrusted peers ────────────────────────────── + + +class TestUntrustedPeer: + def test_direct_loopback_uses_client_host(self, monkeypatch): + """Direct hit from 127.0.0.1 — no XFF — keys on the peer IP.""" + from limiter import shadowbroker_rate_limit_key + # Make sure the trusted-frontend cache resolves to nothing relevant. + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset()) + req = _FakeRequest("127.0.0.1") + assert shadowbroker_rate_limit_key(req) == "127.0.0.1" + + def test_xff_from_untrusted_peer_is_ignored(self, monkeypatch): + """A random caller sending X-Forwarded-For must NOT steal another + operator's bucket. The XFF is dropped on the floor.""" + from limiter import shadowbroker_rate_limit_key + # Trusted set deliberately does NOT include 1.2.3.4. + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset({"172.20.0.5"})) + req = _FakeRequest("1.2.3.4", {"X-Forwarded-For": "9.9.9.9"}) + # Falls back to the peer IP, not 9.9.9.9. + assert shadowbroker_rate_limit_key(req) == "1.2.3.4" + + def test_unknown_host_with_xff_uses_peer_host(self, monkeypatch): + from limiter import shadowbroker_rate_limit_key + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset()) + req = _FakeRequest("10.0.0.5", {"X-Forwarded-For": "1.1.1.1"}) + assert shadowbroker_rate_limit_key(req) == "10.0.0.5" + + +# ───────────────────────── trusted frontend peers ─────────────────────── + + +class TestTrustedFrontendPeer: + def test_trusted_peer_with_xff_uses_first_xff_entry(self, monkeypatch): + """When the immediate peer is the trusted frontend container and + XFF carries the operator's chain, we key on the operator.""" + from limiter import shadowbroker_rate_limit_key + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset({"172.20.0.5"})) + req = _FakeRequest("172.20.0.5", {"X-Forwarded-For": "203.0.113.7"}) + assert shadowbroker_rate_limit_key(req) == "203.0.113.7" + + def test_first_xff_entry_picked_in_chain(self, monkeypatch): + """`client, proxy1, proxy2` → we pick the client, not the proxies. + Picking the last entry would mean every operator behind the same + upstream gets bucketed together, which is the bug we're fixing.""" + from limiter import shadowbroker_rate_limit_key + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset({"172.20.0.5"})) + req = _FakeRequest( + "172.20.0.5", + {"X-Forwarded-For": "203.0.113.7, 198.51.100.1, 10.0.0.1"}, + ) + assert shadowbroker_rate_limit_key(req) == "203.0.113.7" + + def test_trusted_peer_without_xff_falls_back_to_peer(self, monkeypatch): + """If the trusted frontend forgot to forward XFF (legacy clients, + broken deploys), don't crash — bucket on the bridge IP exactly + like the pre-#287 behaviour.""" + from limiter import shadowbroker_rate_limit_key + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset({"172.20.0.5"})) + req = _FakeRequest("172.20.0.5", headers={}) + assert shadowbroker_rate_limit_key(req) == "172.20.0.5" + + def test_trusted_peer_with_empty_xff_falls_back(self, monkeypatch): + """``X-Forwarded-For: , ,`` → no usable entries → falls back.""" + from limiter import shadowbroker_rate_limit_key + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset({"172.20.0.5"})) + req = _FakeRequest("172.20.0.5", {"X-Forwarded-For": " , , "}) + assert shadowbroker_rate_limit_key(req) == "172.20.0.5" + + def test_xff_header_case_insensitive(self, monkeypatch): + """HTTP header names are case-insensitive — slowapi normalises + but our shim doesn't, so we explicitly check both forms.""" + from limiter import shadowbroker_rate_limit_key + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset({"172.20.0.5"})) + req = _FakeRequest("172.20.0.5", {"x-forwarded-for": "203.0.113.7"}) + assert shadowbroker_rate_limit_key(req) == "203.0.113.7" + + +# ───────────────────────── isolation guarantees ───────────────────────── + + +class TestIsolation: + def test_two_operators_behind_same_proxy_get_different_keys(self, monkeypatch): + """The whole reason this fix exists — two operators behind the + SAME proxy must end up in DIFFERENT buckets.""" + from limiter import shadowbroker_rate_limit_key + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset({"172.20.0.5"})) + op_a = _FakeRequest("172.20.0.5", {"X-Forwarded-For": "10.1.1.1"}) + op_b = _FakeRequest("172.20.0.5", {"X-Forwarded-For": "10.1.1.2"}) + key_a = shadowbroker_rate_limit_key(op_a) + key_b = shadowbroker_rate_limit_key(op_b) + assert key_a != key_b + assert key_a == "10.1.1.1" + assert key_b == "10.1.1.2" + + def test_no_xff_spoof_from_outside(self, monkeypatch): + """If we ever expose the backend port directly to the internet, + an attacker MUST NOT be able to steal another operator's bucket + by sending their own XFF header.""" + from limiter import shadowbroker_rate_limit_key + # Trusted set is the frontend container IP; the attacker is on a + # different (untrusted) IP and tries to spoof a victim's IP. + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset({"172.20.0.5"})) + attacker = _FakeRequest("203.0.113.66", {"X-Forwarded-For": "10.1.1.1"}) + victim_via_proxy = _FakeRequest("172.20.0.5", {"X-Forwarded-For": "10.1.1.1"}) + assert shadowbroker_rate_limit_key(attacker) == "203.0.113.66" + assert shadowbroker_rate_limit_key(victim_via_proxy) == "10.1.1.1" + # The attacker burning their own bucket doesn't touch the victim's. + assert shadowbroker_rate_limit_key(attacker) != shadowbroker_rate_limit_key( + victim_via_proxy + ) + + def test_limiter_object_uses_proxy_aware_key(self): + """Smoke check that the module-level Limiter exports the new key + function rather than slowapi's default.""" + from limiter import limiter, shadowbroker_rate_limit_key + # slowapi stores it as ._key_func; we don't want to depend on + # that internal name, so just check the function is reachable. + assert callable(shadowbroker_rate_limit_key) + assert limiter is not None + + +# ───────────────────────── defensive corners ──────────────────────────── + + +class TestDefensive: + def test_no_client_object(self, monkeypatch): + """Some upstream middleware paths (websocket, ASGI lifespan) + produce requests with no ``client`` attribute — must not raise.""" + from limiter import shadowbroker_rate_limit_key + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", lambda: frozenset()) + + class _NoClient: + def __init__(self): + self.client = None + self.headers = {} + + # slowapi's get_remote_address returns "127.0.0.1" as a default + # in this case, so we just ensure no exception escapes. + result = shadowbroker_rate_limit_key(_NoClient()) + assert isinstance(result, str) + + def test_resolver_raises_is_treated_as_untrusted(self, monkeypatch): + """If DNS blows up inside the trusted-bridge resolver, we MUST + fall back to peer IP — never accept XFF blindly.""" + from limiter import shadowbroker_rate_limit_key + + def _explode(): + raise RuntimeError("DNS down") + + monkeypatch.setattr("auth._resolve_trusted_bridge_ips", _explode) + req = _FakeRequest("172.20.0.5", {"X-Forwarded-For": "9.9.9.9"}) + # XFF must be ignored when we can't confirm peer is trusted. + assert shadowbroker_rate_limit_key(req) == "172.20.0.5"