From 708ef45a9610067027440133173ecec224db227b Mon Sep 17 00:00:00 2001 From: varutasu <104105839+varutasu@users.noreply.github.com> Date: Sun, 24 May 2026 22:59:59 -0500 Subject: [PATCH] feat(security): rate-limit search/upload/import + gate import routes (P0 #6 - closes last P0) Closes P0 #6 from PARTIAL to RESOLVED. 8/8 P0s now closed. Extends lib/rate-limit.js from single-class to 5 named limiters (auth/search/upload/generate/import). Atomically gates the 3 import routes (auth + admin-role check + rate limit) and fixes pages/admin/card-import.js's missing Bearer header in the same commit (architect's critical discovery: API gating alone would have broken the admin UI). Per Decision 1 Option A. 10 files +185/-23. Local: lint 128 baseline, vitest 21/21. CI: Playwright smoke 3/3 in 3.8s, forbidden-cors-headers pass, all gates green. PR #20 architect-commit 60b842e, implementer-commit 51a3a97. Brief 4's login.js + register.js byte-identical. --- .convoys/add-rate-limiting.md | 676 ++++++++++++++++ ...ief-1-extend-rate-limit-and-wire-routes.md | 751 ++++++++++++++++++ .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 + 12 files changed, 1612 insertions(+), 23 deletions(-) create mode 100644 .convoys/add-rate-limiting.md 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 new file mode 100644 index 0000000..24ccb66 --- /dev/null +++ b/.convoys/add-rate-limiting.md @@ -0,0 +1,676 @@ +--- +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: in-progress +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. + +## 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. 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) {