mirror of
https://github.com/BigBodyCobain/Shadowbroker.git
synced 2026-05-28 10:01:31 +02:00
Fix #287: proxy-aware rate-limit key
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.
This commit is contained in:
BigBodyCobain
+105
-1
@@ -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)
|
||||
|
||||
@@ -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"
|
||||
Reference in New Issue
Block a user