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
14 KiB
School Autosuggest — Design
Date: 2026-08-26 Status: approved for planning Depends on: the feature-flag layer (PR #125, merged)
Goal
Suggest schools by name as someone types in the site's main search box, so a parent who knows the school they want reaches it in one step instead of searching, scanning a result list, and clicking.
Scope is schools only. Places and postcodes were considered and excluded — see Out of scope.
The finding that shapes everything
The site's rate limiter does not do what it looks like it does.
limiter = Limiter(key_func=get_remote_address) with 60/minute reads
request.client.host. In staging and production the backend has no published
ports and sits on the internal backend network, so its only caller is the
Next proxy — and request.client.host is therefore the Next container, for
every browser user on the site.
Measured against staging: 70 concurrent requests to /api/schools returned
60 × 200 and 10 × 429. One machine consumed the whole site's budget for
that minute.
Autosuggest is the worst possible feature to build on that. One person typing "st marys primary" produces six to eight debounced requests; eight concurrent searchers would 429 the site. The compare modal's search-as-you-type already shares this bucket, so the exposure exists today — autosuggest makes it certain.
Fixing the keying is therefore part of this work, not a follow-up.
1. Rate-limit keying
Both environments sit behind Cloudflare (server: cloudflare, cf-ray present
on staging and production). Cloudflare sets CF-Connecting-IP on every request
to the origin and overwrites any client-supplied value, which makes it
trustworthy in a way a parsed X-Forwarded-For chain is not.
def client_key(request: Request) -> str:
"""Rate-limit bucket: the real caller, not the proxy in front of them."""
cf = request.headers.get("cf-connecting-ip")
if cf:
return cf.strip()
xff = request.headers.get("x-forwarded-for")
if xff:
return xff.split(",")[0].strip()
return get_remote_address(request)
nextjs-app/app/api/[...path]/route.ts already forwards every inbound header
except host and connection, so CF-Connecting-IP reaches the backend with
no proxy change.
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 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.
Two mitigations, and they are not interchangeable:
- 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.
- 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 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.
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:
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.
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.
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
A dedicated endpoint, not a mode of /api/schools.
The existing search path calls Typesense for URNs and then filters, ranks and
sorts the full in-memory DataFrame — a pandas pass per keystroke, holding the
GIL and blocking other requests in the same worker. Suggestions need none of
it: urn, school_name, phase, school_type, local_authority,
postcode and ofsted_rating are all already in the Typesense document
(pipeline/scripts/sync_typesense.py).
GET /api/suggest?q=<query>&limit=8
→ 200 {"suggestions": [
{"urn": 100010, "school_name": "Brecknock Primary School",
"local_authority": "Camden", "postcode": "NW1 1AA",
"phase": "Primary", "school_type": "Community school"}
]}
- Under two characters returns
{"suggestions": []}with 200. The keystroke path never returns an error for ordinary input. - Typesense unavailable returns
{"suggestions": []}with 200. There is deliberately no DataFrame fallback: the substring scan/api/schoolsfalls back to is precisely the cost this endpoint exists to avoid, and a silent 25,000-row scan per keystroke is worse than no suggestions. limitis clamped to 20. It is a public endpoint.- Rate limit
120/minuteper client, not the default 60. A 200 ms debounce tops out near 5 requests/second while someone is actively typing, but averages far below that across a real search; 120 leaves headroom for bursts while still bounding one client. - Local authority is part of the payload, not decoration. There are many schools called "St Mary's"; a suggestion list without the authority is unusable for exactly the queries autosuggest is meant to serve.
Caching
CACHE_RULES gains ("/api/suggest", (60, 3600, 86400)). Prefix queries
repeat enormously across users and school names change once a year.
The client fetch must not use cache: "no-store". The compare modal does,
and copying that pattern would throw away both the browser cache and the ETag
304s the existing CacheAndETagMiddleware already provides.
Both environments currently report cf-cache-status: DYNAMIC — Cloudflare
ignores the Cache-Control the API already sends, because it does not cache
dynamic paths by default. A Cloudflare Cache Rule for /api/suggest* would
let the edge absorb most of this traffic and never reach the origin. That is
a dashboard change, it is optional, and nothing here depends on it.
3. The combobox
This is an ARIA combobox, not a text input with a list underneath.
Files. FilterBar.tsx is already long. The work splits three ways:
hooks/useSchoolSuggest.ts owns fetching, debouncing and cancellation;
components/SuggestList.tsx owns rendering and ARIA; FilterBar.tsx wires
them to the existing input and form.
Fetching. 200 ms debounce; minimum two characters; an AbortController
cancels the superseded request on every keystroke. Cancellation is not an
optimisation — without it, a slow response for "st" can land after the fast
one for "st marys" and replace a correct list with a stale one.
Suppressed during postcode entry. The box takes a school name or a
postcode, and isValidPostcode already distinguishes them. Suggestions do not
appear once the value parses as a postcode.
Keyboard. ArrowDown/ArrowUp move the active option, Escape closes and
keeps the typed text, Tab closes. Enter with an option active navigates
to that school's page. Enter with none active submits the free-text
search exactly as it does today — the existing behaviour is preserved, not
replaced.
ARIA. role="combobox" with aria-expanded and aria-controls on the
input, aria-activedescendant pointing at the active option, role="listbox"
on the list and role="option" on each row.
Both instances get it. HomeView renders FilterBar twice — hero and
sticky — from one component, so there is one implementation.
4. Behind a flag
Flag school_autosuggest, declared in backend/flags.py, default off.
This is the most-used control on the site and the first change to it in a
while. app/page.tsx is an async server component, so it reads the flag and
threads it to FilterBar through HomeView — two prop hops, explicit, no
client-side flag read.
Off means the input behaves exactly as it does today: no listener, no fetch, no markup. Not a rendered-then-hidden dropdown.
The rate-limit keying is not flagged. It is a correctness fix that should apply whether or not autosuggest is on, and flagging it would mean shipping a known-wrong limiter into production deliberately.
5. Analytics
search_submitted already carries via: 'input'. Selecting a suggestion fires
it with via: 'suggestion' plus the chosen urn, so the obvious question —
does this actually help, or do people ignore it — has an answer in the data
rather than an opinion.
6. Testing
Backend. client_key prefers CF-Connecting-IP, falls back through
X-Forwarded-For to the remote address, and two different values get two
different buckets. /api/suggest returns matches, returns empty below two
characters, returns empty and 200 when Typesense is unavailable, and clamps
limit. That it never touches the DataFrame is asserted by making
load_school_data raise and requiring the endpoint to answer anyway.
Frontend. The hook debounces, aborts superseded requests, and drops a
late-arriving response for a stale query. The list renders the ARIA
attributes. Keyboard navigation moves the active option; Enter on an option
navigates; Enter on none submits the search.
E2E. With the flag on, typing a known school name shows it and selecting it
lands on that school's page. With the flag off, no combobox markup exists.
Gated on the flag the same way the distance journeys are — read the observable
effect, since /api/flags is denied to the public.
7. Risks
Removing the accidental throttle. Covered in §1. Correct per-user keying means the origin is reachable at 60/minute per user where it was 60/minute in total, and no in-app global cap replaces it — that job goes to Cloudflare, which is not done as part of this change. Until it is, a determined caller 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 — 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 stops appearing. That is the correct failure — quiet, not broken — but it makes Typesense health worth monitoring in a way it was not before.
Out of scope
- Place suggestions. The 2,646 town, authority and outcode pages are a strong candidate and would route people onto the pages W2 built, but they live in the place registry rather than Typesense, so it is a second index and a ranking rule for comparing two kinds of result. Worth its own change.
- Postcode completion. Would put postcodes.io in the keystroke path, with its own latency and rate limits.
- The compare modal. It already has search-as-you-type. Converting it to this component is a reasonable follow-up, not part of this.
- Recent or popular searches. No storage for either, and no evidence yet that they are wanted.