.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<className,
+  Ratelimit> 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.