Spaces:
Sleeping
Fix rate limiting: key off X-Forwarded-For, not the proxy peer address
Browse filesLive-tested against the deployed HF Space right after the previous
merge: fired 25 rapid requests against a 20/60s limit and every single
one returned 200. Root cause: the Space is only reachable through
Hugging Face's own reverse proxy, so request.client.host (what the
rate limiter was keying on) is the proxy's own connecting address, not
the real caller - so requests either share one wrong bucket or never
accumulate against a consistent key at all.
This is also why the mocked integration tests never caught it:
Starlette's TestClient always presents a fixed synthetic peer address,
so it could validate the RateLimiter's own logic (already covered)
but not this deployment-specific IP-detection gap.
Fix: app/main.py now derives the client key via a new get_client_ip()
helper that prefers X-Forwarded-For's left-most entry - trustworthy
here since the container has no other ingress path. Added a
TDD-first regression test (test_rate_limit_keys_on_x_forwarded_for_not_the_proxy_peer)
that reproduces the bug against a fixed TestClient peer and proves
distinct forwarded clients are tracked independently.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184VjamhUurkWmPhkhEC7iC
- app/main.py +18 -1
- tests/test_api.py +30 -0
|
@@ -22,6 +22,23 @@ class ScrapeRequest(BaseModel):
|
|
| 22 |
url: HttpUrl
|
| 23 |
|
| 24 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 25 |
def create_app(
|
| 26 |
settings: Optional[Settings] = None,
|
| 27 |
browser_manager: Optional[BrowserManager] = None,
|
|
@@ -75,7 +92,7 @@ def create_app(
|
|
| 75 |
return {"status": "ok", "browser_ready": browser_manager.get_browser() is not None}
|
| 76 |
|
| 77 |
async def process_scrape(url: str, request: Request) -> dict:
|
| 78 |
-
client_ip = request
|
| 79 |
|
| 80 |
try:
|
| 81 |
if not rate_limiter.allow(client_ip):
|
|
|
|
| 22 |
url: HttpUrl
|
| 23 |
|
| 24 |
|
| 25 |
+
def get_client_ip(request: Request) -> str:
|
| 26 |
+
"""Identify the caller for rate limiting.
|
| 27 |
+
|
| 28 |
+
Prefers X-Forwarded-For's left-most (original client) entry over
|
| 29 |
+
request.client.host. The app is only reachable through a platform
|
| 30 |
+
reverse proxy (e.g. Hugging Face Spaces' gateway) which sets this
|
| 31 |
+
header on every request and is the only path to the container - so it
|
| 32 |
+
can be trusted here. request.client.host would instead reflect that
|
| 33 |
+
proxy's own connecting address, not the real caller, which silently
|
| 34 |
+
breaks per-client rate limiting.
|
| 35 |
+
"""
|
| 36 |
+
forwarded_for = request.headers.get("x-forwarded-for")
|
| 37 |
+
if forwarded_for:
|
| 38 |
+
return forwarded_for.split(",")[0].strip()
|
| 39 |
+
return request.client.host if request.client else "unknown"
|
| 40 |
+
|
| 41 |
+
|
| 42 |
def create_app(
|
| 43 |
settings: Optional[Settings] = None,
|
| 44 |
browser_manager: Optional[BrowserManager] = None,
|
|
|
|
| 92 |
return {"status": "ok", "browser_ready": browser_manager.get_browser() is not None}
|
| 93 |
|
| 94 |
async def process_scrape(url: str, request: Request) -> dict:
|
| 95 |
+
client_ip = get_client_ip(request)
|
| 96 |
|
| 97 |
try:
|
| 98 |
if not rate_limiter.allow(client_ip):
|
|
@@ -158,6 +158,36 @@ def test_rate_limit_blocks_after_the_configured_number_of_requests():
|
|
| 158 |
assert r3.status_code == 429
|
| 159 |
|
| 160 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 161 |
def test_api_key_required_when_configured():
|
| 162 |
settings = base_settings(api_key="secret123")
|
| 163 |
with make_client(settings=settings) as client:
|
|
|
|
| 158 |
assert r3.status_code == 429
|
| 159 |
|
| 160 |
|
| 161 |
+
def test_rate_limit_keys_on_x_forwarded_for_not_the_proxy_peer():
|
| 162 |
+
# The app is only reachable through Hugging Face's reverse proxy, so
|
| 163 |
+
# request.client.host is always the proxy's own connecting address, not
|
| 164 |
+
# the real caller. Without honoring X-Forwarded-For, every caller would
|
| 165 |
+
# share (or fragment across) the wrong bucket and the limit would never
|
| 166 |
+
# engage correctly - this reproduces that against a fixed TestClient
|
| 167 |
+
# peer address, where two distinct forwarded IPs must be tracked
|
| 168 |
+
# independently and a repeated one must accumulate hits.
|
| 169 |
+
settings = base_settings(rate_limit_requests=2, rate_limit_window_seconds=60, cache_ttl_seconds=0)
|
| 170 |
+
with make_client(settings=settings) as client:
|
| 171 |
+
a1 = client.get(
|
| 172 |
+
"/scrape", params={"url": "https://example.com/a1"}, headers={"X-Forwarded-For": "1.2.3.4"}
|
| 173 |
+
)
|
| 174 |
+
a2 = client.get(
|
| 175 |
+
"/scrape", params={"url": "https://example.com/a2"}, headers={"X-Forwarded-For": "1.2.3.4"}
|
| 176 |
+
)
|
| 177 |
+
a3 = client.get(
|
| 178 |
+
"/scrape", params={"url": "https://example.com/a3"}, headers={"X-Forwarded-For": "1.2.3.4"}
|
| 179 |
+
)
|
| 180 |
+
# A different forwarded client is not affected by 1.2.3.4's limit.
|
| 181 |
+
b1 = client.get(
|
| 182 |
+
"/scrape", params={"url": "https://example.com/b1"}, headers={"X-Forwarded-For": "5.6.7.8"}
|
| 183 |
+
)
|
| 184 |
+
|
| 185 |
+
assert a1.status_code == 200
|
| 186 |
+
assert a2.status_code == 200
|
| 187 |
+
assert a3.status_code == 429
|
| 188 |
+
assert b1.status_code == 200
|
| 189 |
+
|
| 190 |
+
|
| 191 |
def test_api_key_required_when_configured():
|
| 192 |
settings = base_settings(api_key="secret123")
|
| 193 |
with make_client(settings=settings) as client:
|