Files
school_compare/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md
T
TudorandClaude Opus 5 0fa1a292c7
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
fix(api): bound what a forged CF-Connecting-IP can buy
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
2026-08-26 20:56:32 +01:00

295 lines
14 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# 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.
```python
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:
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 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/schools`
falls 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.
- **`limit` is clamped** to 20. It is a public endpoint.
- **Rate limit `120/minute`** per 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.