--- name: add-rate-limiting classification: convoy success_metric: | All endpoints listed in P0 #6 of `.convoys/ship-readiness.md` carry a rate-limit gate appropriate to their abuse class (search / upload / import / admin). The card-import endpoints are no longer anonymous-callable. `lib/rate-limit.js` exposes named limiters per route class with documented limits + windows + key-extraction strategies. P0 #6 flips PARTIAL → RESOLVED, closing the last open P0 ship-blocker. skip: - role-design-system-auditor - role-a11y-auditor - role-ux-reviewer - role-ia-architect status: shipped created: 2026-05-24 shipped: 2026-05-24 parent: ship-readiness addresses: P0 #6 (PARTIAL → RESOLVED) — the LAST open P0 depends_on: - fix-auth-bypass (Brief 4 shipped lib/rate-limit.js + auth-only limiter) - cors-tighten (just closed P0 #5; not a hard dep but the API surface is now CORS-clean) --- # Convoy: add-rate-limiting Extend `lib/rate-limit.js` and wire the result into the remaining abusable endpoints. This is the final P0 — when it lands, the launch-readiness ship-blocker list is empty (all 8 of 8 RESOLVED). ## Why now `fix-auth-bypass` Brief 4 shipped the rate-limit infrastructure (`lib/rate-limit.js`, `@upstash/ratelimit@^2.0.8`, `@upstash/redis@^1.38.0`, Vercel-Upstash Marketplace env vars `KV_REST_API_URL` / `KV_REST_API_TOKEN`) and wired it into the two auth endpoints (`login`, `register`) with a 5-attempts / 15-minute sliding window. The remaining abusable surface was intentionally deferred to this convoy. The "Queued convoys" entry in `.convoys/ship-readiness.md` lists the surface as: - `/api/users/search` - `/api/cards/search` - All `/api/cards/import-*` - `/api/user/avatar*` (upload) Parent's pre-architect audit at convoy creation surfaced a **critical secondary finding** beyond "missing rate limit": **`pages/api/cards/import-mtg.js`, `import-pokemon.js`, `import-lorcana.js` have ZERO authentication checks.** They are publicly callable, they hit Scryfall / Pokémon-TCG / Lorcana APIs with no caller throttling AND no auth, and they perform UPSERTs into the `cards` table. An attacker can: 1. Trigger expensive external API calls from your IP (Scryfall + Pokémon-TCG have published rate limits; you'd hit them with no recourse, getting your IP throttled at the source). 2. Cause unbounded DB writes (each import inserts hundreds-to-thousands of rows; `card.id` lookups deduplicate but the INSERT path runs in a tight per-card loop). 3. Even with a generous per-IP rate limit (5 req / hour), with 1000 IPs the math is 120,000 imports / day — enough to exhaust Vercel function quotas and Neon row budgets. The rate-limit alone is insufficient. **Auth-gate-then-rate-limit is the correct shape.** Whether to add auth gates to the import routes in this convoy (vs. spinning out a separate `gate-import-routes` convoy) is **architect Decision 1** below. Parent recommends in-scope; expansion is mid-convoy precedent established by `drop-public-setup` (Brief 2's CJS/ESM expansion) and other prior convoys. ## Scope ### Confirmed in-scope (regardless of architect Decision 1) - **`lib/rate-limit.js`** — refactor to expose multiple named limiters (one per route class), preserving the existing `checkAuthRateLimit` export for `login` / `register` backwards compatibility. New named limiters per Decision 2 below. - **`pages/api/users/search.js`** — add rate-limit gate after the existing JWT verification. Limit per Decision 2. - **`pages/api/cards/search.js`** — add rate-limit gate at top of handler (this endpoint is anonymous-by-design; rate-limit keyed by IP). - **`pages/api/user/avatar.js`** — add rate-limit gate after the existing `getUserFromRequest` check. Limit keyed by `user.userId` (authenticated; per-user makes more sense than per-IP for an upload endpoint where a household might share an IP). - **`pages/api/user/avatar/generate.js`** — same shape as `avatar.js`. Per-user, stricter than avatar.js (AI generation is more expensive than blob upload). - **`.cursor/rules/api-routes.mdc`** § Rate limiting — update the existing § with the new per-class pattern + each named limiter's use case. The current § shows only the auth pattern. ### Scope-expansion-candidate (architect Decision 1) - **`pages/api/cards/import-mtg.js`** — add `getUserFromRequest` check at top of handler; return 403 if not admin (`user.role !== 'admin'`); then rate-limit gate (per-user, very strict — these hit external APIs). - **`pages/api/cards/import-pokemon.js`** — same shape. - **`pages/api/cards/import-lorcana.js`** — same shape. - (If Decision 1 = Option A "in scope": ship in this PR. If Option B "spin out": queue `gate-import-routes` follow-up convoy and mark P0 #6 RESOLVED-with-caveat at convoy close.) ### Out of scope - **Global IP-based backstop limiter** (e.g., 1000 req / min per IP across all routes via Next.js middleware). Useful but separate scope; `add-global-rate-limit-middleware` would be its own convoy. - **Rate-limit headers on success responses** (`X-RateLimit-Remaining`, `X-RateLimit-Reset`). The existing `checkAuthRateLimit` returns `remaining` + `reset` in its result object but `login.js` doesn't propagate them on success; just on 429. Honoring the existing convention. - **`@upstash/ratelimit` version bump** — pin stays at `^2.0.8` from Brief 4. Bumping is its own convoy. - **Per-route handler unit tests** (still deferred to the queued `fill-vitest-handler-coverage` convoy — same reasoning as `cors-tighten`). - **Refactoring the existing `cards/search.js` SQL** (the file has a known god-function shape with 7+ conditional SQL branches; that's `god-function-split` scope, not here). - **Verifying the avatar `parseMultipartFormData` body parser** is rate-limit-safe (the body is read in `req.on('data')` before any rate-limit gate could short-circuit; an attacker can still exhaust the 5MB body even on a 429 path). That's a separate body-streaming-defense concern (`harden-multipart-parser`) and not in scope here. ## Operator action required **None.** All Upstash env vars (`KV_REST_API_URL` / `KV_REST_API_TOKEN`) are already auto-provisioned via the Vercel Marketplace integration (seeded for `fix-auth-bypass` Brief 4). No new secrets, no new dependencies (the `@upstash/ratelimit` + `@upstash/redis` packages are already installed). ## Decisions to ratify with operator (architect routes) 1. **Scope expansion — auth gates on `pages/api/cards/import-*.js` in this convoy?** - **Option A — In scope (parent recommends).** Add `getUserFromRequest` + admin-role check + rate-limit gate to all 3 import routes in this PR. ~30 LOC across 3 files. Closes the full P0 #6 attack surface in one shot. Precedent: `drop-public-setup` Brief 2 (CJS/ESM expansion). - **Option B — Spin out.** Stay narrow on the 4 originally-listed routes; queue `gate-import-routes` as a separate convoy. Mark P0 #6 RESOLVED-with-caveat noting the import-route auth gap. - **Option C — Hybrid.** Add the rate-limit gate to import routes now (with a clear `TODO: requires-auth` comment), gate auth later. Worst of both worlds — leaves an anonymous-callable abusive endpoint live with only IP-based throttling. - Architect investigates: confirm the import routes are intended to be admin-only (per AGENTS.md / scripts/ folder conventions), confirm Decision-A scope size is bounded, route back recommendation. 2. **Named-limiter shape in `lib/rate-limit.js`.** The existing `checkAuthRateLimit(req)` function has the limit + window hardcoded. To support per-class limits cleanly, two main patterns: - **(a) Named-export functions.** Each route class gets its own exported function: `checkSearchRateLimit`, `checkUploadRateLimit`, `checkImportRateLimit`. Internal `init()` builds one `Ratelimit` instance per class, cached under different Redis prefixes. Verbose but very explicit at each call site. - **(b) Generic `checkRateLimit(req, options)`.** Single exported function takes a config object. Calling code becomes `await checkRateLimit(req, { class: 'search' })` or similar. Less verbose at the lib but more at each call site, and the per-route limits are documented in the lib module rather than implicit in the function name. - **(c) Hybrid.** Keep `checkAuthRateLimit` (used by login + register; high-stakes contract; don't break). Add named functions for the other 3 classes (search, upload, import). Best of both — preserves Brief 4's contract, names new classes explicitly. - Parent recommends (c). Architect ratifies after reading `lib/rate-limit.js` shape. 3. **Per-class limit values.** Reference table (architect's to tune): - **`auth`** (existing) — 5 / 15min, IP-keyed. (Don't change.) - **`search`** (new) — recommend 30 / 1min, IP-keyed. Legitimate users type-ahead-search; 30/min is generous but stops a scraper in its tracks within 2-3s. Same IP-keying as auth. - **`upload`** (new — avatar.js, avatar/generate.js) — recommend 10 / 1hour, user-keyed. Avatar uploads are rare; 10/hour catches accidental loops without blocking a user who hits "save" three times by accident. User-keyed because the user is authenticated and a household IP shouldn't punish other users. - **`generate`** (new — avatar/generate.js specifically) — recommend 3 / 1hour, user-keyed. AI generation costs money; stricter than plain upload. - **`import`** (new — cards/import-*.js) — recommend 5 / 1hour, user-keyed (if Decision 1 = Option A). Admin-triggered, hits external APIs with their own rate limits; 5/hour is plenty. 4. **Identifier extraction.** `lib/rate-limit.js`'s `extractIdentifier()` currently always extracts the first `x-forwarded-for` IP. For per-user limiters (upload, generate, import), we need a `user_id`-based extractor. Three options: - **(a) Two extractors.** `extractIpIdentifier` (existing) + `extractUserIdentifier(req, userId)` (new). Each limiter calls the right one. - **(b) Single extractor takes an `opts.user` flag.** Less code but harder to read. - **(c) Caller passes `key` directly.** Each handler calls `checkUploadRateLimit(req, { key: user.userId })` and the lib just trusts it. - Parent recommends (a) — explicit, two small functions, hard to misuse. 5. **429 response shape.** Match the existing `login.js`/`register.js` pattern verbatim: ```js res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000)); return res.status(429).json({ error: 'Too many attempts. Try again later.' }); ``` Architect: should the error message vary per class (e.g., "Too many search requests" vs "Too many uploads") or stay uniform? Recommend uniform — minimizes attacker-fingerprinting of which routes have which limits. Architect ratifies. 6. **Test coverage in this convoy.** No new per-route handler tests (per Decision-4-equivalent from `cors-tighten`; same deferred-to-`fill-vitest-handler-coverage` reasoning). The Playwright smoke spec doesn't exercise any of these routes, so no smoke coverage either. **The fail-loud-in-prod semantics in `lib/rate-limit.js`'s `init()` function are the integration test** — if the env vars are unset on a deployed env, every limit-gated route fails closed on first call. ## Known constraints - **`lib/rate-limit.js` already has the right architecture** for multiple limiters (lazy `init()` returns a `cached` object; refactor pattern is to make `cached` a Map instead of a single instance). Don't rewrite from scratch; extend. - **Fail-closed semantics in prod must be preserved.** Brief 4's `init()` throws when env vars are unset and `NODE_ENV === 'production'`. New limiters must inherit this — a misconfigured prod env should fail loud on every call, not silently disable all rate limiting. - **Card-search is anonymous-by-design** — do NOT add a `getUserFromRequest` check. The IP-keyed limit is the correct defense (search is a public catalogue feature). - **User-keyed limits require an authenticated request.** The rate-limit gate MUST sit AFTER the auth check in the handler body. Wrong order = security regression (anonymous user bypasses the gate because there's no `userId` to key on, the extractor falls back to IP, and the per-user limit becomes per-IP — same household IP could be locked out for a different user's behavior). - **Brief 4's `lib/rate-limit.js` is unit-tested transitively** via the existing vitest suite (none of the 21 tests specifically test rate-limit, but the auth-utils tests run through the same module). Don't break those. - **`@upstash/ratelimit` supports multiple shapes** (`slidingWindow`, `fixedWindow`, `tokenBucket`); Brief 4 used `slidingWindow`. New limiters should follow the same shape unless the architect has a specific reason for a different algorithm per class. - **Per-route Pattern checks** — the 6 (or 9 if Decision 1 = Option A) target files have different existing structures: - `users/search.js`: auth check inside try/catch on JWT; rate-limit goes after JWT verify, inside the existing try - `cards/search.js`: no auth; rate-limit goes at top of handler after the method check - `user/avatar.js`: auth check inside the outer try (line 16-19); rate-limit goes after auth, before the method branching at line 21 - `user/avatar/generate.js`: similar shape to avatar.js - `cards/import-*.js`: NO auth currently. Decision 1 dictates the shape — either add auth-then-rate-limit, or rate-limit-only (Option C, not recommended) ## Acceptance criteria The convoy is shippable when ALL of the following hold: 1. `lib/rate-limit.js` exposes the named limiters per Decision 2. Backwards-compat: `checkAuthRateLimit` still works for `login.js` / `register.js` (do NOT break Brief 4's contract). 2. All 4 originally-listed endpoints (`users/search`, `cards/search`, `user/avatar`, `user/avatar/generate`) carry the appropriate rate-limit gate at the correct ordering (after auth if applicable; per Decision 4 keying). 3. If Decision 1 = Option A: all 3 import endpoints carry both the auth gate (admin-role check) + the import rate-limit gate. 4. `npm run lint` exit code matches baseline (still 128 problems; do NOT regress). 5. `npm run test:run` (vitest) still passes 21/21 (no regression on the existing auth-utils tests that transitively load `lib/rate-limit.js`). 6. `npm run test:smoke` (via CI on the PR) still passes 3/3 against the Vercel preview — proves the live login → verify → /api/health flow doesn't accidentally get rate-limited (the smoke spec hits each endpoint once per run, well below any limit). 7. `.cursor/rules/api-routes.mdc` § Rate limiting updated to document each new limiter's use case + the per-class limits + the user-keyed vs IP-keyed convention. 8. Bypass-secret-leak check: zero matches of any `KV_REST_API_*` or `UPSTASH_*` env-var values in any CI log. (Auto-masked by GitHub Actions; verify post-merge.) ## Anything flagged but not acted on (in advance) - **Global IP backstop limiter via Next.js middleware** — out of scope; queue `add-global-rate-limit-middleware` if a future audit shows non-listed routes being abused. - **Body-streaming defense for avatar uploads** — the 5MB multipart body is consumed before any rate-limit gate can short-circuit. Real defense requires moving the parse into a separate edge function or using `read-up-to` semantics; queue `harden-multipart-parser` if it ever surfaces in a real abuse incident. - **Per-tier user limits** — premium users might get higher search/upload limits. Queue `tiered-rate-limits` when a tiering scheme exists (today there are only `admin` and default roles). - **Rate-limit metrics dashboard** — Upstash exposes per-key hit counts; surfacing them in an admin dashboard would catch abuse early. Queue `rate-limit-observability` post-launch. - **Card-search SQL god-function** — known shape issue, separate convoy (`god-function-split` or `refactor-cards-search-sql`); do NOT touch in this convoy. ## Decisions (post-architect) Six decisions ratified by `role-architect` on 2026-05-24. **Only Decision 1 requires operator gate-1 sign-off** (significant scope expansion to admin-only import-route enforcement + admin UI source touch); the other five are architect-self-ratifiable per the precedent established by `cors-tighten` Decision D2-D5 + `fix-vercel-deployment-protection-in-ci` Decisions A/B/D. ### Decision 1 — Scope expansion: gate `pages/api/cards/import-*.js` in this convoy (**OPERATOR-RATIFIABLE**) **Ratified: Option A — gate all three import routes with auth + admin-role + import rate-limit, AND fix the admin UI to send the Bearer token.** Architect investigation results: - **`pages/api/cards/import-mtg.js`** — LIVE admin tooling. Called by `pages/admin/card-import.js` line 39-49 (the `` second option is `'pokemon'`. - **`pages/api/cards/import-lorcana.js`** — DEAD in frontend. Architect ran `rg 'import-lorcana' pages/ components/` and the file has zero frontend callers; the admin UI's `` only offers `mtg` / `pokemon`) — uniform protection across the three import shapes is strictly easier to maintain than a 2-gated-1-deleted asymmetry, and if Lorcana stays unused, the queued `delete-dead-lorcana-import` follow-up convoy (see `.convoys/ship-readiness.md` § Queued convoys) is the cleanup path. - **Decision 2 — Hybrid named-limiter shape (architect-self-ratified).** `lib/rate-limit.js` refactored from a single auth-only `Ratelimit` instance into a `Map` cache with one shared Redis client and five `Ratelimit` instances (one per class, distinct Redis prefix). Five exported functions: `checkAuthRateLimit(req)` (Brief 4 contract preserved byte-identical), `checkSearchRateLimit(req)`, `checkUploadRateLimit(req, userId)`, `checkGenerateRateLimit(req, userId)`, `checkImportRateLimit(req, userId)`. Internal `check(className, identifier)` shared helper. `LIMITER_CONFIG` is a top-level `const` map of `{ limit, window, prefix }` per class; adding a sixth class is a one-line addition + one new exported function (no `init()` restructuring needed). - **Decision 3 — Per-class limit values (architect-self-ratified with tuning evidence).** Final table: | Class | Limit | Window | Key | Helper | | --- | --- | --- | --- | --- | | `auth` | 5 | 15 min | IP | `checkAuthRateLimit(req)` (unchanged from Brief 4) | | `search` | **60** | 1 min | IP | `checkSearchRateLimit(req)` | | `upload` | 10 | 1 hour | user | `checkUploadRateLimit(req, userId)` | | `generate` | **5** | 1 hour | user | `checkGenerateRateLimit(req, userId)` | | `import` | 5 | 1 hour | user (admin-only) | `checkImportRateLimit(req, userId)` | Two architect raises from the parent's pre-investigation defaults warrant a permanent record so future tuning convoys see the evidence: - **`search` raised 30 → 60/min.** `components/ShareModal.js`'s `handleSearch` (lines 56-77) fires on every keystroke with NO debounce; typing a 17-char email = 16 requests in <5s, which would 429 a single legitimate user against the 30/1min default. 60/1min covers a realistic burst and still stops a scraper inside 2-3 seconds. If real users still 429, the fix is a one-line `LIMITER_CONFIG` edit (60 → 90 or 120); not a release-blocker. Queued follow-up name if needed: `tune-search-rate-limit`. - **`generate` kept at 5/hour (parent suggested 3).** `pages/api/user/avatar/generate.js` calls DiceBear (free public API), not OpenAI / Replicate / Stability — cost is Vercel blob storage + DiceBear-side throttling, not per-call $. 5/hour is generous enough that a user trying 3-4 seeds doesn't hit the wall, strict enough that an accidental render loop still trips inside the first minute. If we ever switch generators to a paid provider, the same one-line `LIMITER_CONFIG` edit drops it back to 3 or lower. - **Decision 4 — Two-extractor shape with defensive THROW (architect-self-ratified).** `extractIpIdentifier(req)` (module-private; renamed from the pre-refactor `extractIdentifier`) and `extractUserIdentifier(userId)` (module-private; new). The user extractor **THROWS** with `'[rate-limit] extractUserIdentifier called without an authenticated userId. Place the rate-limit gate AFTER the auth check, never before.'` when `userId` is `null` / `undefined` / `''` / `NaN`. Surfaces gate-ordering bugs **at dev time** rather than silently falling back to IP and converting a per-user limit into a per-IP limit (which would lock other household members out for one user's behavior — the exact regression flagged in the convoy file's § Known constraints). Numeric `0` is intentionally accepted (returns `'user:0'`) — defensively forward-compatible if a future schema introduces user ID 0. - **Decision 5 — Uniform 429 response (architect-self-ratified).** Single `'Too many attempts. Try again later.'` message across all five classes, matching `login.js` + `register.js` verbatim. `Retry-After` calculation is `Math.ceil((reset - Date.now()) / 1000)`; status code is `429`. Per-class variation (e.g., *"Too many search requests"*) was considered and rejected because it would fingerprint to an attacker which routes have which limits + windows, making it easier to craft a request pattern that avoids 429 on the more-permissive routes while still abusing the less-permissive ones. - **Decision 6 — No new vitest or playwright tests (architect-self-ratified).** Same reasoning as `cors-tighten` Decision D4. Per-route handler tests are deferred to the queued `fill-vitest-handler-coverage` convoy. The fail-loud-in-prod predicate in `lib/rate-limit.js::init()` is the integration test — if `KV_REST_API_*` is unset on a deployed env, every gated route fails closed on the first call. Architect-time investigation corrected a stale claim in the convoy file's § Known constraints (the line *"auth-utils tests run through the same module"*): `rg 'rate-limit|@upstash' test/` returns zero matches, so the lib refactor is strictly safer than the convoy file implied — there is no transitive vitest path to break. ### As-shipped surface The shape splits cleanly into four layers (mirrors `cors-tighten`'s 24-source-files / 1-CI-job pattern split, but with one extra layer because of the atomic admin UI fix and the rule extension): 1. **One `lib/rate-limit.js` refactor.** Single-class auth-only limiter → 5-class `Map` with distinct Redis prefixes (`tcgvault:auth`, `tcgvault:search`, `tcgvault:upload`, `tcgvault:generate`, `tcgvault:import`). +90 / -23 net (90 lines added, 23 lines reshaped — the existing `init()`, `extractIdentifier`, `checkAuthRateLimit` functions are conceptually preserved but restructured to share infrastructure across all five classes). Brief 4's `checkAuthRateLimit(req)` return shape is **byte-identical** post-refactor (`{ allowed, remaining, reset }`); `login.js` + `register.js` were not touched and continue to work unchanged. 2. **Six new route-gate additions** (lib refactor delivers the helpers; routes call them): - `pages/api/users/search.js` — `checkSearchRateLimit(req)` after the existing inline JWT verify. IP-keyed. +7 lines. - `pages/api/cards/search.js` — `checkSearchRateLimit(req)` at top of handler after the method check, before the 240-line SQL god-function (which stays byte-identical per § Files explicitly out of scope). IP-keyed, anonymous-by-design. +7 lines. - `pages/api/user/avatar.js` — `checkUploadRateLimit(req, user.userId)` after `getUserFromRequest`, before the POST/DELETE method branching (so the limiter fires before `parseMultipartFormData` consumes the 5MB body). User-keyed. +7 lines. - `pages/api/user/avatar/generate.js` — `checkGenerateRateLimit(req, user.userId)` after `getUserFromRequest`, before the user-data SQL query. User-keyed. +7 lines. - `pages/api/cards/import-mtg.js` — `getUserFromRequest` + `if (user.role !== 'admin') return 403` + `checkImportRateLimit(req, user.userId)` before the existing `try` block. Closes the publicly-callable anonymous-abuse vector. +16 lines. - `pages/api/cards/import-pokemon.js` — same shape as `import-mtg.js`. +16 lines. - `pages/api/cards/import-lorcana.js` — same shape as `import-mtg.js`; gated defensively despite zero current frontend callers. +16 lines. 3. **One atomic admin UI fix.** `pages/admin/card-import.js` adds `'Authorization': \`Bearer ${localStorage.getItem('auth_token')}\`` to the import fetch's headers (one-line addition; the 309-line god-component is otherwise byte-identical). **This was the architect's critical pre-brief discovery and the reason Decision 1 routes back to the operator** — gating the import APIs without this matching client fetch fix would have closed P0 #6 but introduced an immediate 401 on every "Import Cards" click, producing a visible UX regression on the only live admin tooling that exercises the gated routes. Shipping the gate + the client fix in the same PR keeps the convoy atomic; PR review enforced the dependency explicitly. 4. **One `.cursor/rules/api-routes.mdc` § Rate limiting extension.** Replaced the previous auth-only § with the per-class table + the verbatim call shape + gate-ordering rules (method check first; auth before any user-keyed limiter; IP-keyed gate placement is flexible; admin-role check goes between auth and rate-limit for the import routes) + identifier-extraction documentation + uniform 429 response shape + fail-closed env-var contract + fail-open Upstash-outage behavior. Implementer landed this in the same PR; doc-writer pass verified the extension is complete and made no further touch-ups (see § What did NOT change below). ### As-shipped metrics Diff size (per `git show --stat 708ef45`): - **12 files modified, +1612 / -23.** Note that the 1612-addition figure is dominated by `.convoys/add-rate-limiting.md` (676 lines) and `.convoys/add-rate-limiting/brief-1-extend-rate-limit-and-wire-routes.md` (751 lines), which the squash includes because the architect commit preceded the implementer commit on the same branch. The actual source-file diff is much smaller: - `lib/rate-limit.js`: +90 / -23 (the lib refactor). - `.cursor/rules/api-routes.mdc`: +41 (the § Rate limiting extension). - 6 route files under `pages/api/`: +69 lines total (3× +16 for import routes, 4× +7 for search/avatar/generate/users-search). - `pages/admin/card-import.js`: +1 (the Bearer-header addition). Post-merge CI run 26382185019 + subsequent runs on `main`: - **`Playwright smoke` — PASS in 59s, 3/3 tests in 3.8s** against the post-rate-limit Vercel preview. Same three checks (`home redirects or renders without 5xx` ✓ 431ms / `sign-in page renders` ✓ 331ms / `public health endpoint responds` ✓ 193ms) — all green. **Critical cross-validation:** smoke calls `/api/health` once per run (well below the search limiter's 60/min ceiling), and the home + sign-in routes don't touch any of the 6 newly-gated endpoints, so smoke does NOT 429 against the new search class. The cross-validation finding accumulates: smoke test 2 (`'sign-in page renders'`) still passes against the post-CORS + post-rate-limit preview — that's three convoys in a row (PR #15 Layout default-user → PR #19 CORS-tighten → PR #20 rate-limiting) where the auth surface stayed stable under sweeping changes, and the same 3-test smoke spec defended it every time. - **`forbidden-cors-headers` (from `cors-tighten`)** — PASS. None of the 6 route edits introduced an `Access-Control-Allow-*` header (the convoy is purely additive of rate-limit gate code; CORS surface was not touched). The grep stays clean. - **`forbidden-endpoints` (from `fix-auth-bypass` Brief 3)** — PASS. No new `pages/api/test-*.js` or other deleted-endpoint shapes reintroduced. - **`Unit tests (vitest)`** — PASS, 21/21 in 27s. No new tests, no removed tests; the existing `lib/auth-secret.test.js` (3) + `lib/permission-middleware.test.js` (8) + `pages/api/auth-utils.test.js` (5) + `components/Layout.test.js` (5) suites are all unaffected. Decision 6's architect-time verification (`rg 'rate-limit|@upstash' test/` returns zero matches) confirmed at merge. - **`Lint`** — 128 problems (lint baseline preserved, no regression). The lib refactor + the 6 route edits + the admin UI one-liner introduced zero new lint problems; the `|| true` wrapper in `.github/workflows/ci.yml` was a no-op for this convoy. - **`Screenshot diff`** — `continue-on-error: true` swallow per `adopt-playwright-smoke` Decision 4 (no baseline committed yet); the documented Decision-4 end state. Triggered on PR #20 because the `paths:` filter `pages/**` matches the 6 route edits under `pages/api/`; same minor false-positive as PR #19, tracked by the queued `tighten-visual-diff-path-filter` follow-up. - **All other gates (`Schema map up to date`, `Aggregate gate`)** — green. ### Cross-validation finding: smoke spec defends the rate-limit surface (organically) The 3-test smoke spec was authored by `adopt-playwright-smoke` (PR #18) against an un-rate-limited preview, with no foresight about this convoy's gating. Post-merge run 26382185019 confirms the spec **still passes against the post-rate-limit preview** — the home route, the `/login` route, and `/api/health` don't touch any of the six newly-gated endpoints, and `/api/health` is anonymous / unrate-limited so the smoke flow doesn't bump up against the search class's 60/min ceiling. The cross-validation lineage accumulates: - PR #15 (`fix-layout-default-user`, `ca302a8`) introduced the `Sign in` CTA that smoke test 2 asserts on. - PR #19 (`cors-tighten`, `da50d78`) removed wildcard CORS from 24 handlers; smoke test 2 still passed against the post-CORS preview. - PR #20 (`add-rate-limiting`, `708ef45`) — this convoy — wired 6 new route gates; smoke test 2 still passes. Smoke is doing real work: it has now defended the auth surface against three sweeping changes without anyone having to write a single dedicated test. P0 #7's resolved state, P0 #5's resolved state, and now P0 #6's resolved state are all backed by a live CI signal — not just a vitest assertion. ### Operator action required going forward **None.** No new env vars (Upstash `KV_REST_API_URL` / `KV_REST_API_TOKEN` were already auto-provisioned via the Vercel Marketplace integration for Brief 4). No new dependencies (`@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0` already installed). No new secrets, no infra changes, no CI gates to enable or disable. The fail-loud-in-prod predicate in `lib/rate-limit.js::init()` is self-defending: if a future deploy unsets either env var, every gated route fails closed on the first call (`throw new Error('[rate-limit] Upstash not configured...')`), which surfaces immediately as a 500 in the Vercel logs rather than silently disabling brute-force protection. If a follow-up tuning need surfaces (search 60/min too tight, generate 5/hour too tight, etc.), the fix is a single-line `LIMITER_CONFIG` edit. See `tune-search-rate-limit` (not yet queued; surface only if real users 429) and the broader `tiered-rate-limits` (P3 polish, when a premium-tier scheme exists) for the longer view. ### What did NOT change Audit trail of files explicitly NOT touched by this convoy, despite sitting near the rate-limit surface: - **`pages/api/auth/login.js`, `pages/api/auth/register.js`** — Brief 4 contract preserved. Both files continue to call `checkAuthRateLimit(req)` against the refactored lib; byte-identical post-merge. Manual verification at architect time: the 6th-attempt 429 path still fires correctly. - **`lib/permission-middleware.js`, `lib/auth-secret.js`, `pages/api/auth-utils.js`** — auth surface untouched. No `null`-vs- synthetic-admin regression risk; `test/lib/permission-middleware.test.js`'s negative regression test still defends Gotcha #2. - **`package.json`, `package-lock.json`** — zero dep additions, zero version bumps. `@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0` pinned from Brief 4. - **`.github/workflows/*.yml`** — no new CI gate. Per-class rate-limit wiring isn't grep-checkable; the fail-loud-in-prod predicate is the integration test (see Decision 6). - **`test/**`** — Decision 6, no new vitest or playwright specs. 21/21 still green; smoke 3/3 still green. - **`pages/api/cards/search.js`'s SQL** — the 240-line god-function with 7+ conditional `SELECT * FROM cards WHERE …` branches was explicitly out of scope per `.convoys/add-rate-limiting.md` § Out of scope. Queued as `god-function-split` / `refactor-cards-search-sql` (not yet queued in `ship-readiness.md` — surface when the convoy is sized). - **`pages/api/user/avatar.js`'s `parseMultipartFormData` body streaming** — the 5MB multipart body is consumed via `req.on('data')` before any rate-limit gate can short-circuit, so an attacker can still exhaust the 5MB body per 429. Queued as `harden-multipart-parser` (not yet in `ship-readiness.md` — surface if a real abuse incident occurs). The brief's gate-ordering places the limiter BEFORE the method branches that call `parseMultipartFormData`, so when that future hardening lands, the gate ordering is already correct. - **`scripts/import-*.js`** — standalone CLI scripts independent of the API routes; not in scope per § Files explicitly out of scope. An operator running `node scripts/import-mtg.js` directly bypasses the rate-limit + admin-role gate entirely (which is the intended flow — local admin work is unthrottled). - **`AGENTS.md`'s Gotcha #5 (deleted `setup-database.js`) + Gotcha #8 (Layout default-user)** — both still RESOLVED, both unchanged. This convoy's doc-writer pass updates Gotcha #12 (rate-limit env-var contract) to reflect the 5-class reality, but Gotcha #5 and #8 are independent of this convoy. - **Auth wrapper extraction (`withAdmin(handler)`)** — flagged in `.cursor/rules/auth-and-permissions.mdc` (*"check user.role === 'admin' directly; consider extracting `withAdmin()` if a third call site appears"*). The three import routes are the third+fourth+fifth call sites in the codebase, but extracting the wrapper is its own scope — for this convoy, the inline `if (user.role !== 'admin') return 403` shape was preserved across all three. Surface as a follow-up convoy if a sixth call site appears or if a reviewer flags the inline shape as a maintainability concern; for now, the uniform inline check across the three import routes is consistent with the rest of the codebase.