From 60b842ee414d0ea559ab9fccf993f9838e784aa0 Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Sun, 24 May 2026 22:30:09 -0500 Subject: [PATCH] =?UTF-8?q?architect:=20add-rate-limiting=20(queued=20?= =?UTF-8?q?=E2=86=92=20in-progress;=201=20brief,=20D1=20routes=20back=20fo?= =?UTF-8?q?r=20operator=20gate-1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six decisions ratified — five architect-self-ratifiable, one (D1, scope expansion to gate the three import routes with auth + admin-role + import rate-limit, plus the matching Bearer-token fix in pages/admin/card-import.js) routed back for operator approval before implementer dispatch. Architecture: single brief, ~180 LOC across 10 files. Lib refactor preserves Brief 4's checkAuthRateLimit(req) contract; adds four named exports (search/upload/generate/import) with a Map cache and distinct Redis prefix per class. Per-class limits tuned against real client behavior — search bumped from 30 to 60/min after finding ShareModal handleSearch has no debounce; generate bumped from 3 to 5/hour after confirming the generator uses DiceBear (free) not a paid AI service. Brief includes verbatim new lib/rate-limit.js shape so the implementer has zero design discretion, plus per-route verbatim post-edit shapes for all seven gated handlers and the admin-UI fetch fix. Decision 1 investigation finding: pages/admin/card-import.js currently fetches without an Authorization header. Without the matching fix in this brief, adding getUserFromRequest to the import APIs immediately breaks the admin UI on first run — the brief is internally consistent only with both edits shipping together. Decision 6 correction: the convoy file's claim that vitest transitively loads lib/rate-limit.js is stale (rg verified no test imports it). The lib refactor is strictly safer than the convoy file implied. Co-authored-by: Cursor --- .convoys/add-rate-limiting.md | 336 +++++++- ...ief-1-extend-rate-limit-and-wire-routes.md | 751 ++++++++++++++++++ 2 files changed, 1086 insertions(+), 1 deletion(-) create mode 100644 .convoys/add-rate-limiting/brief-1-extend-rate-limit-and-wire-routes.md diff --git a/.convoys/add-rate-limiting.md b/.convoys/add-rate-limiting.md index 89a8356..24ccb66 100644 --- a/.convoys/add-rate-limiting.md +++ b/.convoys/add-rate-limiting.md @@ -14,7 +14,7 @@ skip: - role-a11y-auditor - role-ux-reviewer - role-ia-architect -status: queued +status: in-progress created: 2026-05-24 parent: ship-readiness addresses: P0 #6 (PARTIAL → RESOLVED) — the LAST open P0 @@ -340,3 +340,337 @@ The convoy is shippable when ALL of the following hold: - **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 ``, do NOT touch the `popularSets` or any other UI logic. + +**Verbatim post-edit shape (fetch call only):** + +```js + const response = await fetch(endpoint, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Authorization': `Bearer ${localStorage.getItem('auth_token')}`, + }, + body: JSON.stringify({ setCode: setCode.trim() }), + }); +``` + +Acceptance: + +- [ ] Net diff: +1 line (the `Authorization` header entry inside the `headers:` object on line 45-47). 0 deletions. +- [ ] The `'auth_token'` localStorage key matches every other authenticated fetch in the codebase (`components/ShareModal.js` line 44 + 67, etc.). Do NOT use a different key. +- [ ] The 309-line component otherwise stays byte-identical. No refactor of the `` wrapper, the `dynamic(... { ssr: false })` export, the `useState` block, the popular-sets grid, or the result-display logic. + +### `.cursor/rules/api-routes.mdc` (modified) + +Replace the existing § "Rate limiting" subsection (lines 104-135 in the current file). Keep every other section byte-identical. The updated subsection: + +````markdown +## Rate limiting + +`lib/rate-limit.js` exposes five named limiters, one per route class. Each named export takes `req` (and `userId` for user-keyed classes) and returns `{ allowed, remaining, reset }`. + +| Class | Limit | Window | Key | Used by | Helper | +| --- | --- | --- | --- | --- | --- | +| `auth` | 5 | 15 min | IP | `/api/auth/login`, `/api/auth/register` | `checkAuthRateLimit(req)` | +| `search` | 60 | 1 min | IP | `/api/users/search`, `/api/cards/search` | `checkSearchRateLimit(req)` | +| `upload` | 10 | 1 hour | user | `/api/user/avatar` | `checkUploadRateLimit(req, userId)` | +| `generate` | 5 | 1 hour | user | `/api/user/avatar/generate` | `checkGenerateRateLimit(req, userId)` | +| `import` | 5 | 1 hour | user | `/api/cards/import-mtg`, `/api/cards/import-pokemon`, `/api/cards/import-lorcana` | `checkImportRateLimit(req, userId)` | + +**Verbatim call shape** (identical across all five classes — only the helper name and the optional `userId` argument differ): + +```js +import { checkSearchRateLimit } from '../../../lib/rate-limit.js'; + +export default async function handler(req, res) { + if (req.method !== 'GET') { + return res.status(405).json({ error: 'Method not allowed' }); + } + + // For user-keyed classes, auth check goes HERE first; see "Gate ordering" below. + + const { allowed, reset } = await checkSearchRateLimit(req); + if (!allowed) { + res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000)); + return res.status(429).json({ error: 'Too many attempts. Try again later.' }); + } + + try { + // ... handler body ... + } catch (err) { + // ... + } +} +``` + +**Gate ordering rules:** + +1. **Method check first.** Reject the wrong verb with 405 before doing any limiter work. +2. **Auth check before any user-keyed limiter.** `extractUserIdentifier(userId)` THROWS when `userId` is null/undefined/empty (defensive). For `upload`, `generate`, and `import`, the handler MUST call `getUserFromRequest(req)` (or equivalent JWT verification) and confirm a non-null user BEFORE calling the limiter. Wrong order = anonymous user bypasses (the THROW surfaces immediately during dev; do not catch and silently fall back to IP). +3. **For IP-keyed limiters (`auth`, `search`), gate placement is flexible** — either at the top of the handler (after the method check) or after a separate auth check that the route happens to also have (e.g. `users/search` JWT-verifies before rate-limiting, both are correct). The limiter only needs `req` for IP extraction. +4. **Admin-role check, if applicable, goes between auth and rate-limit.** Used by all three `/api/cards/import-*` routes: `if (user.role !== 'admin') return res.status(403).json({ error: 'Admin access required' })` sits between the `if (!user)` 401 and the import rate-limit call. + +**Identifier extraction:** + +- `extractIpIdentifier(req)` (module-private) — first hop in `x-forwarded-for` (Vercel's edge), falling back to `req.socket.remoteAddress`, falling back to the literal `'anonymous'`. Do NOT key off `req.body.email` (rotates) or `req.headers.authorization` (unauthenticated endpoints don't have one). +- `extractUserIdentifier(userId)` (module-private) — formats as `user:${userId}`. Throws on null/undefined/empty/NaN to surface gate-ordering bugs at dev time rather than silently falling back to IP and creating a per-IP-not-per-user limit. + +**Env vars (unchanged from Brief 4):** `KV_REST_API_URL` + `KV_REST_API_TOKEN` (auto-provisioned by Vercel's Upstash Marketplace integration). In prod, missing either var is a **fail-closed throw** on the first call. In dev / test, the module warn-and-no-ops so local work isn't blocked. See `AGENTS.md` Gotcha #12 for the full env-var contract. + +**429 response shape is uniform across all five classes.** Same error message (`'Too many attempts. Try again later.'`) and same `Retry-After` header calculation. Per-class variation would fingerprint the limits to an attacker. + +**Fail-open on Upstash outage.** A network failure inside `ratelimit.limit(...)` returns `{ allowed: true, remaining: Infinity, reset: 0 }` with a single `console.error('[rate-limit]', err)`. Reasoning: a hard Upstash outage should not lock the entire user base out of every gated route. Brute-force / abuse protection lives behind defense-in-depth (Vercel firewall, future fail2ban-style lockout). +```` + +Acceptance: + +- [ ] Replace lines 104-135 (the existing § "Rate limiting" subsection — starts with `## Rate limiting` and ends right before `## Dev/test endpoints (removed)`). Use the verbatim content above. +- [ ] Every OTHER section in the file stays byte-identical. No edits to § Authentication & Authorization, § Request validation, § Method gating, § Error handling, § Database access, § Response shape, § Activity logging, § Dev/test endpoints, or § CORS. +- [ ] The Markdown table renders cleanly (5 columns: Class | Limit | Window | Key | Used by | Helper — 6 columns actually, count the pipes; ensure alignment). +- [ ] No mention of the now-stale `Sweeping the rest of the API ... is the queued add-rate-limiting convoy` line — that sentence in the current rule gets replaced by the full new content. + +### Cross-file checks + +- [ ] **`npm run lint` exit code unchanged.** The current baseline is `✖ 128 problems (81 errors, 47 warnings)` (per `bump-next-js` Decision D + `fix-lint-baseline` tracking). Each per-route edit is an import + a small gate block — no new `react-hooks/*` paths, no new unused vars, no new `no-img-element` triggers. If the count grows, investigate before commit. +- [ ] **`npm run test:run` (vitest) passes 21/21.** No test file is touched in this convoy. The lib refactor preserves `checkAuthRateLimit(req)`'s return shape so any indirect dependency is irrelevant; architect verified at brief time that no current vitest spec actually imports `lib/rate-limit.js` (the convoy file's stale claim about transitive loading is corrected in Decision 6). +- [ ] **`npm run build` exit 0.** Turbopack compile time should be unchanged. The 10 modified files still compile to the same shape. +- [ ] **`npm run test:smoke` against the Vercel preview passes 3/3.** None of the 3 smoke tests (`'home redirects or renders without 5xx'`, `'sign-in page renders'`, `'public health endpoint responds'`) hit any of the 7 gated endpoints, so no smoke regression. Verify in CI on PR push. +- [ ] **Repo-wide grep clean.** After the sweep: + ```bash + rg "checkAuthRateLimit" pages/api/ + ``` + Expected: 2 matches (`login.js` + `register.js`) — same as before this convoy. + + ```bash + rg "checkSearchRateLimit|checkUploadRateLimit|checkGenerateRateLimit|checkImportRateLimit" pages/api/ + ``` + Expected: 6 matches total (search-2, upload-1, generate-1, import-3 — matching the 7 surfaces; users/search counts as search-1, cards/search counts as search-2). + +- [ ] **Per-class prefix uniqueness check.** + ```bash + rg "tcgvault:" lib/rate-limit.js | sort -u + ``` + Expected: 5 distinct lines, one per class (`tcgvault:auth`, `tcgvault:search`, `tcgvault:upload`, `tcgvault:generate`, `tcgvault:import`). No duplicates. + +- [ ] **Auth-then-rate-limit ordering check** for user-keyed routes. In `import-mtg.js`, `import-pokemon.js`, `import-lorcana.js`, `avatar.js`, `avatar/generate.js`: visually confirm via `git diff` that `getUserFromRequest` (or equivalent) appears BEFORE the `check*RateLimit` call. If the order is reversed, the `extractUserIdentifier` throw fires on every anonymous request — the test would surface as a 500 in dev, but better to never ship that shape. + +## Manual verification (in addition to CI on push) + +Run these in order. Paste relevant output (with secrets redacted) into the PR description. + +- [ ] **Local dev boot.** + ```bash + npm run dev + ``` + Expected: clean boot, no `[rate-limit]` warn-spam at startup (the limiter is lazy-init; no warn until first gated call). If `KV_REST_API_*` env vars are absent in `.env.local`, the first request to ANY gated route will emit one `[rate-limit] KV_REST_API_URL / KV_REST_API_TOKEN not set — rate limiting disabled (dev/test only)` line — that's correct dev-mode behavior. + +- [ ] **Auth limiter regression check** — Brief 4's contract must be preserved. + ```bash + for i in 1 2 3 4 5 6; do + curl -sS -o /dev/null -w "POST /api/auth/login attempt $i: %{http_code}\n" \ + -X POST -H "Content-Type: application/json" \ + -d '{"email":"nobody@example.com","password":"wrong"}' \ + http://localhost:3000/api/auth/login + done + ``` + Expected (only meaningful with `KV_REST_API_*` set): + - Attempts 1-5: `401` + - Attempt 6: `429` with `Retry-After` header + + Without Upstash configured locally, all 6 will return `401` — the dev-mode no-op limiter — and that's also correct. Either outcome confirms `checkAuthRateLimit` still works through the refactored module. + +- [ ] **Search limiter (anonymous, IP-keyed).** Without Upstash, this should never 429 in dev: + ```bash + for i in $(seq 1 5); do + curl -sS -o /dev/null -w "GET /api/cards/search: %{http_code}\n" \ + "http://localhost:3000/api/cards/search?query=test" + done + ``` + Expected: `200` each call (dev-mode noop). The gate is wired but won't fire without Upstash. To exercise the live path, set `KV_REST_API_*` and burst >60 in <60s. + +- [ ] **User-keyed limiter — verify gate-ordering throws on misuse.** This is a one-shot sanity check that the `extractUserIdentifier` throw fires when called pre-auth. Boot dev, then: + ```bash + node -e " + const { checkUploadRateLimit } = require('./lib/rate-limit.js'); + checkUploadRateLimit({ headers: {} }, null).catch(err => { + console.log('OK - throw fired:', err.message.startsWith('[rate-limit] extractUserIdentifier')); + }); + " + ``` + Expected: `OK - throw fired: true`. (If you get an `ERR_REQUIRE_ESM` error, use `node --experimental-vm-modules` or write a tiny `.mjs` wrapper — the module is ESM. The point is the throw, not the invocation shape.) + +- [ ] **Admin-only enforcement on import routes** — anonymous → 401, authenticated-non-admin → 403, authenticated-admin → 200 (or whatever the import returns). + ```bash + # 1. Anonymous: + curl -sS -o /dev/null -w "anonymous import-mtg: %{http_code}\n" \ + -X POST -H "Content-Type: application/json" \ + -d '{"setCode":"neo"}' \ + http://localhost:3000/api/cards/import-mtg + # Expected: 401 + + # 2. Auth'd non-admin (use a regular user's token): + curl -sS -o /dev/null -w "user import-mtg: %{http_code}\n" \ + -X POST -H "Content-Type: application/json" \ + -H "Authorization: Bearer " \ + -d '{"setCode":"neo"}' \ + http://localhost:3000/api/cards/import-mtg + # Expected: 403 + + # 3. Auth'd admin: (optional — actually triggers Scryfall fetch + DB writes; skip + # unless you're staging-pointed and want to exercise the full happy path): + curl -sS -w "admin import-mtg: %{http_code}\n" \ + -X POST -H "Content-Type: application/json" \ + -H "Authorization: Bearer " \ + -d '{"setCode":"neo"}' \ + http://localhost:3000/api/cards/import-mtg + # Expected: 200 with {imported, skipped, total} + ``` + + Repeat for `import-pokemon` and `import-lorcana`. The first two checks (anonymous + non-admin) are the meaningful security check; the admin check is optional smoke and SHOULD ONLY run against a staging DB per `.cursor/rules/no-go-zones.mdc`. + +- [ ] **Admin UI smoke** — log in as admin in the browser, visit `/admin/card-import`, type a set code (e.g. `neo` for MTG), click "Import Cards". Expected: the request succeeds (or returns whatever Scryfall would return). If the request 401s, the `pages/admin/card-import.js` Bearer-token edit didn't land — check the browser's network tab for the Authorization header on the POST. + +- [ ] **Avatar upload smoke** — log in, visit `/profile` (or wherever the avatar uploader lives), upload an image. Expected: success. Then submit the form 11 times in <1 hour to verify the gate fires (with Upstash configured); without Upstash, no 429 in dev. + +- [ ] **Vitest pass count unchanged.** + ```bash + npm run test:run 2>&1 | tail -5 + ``` + Expected: `Tests 21 passed (21)`. If the count or any individual test changes, the lib refactor broke a contract — investigate immediately. Most likely culprit: `checkAuthRateLimit`'s return shape drifted from `{ allowed, remaining, reset }`. + +- [ ] **Diff hygiene.** `git diff main..HEAD --stat` should show: + - `lib/rate-limit.js`: ~70 lines + / ~5 lines - (net add of ~65 lines). + - 7 source-file additions (~14 lines + / ~0 lines - each): `users/search.js` (~6/0), `cards/search.js` (~6/0), `user/avatar.js` (~6/0), `user/avatar/generate.js` (~6/0), `cards/import-mtg.js` (~16/0), `cards/import-pokemon.js` (~16/0), `cards/import-lorcana.js` (~16/0). + - `pages/admin/card-import.js`: +1 / 0 lines. + - `.cursor/rules/api-routes.mdc`: ~70 lines + / ~32 lines - (replacing the existing § Rate limiting subsection). + - No whitespace-only changes elsewhere. + +## Boot-the-brief findings (preempted by the architect; do not re-investigate) + +### Finding 1 — `pages/admin/card-import.js` does NOT currently send the Bearer token + +Architect read the file at brief time (309 lines). Line 43-49 calls `fetch(endpoint, { method: 'POST', headers: { 'Content-Type': 'application/json' } })` with NO `Authorization` header. Without the fix in this brief, the moment the import APIs gain `getUserFromRequest`, the admin UI starts returning 401 on every import attempt. This is the documented scope expansion under Decision 1 — it's not optional, the brief is internally consistent only with both edits (API gate + admin UI fix) shipping together. + +### Finding 2 — `import-lorcana.js` has zero frontend callers + +Architect ran `rg 'import-lorcana' pages/ components/` and only `pages/admin/card-import.js` matched — but that match is in a comment / file-listing context, not a code-execution call (the `` in `pages/admin/card-import.js`. The only edit to that file is the Bearer-token addition on line 45-47. +- [ ] Do NOT extract a `withAdmin(handler)` wrapper from the three new admin-role checks. The convention rule says to extract when a third call site appears; these ARE the third+fourth+fifth, but extraction is its own auth-surface refactor (queued `single-auth-provider` adjacent). Inline for this convoy. +- [ ] Do NOT touch `AGENTS.md` Gotcha #12. Doc-writer pass at convoy close owns the update; preempting here creates merge conflicts. +- [ ] Do NOT touch `.github/workflows/ci.yml`. No new CI gate is added in this convoy (per-class rate-limit wiring isn't grep-checkable; the existing `forbidden-endpoints` + `forbidden-cors-headers` jobs suffice for the API surface). +- [ ] Do NOT bump `@upstash/ratelimit` or `@upstash/redis` versions. Pins stay at `^2.0.8` and `^1.38.0` from Brief 4. +- [ ] Do NOT add `KV_REST_API_*` to `test/setup.js`. The warn-and-noop branch is the correct test behavior. + +## Rationale (≤3 sentences) + +Extending `lib/rate-limit.js` from one auth-only limiter to five named per-class limiters closes the last open P0 (#6 PARTIAL → RESOLVED) by wiring rate-limit + auth+admin gates into the remaining abusable surface; the hybrid named-export shape preserves Brief 4's `checkAuthRateLimit(req)` contract so `login.js` + `register.js` stay untouched. Auth-gating the three `pages/api/cards/import-*.js` routes (currently anonymous, hitting external Scryfall / Pokémon-TCG / Lorcana APIs with no caller throttling AND performing unbounded DB writes) is the security-critical scope expansion under Decision 1; adding the matching Bearer-token send to `pages/admin/card-import.js` is the necessary admin-UI fix to keep the gated APIs callable. Once this lands, the launch-readiness ship-blocker list is empty (8 of 8 RESOLVED), and the per-class shape is documented in `.cursor/rules/api-routes.mdc` for any future route to follow without architect re-derivation.