Compare commits
5
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
d5a6db289d | ||
|
|
d8ccb5b733 | ||
|
|
0fa1a292c7 | ||
|
|
c3f044bd65 | ||
|
|
59265f78b6 |
No files matched your search
+76
-6
@@ -6,6 +6,7 @@ Uses real data from UK Government Compare School Performance downloads.
|
|||||||
|
|
||||||
import hashlib
|
import hashlib
|
||||||
import re
|
import re
|
||||||
|
import time
|
||||||
from contextlib import asynccontextmanager
|
from contextlib import asynccontextmanager
|
||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
from typing import Optional
|
from typing import Optional
|
||||||
@@ -15,7 +16,7 @@ import pandas as pd
|
|||||||
from fastapi import FastAPI, HTTPException, Query, Request, Depends, Header
|
from fastapi import FastAPI, HTTPException, Query, Request, Depends, Header
|
||||||
from fastapi.middleware.cors import CORSMiddleware
|
from fastapi.middleware.cors import CORSMiddleware
|
||||||
from fastapi.middleware.gzip import GZipMiddleware
|
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 fastapi.staticfiles import StaticFiles
|
||||||
from slowapi import Limiter, _rate_limit_exceeded_handler
|
from slowapi import Limiter, _rate_limit_exceeded_handler
|
||||||
from slowapi.util import get_remote_address
|
from slowapi.util import get_remote_address
|
||||||
@@ -321,14 +322,79 @@ def client_key(request: Request) -> str:
|
|||||||
return get_remote_address(request)
|
return get_remote_address(request)
|
||||||
|
|
||||||
|
|
||||||
# Rate limiter. No in-app global ceiling: slowapi's default_limits and
|
# Per-client limiter. Paired with the global ceiling below — the two do
|
||||||
# application_limits are both keyed by key_func (so per-client, not global)
|
# different jobs and neither substitutes for the other.
|
||||||
# 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.
|
|
||||||
limiter = Limiter(key_func=client_key)
|
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):
|
class SecurityHeadersMiddleware(BaseHTTPMiddleware):
|
||||||
"""Add security headers to all responses."""
|
"""Add security headers to all responses."""
|
||||||
|
|
||||||
@@ -532,6 +598,10 @@ app.add_middleware(CacheAndETagMiddleware)
|
|||||||
app.add_middleware(SecurityHeadersMiddleware)
|
app.add_middleware(SecurityHeadersMiddleware)
|
||||||
app.add_middleware(RequestSizeLimitMiddleware)
|
app.add_middleware(RequestSizeLimitMiddleware)
|
||||||
app.add_middleware(GZipMiddleware, minimum_size=512)
|
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
|
# CORS middleware - restricted for production
|
||||||
app.add_middleware(
|
app.add_middleware(
|
||||||
|
|||||||
@@ -35,6 +35,11 @@ class Settings(BaseSettings):
|
|||||||
# Security
|
# Security
|
||||||
admin_api_key: str = Field(default_factory=lambda: secrets.token_urlsafe(32))
|
admin_api_key: str = Field(default_factory=lambda: secrets.token_urlsafe(32))
|
||||||
rate_limit_per_minute: int = 60 # Requests per minute per IP
|
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
|
rate_limit_burst: int = 10 # Allow burst of requests
|
||||||
max_request_size: int = 1024 * 1024 # 1MB max request size
|
max_request_size: int = 1024 * 1024 # 1MB max request size
|
||||||
|
|
||||||
|
|||||||
+15
-3
@@ -132,9 +132,21 @@ def suggest_schools_typesense(query: str, limit: int = 8) -> List[dict]:
|
|||||||
return []
|
return []
|
||||||
|
|
||||||
rows = []
|
rows = []
|
||||||
for hit in result.get("hits", []):
|
for hit in result.get("hits", []) or []:
|
||||||
doc = hit.get("document", {})
|
doc = (hit or {}).get("document") or {}
|
||||||
row = {"urn": int(doc.get("urn", 0))}
|
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})
|
row.update({f: str(doc.get(f, "") or "") for f in _SUGGEST_FIELDS})
|
||||||
rows.append(row)
|
rows.append(row)
|
||||||
return rows
|
return rows
|
||||||
|
|||||||
@@ -56,3 +56,92 @@ def test_whitespace_is_stripped():
|
|||||||
# first; an unstripped key silently creates a second bucket per client.
|
# 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"})) \
|
assert client_key(_Req({"x-forwarded-for": " 203.0.113.7 ,10.0.0.2"})) \
|
||||||
== "203.0.113.7"
|
== "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"))
|
||||||
@@ -126,3 +126,32 @@ def test_the_response_is_cacheable(monkeypatch):
|
|||||||
res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=breck")
|
res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=breck")
|
||||||
assert "s-maxage" in res.headers.get("cache-control", "")
|
assert "s-maxage" in res.headers.get("cache-control", "")
|
||||||
assert res.headers.get("etag")
|
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") == []
|
||||||
+52
-3
@@ -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
|
**severe** (would break prod, leak data, or corrupt data). Minor findings are
|
||||||
informational and never block a merge.
|
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)
|
## Feature flags (Unleash)
|
||||||
|
|
||||||
Flag state lives in a self-hosted Unleash instance, deployed as its own
|
Flag state lives in a self-hosted Unleash instance, deployed as its own
|
||||||
@@ -176,11 +206,30 @@ registry is orphaned and nothing reads it.
|
|||||||
variable, and set `UNLEASH_URL` to `http://<UNLEASH_IP>:4242/api`.
|
variable, and set `UNLEASH_URL` to `http://<UNLEASH_IP>:4242/api`.
|
||||||
5. Redeploy the application stacks.
|
5. Redeploy the application stacks.
|
||||||
|
|
||||||
|
### Adding a flag to Unleash
|
||||||
|
|
||||||
|
**Unleash does not create flags by itself.** The SDK reads definitions from the
|
||||||
|
server and never registers anything, and metrics for a flag the server has
|
||||||
|
never heard of are discarded. So a flag declared in `backend/flags.py` will be
|
||||||
|
evaluated on every request, stay `False` forever, and never appear in the UI
|
||||||
|
until someone creates it there by hand.
|
||||||
|
|
||||||
|
For each flag in the registry, create one in Unleash with:
|
||||||
|
|
||||||
|
- **Name** — character for character what `backend/flags.py` declares.
|
||||||
|
snake_case, no hyphens or spaces. A typo produces a flag that looks correct
|
||||||
|
in the UI and is read by nothing.
|
||||||
|
- **Type** — Release. No strategies, constraints or variants: these are plain
|
||||||
|
on/off switches, by design.
|
||||||
|
|
||||||
### Turning a feature on
|
### Turning a feature on
|
||||||
|
|
||||||
Toggle the flag in the environment you want. Flags appear in the Unleash UI
|
Toggle the flag in the environment matching the stack you mean: **development**
|
||||||
after the backend has evaluated them once, so a newly declared flag shows up
|
for staging, **production** for prod. The token in each stack is scoped to one
|
||||||
shortly after the deploy that introduced it.
|
environment, so toggling the other one has no visible effect.
|
||||||
|
|
||||||
|
The SDK refreshes every 15 seconds, so the API reflects the change almost at
|
||||||
|
once; the pages follow on their own schedule, below.
|
||||||
|
|
||||||
A flip reaches school pages within about five minutes and place pages within
|
A flip reaches school pages within about five minutes and place pages within
|
||||||
the hour. Next's ISR does the propagating — it revalidates a route at the
|
the hour. Next's ISR does the propagating — it revalidates a route at the
|
||||||
|
|||||||
@@ -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
|
**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.
|
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
|
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
|
their own, bounded by a global ceiling. The header it keys on is only
|
||||||
throttle — the spec's §1 and Risks say so plainly. A global ceiling belongs at
|
trustworthy for traffic that actually passed through Cloudflare, which this
|
||||||
Cloudflare and is deliberately not built here.
|
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
|
**The in-app global ceiling is required, and slowapi cannot express it.**
|
||||||
`default_limits` and `application_limits` are both keyed by `key_func`, so they
|
Code review found that `client_key` trusts `CF-Connecting-IP` with no way to
|
||||||
are per-client rather than global, and `application_limits` only apply with
|
verify the request reached the origin through Cloudflare — so rotating that
|
||||||
`SlowAPIMiddleware` installed, which this app does not use.
|
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
|
**`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
|
the list and blur fires first, so a click handler never runs. This is the
|
||||||
|
|||||||
@@ -58,42 +58,80 @@ def client_key(request: Request) -> str:
|
|||||||
except `host` and `connection`, so `CF-Connecting-IP` reaches the backend with
|
except `host` and `connection`, so `CF-Connecting-IP` reaches the backend with
|
||||||
no proxy change.
|
no proxy change.
|
||||||
|
|
||||||
Two things make this safe: Cloudflare replaces the header, so a browser cannot
|
**This header is trustworthy only for traffic that actually passed through
|
||||||
forge it; and the backend is unreachable from outside the Docker network, so
|
Cloudflare, and nothing in the application can verify that it did.** An earlier
|
||||||
nothing can reach it without passing through the proxy. The `X-Forwarded-For`
|
draft of this section claimed Cloudflare "replaces the header, so a browser
|
||||||
fallback *is* forgeable, but only by a caller already inside that network.
|
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.
|
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
|
Correct per-user keying removes it: the origin becomes reachable at 60/min *per
|
||||||
60/min *per user* rather than 60/min in total.
|
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
|
An earlier draft dropped the in-app ceiling, arguing it belonged at Cloudflare.
|
||||||
is what produced the current behaviour, and the fix must not quietly do it
|
That argument assumed the keying was sound. It is not, so the ceiling is
|
||||||
again in the other direction.
|
load-bearing rather than redundant, and it ships here:
|
||||||
|
|
||||||
**The global ceiling does not go in this app.** An earlier draft of this
|
`GlobalRateLimitMiddleware` counts all `/api/` requests in a fixed 60-second
|
||||||
section specified one via slowapi's `default_limits`. Reading the library
|
window against `global_rate_limit_per_minute` (3000), independent of any client
|
||||||
shows that would not have worked, twice over: `default_limits` and
|
identity, and refuses with a 429 that names capacity rather than the client —
|
||||||
`application_limits` are both evaluated with the same `key_func`, so they are
|
an operator has to be able to tell "one noisy client" from "the origin is
|
||||||
per-client across all routes rather than global; and `application_limits` are
|
saturated". It is registered last so it is outermost: a ceiling that applies
|
||||||
only applied `if in_middleware`, while this app installs no `SlowAPIMiddleware`
|
after the expensive work has run is not a ceiling.
|
||||||
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.
|
|
||||||
|
|
||||||
Cloudflare is already in the request path on both environments and does
|
slowapi cannot express this. `default_limits` and `application_limits` are both
|
||||||
edge-level rate limiting properly, before traffic reaches a single-process
|
evaluated with the same `key_func`, making them per-client rather than global,
|
||||||
origin at all. That is where a global ceiling belongs, and it is a dashboard
|
and `application_limits` only apply with `SlowAPIMiddleware` installed, which
|
||||||
change rather than code. Flagged as a follow-up, deliberately not built here.
|
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
|
Requests from `127.0.0.1` are exempt. The container healthcheck runs
|
||||||
default is unchanged, and `/api/suggest` gets 120/minute. Both are estimates
|
`curl http://localhost:80/api/data-info` from inside the container, and
|
||||||
rather than measurements, and they are a starting point to revisit once the
|
starving it would fail the check, restart the container, and turn a load spike
|
||||||
keying is correct enough for real per-user traffic to be visible — which it
|
into an outage loop. The exemption keys on the peer address, never the `Host`
|
||||||
is not today, because everyone shares one bucket.
|
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`
|
## 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
|
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.
|
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 bypass — the open one.** If the origin is reachable without
|
||||||
Cloudflare, `CF-Connecting-IP` is absent and the `X-Forwarded-For` fallback is
|
passing through Cloudflare, `CF-Connecting-IP` is attacker-controlled, and
|
||||||
forgeable, so limits could be evaded per-request. Closing that properly means
|
rotating it per request defeats per-client limits on every endpoint. The
|
||||||
Authenticated Origin Pulls or an origin firewall, which is infrastructure work
|
ceiling in §1.1 bounds the damage to the origin's total capacity; it does not
|
||||||
outside this change. Worth doing separately.
|
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
|
**Typesense becomes user-visible.** Today a Typesense outage degrades search to
|
||||||
a slow substring match. With autosuggest it also means the dropdown silently
|
a slow substring match. With autosuggest it also means the dropdown silently
|
||||||
|
|||||||
@@ -2163,6 +2163,41 @@ test('typing a school name suggests it, and choosing it opens that school', asyn
|
|||||||
await expect(page).toHaveURL(/\/school\/\d+/);
|
await expect(page).toHaveURL(/\/school\/\d+/);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('the whole dropdown is reachable, not clipped by the hero', async ({ page }) => {
|
||||||
|
/*
|
||||||
|
* The hero panel had overflow: hidden to clip its artwork to the rounded
|
||||||
|
* corners, and it clipped the dropdown too — 320px of list against 145px of
|
||||||
|
* panel below the input, so roughly half was cut off with nothing to say so.
|
||||||
|
*
|
||||||
|
* toBeVisible() does not catch this: it checks the box is non-empty and not
|
||||||
|
* visibility:hidden, and an ancestor's overflow clips neither. The invariant
|
||||||
|
* that does catch it is that the LAST option is the thing actually painted
|
||||||
|
* at its own coordinates — which fails for clipping and for occlusion alike.
|
||||||
|
*/
|
||||||
|
test.skip(!(await autosuggestIsOn(page)),
|
||||||
|
'the school_autosuggest flag is off in this environment');
|
||||||
|
|
||||||
|
const { schools } = await (await page.request.get('/api/schools?page_size=1')).json();
|
||||||
|
test.skip(!schools?.length, 'no schools in this environment');
|
||||||
|
|
||||||
|
await page.goto('/');
|
||||||
|
await page.getByRole('combobox').first().fill(
|
||||||
|
(schools[0].school_name as string).slice(0, 6));
|
||||||
|
|
||||||
|
const options = page.getByRole('option');
|
||||||
|
await expect(options.first()).toBeVisible();
|
||||||
|
const count = await options.count();
|
||||||
|
|
||||||
|
const painted = await options.nth(count - 1).evaluate((el) => {
|
||||||
|
const r = el.getBoundingClientRect();
|
||||||
|
const hit = document.elementFromPoint(r.left + r.width / 2, r.top + r.height / 2);
|
||||||
|
return { inside: el.contains(hit) || el === hit, bottom: Math.round(r.bottom) };
|
||||||
|
});
|
||||||
|
expect(painted.inside,
|
||||||
|
`the last option is not painted at its own coordinates (bottom ${painted.bottom}) `
|
||||||
|
+ '— an ancestor is clipping or covering the dropdown').toBeTruthy();
|
||||||
|
});
|
||||||
|
|
||||||
test('with autosuggest off, the search box is a plain input', async ({ page }) => {
|
test('with autosuggest off, the search box is a plain input', async ({ page }) => {
|
||||||
test.skip(await autosuggestIsOn(page),
|
test.skip(await autosuggestIsOn(page),
|
||||||
'the school_autosuggest flag is on in this environment');
|
'the school_autosuggest flag is on in this environment');
|
||||||
|
|||||||
@@ -90,7 +90,18 @@
|
|||||||
isolation: isolate;
|
isolation: isolate;
|
||||||
background: var(--hero-ground);
|
background: var(--hero-ground);
|
||||||
border-radius: var(--radius-xl);
|
border-radius: var(--radius-xl);
|
||||||
overflow: hidden;
|
/*
|
||||||
|
* Deliberately NOT overflow: hidden.
|
||||||
|
*
|
||||||
|
* It used to be, to clip the artwork and the scrim to the rounded corners —
|
||||||
|
* and it also clipped the search box's suggestion dropdown, which is 320px
|
||||||
|
* tall against 145px of panel below the input. Roughly half the list was cut
|
||||||
|
* off with no indication anything was missing.
|
||||||
|
*
|
||||||
|
* The two things that actually needed clipping round themselves instead, so
|
||||||
|
* the panel can let a dropdown out. Anything absolutely positioned inside
|
||||||
|
* this panel and taller than the space below it depends on this.
|
||||||
|
*/
|
||||||
}
|
}
|
||||||
|
|
||||||
.heroContent {
|
.heroContent {
|
||||||
@@ -107,6 +118,10 @@
|
|||||||
position: absolute;
|
position: absolute;
|
||||||
inset: 0;
|
inset: 0;
|
||||||
z-index: 0;
|
z-index: 0;
|
||||||
|
/* Rounds itself, because the panel no longer clips it. inset: 0 makes this
|
||||||
|
exactly the panel's own corners. */
|
||||||
|
border-radius: inherit;
|
||||||
|
overflow: hidden;
|
||||||
}
|
}
|
||||||
|
|
||||||
.heroArt picture,
|
.heroArt picture,
|
||||||
@@ -143,6 +158,9 @@
|
|||||||
inset: 0;
|
inset: 0;
|
||||||
z-index: 1;
|
z-index: 1;
|
||||||
pointer-events: none;
|
pointer-events: none;
|
||||||
|
/* Same reason as .heroArt: the panel stopped clipping, so the scrim keeps
|
||||||
|
its own corners rather than squaring off over the panel's. */
|
||||||
|
border-radius: inherit;
|
||||||
background: linear-gradient(
|
background: linear-gradient(
|
||||||
to right,
|
to right,
|
||||||
var(--hero-ground) 0%,
|
var(--hero-ground) 0%,
|
||||||
@@ -331,6 +349,10 @@
|
|||||||
position: static;
|
position: static;
|
||||||
order: -1;
|
order: -1;
|
||||||
height: 13rem;
|
height: 13rem;
|
||||||
|
/* Top corners only. Here the artwork is a band flush with the top of the
|
||||||
|
panel, not a layer covering it — inheriting all four would leave it
|
||||||
|
floating with rounded bottom corners against the copy below. */
|
||||||
|
border-radius: var(--radius-xl) var(--radius-xl) 0 0;
|
||||||
}
|
}
|
||||||
/* The band crop puts the schoolhouse at 73% across — reported by
|
/* The band crop puts the schoolhouse at 73% across — reported by
|
||||||
scripts/build-hero-images.js, which derives it from the crop box rather
|
scripts/build-hero-images.js, which derives it from the crop box rather
|
||||||
|
|||||||
Reference in new issue
Block a user