PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m3s
PR Checks / Backend Smoke (pull_request) Successful in 8s
PR Checks / Build Backend (no push) (pull_request) Successful in 18s
PR Checks / Build Frontend (no push) (pull_request) Successful in 45s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 10s
Code review, both findings valid. The design doc claimed Cloudflare "replaces the header, so a browser cannot forge it", and that only the X-Forwarded-For fallback was forgeable. That is true only for traffic that actually passed through Cloudflare, and nothing in this process can verify that it did. Reaching the origin directly, both headers are equally attacker-controlled — and rotating CF-Connecting-IP mints a fresh rate-limit bucket per request, defeating per-client limits on every endpoint including the DataFrame-heavy /api/schools. Against abuse that is worse than the shared bucket it replaced, which at least capped everyone together. So the ceiling comes back. I dropped it earlier arguing it belonged at Cloudflare; that argument assumed the keying was sound, and it is not. GlobalRateLimitMiddleware counts all /api/ traffic in a fixed window against a total, independent of client identity, outermost so it refuses before any work happens. Written by hand because slowapi cannot express a global cap: default_limits and application_limits are both keyed by key_func, and the latter needs middleware this app does not install. It does not make the header trustworthy — it makes trusting it survivable. The real fix is Authenticated Origin Pulls or an origin firewall, now documented in DEPLOY.md as the open gap it is. 127.0.0.1 is exempt: the healthcheck curls localhost from inside the container, and starving it would restart the container and turn a load spike into an outage loop. Keyed on the peer address, never the Host header, which the caller sets. Second finding: suggest_schools_typesense promised "never raises" while the parsing loop sat outside the try, so int(None) on a malformed document would have made a keystroke a 500. The loop now skips bad rows rather than dropping the whole list — and a hit with no document no longer becomes a suggestion pointing at /school/0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
148 lines
5.7 KiB
Python
148 lines
5.7 KiB
Python
"""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"
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# The ceiling that header rotation cannot raise.
|
|
# ---------------------------------------------------------------------------
|
|
|
|
import pytest
|
|
from fastapi.testclient import TestClient
|
|
|
|
|
|
@pytest.fixture()
|
|
def api(monkeypatch):
|
|
from backend import app as app_module
|
|
from backend.config import settings
|
|
|
|
monkeypatch.setattr(settings, "global_rate_limit_per_minute", 5)
|
|
monkeypatch.setattr(app_module, "_global_window", None)
|
|
return TestClient(app_module.app, raise_server_exceptions=False)
|
|
|
|
|
|
|
|
def _ceiling_req(path: str, host: str):
|
|
"""Enough of a Request for exempt_from_ceiling: a path and a peer host."""
|
|
return type("R", (), {
|
|
"url": type("U", (), {"path": path})(),
|
|
"client": type("C", (), {"host": host})(),
|
|
})()
|
|
|
|
|
|
def _get(client, path="/api/flags", cf=None):
|
|
headers = {"cf-connecting-ip": cf} if cf else {}
|
|
return client.get(path, headers=headers)
|
|
|
|
|
|
def test_rotating_the_cloudflare_header_cannot_buy_unlimited_requests(api):
|
|
"""The attack the per-client keying opened up.
|
|
|
|
client_key trusts CF-Connecting-IP, and nothing in this process can tell an
|
|
edge-set header from an attacker-set one — that distinction can only be
|
|
made at Cloudflare, with Authenticated Origin Pulls or an origin firewall.
|
|
A caller reaching the origin directly can therefore mint a fresh
|
|
rate-limit bucket per request and evade per-client limits entirely.
|
|
|
|
Per-client fairness is still the right default; this is the backstop that
|
|
bounds what evading it can achieve. Without it, correct keying would be a
|
|
net regression against abuse compared with the shared bucket it replaced.
|
|
"""
|
|
codes = [_get(api, cf=f"203.0.113.{i}").status_code for i in range(8)]
|
|
assert codes.count(200) == 5
|
|
assert codes.count(429) == 3
|
|
|
|
|
|
def test_the_ceiling_says_which_limit_was_hit(api):
|
|
# Distinguishable from slowapi's per-client 429, or an operator reading
|
|
# logs cannot tell "one noisy client" from "the origin is saturated".
|
|
for i in range(5):
|
|
_get(api, cf=f"203.0.113.{i}")
|
|
refused = _get(api, cf="203.0.113.99")
|
|
assert refused.status_code == 429
|
|
assert "capacity" in refused.json()["detail"].lower()
|
|
assert refused.headers.get("retry-after")
|
|
|
|
|
|
def test_traffic_below_the_ceiling_is_untouched(api):
|
|
codes = [_get(api, cf=f"203.0.113.{i}").status_code for i in range(5)]
|
|
assert codes == [200] * 5
|
|
|
|
|
|
def test_the_container_healthcheck_is_exempt(api):
|
|
"""The healthcheck runs `curl http://localhost:80/api/data-info` inside the
|
|
container. If the ceiling could starve it, saturation would fail the
|
|
healthcheck, restart the container, and turn a load spike into an outage
|
|
loop — the ceiling has to protect the origin, not kill it.
|
|
"""
|
|
from backend.app import exempt_from_ceiling
|
|
|
|
assert exempt_from_ceiling(_ceiling_req("/api/data-info", "127.0.0.1"))
|
|
assert exempt_from_ceiling(_ceiling_req("/api/data-info", "::1"))
|
|
# Everyone else is counted.
|
|
assert not exempt_from_ceiling(_ceiling_req("/api/data-info", "10.0.0.9"))
|
|
|
|
|
|
def test_the_ceiling_ignores_non_api_paths():
|
|
# Sitemaps and robots.txt are served by this app too, and a crawler
|
|
# fetching them must not be refused because the API is busy.
|
|
from backend.app import exempt_from_ceiling
|
|
|
|
assert exempt_from_ceiling(_ceiling_req("/sitemap.xml", "10.0.0.9"))
|
|
assert exempt_from_ceiling(_ceiling_req("/robots.txt", "10.0.0.9"))
|