--- convoy: add-rate-limiting brief_number: 1 depends_on: [] files: - lib/rate-limit.js - pages/api/users/search.js - pages/api/cards/search.js - pages/api/user/avatar.js - pages/api/user/avatar/generate.js - pages/api/cards/import-mtg.js - pages/api/cards/import-pokemon.js - pages/api/cards/import-lorcana.js - pages/admin/card-import.js - .cursor/rules/api-routes.mdc --- # Brief 1: Extend `lib/rate-limit.js` to named per-class limiters + wire into the remaining abusable endpoints + gate the import routes ## Goal (1 sentence) Refactor `lib/rate-limit.js` from a single auth-only limiter into a Map-of-named-limiters shape (preserving the `checkAuthRateLimit(req)` contract that `login.js` + `register.js` depend on per Brief 4), add four new named exports — `checkSearchRateLimit(req)`, `checkUploadRateLimit(req, userId)`, `checkGenerateRateLimit(req, userId)`, `checkImportRateLimit(req, userId)` — wire each into the appropriate handler at the documented gate-ordering (auth before rate-limit for user-keyed classes), add `getUserFromRequest` + admin-role check + import rate-limit to the three currently-anonymous `pages/api/cards/import-*.js` routes, fix `pages/admin/card-import.js` to send the Bearer token the newly-gated import routes require, and extend `.cursor/rules/api-routes.mdc` § Rate limiting with the per-class pattern + a per-class limit table. ## Files in scope (do not edit anything else) The 10 files listed in `files:` above (all modified, no new files, no deletions). **Files explicitly out of scope** (do not touch even if it seems related): - `pages/api/auth/login.js`, `pages/api/auth/register.js` — already wired by `fix-auth-bypass` Brief 4. **Verify post-edit that they still work** (call `checkAuthRateLimit(req)` against the refactored module), but do NOT modify them. - `lib/permission-middleware.js`, `lib/auth-secret.js`, `pages/api/auth-utils.js` — auth surface is untouched by this convoy. - `package.json`, `package-lock.json` — `@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0` are already installed (Brief 4). No new dependencies. No version bumps. - `AGENTS.md` — Gotcha #12 documents the env-var requirement; the **doc-writer pass at convoy close** will update the gotcha to reflect the new per-class limits. Do NOT preempt that edit here. - `.github/workflows/ci.yml` — no new CI gate is added. The `forbidden-endpoints` + `forbidden-cors-headers` jobs already defend the API surface; per-class rate-limit wiring isn't grep-checkable. - `test/**` — no new per-route handler tests in this convoy (Decision 6 below). Vitest 21/21 must still pass with no spec changes. - `tests/smoke/**`, `tests/visual/**` — smoke + visual suites don't exercise any of these endpoints; do NOT modify. - `pages/api/cards/search.js`'s SQL — the file has a known god-function shape with 7+ conditional SQL branches (`SELECT * FROM cards WHERE …` repeated). That's `god-function-split` / `refactor-cards-search-sql` scope, NOT here. Do NOT touch any of the SQL branches; only add the rate-limit gate at the top. - `pages/api/user/avatar.js`'s `parseMultipartFormData` body-streaming behavior — the 5MB multipart body is consumed before any rate-limit gate could short-circuit, meaning an attacker can still exhaust the 5MB body even on a 429 path. That's `harden-multipart-parser` scope (queued as a follow-up); the gate-ordering in this brief is purely defensive (rate-limit BEFORE the method branches so the gate fires before the body parse). - `scripts/import-*.js` — standalone scripts independent of the API routes; do NOT touch. - Any other `pages/api/**/*.js` file. The convoy scope is the 7 surfaces listed in `.convoys/add-rate-limiting.md` § Scope. ## Conventions to follow ### Decisions from the convoy file (cite when implementing) - **D1 (operator-ratified):** Option A — gate all three `pages/api/cards/import-*.js` routes in this convoy with `getUserFromRequest` + admin-role check + per-user rate-limit. ALSO update `pages/admin/card-import.js` to send the `Authorization: Bearer ${localStorage.getItem('auth_token')}` header on the import fetch (necessary scope expansion — without it, the gated APIs immediately break the admin UI). Lorcana is gated defensively even though no current frontend caller exists; future cleanup convoy can delete if it stays unused. - **D2 (architect-self-ratified):** Hybrid named-limiter shape — preserve `checkAuthRateLimit(req)` (Brief 4 contract, used by login + register), add four new named functions (`checkSearchRateLimit`, `checkUploadRateLimit`, `checkGenerateRateLimit`, `checkImportRateLimit`). Internal `Map` cache, distinct Redis prefix per class. - **D3 (architect-self-ratified):** Per-class limits — `auth` 5/15min IP (unchanged), `search` 60/1min IP, `upload` 10/1hour user, `generate` 5/1hour user, `import` 5/1hour user. Search raised from parent's 30 because `components/ShareModal.js`'s `handleSearch` fires on every keystroke (no debounce); typing a 17-char email = 17 requests in <5s, which would 429 at 30/1min. Generate raised from parent's 3 because `pages/api/user/avatar/generate.js` calls DiceBear (free public API), not OpenAI/Replicate; cost is just Vercel blob storage + DiceBear-side throttling. - **D4 (architect-self-ratified):** Two-extractor shape — `extractIpIdentifier(req)` (existing) + `extractUserIdentifier(userId)` (new). `extractUserIdentifier` **throws** when `userId` is null/undefined/'' (defensive — if a future handler accidentally calls a user-keyed limiter before the auth check, the throw surfaces the misordering immediately rather than silently falling back to IP and quietly converting a per-user limit into a per-IP limit, which would lock out other household members for one user's behavior). Documented in the verbatim shape below. - **D5 (architect-self-ratified):** Uniform 429 message — `'Too many attempts. Try again later.'` matches `login.js` + `register.js` verbatim. Per-class variation would fingerprint which routes have which limits to an attacker. - **D6 (architect-self-ratified):** No new per-route handler tests in this convoy. Deferred to queued `fill-vitest-handler-coverage` (same reasoning as `cors-tighten` Decision D4). Vitest 21/21 MUST still pass after the lib refactor — verified at architect time that no current vitest spec transitively imports `lib/rate-limit.js` (only `login.js` + `register.js` import it, and neither is covered by vitest; the convoy file's claim that "auth-utils tests transitively load this module" is stale). ### Repo conventions (cite + match) - **`.cursor/rules/no-go-zones.mdc`.** None of the 10 source files are listed under no-go zones. The "Card-import jobs" entry warns *"Don't run them ad-hoc against prod data; use staging"* — this brief gates them with admin-role enforcement which **directly answers** that no-go-zones warning (only admins can trigger imports; non-admins get 403). - **`.cursor/rules/api-routes.mdc` § Rate limiting.** The existing pattern documents the `auth` class (login/register). This brief extends it with the four new classes; the verbatim updated content is in the Acceptance criteria § for `.cursor/rules/api-routes.mdc` below. Keep the existing § Authentication, § Request validation, § Method gating, § Error handling, § Database access, § Response shape, § Activity logging, § Dev/test endpoints, and § CORS subsections byte-identical — only § Rate limiting changes. - **`.cursor/rules/auth-and-permissions.mdc`.** Admin-role check uses `if (user.role !== 'admin')` directly (per the rule's "Admin-only" pattern: *"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 `withAdmin()` is its own convoy — for this brief, inline the check. - **Brief 4 precedent shape (`.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md`).** The verbatim 429 response shape comes from there: ```js const { allowed, reset } = await checkXxxRateLimit(req[, userId]); if (!allowed) { res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000)); return res.status(429).json({ error: 'Too many attempts. Try again later.' }); } ``` Apply this shape at each call site. **Do not deviate** — same error message, same `Retry-After` calculation, same status code. - **`@upstash/ratelimit` per-class prefix isolation.** Each class gets a distinct Redis key prefix (`tcgvault:auth`, `tcgvault:search`, `tcgvault:upload`, `tcgvault:generate`, `tcgvault:import`). Without distinct prefixes, hits on one class would consume the budget of another (e.g., a search hit would eat the auth budget for the same IP). Verified against `@upstash/ratelimit@2.0.8`'s `prefix:` option which scopes all keys with the given string. - **Lazy `init()` + fail-closed-in-prod / warn-and-noop-in-dev.** Both behaviors carry through unchanged from Brief 4. New limiters inherit them via the shared `init()` function. Do NOT reintroduce module-top-level `new Redis(...)` — it would throw at import time in any environment without `KV_REST_API_URL` / `KV_REST_API_TOKEN`, breaking local dev, vitest, and Vercel build-time bundling. - **`@vercel/postgres` tagged-templates only.** None of the per-route edits touch SQL. (`cards/search.js` is excluded from SQL refactoring per § Files explicitly out of scope.) ## Acceptance criteria ### `lib/rate-limit.js` (modified) Replace the current 70-line module with the verbatim shape below. The diff is mostly net-additive (~70 lines added, ~5 lines reshaped); the existing `init()`, `extractIdentifier()`, and `checkAuthRateLimit()` functions are conceptually preserved but restructured to share infrastructure across all five classes. **Verbatim new module shape:** ```js import { Ratelimit } from '@upstash/ratelimit'; import { Redis } from '@upstash/redis'; // Per-class limiter configuration. Distinct Redis prefix per class is // REQUIRED — without it, a search-class hit would consume the auth-class // budget for the same identifier. `slidingWindow` chosen across all // classes to match Brief 4's existing algorithm; switching to // `tokenBucket` per-class would be its own convoy. const LIMITER_CONFIG = { auth: { limit: 5, window: '15 m', prefix: 'tcgvault:auth' }, search: { limit: 60, window: '1 m', prefix: 'tcgvault:search' }, upload: { limit: 10, window: '1 h', prefix: 'tcgvault:upload' }, generate: { limit: 5, window: '1 h', prefix: 'tcgvault:generate' }, import: { limit: 5, window: '1 h', prefix: 'tcgvault:import' }, }; // Lazy singleton. Module-load init would throw in environments without // Upstash env vars (local dev pre-onboarding, tests that transitively // import the auth handlers, Vercel build-time bundling). Defer // construction until the first request actually arrives. let cached = null; function init() { // Env-var names match Vercel's Upstash Marketplace integration, which // auto-provisions KV_REST_API_URL and KV_REST_API_TOKEN. See // https://upstash.com/docs/redis/howto/vercelintegration. Single-source- // of-truth — do NOT alias to UPSTASH_REDIS_REST_*. const url = process.env.KV_REST_API_URL; const token = process.env.KV_REST_API_TOKEN; if (url && token) { const redis = new Redis({ url, token }); const instances = new Map(); for (const [name, cfg] of Object.entries(LIMITER_CONFIG)) { instances.set( name, new Ratelimit({ redis, limiter: Ratelimit.slidingWindow(cfg.limit, cfg.window), prefix: cfg.prefix, }) ); } return { mode: 'live', instances }; } if (process.env.NODE_ENV === 'production') { // Fail-closed in production. A single failed login is a better outcome // than silently disabling brute-force protection on the live site. throw new Error( '[rate-limit] Upstash not configured. Set KV_REST_API_URL and KV_REST_API_TOKEN in the deployment environment (auto-provisioned by the Vercel Upstash Marketplace integration) before serving auth traffic.' ); } console.warn( '[rate-limit] KV_REST_API_URL / KV_REST_API_TOKEN not set — rate limiting disabled (dev/test only)' ); return { mode: 'noop' }; } function extractIpIdentifier(req) { const xff = req.headers?.['x-forwarded-for']; const firstHop = Array.isArray(xff) ? xff[0] : xff?.split(',')[0]?.trim(); return firstHop || req.socket?.remoteAddress || 'anonymous'; } // THROWS on missing userId. Per-user limiters MUST sit AFTER the auth // check in the handler body — silently falling back to IP here would // convert a per-user limit into a per-IP limit, locking out other // household members for one user's behavior. The throw surfaces the // misordering immediately during development rather than at first // production incident. function extractUserIdentifier(userId) { if ( userId === null || userId === undefined || userId === '' || (typeof userId === 'number' && Number.isNaN(userId)) ) { throw new Error( '[rate-limit] extractUserIdentifier called without an authenticated userId. Place the rate-limit gate AFTER the auth check, never before.' ); } return `user:${userId}`; } async function check(className, identifier) { if (!cached) { cached = init(); } if (cached.mode === 'noop') { return { allowed: true, remaining: Infinity, reset: 0 }; } const limiter = cached.instances.get(className); if (!limiter) { throw new Error(`[rate-limit] Unknown limiter class: ${className}`); } try { const { success, remaining, reset } = await limiter.limit(identifier); return { allowed: success, remaining, reset }; } catch (err) { // Fail-open on Upstash outage. A hard outage at the rate-limit backend // should not lock the entire user base out. Brute-force protection // lives behind defense-in-depth (Vercel firewall, etc.). console.error('[rate-limit]', err); return { allowed: true, remaining: Infinity, reset: 0 }; } } export async function checkAuthRateLimit(req) { return check('auth', extractIpIdentifier(req)); } export async function checkSearchRateLimit(req) { return check('search', extractIpIdentifier(req)); } export async function checkUploadRateLimit(req, userId) { return check('upload', extractUserIdentifier(userId)); } export async function checkGenerateRateLimit(req, userId) { return check('generate', extractUserIdentifier(userId)); } export async function checkImportRateLimit(req, userId) { return check('import', extractUserIdentifier(userId)); } ``` Acceptance: - [ ] File ends up as the verbatim shape above (whitespace and comments preserved). 2-space indent. ESM. No default export. - [ ] **`checkAuthRateLimit(req)` return shape is byte-identical to Brief 4's** — `{ allowed: boolean, remaining: number, reset: number }`. `login.js` + `register.js` MUST continue to work without any change to their import or call shape. - [ ] No top-level `await`. No module-load `new Redis(...)`. The `cached = null` declaration is the only top-level side effect. - [ ] `LIMITER_CONFIG` keys are exactly `auth`, `search`, `upload`, `generate`, `import` — five entries, no more, no less. - [ ] Each `LIMITER_CONFIG[*].prefix` is unique and follows the `tcgvault:` pattern. - [ ] `extractUserIdentifier(userId)` THROWS the documented error message on `null`, `undefined`, empty string, or `NaN`. (Numeric `0` is technically valid — there's no user with ID 0 in the schema, but the check is defensive against future ID types; the conditional explicitly does NOT throw on `0` because `0 === null` is false and `0 === undefined` is false. This is intentional — if a future change introduces user ID 0 the limiter still keys correctly.) - [ ] `check('unknown-class', ...)` throws `[rate-limit] Unknown limiter class: unknown-class` (defensive; should never fire in shipped code). - [ ] The five exported `check*RateLimit` functions are the ONLY exports. No legacy `extractIdentifier` re-export — it's been renamed to `extractIpIdentifier` and is module-private. ### `pages/api/users/search.js` (modified) Add a new import and a new rate-limit gate after the JWT verify success, before the query-length validation. The route uses inline `jwt.verify` (not `getUserFromRequest`) but that doesn't matter — the search class is **IP-keyed**, not user-keyed, so the gate doesn't need the user id. **Verbatim post-edit shape:** ```js import { sql } from '@vercel/postgres'; import jwt from 'jsonwebtoken'; import { JWT_SECRET } from '../../../lib/auth-secret.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' }); } // Verify authentication const authHeader = req.headers.authorization; if (!authHeader || !authHeader.startsWith('Bearer ')) { return res.status(401).json({ error: 'Authentication required' }); } const token = authHeader.substring(7); try { jwt.verify(token, JWT_SECRET); } catch (error) { return res.status(401).json({ error: 'Invalid token' }); } 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.' }); } const { q: query } = req.query; if (!query || query.length < 2) { return res.status(400).json({ error: 'Query must be at least 2 characters' }); } try { // Search users by email (partial match) const result = await sql` SELECT id, email, role, created_at FROM users WHERE email ILIKE ${`%${query}%`} ORDER BY email LIMIT 10 `; res.status(200).json({ users: result.rows }); } catch (error) { console.error('User search error:', error); res.status(500).json({ error: 'Internal server error' }); } } ``` Acceptance: - [ ] One new import line: `import { checkSearchRateLimit } from '../../../lib/rate-limit.js';` (relative path matches the existing `../../../lib/auth-secret.js` precedent on line 3). - [ ] Gate sits between the JWT-verify try/catch (lines 17-21) and the query-length validation (line 25). NOT inside the JWT try block. - [ ] Net diff: +1 import, +5 lines (the gate block), 0 deletions, 0 reorderings. ### `pages/api/cards/search.js` (modified) The route is anonymous-by-design (cards are a public catalogue). Gate at the very top of the handler, after the method check, before the existing `try` block. **IP-keyed.** **Verbatim post-edit shape (top of file only):** ```js import { sql } from '@vercel/postgres'; 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' }); } 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 { const { query = '', // ... rest of the file unchanged ... ``` Acceptance: - [ ] One new import line. Relative path `'../../../lib/rate-limit.js'`. - [ ] Gate sits between the method check (lines 4-6) and the `try` block (current line 8). - [ ] **The 240-line SQL god-function inside the try block is BYTE-IDENTICAL post-edit.** Do NOT touch any of the 7 conditional SQL branches, the filter object, the response shape, or the closing `catch`. The only diff is +1 import and +5 lines for the gate block. - [ ] Do NOT add `getUserFromRequest` to this route. It's anonymous-by-design per the convoy file's "Known constraints" § *"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)."* ### `pages/api/user/avatar.js` (modified) The handler has a structure with NO top-level method gate; method-branches inside the outer try block. Auth check sits inside the try (lines 16-19). Gate goes AFTER the auth check, BEFORE the method-branching (`if (req.method === 'POST')` at line 21), so both the POST upload AND the DELETE branches inherit the limit. **User-keyed**, passing `user.userId`. **Verbatim post-edit shape (auth + gate region only):** ```js import { put, del } from '@vercel/blob'; import { sql } from '@vercel/postgres'; import { getUserFromRequest } from '../../../lib/permission-middleware'; import { checkUploadRateLimit } from '../../../lib/rate-limit.js'; export const config = { api: { bodyParser: { sizeLimit: '5mb', }, }, }; export default async function handler(req, res) { try { // Get authenticated user const user = await getUserFromRequest(req); if (!user) { return res.status(401).json({ error: 'Authentication required' }); } const { allowed, reset } = await checkUploadRateLimit(req, user.userId); if (!allowed) { res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000)); return res.status(429).json({ error: 'Too many attempts. Try again later.' }); } if (req.method === 'POST') { // Handle avatar upload // ... rest of the file unchanged ... ``` Acceptance: - [ ] One new import line: `import { checkUploadRateLimit } from '../../../lib/rate-limit.js';`. - [ ] Gate sits between the `if (!user)` 401 (line 17-19) and the `if (req.method === 'POST')` branch (line 21). - [ ] Gate fires BEFORE `parseMultipartFormData(req)` runs. The body-streaming bypass concern (5MB consumed before the gate) is acknowledged out-of-scope (see § Files explicitly out of scope) — but the gate ordering itself MUST be correct so that future hardening of the body parser doesn't need to also reorder the gate. - [ ] Net diff: +1 import, +5 lines, 0 deletions. The POST branch, DELETE branch, helper functions (`parseMultipartFormData`, `deleteOldAvatar`), and the `config` export are byte-identical. ### `pages/api/user/avatar/generate.js` (modified) The handler HAS a top-level method gate (`if (req.method !== 'POST')` at line 6). Auth check sits inside the try block (lines 12-15). Gate goes AFTER the auth check, BEFORE the SQL query that fetches user data (line 18). **User-keyed**, passing `user.userId`. **Verbatim post-edit shape (top of handler only):** ```js import { put } from '@vercel/blob'; import { sql } from '@vercel/postgres'; import { getUserFromRequest } from '../../../../lib/permission-middleware'; import { checkGenerateRateLimit } from '../../../../lib/rate-limit.js'; export default async function handler(req, res) { if (req.method !== 'POST') { return res.status(405).json({ error: 'Method not allowed' }); } try { // Get authenticated user const user = await getUserFromRequest(req); if (!user) { return res.status(401).json({ error: 'Authentication required' }); } const { allowed, reset } = await checkGenerateRateLimit(req, user.userId); if (!allowed) { res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000)); return res.status(429).json({ error: 'Too many attempts. Try again later.' }); } // Get user information for avatar generation const userResult = await sql` SELECT email, first_name, last_name, username FROM users WHERE id = ${user.userId} `; // ... rest of the file unchanged ... ``` Acceptance: - [ ] One new import line: `import { checkGenerateRateLimit } from '../../../../lib/rate-limit.js';` (note FOUR `../` levels — this file is at `pages/api/user/avatar/generate.js`). - [ ] Gate sits between the `if (!user)` 401 (lines 13-15) and the user-data SQL query (current line 18). - [ ] Net diff: +1 import, +5 lines, 0 deletions. ### `pages/api/cards/import-mtg.js` (modified) Currently has NO auth, NO rate-limit. Add three gates in order: method check (already present), auth check (NEW), admin-role check (NEW), import rate-limit (NEW). **User-keyed**, passing `user.userId`. **Verbatim post-edit shape (top of handler only):** ```js import { sql } from '@vercel/postgres'; import { getUserFromRequest } from '../../../lib/permission-middleware'; import { checkImportRateLimit } from '../../../lib/rate-limit.js'; export default async function handler(req, res) { if (req.method !== 'POST') { return res.status(405).json({ error: 'Method not allowed' }); } const user = await getUserFromRequest(req); if (!user) { return res.status(401).json({ error: 'Authentication required' }); } if (user.role !== 'admin') { return res.status(403).json({ error: 'Admin access required' }); } const { allowed, reset } = await checkImportRateLimit(req, user.userId); 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 { const { setCode } = req.body; // ... rest of the file unchanged ... ``` Acceptance: - [ ] Two new import lines (one for `getUserFromRequest`, one for `checkImportRateLimit`). Relative paths `'../../../lib/permission-middleware'` and `'../../../lib/rate-limit.js'` — verified at architect time against the directory depth. - [ ] All three gates sit BEFORE the existing `try` block (current line 8). Order: method → auth → admin → rate-limit. - [ ] Net diff: +2 imports, +14 lines, 0 deletions. The Scryfall fetch + INSERT loop + response shape are byte-identical. ### `pages/api/cards/import-pokemon.js` (modified) Same shape as `import-mtg.js` — three new gates added before the existing `try` block (current line 50). The `delay` + `fetchWithRetry` helpers above the handler stay unchanged. Acceptance: - [ ] Two new import lines, same paths as `import-mtg.js`. - [ ] All three gates sit BEFORE the `try` block (current line 50), AFTER the method check (current lines 46-48). - [ ] Net diff: +2 imports, +14 lines, 0 deletions. The `fetchWithRetry` + `delay` helpers + Pokemon-TCG fetch + INSERT loop + response shape are byte-identical. ### `pages/api/cards/import-lorcana.js` (modified) Same shape as `import-mtg.js` — three new gates added before the existing `try` block (current line 50). Despite having NO frontend caller today (architect-verified: `rg 'import-lorcana' pages/ components/` returns zero matches in source code), gate defensively so a future Lorcana admin UI addition inherits the protection automatically. The `delay` + `fetchWithRetry` helpers above the handler stay unchanged. Acceptance: - [ ] Two new import lines, same paths as `import-mtg.js`. - [ ] All three gates sit BEFORE the `try` block (current line 50), AFTER the method check (current lines 46-48). - [ ] Net diff: +2 imports, +14 lines, 0 deletions. ### `pages/admin/card-import.js` (modified — scope expansion for D1) The admin UI currently calls `fetch(endpoint, { method: 'POST', headers: { 'Content-Type': 'application/json' } })` with NO Authorization header (line 43-49). Adding `getUserFromRequest` to the import APIs would 401 the admin UI on first run. Add the Bearer token to the fetch call. **This is the only edit to this file** — do NOT refactor the 309-line god-component, do NOT add Lorcana to the `` in card-import.js only has `mtg` and `pokemon` options). So gating Lorcana is purely defensive. A future cleanup convoy may delete `pages/api/cards/import-lorcana.js` if it's never wired up; for now, gating with the same shape as mtg/pokemon is the smaller-diff path. ### Finding 3 — `components/ShareModal.js`'s user-search has NO debounce Architect read `components/ShareModal.js::handleSearch` (lines 56-77). It calls `fetch('/api/users/search?q=...')` on every keystroke when `query.length >= 2`. Typing a 17-char email like `alice@example.com` fires 16 requests within ~3 seconds (one per char after the 2-char minimum). Parent's recommendation of `search: 30 / 1min` would 429 on a single legitimate email entry. **Tuned up to 60 / 1min** in Decision 3 to fit the realistic burst pattern without blocking the search-as-you-type UX. A future client-side fix (adding debounce in ShareModal) would let us re-tighten this; queue as `debounce-share-modal-search` if it surfaces. ### Finding 4 — `pages/api/user/avatar/generate.js` uses DiceBear, not a paid AI service Architect read the file (133 lines). It calls `https://api.dicebear.com/7.x/${avatarStyle}/svg?...` — free public API for SVG initials avatars. No OpenAI / Anthropic / Replicate cost. The "cost" of abuse is Vercel blob storage (the generated SVG gets `put()` into blob storage on every successful call) + DiceBear's own rate-limiting if we hammer them. **Tuned generate up to 5 / 1hour** from parent's 3 — still catches accidental loops (user mashing "regenerate avatar" button) without blocking legitimate "I want to try 4 different seeds" workflow. ### Finding 5 — `pages/api/cards/search.js` is a 240-line SQL god-function (do NOT refactor) Architect read the file in full. Lines 40-186 are seven conditional SQL branches plus a fallback JS-filter path. Already flagged in `.convoys/ship-readiness.md` as `god-function-split` / `refactor-cards-search-sql` scope. The brief is explicit: only add the rate-limit gate at the top, do NOT touch any SQL. The implementer MUST resist the urge to "clean up while I'm in here" — that's a separate convoy with its own architect pass. ### Finding 6 — `pages/api/user/avatar.js` has NO top-level method gate; method-branches inside the outer `try` Architect read the file (209 lines). Handler structure is `try { getUserFromRequest; if POST {...} else if DELETE {...} else 405 }` — the method check is the LAST branch, after both POST and DELETE bodies. This is unusual but the brief accommodates it by placing the rate-limit gate AFTER the auth check, BEFORE the method-branching. Both POST and DELETE branches inherit the limit. (DELETE is rare — only fires on "remove my avatar" — so the upload limit applying to both is fine.) Do NOT restructure the handler to add a top-level method gate; that's a cosmetic refactor and out of scope. ### Finding 7 — `pages/api/users/search.js` uses inline `jwt.verify`, not `getUserFromRequest` Architect read the file. Lines 17-21 do `const token = authHeader.substring(7); try { jwt.verify(token, JWT_SECRET); } catch { 401; }` — but the verified `decoded` payload is discarded (the route doesn't need the user ID, only the proof of auth). For the IP-keyed search limiter, we don't need the user either — the gate just goes after the JWT-verify catch block, before the query-length validation. **Do NOT refactor to use `getUserFromRequest`** — that's its own auth-surface convoy (queued `single-auth-provider`). ### Finding 8 — Vitest does NOT currently transitively import `lib/rate-limit.js` The convoy file claims `lib/rate-limit.js` is "unit-tested transitively via the existing vitest suite" — that's stale. Architect ran `rg 'rate-limit|@upstash' test/` → zero matches. Only `pages/api/auth/login.js` + `register.js` import `lib/rate-limit.js`, and neither has a vitest spec. The lib refactor is therefore **strictly safer** than the convoy file implies — there's no transitive test path to break. (`test/api/auth-utils.test.js` only imports `pages/api/auth-utils.js` + `lib/auth-secret.js`; no handler imports.) Decision 6 still holds: no NEW tests this convoy. ### Finding 9 — Test setup file doesn't set `KV_REST_API_*` (intentionally) `test/setup.js` sets only `JWT_SECRET` and `NODE_ENV=test`. With `NODE_ENV=test`, `lib/rate-limit.js`'s `init()` falls into the warn-and-noop branch (`NODE_ENV !== 'production'`), so vitest never tries to construct a real Redis client. Even if a future vitest spec adds a handler import, the limiter no-ops in test. This is the correct shape; do NOT add `KV_REST_API_*` to `test/setup.js`. ### Finding 10 — `parseMultipartFormData` body-streaming is acknowledged out-of-scope but the gate ordering still matters `pages/api/user/avatar.js::parseMultipartFormData` consumes the multipart body via `req.on('data')` + `req.on('end')`. If the rate-limit gate were placed AFTER `parseMultipartFormData`, an attacker could flood the 5MB ceiling even on a 429 path. The brief places the gate BEFORE the method-branching (which calls `parseMultipartFormData` inside the POST branch), so the gate fires before the body parse. **This is the correct ordering even though the body-streaming defense is out of scope** — when `harden-multipart-parser` eventually lands, the gate ordering will already be correct and won't need adjustment. ### Finding 11 — Per-class Redis prefix isolation is required for correctness `@upstash/ratelimit@2.0.8`'s `prefix:` option scopes all keys for that limiter. Without distinct prefixes, two limiters sharing a prefix would share a sliding-window counter, meaning a search hit would consume the auth budget for the same identifier (or, for user-keyed classes, a search hit from user X would consume their upload budget). The brief enforces five distinct prefixes (`tcgvault:auth`, `tcgvault:search`, `tcgvault:upload`, `tcgvault:generate`, `tcgvault:import`). Verified at architect time against the @upstash/ratelimit README's prefix semantics. ## Out of scope (do not do these) - [ ] Do NOT add new vitest or playwright tests. Deferred to `fill-vitest-handler-coverage` (Decision 6). - [ ] Do NOT add a `withRateLimit(handler)` higher-order wrapper. The 7 call sites justify inline; a wrapper is premature abstraction. - [ ] Do NOT migrate `lib/rate-limit.js` to Next.js middleware (Edge runtime). Pages Router serverless functions don't share the Edge runtime cleanly with `@upstash/ratelimit`'s default Node-fetch path; inline is simpler. - [ ] Do NOT add rate-limit headers to SUCCESS responses (`X-RateLimit-Remaining`, `X-RateLimit-Reset`). Honoring the existing `login.js` / `register.js` convention — only the 429 path sets `Retry-After`. - [ ] Do NOT vary the 429 error message per class (Decision 5). Uniform message minimizes attacker fingerprinting. - [ ] Do NOT add a global IP-based backstop limiter (Next.js middleware). Queued as `add-global-rate-limit-middleware` if a future audit shows non-listed routes being abused. - [ ] Do NOT touch `pages/api/cards/search.js`'s 240-line SQL god-function. Only add the rate-limit gate at the top. - [ ] Do NOT touch `pages/api/user/avatar.js`'s `parseMultipartFormData`. Body-streaming defense is `harden-multipart-parser` scope. - [ ] Do NOT delete `pages/api/cards/import-lorcana.js`. Gating with the same shape as mtg/pokemon is the chosen path under Decision 1 (Option A applied uniformly to all three). - [ ] Do NOT add Lorcana to the `