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<className, Ratelimit> 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 <token> 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 <cursoragent@cursor.com>
172 lines
10 KiB
Text
172 lines
10 KiB
Text
---
|
|
description: Conventions for Next.js Pages-router API route handlers in tcg-vault
|
|
globs: pages/api/**/*.js
|
|
---
|
|
|
|
# API Route Conventions
|
|
|
|
Pages router handlers; `(req, res)` signature; Vercel serverless functions.
|
|
|
|
## Authentication & Authorization
|
|
|
|
Two paths are in use; both go through `lib/permission-middleware.js`.
|
|
|
|
```js
|
|
// 1. Generic auth — every protected route
|
|
import { getUserFromRequest } from '../../lib/permission-middleware';
|
|
|
|
export default async function handler(req, res) {
|
|
const user = await getUserFromRequest(req);
|
|
if (!user) return res.status(401).json({ error: 'Authentication required' });
|
|
// user = { userId, email, role }
|
|
// ...
|
|
}
|
|
```
|
|
|
|
```js
|
|
// 2. Collection-scoped auth — when the route operates on a specific collection
|
|
import { withCollectionPermission } from '../../lib/permission-middleware';
|
|
|
|
async function handler(req, res) {
|
|
// req.user and req.permission are populated by the wrapper
|
|
const { userId, role } = req.user;
|
|
}
|
|
|
|
export default withCollectionPermission('viewer')(handler);
|
|
// 'viewer' | 'editor' | 'owner' — checks owner OR is_public OR explicit collection_permissions row
|
|
```
|
|
|
|
`getUserFromRequest` returns `null` for any unauthenticated request (missing header, malformed token, wrong signature, expired token, unknown user id). The early `if (!user) return res.status(401)` pattern in the snippet above is the canonical guard for every authenticated route. (The pre-`fix-auth-bypass` synthetic-admin fallback for missing tokens has been removed — see `AGENTS.md` Gotcha #2 for the audit trail.)
|
|
|
|
## Request validation
|
|
|
|
No schema validator is installed (no zod / yup / valibot). Validate manually:
|
|
|
|
```js
|
|
const { name, game } = req.body || {};
|
|
if (!name || typeof name !== 'string' || name.trim().length === 0) {
|
|
return res.status(400).json({ error: 'name is required' });
|
|
}
|
|
if (!['mtg', 'pokemon', 'lorcana'].includes(game)) {
|
|
return res.status(400).json({ error: 'invalid game' });
|
|
}
|
|
```
|
|
|
|
When adding zod (planned in `.convoys/`), define schemas at the top of the file.
|
|
|
|
## Method gating
|
|
|
|
Reject unsupported methods explicitly — Next.js will otherwise call the handler for any method:
|
|
|
|
```js
|
|
if (req.method !== 'POST') {
|
|
return res.status(405).json({ error: 'Method not allowed' });
|
|
}
|
|
```
|
|
|
|
## Error handling
|
|
|
|
Wrap the handler body in `try/catch`. Never throw unhandled — leaks stack traces in the Vercel response.
|
|
|
|
```js
|
|
export default async function handler(req, res) {
|
|
try {
|
|
// ...
|
|
} catch (err) {
|
|
console.error('[POST /api/collections]', err);
|
|
return res.status(500).json({ error: 'Internal server error' });
|
|
}
|
|
}
|
|
```
|
|
|
|
## Database access
|
|
|
|
- **Prefer:** `import { sql } from '@vercel/postgres'` and tagged templates: `` await sql`SELECT … WHERE id = ${id}` ``.
|
|
- **Avoid:** `import { db } from '../../lib/database'`. Its `query(str, params)` API uses `sql.unsafe` after manual interpolation — SQL-injection vector. Slated for removal in a convoy.
|
|
- **Always select narrow columns** — don't `SELECT *` from `cards` (large `oracle_text`, `colors` JSONB).
|
|
|
|
## Response shape
|
|
|
|
- Success (GET / read): `res.status(200).json({ data: ... })` OR direct payload — codebase is inconsistent; match the surrounding route's existing shape.
|
|
- Created (POST): `res.status(201).json({ data: ... })`.
|
|
- Errors: `res.status(<4xx|5xx>).json({ error: string, details?: unknown })`.
|
|
|
|
## Activity logging
|
|
|
|
Any handler that mutates a collection must call `logCollectionActivity`:
|
|
|
|
```js
|
|
import { logCollectionActivity } from '../../lib/permission-middleware';
|
|
|
|
await logCollectionActivity(collectionId, userId, 'card_added', { cardId, quantity });
|
|
```
|
|
|
|
## 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).
|
|
|
|
## Dev/test endpoints (removed)
|
|
|
|
The four endpoints `pages/api/simple.js`, `pages/api/test-auth.js`, `pages/api/test-db.js`, and `pages/api/setup-database.js` used to exist as unauthenticated dev / diagnostic routes. They were **deleted** by `fix-auth-bypass` Brief 3 (commit `fc0dd73`) and `.github/workflows/ci.yml`'s `forbidden-endpoints` job now fails the build if any of them are re-introduced, or if any new file matching `pages/api/test-*.js` is added. **Do not re-create these files.** If a future agent searches for `test-db` or `setup-database` and finds them missing, this section is the explanation — diagnostics belong outside the public API surface (a CLI script, an admin-gated route, or `npm run` task).
|
|
|
|
## CORS
|
|
|
|
No `pages/api/**` route ships CORS headers. The frontend and the API are same-origin on Vercel (same project, same domain), and cross-origin reads serve no legitimate purpose on this API surface — the wildcard `Access-Control-Allow-Origin: *` that 24 handlers used to carry was scaffolding cruft, not a deliberate cross-origin design. `fix-auth-bypass` Brief 4 (commit `297afca`) removed it from `pages/api/auth/login.js` + `pages/api/auth/register.js`; the `cors-tighten` convoy (squash commit `da50d78`, PR #19) swept the remaining 24 handlers and added a new blocking `forbidden-cors-headers` job to `.github/workflows/ci.yml` (modeled on `forbidden-endpoints`) that fails the build if any `Access-Control-Allow-(Origin|Methods|Headers)` reference reappears under `pages/api/`.
|
|
|
|
Conventions to follow:
|
|
|
|
- **Do not add `res.setHeader('Access-Control-Allow-*', ...)` to any new route.** The CI gate will fail the build with a file-and-line pointer.
|
|
- **Do not add `if (req.method === 'OPTIONS')` preflight handlers.** Same-origin requests don't preflight; cross-origin requests are blocked at the browser CORS layer (the desired end state). If an OPTIONS request ever arrives, the existing method gate (`if (req.method !== '<verb>') return res.status(405)`) returns 405 — strictly safer than the pre-sweep 200-to-everyone.
|
|
- **If a future cross-origin caller is legitimately needed** (third-party app, mobile client, public API key program — none exist today), design a proper CORS layer — probably as Next.js middleware reading an allowed-origin list from env — rather than scaffolding wildcards back into individual handlers. That's a separate convoy (`add-cors-layer` or similar); flag it as a new follow-up in `.convoys/ship-readiness.md` § Queued convoys at the time the need surfaces.
|