fix(api): rate-limit per caller, not per proxy
The limiter keyed on request.client.host, which in staging and prod is the Next container — the backend has no published ports and nothing else can reach it. So every browser user on the site shared one 60/minute bucket per route. Measured against staging: 70 concurrent requests to /api/schools returned exactly 60 OK and 10 refused, from one machine. CF-Connecting-IP first. Cloudflare fronts both environments and overwrites any client-supplied value, which a parsed X-Forwarded-For chain does not guarantee. The XFF fallback is forgeable only from inside the Docker network. Named rather than hidden: the shared bucket was an accidental global throttle on a single-process backend, and correct per-user keying removes it. A real global ceiling belongs at Cloudflare, which is already in the path; slowapi cannot express one without a second Limiter and middleware this app does not install. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
This commit is contained in:
1 parent
6e0a278340
commit
ff041544f2
2 files changed
+88
-2
No files matched your search
+30
-2
@@ -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):
|
||||
|
||||
@@ -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"
|
||||
Reference in new issue
Block a user