From 51a3a970e0a164bba927b7e865637bb88c50c75f Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Sun, 24 May 2026 22:46:27 -0500 Subject: [PATCH] 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) {