diff --git a/.convoys/add-rate-limiting.md b/.convoys/add-rate-limiting.md new file mode 100644 index 0000000..89a8356 --- /dev/null +++ b/.convoys/add-rate-limiting.md @@ -0,0 +1,342 @@ +--- +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: queued +created: 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.