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) {