fix(api): bound what a forged CF-Connecting-IP can buy
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
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
This commit is contained in:
1 parent
d2115364ae
commit
0fa1a292c7
8 files changed
+332
-50
No files matched your search
+76
-6
@@ -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(
|
||||
|
||||
Reference in new issue
Block a user