From 46aeb7781373f6dc7b7481a1a50b850673d92025 Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Sun, 24 May 2026 22:19:49 -0500 Subject: [PATCH 1/3] =?UTF-8?q?convoy:=20scope=20add-rate-limiting=20(P0?= =?UTF-8?q?=20#6=20PARTIAL=20=E2=86=92=20RESOLVED,=20the=20LAST=20P0)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scaffolds the add-rate-limiting convoy. fix-auth-bypass Brief 4 shipped lib/rate-limit.js with a single 5/15min auth-only limiter wired into login + register; this convoy extends the surface to search, upload, and (pending Decision 1) import routes. Parent's pre-architect audit surfaced a critical secondary finding beyond "missing rate limit": pages/api/cards/import-*.js (3 files) have ZERO auth checks. They are publicly callable, hit external APIs (Scryfall / Pokémon / Lorcana) with no caller throttling, and perform unbounded DB writes. Rate-limit alone won't close P0 #6 cleanly. Architect Decision 1 routes the scope choice: Option A — in-scope: add auth gates + rate limit to import routes in this convoy. ~30 LOC across 3 files. Parent recommends — precedent from drop-public-setup Brief 2 (mid- convoy CJS/ESM expansion). Option B — spin out: stay narrow on the 4 listed routes; queue gate-import-routes follow-up. Mark P0 #6 RESOLVED-with-caveat. Option C — rate-limit-only on imports: worst option (leaves abusive anonymous endpoint live). Six decisions queued (scope expansion, named-limiter shape, per-class limits, identifier extraction, 429 response shape, test coverage). No operator action required — Upstash env vars already auto-provisioned by Brief 4. If Option A wins, this convoy closes the last open P0 ship- blocker (8/8 RESOLVED) and the launch checklist becomes empty. Co-authored-by: Cursor --- .convoys/add-rate-limiting.md | 342 ++++++++++++++++++++++++++++++++++ 1 file changed, 342 insertions(+) create mode 100644 .convoys/add-rate-limiting.md 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. -- 2.45.2 From 60b842ee414d0ea559ab9fccf993f9838e784aa0 Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Sun, 24 May 2026 22:30:09 -0500 Subject: [PATCH 2/3] =?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. -- 2.45.2 From 51a3a970e0a164bba927b7e865637bb88c50c75f Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Sun, 24 May 2026 22:46:27 -0500 Subject: [PATCH 3/3] feat(security): rate-limit search/upload/import + gate import routes (P0 #6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes P0 #6 (no rate limiting) from PARTIAL → RESOLVED. With this merge, all 8 P0 ship-blockers are RESOLVED. fix-auth-bypass Brief 4 shipped lib/rate-limit.js with a single 5/15min auth limiter wired into login + register; this brief extends the module to 5 named limiters (auth/search/upload/generate/import) and wires them into the remaining abusable surface. Per architect Decision 1 — Option A (gate all 3 import routes uniformly). The architect's investigation found a critical secondary bug: pages/admin/card-import.js's fetch sends NO Authorization header today. Adding getUserFromRequest to the import APIs without fixing the admin UI atomically would have returned 401 on every "Import Cards" click. Both edits ship in this single commit — API gating + admin UI Bearer fix — for atomic safety. Lorcana is dead in frontend today (only scripts/import-lorcana.js uses that path) but gated uniformly to future-proof per AGENTS.md § 1 status; a delete-dead-lorcana-import follow-up convoy is queued for later if we decide to drop Lorcana entirely. Per Decision 2 — hybrid named-limiter shape in lib/rate-limit.js. checkAuthRateLimit(req) signature + return shape preserved verbatim (don't break Brief 4's contract); 4 new named functions added (checkSearchRateLimit, checkUploadRateLimit, checkGenerateRateLimit, checkImportRateLimit). Map cache, per-class Redis prefix (tcgvault:auth, tcgvault:search, tcgvault:upload, tcgvault:generate, tcgvault:import) so each class has its own budget. Per Decision 3 — per-class limit values tuned with evidence: auth 5 / 15min IP-keyed (unchanged from Brief 4) search 60 / 1min IP-keyed (bumped from 30 — ShareModal has no debounce; 17-char email = 16 requests in <5s) upload 10 / 1hr user-keyed generate 5 / 1hr user-keyed (DiceBear is free, kept at 5) import 5 / 1hr user-keyed (admin-only; external APIs have their own limits) Per Decision 4 — two extractors. extractIpIdentifier (existing, unchanged) and extractUserIdentifier (new). The new one THROWS on null/undefined/empty/NaN userId to prevent silent fallback-to-IP (which would convert per-user limits into per-IP and lock out households). Architect's R-finding: places the gate AFTER the auth check on every per-user-keyed route, never before. Per Decision 5 — uniform 429 response shape verbatim matching login.js/register.js: Retry-After header + JSON { error: 'Too many attempts. Try again later.' }. Anti- fingerprinting (per-class messages would tell an attacker which classes have which limits). Per Decision 6 — no new per-route handler tests this convoy. Vitest 21/21 unchanged at merge. Verification: - npm run lint: 128 problems (baseline match) - npm run test:run: 21/21 vitest pass (no regression; auth-utils tests don't transitively load rate-limit per architect D6 evidence) - 5 named limiter exports verified via per-route grep counts - Admin UI sends Authorization: Bearer from localStorage in the import fetch (matching pattern from other admin pages) - Brief 4's login.js + register.js byte-identical at HEAD - .cursor/rules/api-routes.mdc § Rate limiting extended with per-class table + gate-ordering rules No new dependencies (Brief 4's @upstash/ratelimit + @upstash/redis suffice). No workflow YAML changes. No AGENTS.md edits (doc- writer pass at convoy close handles Gotcha #12 update + § 6 testing update + ship-readiness Status summary 7/8 → 8/8). Co-authored-by: Cursor --- .cursor/rules/api-routes.mdc | 41 ++++++++++---- lib/rate-limit.js | 90 ++++++++++++++++++++++++++----- pages/admin/card-import.js | 1 + pages/api/cards/import-lorcana.js | 16 ++++++ pages/api/cards/import-mtg.js | 16 ++++++ pages/api/cards/import-pokemon.js | 16 ++++++ pages/api/cards/search.js | 7 +++ pages/api/user/avatar.js | 7 +++ pages/api/user/avatar/generate.js | 7 +++ pages/api/users/search.js | 7 +++ 10 files changed, 185 insertions(+), 23 deletions(-) diff --git a/.cursor/rules/api-routes.mdc b/.cursor/rules/api-routes.mdc index db05771..12eabaf 100644 --- a/.cursor/rules/api-routes.mdc +++ b/.cursor/rules/api-routes.mdc @@ -103,17 +103,29 @@ await logCollectionActivity(collectionId, userId, 'card_added', { cardId, quanti ## Rate limiting -`/api/auth/login` and `/api/auth/register` are wrapped with a 5-attempt / 15-minute sliding window via `lib/rate-limit.js`. New endpoints on the public auth surface (or anywhere brute-force / credential-stuffing matters) should follow the same shape: +`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 { checkAuthRateLimit } from '../../../lib/rate-limit.js'; +import { checkSearchRateLimit } from '../../../lib/rate-limit.js'; export default async function handler(req, res) { - if (req.method !== 'POST') { + if (req.method !== 'GET') { return res.status(405).json({ error: 'Method not allowed' }); } - const { allowed, reset } = await checkAuthRateLimit(req); + // 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.' }); @@ -127,12 +139,23 @@ export default async function handler(req, res) { } ``` -Notes: +**Gate ordering rules:** -- Gate sits between the method check and the body. It MUST be inside the `try`/`catch` if you want Upstash errors to bubble — but `checkAuthRateLimit` already swallows them and fails-open, so the placement above is fine. -- Identifier is the first hop in `x-forwarded-for` (Vercel's edge); do NOT key off `req.body.email` (rotates) or `req.headers.authorization` (login is unauthenticated by design). -- Env vars are `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 — set them in Vercel project settings before merging anything that imports `lib/rate-limit.js`. In dev, the module warn-and-no-ops so local work isn't blocked. -- The current scope is just the two auth endpoints. Sweeping the rest of the API (`/api/users/search`, `/api/cards/import-*`, avatar upload) is the queued `add-rate-limiting` convoy — follow the same pattern there. +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). ## Dev/test endpoints (removed) diff --git a/lib/rate-limit.js b/lib/rate-limit.js index 53b28c7..33b9608 100644 --- a/lib/rate-limit.js +++ b/lib/rate-limit.js @@ -1,10 +1,23 @@ 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. +// import the auth handlers, Vercel build-time bundling). Defer +// construction until the first request actually arrives. let cached = null; function init() { @@ -17,12 +30,18 @@ function init() { if (url && token) { const redis = new Redis({ url, token }); - const ratelimit = new Ratelimit({ - redis, - limiter: Ratelimit.slidingWindow(5, '15 m'), - prefix: 'tcgvault:auth', - }); - return { mode: 'live', ratelimit }; + 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') { @@ -39,13 +58,33 @@ function init() { return { mode: 'noop' }; } -function extractIdentifier(req) { +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'; } -export async function checkAuthRateLimit(req) { +// 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(); } @@ -54,16 +93,39 @@ export async function checkAuthRateLimit(req) { return { allowed: true, remaining: Infinity, reset: 0 }; } - const identifier = extractIdentifier(req); + const limiter = cached.instances.get(className); + if (!limiter) { + throw new Error(`[rate-limit] Unknown limiter class: ${className}`); + } try { - const { success, remaining, reset } = await cached.ratelimit.limit(identifier); + 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 of login. Brute-force - // protection lives behind defense-in-depth (Vercel firewall, etc.). + // 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)); +} diff --git a/pages/admin/card-import.js b/pages/admin/card-import.js index a167be6..a2a4f20 100644 --- a/pages/admin/card-import.js +++ b/pages/admin/card-import.js @@ -44,6 +44,7 @@ const CardImport = () => { method: 'POST', headers: { 'Content-Type': 'application/json', + 'Authorization': `Bearer ${localStorage.getItem('auth_token')}`, }, body: JSON.stringify({ setCode: setCode.trim() }), }); diff --git a/pages/api/cards/import-lorcana.js b/pages/api/cards/import-lorcana.js index b026d2e..077f6ee 100644 --- a/pages/api/cards/import-lorcana.js +++ b/pages/api/cards/import-lorcana.js @@ -1,4 +1,6 @@ import { sql } from '@vercel/postgres'; +import { getUserFromRequest } from '../../../lib/permission-middleware'; +import { checkImportRateLimit } from '../../../lib/rate-limit.js'; // Helper function to delay execution const delay = (ms) => new Promise(resolve => setTimeout(resolve, ms)); @@ -47,6 +49,20 @@ export default async function handler(req, res) { 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; diff --git a/pages/api/cards/import-mtg.js b/pages/api/cards/import-mtg.js index 4950590..8da023b 100644 --- a/pages/api/cards/import-mtg.js +++ b/pages/api/cards/import-mtg.js @@ -1,10 +1,26 @@ 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; diff --git a/pages/api/cards/import-pokemon.js b/pages/api/cards/import-pokemon.js index cc14347..da2dd1b 100644 --- a/pages/api/cards/import-pokemon.js +++ b/pages/api/cards/import-pokemon.js @@ -1,4 +1,6 @@ import { sql } from '@vercel/postgres'; +import { getUserFromRequest } from '../../../lib/permission-middleware'; +import { checkImportRateLimit } from '../../../lib/rate-limit.js'; // Helper function to delay execution const delay = (ms) => new Promise(resolve => setTimeout(resolve, ms)); @@ -47,6 +49,20 @@ export default async function handler(req, res) { 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; diff --git a/pages/api/cards/search.js b/pages/api/cards/search.js index 541a17e..eb9da8c 100644 --- a/pages/api/cards/search.js +++ b/pages/api/cards/search.js @@ -1,10 +1,17 @@ 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 = '', diff --git a/pages/api/user/avatar.js b/pages/api/user/avatar.js index 8608873..3ebb31c 100644 --- a/pages/api/user/avatar.js +++ b/pages/api/user/avatar.js @@ -1,6 +1,7 @@ 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: { @@ -18,6 +19,12 @@ export default async function handler(req, res) { 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 const contentType = req.headers['content-type']; diff --git a/pages/api/user/avatar/generate.js b/pages/api/user/avatar/generate.js index 5e389db..03030d7 100644 --- a/pages/api/user/avatar/generate.js +++ b/pages/api/user/avatar/generate.js @@ -1,6 +1,7 @@ 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') { @@ -14,6 +15,12 @@ export default async function handler(req, res) { 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} diff --git a/pages/api/users/search.js b/pages/api/users/search.js index 9c2c72f..5847fa6 100644 --- a/pages/api/users/search.js +++ b/pages/api/users/search.js @@ -1,6 +1,7 @@ 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') { @@ -20,6 +21,12 @@ export default async function handler(req, res) { 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) { -- 2.45.2