From 0fa1a292c76926cb04cd9bccc7ceec36caaaaad3 Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 20:56:32 +0100 Subject: [PATCH] fix(api): bound what a forged CF-Connecting-IP can buy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 82 ++++++++++++- backend/config.py | 5 + backend/data_loader.py | 18 ++- backend/tests/test_rate_limit_key.py | 89 ++++++++++++++ backend/tests/test_suggest.py | 29 +++++ docs/DEPLOY.md | 30 +++++ .../plans/2026-08-26-school-autosuggest.md | 20 ++-- .../2026-08-26-school-autosuggest-design.md | 109 ++++++++++++------ 8 files changed, 332 insertions(+), 50 deletions(-) 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