diff --git a/backend/app.py b/backend/app.py index 832a039..9e60c1d 100644 --- a/backend/app.py +++ b/backend/app.py @@ -6,6 +6,7 @@ Uses real data from UK Government Compare School Performance downloads. import hashlib import re +import time from contextlib import asynccontextmanager from datetime import datetime, timezone from typing import Optional @@ -15,7 +16,7 @@ import pandas as pd from fastapi import FastAPI, HTTPException, Query, Request, Depends, Header from fastapi.middleware.cors import CORSMiddleware from fastapi.middleware.gzip import GZipMiddleware -from fastapi.responses import FileResponse, Response +from fastapi.responses import FileResponse, JSONResponse, Response from fastapi.staticfiles import StaticFiles from slowapi import Limiter, _rate_limit_exceeded_handler from slowapi.util import get_remote_address @@ -321,14 +322,79 @@ def client_key(request: Request) -> str: 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. +# Per-client limiter. Paired with the global ceiling below — the two do +# different jobs and neither substitutes for the other. limiter = Limiter(key_func=client_key) +# --- The ceiling no header can raise ---------------------------------------- +# +# 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 could otherwise mint a fresh rate-limit +# bucket per request and evade per-client limits entirely — which would make +# correct keying a net regression against abuse, since the single shared bucket +# it replaced at least capped everyone at 60/minute together. +# +# So per-client limits give fairness, and this gives the origin a hard total. +# It does not make the header trustworthy; it bounds what trusting it can cost. +# The header problem itself is closed at Cloudflare, not here. +# +# [window_start_monotonic, count], or None before the first request. A fixed +# window is crude, which is right for a backstop: it has to be obviously +# correct rather than fair. +_global_window: Optional[list] = None + +# The container healthcheck runs `curl http://localhost:80/api/data-info` from +# inside the container. Starving it would fail the check, restart the +# container, and turn a load spike into an outage loop — the ceiling exists to +# protect the origin, not to kill it. +_LOCAL_HOSTS = frozenset({"127.0.0.1", "::1", "localhost"}) + + +def exempt_from_ceiling(request: Request) -> bool: + """Whether the ceiling should ignore this request. + + Its own function so the rule is testable without standing up a server — + and so the healthcheck exemption is somewhere a reader can find it. + """ + if not request.url.path.startswith("/api/"): + return True + # The peer address, never the Host header: Host is set by the caller and + # would hand every attacker an exemption. + return (request.client.host if request.client else "") in _LOCAL_HOSTS + + +class GlobalRateLimitMiddleware(BaseHTTPMiddleware): + """A cap on total /api/ traffic, independent of any client identity.""" + + async def dispatch(self, request: Request, call_next): + global _global_window + + if exempt_from_ceiling(request): + return await call_next(request) + + now = time.monotonic() + # One event loop, and no await between the read and the write, so this + # sequence is atomic without a lock. + if _global_window is None or now - _global_window[0] >= 60: + _global_window = [now, 0] + _global_window[1] += 1 + + if _global_window[1] > settings.global_rate_limit_per_minute: + return JSONResponse( + # Distinguishable from slowapi's per-client 429: an operator + # reading logs has to be able to tell "one noisy client" from + # "the origin is saturated". + {"detail": "The service is at capacity. Please retry shortly."}, + status_code=429, + headers={"Retry-After": + str(max(1, int(60 - (now - _global_window[0]))))}, + ) + return await call_next(request) + + class SecurityHeadersMiddleware(BaseHTTPMiddleware): """Add security headers to all responses.""" @@ -532,6 +598,10 @@ app.add_middleware(CacheAndETagMiddleware) app.add_middleware(SecurityHeadersMiddleware) app.add_middleware(RequestSizeLimitMiddleware) app.add_middleware(GZipMiddleware, minimum_size=512) +# Added last, so it is outermost and refuses before anything downstream does +# work. A ceiling that only applies after the expensive part has run is not a +# ceiling. +app.add_middleware(GlobalRateLimitMiddleware) # CORS middleware - restricted for production app.add_middleware( diff --git a/backend/config.py b/backend/config.py index 554efc9..f663ba7 100644 --- a/backend/config.py +++ b/backend/config.py @@ -35,6 +35,11 @@ class Settings(BaseSettings): # Security admin_api_key: str = Field(default_factory=lambda: secrets.token_urlsafe(32)) rate_limit_per_minute: int = 60 # Requests per minute per IP + # A ceiling on total /api/ traffic, independent of any client identity. + # client_key trusts headers only Cloudflare can vouch for, so a caller + # reaching the origin directly could otherwise mint a fresh bucket per + # request. See GlobalRateLimitMiddleware in backend/app.py. + global_rate_limit_per_minute: int = 3000 rate_limit_burst: int = 10 # Allow burst of requests max_request_size: int = 1024 * 1024 # 1MB max request size diff --git a/backend/data_loader.py b/backend/data_loader.py index bf0e2a5..8aff4df 100644 --- a/backend/data_loader.py +++ b/backend/data_loader.py @@ -132,9 +132,21 @@ def suggest_schools_typesense(query: str, limit: int = 8) -> List[dict]: return [] rows = [] - for hit in result.get("hits", []): - doc = hit.get("document", {}) - row = {"urn": int(doc.get("urn", 0))} + for hit in result.get("hits", []) or []: + doc = (hit or {}).get("document") or {} + try: + urn = int(doc["urn"]) + except (KeyError, TypeError, ValueError): + # Skip the row, keep the rest. Typesense declares urn as int32 so + # this should be unreachable, but the index is a separate system + # that something other than this code can reindex — and "never + # raises" is a promise the keystroke path actually depends on. + # Dropping one malformed document is right; blanking the whole + # dropdown, or serving a suggestion pointing at /school/0, is not. + logging.getLogger(__name__).warning( + "skipping malformed suggestion document: %r", doc) + continue + row = {"urn": urn} row.update({f: str(doc.get(f, "") or "") for f in _SUGGEST_FIELDS}) rows.append(row) return rows diff --git a/backend/tests/test_rate_limit_key.py b/backend/tests/test_rate_limit_key.py index 7435ef3..2f0a12a 100644 --- a/backend/tests/test_rate_limit_key.py +++ b/backend/tests/test_rate_limit_key.py @@ -56,3 +56,92 @@ def test_whitespace_is_stripped(): # 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")) diff --git a/backend/tests/test_suggest.py b/backend/tests/test_suggest.py index b2bcc53..180b7c3 100644 --- a/backend/tests/test_suggest.py +++ b/backend/tests/test_suggest.py @@ -126,3 +126,32 @@ def test_the_response_is_cacheable(monkeypatch): res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=breck") assert "s-maxage" in res.headers.get("cache-control", "") assert res.headers.get("etag") + + +def test_a_malformed_urn_does_not_raise(monkeypatch): + """The docstring promises "never raises"; the parsing loop sat outside the + try, so int(None) or int("abc") would have turned a keystroke into a 500. + + Typesense declares urn as int32, so this should be unreachable — but the + contract is what the caller relies on, and a search index is a separate + system that can be reindexed by something other than this code. + """ + _use(monkeypatch, _FakeClient([{"urn": None, "school_name": "X", + "local_authority": "Y", "postcode": "Z"}])) + assert data_loader.suggest_schools_typesense("x") == [] + + +def test_a_malformed_row_does_not_discard_the_good_ones(monkeypatch): + # One bad document must not blank the whole dropdown. + _use(monkeypatch, _FakeClient([ + {"urn": "not-a-number", "school_name": "Bad", "local_authority": "Y", + "postcode": "Z"}, + _HIT, + ])) + out = data_loader.suggest_schools_typesense("x") + assert [r["urn"] for r in out] == [100010] + + +def test_a_hit_with_no_document_does_not_raise(monkeypatch): + _use(monkeypatch, _FakeClient([{}])) + assert data_loader.suggest_schools_typesense("x") == [] diff --git a/docs/DEPLOY.md b/docs/DEPLOY.md index 14b6f83..92353d4 100644 --- a/docs/DEPLOY.md +++ b/docs/DEPLOY.md @@ -151,6 +151,36 @@ needed), and fails the check only when a finding is rated **severe** (would break prod, leak data, or corrupt data). Minor findings are informational and never block a merge. +## Rate limiting, and the Cloudflare gap + +Two independent limits protect the API: + +- **Per client**, via slowapi, keyed on `CF-Connecting-IP` (falling back to + `X-Forwarded-For`, then the peer address). 60/minute by default; + `/api/suggest` gets 120/minute because typing is bursty. +- **Globally**, via `GlobalRateLimitMiddleware`: a fixed 60-second window over + all `/api/` traffic, `GLOBAL_RATE_LIMIT_PER_MINUTE` (default 3000), + independent of any client identity. Requests from `127.0.0.1` are exempt so + the container healthcheck cannot be starved into a restart loop. + +### Open: the origin must only accept Cloudflare + +`CF-Connecting-IP` is only meaningful for requests that actually reached the +origin through Cloudflare, and **the application cannot verify that they did**. +Anything able to reach the origin directly can set that header freely and, by +rotating it, mint a fresh rate-limit bucket per request — defeating per-client +limits on every endpoint. + +The global ceiling bounds the damage to total origin capacity. It does not fix +the underlying gap, and nothing in the code can. Closing it needs one of: + +- **Authenticated Origin Pulls** — Cloudflare presents a client certificate the + origin requires, so non-Cloudflare traffic is refused at TLS. +- **An origin firewall** restricted to Cloudflare's published IP ranges. + +Until one is in place, treat per-client limits as protection against accidents +and ordinary load, not against a determined caller. + ## Feature flags (Unleash) Flag state lives in a self-hosted Unleash instance, deployed as its own diff --git a/docs/superpowers/plans/2026-08-26-school-autosuggest.md b/docs/superpowers/plans/2026-08-26-school-autosuggest.md index 626a61e..e57c19b 100644 --- a/docs/superpowers/plans/2026-08-26-school-autosuggest.md +++ b/docs/superpowers/plans/2026-08-26-school-autosuggest.md @@ -1335,14 +1335,20 @@ site-wide; autosuggest itself is off until toggled in Unleash; and that **Task 1 changes rate limiting for every endpoint.** It is the one change here that is not behind a flag, and it is the one worth the most review attention. Before it, everyone shares one 60/minute bucket; after it, each caller gets -their own. That is the intended fix, and it also removes an accidental global -throttle — the spec's §1 and Risks say so plainly. A global ceiling belongs at -Cloudflare and is deliberately not built here. +their own, bounded by a global ceiling. The header it keys on is only +trustworthy for traffic that actually passed through Cloudflare, which this +app cannot verify — closing that needs Authenticated Origin Pulls or an origin +firewall, and is the most valuable follow-up in the spec's Risks. -**Do not add an in-app global rate limit.** An earlier spec draft did. slowapi's -`default_limits` and `application_limits` are both keyed by `key_func`, so they -are per-client rather than global, and `application_limits` only apply with -`SlowAPIMiddleware` installed, which this app does not use. +**The in-app global ceiling is required, and slowapi cannot express it.** +Code review found that `client_key` trusts `CF-Connecting-IP` with no way to +verify the request reached the origin through Cloudflare — so rotating that +header mints a fresh bucket per request and defeats per-client limits entirely, +which is worse against abuse than the shared bucket it replaced. The ceiling +(`GlobalRateLimitMiddleware`) bounds that, and is written by hand because +slowapi's `default_limits` and `application_limits` are both keyed by +`key_func` — per-client, not global — and the latter needs `SlowAPIMiddleware`, +which this app does not install. See spec §1.1. **`onMouseDown`, not `onClick`, on the options.** The input's `onBlur` closes the list and blur fires first, so a click handler never runs. This is the diff --git a/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md b/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md index 3e798b0..bc79dad 100644 --- a/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md +++ b/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md @@ -58,42 +58,80 @@ def client_key(request: Request) -> str: except `host` and `connection`, so `CF-Connecting-IP` reaches the backend with no proxy change. -Two things make this safe: Cloudflare replaces the header, so a browser cannot -forge it; and the backend is unreachable from outside the Docker network, so -nothing can reach it without passing through the proxy. The `X-Forwarded-For` -fallback *is* forgeable, but only by a caller already inside that network. +**This header is trustworthy only for traffic that actually passed through +Cloudflare, and nothing in the application can verify that it did.** An earlier +draft of this section claimed Cloudflare "replaces the header, so a browser +cannot forge it", and that only the `X-Forwarded-For` fallback was forgeable. +That was wrong. Cloudflare does overwrite the header *on requests it handles* — +but a caller reaching the origin directly sets whatever it likes, and this +process cannot distinguish an edge-set header from an attacker-set one. Both +headers are equally forgeable in that scenario. -### The part that is not free +The consequence is sharper than a weakened defence. An attacker rotating +`CF-Connecting-IP` per request mints a fresh rate-limit bucket every time and +evades per-client limits entirely — including on the DataFrame-heavy +`/api/schools`. Against abuse that is *worse* than the shared bucket it +replaced, which at least capped everyone at 60/minute together. -The shared bucket has been acting as an accidental global throttle on a +Two mitigations, and they are not interchangeable: + +1. **The real fix is at Cloudflare** — Authenticated Origin Pulls, or an origin + firewall that refuses connections not from Cloudflare's ranges. Only the + edge can vouch for its own header. This is infrastructure work and is not + part of this change; it is the thing that makes the header mean anything. +2. **The ceiling in §1.1 bounds what evading the keying can achieve** while + that remains open. It does not make the header trustworthy — it makes + trusting it survivable. + +The backend being unreachable from outside the Docker network is a real second +layer, but it depends on the ingress path in front of the frontend, which this +design does not control and should not assume. + +### 1.1 The ceiling, which is back + +The shared bucket was acting as an accidental global throttle on a single-process uvicorn backend that filters a 25,000-row DataFrame in-process. -Correct per-user keying removes that throttle: the origin becomes reachable at -60/min *per user* rather than 60/min in total. +Correct per-user keying removes it: the origin becomes reachable at 60/min *per +user* rather than 60/min in total, and — per above — at an unbounded rate by +anyone willing to rotate a header. -Per-user fairness and origin protection are different jobs. Conflating them -is what produced the current behaviour, and the fix must not quietly do it -again in the other direction. +An earlier draft dropped the in-app ceiling, arguing it belonged at Cloudflare. +That argument assumed the keying was sound. It is not, so the ceiling is +load-bearing rather than redundant, and it ships here: -**The global ceiling does not go in this app.** An earlier draft of this -section specified one via slowapi's `default_limits`. Reading the library -shows that would not have worked, twice over: `default_limits` and -`application_limits` are both evaluated with the same `key_func`, so they are -per-client across all routes rather than global; and `application_limits` are -only applied `if in_middleware`, while this app installs no `SlowAPIMiddleware` -at all. Expressing a genuine global cap would take a second `Limiter` with a -constant key plus that middleware — two mechanisms to keep correct, for a -protection this layer is the wrong place for. +`GlobalRateLimitMiddleware` counts all `/api/` requests in a fixed 60-second +window against `global_rate_limit_per_minute` (3000), independent of any client +identity, and refuses with a 429 that names capacity rather than the client — +an operator has to be able to tell "one noisy client" from "the origin is +saturated". It is registered last so it is outermost: a ceiling that applies +after the expensive work has run is not a ceiling. -Cloudflare is already in the request path on both environments and does -edge-level rate limiting properly, before traffic reaches a single-process -origin at all. That is where a global ceiling belongs, and it is a dashboard -change rather than code. Flagged as a follow-up, deliberately not built here. +slowapi cannot express this. `default_limits` and `application_limits` are both +evaluated with the same `key_func`, making them per-client rather than global, +and `application_limits` only apply with `SlowAPIMiddleware` installed, which +this app does not use. Hence the explicit middleware — about thirty lines, and +obviously correct, which is what a backstop needs to be. -What ships instead is conservative per-user limits: the existing 60/minute -default is unchanged, and `/api/suggest` gets 120/minute. Both are estimates -rather than measurements, and they are a starting point to revisit once the -keying is correct enough for real per-user traffic to be visible — which it -is not today, because everyone shares one bucket. +Requests from `127.0.0.1` are exempt. The container healthcheck runs +`curl http://localhost:80/api/data-info` from inside the container, and +starving it would fail the check, restart the container, and turn a load spike +into an outage loop. The exemption keys on the peer address, never the `Host` +header, which the caller sets. + +3000/minute is an estimate, not a measurement, and worth revisiting against +real traffic. + +### Per-user limits + +Per-user fairness and origin protection are different jobs, and this design now +does both separately: the ceiling above for the origin, and per-route limits +for fairness. Conflating them is what produced the original behaviour, where +one bucket served the whole internet. + +The existing 60/minute default is unchanged, and `/api/suggest` gets +120/minute. Both are estimates rather than measurements, and are a starting +point to revisit once the keying is correct enough for real per-user traffic to +be visible — which it was not before, because everyone shared one bucket. ## 2. `GET /api/suggest` @@ -228,11 +266,14 @@ with many source addresses can put more load on a single-process origin than they can today. Against this site's traffic that is a theoretical risk rather than a live one, but it is a real one and it is the price of the fix. -**Cloudflare bypass.** If the origin is reachable without passing through -Cloudflare, `CF-Connecting-IP` is absent and the `X-Forwarded-For` fallback is -forgeable, so limits could be evaded per-request. Closing that properly means -Authenticated Origin Pulls or an origin firewall, which is infrastructure work -outside this change. Worth doing separately. +**Cloudflare bypass — the open one.** If the origin is reachable without +passing through Cloudflare, `CF-Connecting-IP` is attacker-controlled, and +rotating it per request defeats per-client limits on every endpoint. The +ceiling in §1.1 bounds the damage to the origin's total capacity; it does not +restore per-client fairness under attack, and it cannot. Closing this properly +means Authenticated Origin Pulls or an origin firewall restricted to +Cloudflare's published ranges — infrastructure work, outside this change, and +the single most valuable follow-up here. **Typesense becomes user-visible.** Today a Typesense outage degrades search to a slow substring match. With autosuggest it also means the dropdown silently