Reflects the merged add-rate-limiting convoy (PR #20, squash commit708ef45) in repo documentation. **This is the milestone cleanup** — add-rate-limiting closed P0 #6 (No rate limiting anywhere), the LAST open P0 ship-blocker. `.convoys/ship-readiness.md`'s § Status summary flips from "7 of 8 RESOLVED; 1 remains" to **"8 of 8 RESOLVED. Launch-readiness P0 checklist is empty."** One brief in the convoy: Brief 1 shipped as planned with no scope expansions and no implementer deviations from the verbatim spec; all six architect decisions ratified verbatim at gate 1 (D1 operator-ratified Option A; D2-D6 architect-self-ratified). .convoys/add-rate-limiting.md: - frontmatter status: in-progress -> shipped (added shipped: 2026-05-24) - new ## As-shipped section. Opens with the milestone language pointing back at ship-readiness.md's flipped § Status summary. Decisions section captures all 6 ratifications (D1 operator- ratified Option A — Critical: WHY the atomic admin UI fix in pages/admin/card-import.js was Decision 1's hidden coupling requirement, since API gating alone would have broken every "Import Cards" click; D2 hybrid named-limiter shape with Map<className, Ratelimit> cache; D3 per-class table including the two D3 tuning-evidence raises — search 30 -> 60/min because ShareModal.handleSearch has no debounce so a 17-char email = 16 requests in <5s, and generate kept at 5/hour because DiceBear is free not paid AI; D4 two-extractor shape with defensive THROW on null/empty userId; D5 uniform 429 message; D6 no new vitest or playwright specs deferred to fill-vitest-handler-coverage). As-shipped surface broken into 4 layers (1 lib refactor + 6 route gates + 1 atomic admin UI fix + 1 rule extension) mirroring the cors-tighten cleanup's pattern-split shape. Empirical CI metrics from post-merge run 26382185019 (Playwright smoke 59s 3/3 in 3.8s, forbidden-cors-headers pass, vitest 21/21, lint 128 baseline, Screenshot diff continue-on-error swallow per Decision 4). Cross-validation finding: smoke test 2 still passes against the post-rate-limit preview — that's three convoys in a row (PR #15 Layout default-user, PR #19 CORS-tighten, PR #20 rate-limiting) where the same 3-test smoke spec defended the auth surface through sweeping changes. Operator-action-required: none. What did NOT change audit trail. .convoys/ship-readiness.md: - § Status summary at the top flipped from 7/8 to 8/8 RESOLVED. Header text updated to "Launch-readiness P0 checklist is empty." P0 #6 row in the table flips from PARTIAL to RESOLVED with the two-convoy lineage (fix-auth-bypass Brief 4 + add-rate-limiting). Trailing paragraph rewritten as a milestone note: security gate closed; remaining launch work is P1 quality bar + P2/P3 polish. - P0 #6 entry flipped from PARTIAL to RESOLVED with the full add-rate-limiting as-shipped block (8 sub-bullets covering the lib refactor shape, the per-class table, the defensive THROW, the three import routes' auth-gating, the atomic admin UI fix and WHY, the rule extension, the 6 decisions, and the diff breakdown). Brief 4's 2026-05-23 partial is preserved as the prior as-shipped layer to maintain the audit trail. - Launch sequence step 4 marked RESOLVED 2026-05-24 with the squash commit + smoke metrics inline. - Queued convoys: removed the add-rate-limiting entry (it shipped). Added a new delete-dead-lorcana-import entry (P3 polish; the Lorcana import route was gated defensively in PR #20 despite zero current frontend callers — pages/admin/card-import.js's <select> only offers mtg + pokemon — so if Lorcana stays permanently out of the admin UI, this is the cleanup PR). Added three "flagged but kept out of scope" follow-ups per the convoy's § What did NOT change: harden-multipart-parser (P2; 5MB body still consumed before the 429 path on avatar.js), god-function-split / refactor-cards-search-sql (P2; 240-line 7-branch SQL in cards/search.js), and withAdmin(handler) wrapper extraction (P3 DX; the three import routes are call sites #3-5 in the codebase but uniform inline shape was preserved for convoy atomicity). Updated tighten-visual-diff-path-filter to note PR #20 also tripped the same false-positive. AGENTS.md: - Gotcha #12 extended end-to-end. Was the single-class auth-only lib + the env-var contract; is now the 5-class reality with a full per-class table (helper / limit-window / key / routes), the defensive THROW pattern in extractUserIdentifier, the gate-ordering rule for per-user limiters, and the auth → admin-role → rate-limit ordering for the three import routes. Prominent milestone line opens the new content: "add-rate-limiting convoy (squash708ef45, PR #20, 2026-05-24) closed P0 #6 — all 8 P0s now RESOLVED." Original env-var contract paragraph (KV_REST_API_URL / KV_REST_API_TOKEN, fail-closed-in-prod / warn-and-noop-in-dev) is preserved verbatim above the new content. - § 6 Testing: intentionally untouched (no test surface changed; vitest 21/21 and smoke 3/3 still apply). - § 7 Deployment: intentionally untouched (no deployment-shape changed; same KV_REST_API_* env vars from Brief 4). .cursor/rules/api-routes.mdc: - The implementer extended § Rate limiting in PR #20 with the per-class table + verbatim call shape + gate-ordering rules + identifier-extraction + uniform 429 + fail-closed env-var contract + fail-open Upstash-outage behavior. Doc-writer pass verified completeness; added a one-sentence convoy-attribution line at the top of § Rate limiting citing the two-convoy lineage (fix-auth-bypass Brief 4 for the auth class + add-rate-limiting for the other four classes and 7 newly-gated routes), mirroring the post-cors-tighten § CORS attribution shape. No other touch-ups needed. No changes to: package.json, package-lock.json, lib/rate-limit.js, pages/**, components/**, scripts/**, test/**, tests/**, .github/workflows/**, README.md, TESTING_GUIDE.md, playwright.config.js, eslint.config.mjs. Co-authored-by: Cursor <cursoragent@cursor.com>
172 lines
11 KiB
Text
172 lines
11 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 }`. The auth-only limiter shipped in `fix-auth-bypass` Brief 4 (commit `297afca`, login + register); the four remaining classes — `search`, `upload`, `generate`, `import` — and the seven currently-gated routes shipped in `add-rate-limiting` (squash commit `708ef45`, PR #20, 2026-05-24), the convoy that closed P0 #6 and brought the launch-readiness P0 set to 8/8 RESOLVED.
|
|
|
|
| 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.
|