Merge pull request 'feat(suggest): school autosuggest, and the rate-limit fix it needed first' (#127) from feat/school-autosuggest into main

Reviewed-on: #127

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

docs/superpowers/specs/2026-08-26-school-autosuggest-design.md

@@ -0,0 +1,294 @@
+# 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.