diff --git a/backend/app.py b/backend/app.py index d352de5..ab5a3e7 100644 --- a/backend/app.py +++ b/backend/app.py @@ -296,8 +296,36 @@ def clean_filter_values(series: pd.Series) -> list[str]: # SECURITY MIDDLEWARE & HELPERS # ============================================================================= -# Rate limiter -limiter = Limiter(key_func=get_remote_address) +def client_key(request: Request) -> str: + """The rate-limit bucket: the real caller, not the proxy in front of them. + + `get_remote_address` reads request.client.host. In staging and production + the backend has no published ports and sits on the internal network, so its + only caller is the Next proxy — meaning every browser user on the site + shared one bucket. Measured before this fix: 70 concurrent requests to + /api/schools returned 60 OK and 10 refused. + + CF-Connecting-IP first, because Cloudflare (in front of both environments) + sets it on every origin request and *overwrites* any client-supplied value, + which a parsed X-Forwarded-For chain does not guarantee. The XFF fallback is + forgeable, but only by a caller already inside the Docker network, which is + the one place nothing untrusted can reach. + """ + cf = request.headers.get("cf-connecting-ip") + if cf: + return cf.strip() + xff = request.headers.get("x-forwarded-for") + if xff: + return xff.split(",")[0].strip() + return get_remote_address(request) + + +# Rate limiter. No in-app global ceiling: slowapi's default_limits and +# application_limits are both keyed by key_func (so per-client, not global) +# and the latter only applies with SlowAPIMiddleware installed, which this app +# does not use. A global cap belongs at Cloudflare, which is already in the +# path. See the spec's §1 for why that is deliberate. +limiter = Limiter(key_func=client_key) class SecurityHeadersMiddleware(BaseHTTPMiddleware): diff --git a/backend/tests/test_rate_limit_key.py b/backend/tests/test_rate_limit_key.py new file mode 100644 index 0000000..7435ef3 --- /dev/null +++ b/backend/tests/test_rate_limit_key.py @@ -0,0 +1,58 @@ +"""The rate-limit bucket must be the caller, not the proxy in front of them. + +`get_remote_address` reads request.client.host. In staging and production the +backend has no published ports and its only caller is the Next proxy, so that +host is the Next container — one bucket for every browser user on the site. +Measured before this fix: 70 concurrent requests, 60 served and 10 refused. +""" + +from starlette.datastructures import Headers + +from backend.app import client_key + + +class _Req: + """Enough of a Request for the key function: headers and a client host.""" + + def __init__(self, headers: dict, host: str = "10.0.0.9"): + self.headers = Headers(headers) + self.client = type("C", (), {"host": host})() + self.scope = {"type": "http", "client": (host, 0), + "headers": [(k.lower().encode(), v.encode()) + for k, v in headers.items()]} + + +def test_cloudflare_header_wins(): + # Cloudflare sets CF-Connecting-IP and overwrites any client-supplied + # value, so it is trustworthy in a way a parsed XFF chain is not. + assert client_key(_Req({"cf-connecting-ip": "203.0.113.7"})) == "203.0.113.7" + + +def test_forwarded_for_is_the_fallback_and_takes_the_first_entry(): + # Left-most is the original client; everything after it is proxies. + assert client_key( + _Req({"x-forwarded-for": "203.0.113.7, 10.0.0.2"})) == "203.0.113.7" + + +def test_remote_address_is_the_last_resort(): + assert client_key(_Req({}, host="10.0.0.9")) == "10.0.0.9" + + +def test_cloudflare_header_beats_forwarded_for(): + key = client_key(_Req({"cf-connecting-ip": "203.0.113.7", + "x-forwarded-for": "198.51.100.1"})) + assert key == "203.0.113.7" + + +def test_two_callers_get_two_buckets(): + # The whole point: one user exhausting their limit must not refuse another. + a = client_key(_Req({"cf-connecting-ip": "203.0.113.7"})) + b = client_key(_Req({"cf-connecting-ip": "203.0.113.8"})) + assert a != b + + +def test_whitespace_is_stripped(): + # "a, b" split on comma leaves a leading space on every entry but the + # first; an unstripped key silently creates a second bucket per client. + assert client_key(_Req({"x-forwarded-for": " 203.0.113.7 ,10.0.0.2"})) \ + == "203.0.113.7"