From 7832e03dec1b5e13cdcf662606a1367dee3dc9bc Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Sun, 24 May 2026 23:09:07 -0500 Subject: [PATCH] =?UTF-8?q?docs:=20post-convoy=20cleanup=20for=20add-rate-?= =?UTF-8?q?limiting=20=E2=80=94=20MILESTONE,=20last=20P0=20closed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reflects the merged add-rate-limiting convoy (PR #20, squash commit 708ef45) 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 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 ` only offers `mtg` / + `pokemon`) — uniform protection across the three import shapes is + strictly easier to maintain than a 2-gated-1-deleted asymmetry, and + if Lorcana stays unused, the queued `delete-dead-lorcana-import` + follow-up convoy (see `.convoys/ship-readiness.md` § Queued + convoys) is the cleanup path. +- **Decision 2 — Hybrid named-limiter shape (architect-self-ratified).** + `lib/rate-limit.js` refactored from a single auth-only `Ratelimit` + instance into a `Map` cache with one shared + Redis client and five `Ratelimit` instances (one per class, distinct + Redis prefix). Five exported functions: `checkAuthRateLimit(req)` + (Brief 4 contract preserved byte-identical), + `checkSearchRateLimit(req)`, `checkUploadRateLimit(req, userId)`, + `checkGenerateRateLimit(req, userId)`, `checkImportRateLimit(req, + userId)`. Internal `check(className, identifier)` shared helper. + `LIMITER_CONFIG` is a top-level `const` map of + `{ limit, window, prefix }` per class; adding a sixth class is a + one-line addition + one new exported function (no `init()` + restructuring needed). +- **Decision 3 — Per-class limit values (architect-self-ratified with + tuning evidence).** Final table: + + | Class | Limit | Window | Key | Helper | + | --- | --- | --- | --- | --- | + | `auth` | 5 | 15 min | IP | `checkAuthRateLimit(req)` (unchanged from Brief 4) | + | `search` | **60** | 1 min | IP | `checkSearchRateLimit(req)` | + | `upload` | 10 | 1 hour | user | `checkUploadRateLimit(req, userId)` | + | `generate` | **5** | 1 hour | user | `checkGenerateRateLimit(req, userId)` | + | `import` | 5 | 1 hour | user (admin-only) | `checkImportRateLimit(req, userId)` | + + Two architect raises from the parent's pre-investigation defaults + warrant a permanent record so future tuning convoys see the + evidence: + + - **`search` raised 30 → 60/min.** `components/ShareModal.js`'s + `handleSearch` (lines 56-77) fires on every keystroke with NO + debounce; typing a 17-char email = 16 requests in <5s, which + would 429 a single legitimate user against the 30/1min default. + 60/1min covers a realistic burst and still stops a scraper + inside 2-3 seconds. If real users still 429, the fix is a + one-line `LIMITER_CONFIG` edit (60 → 90 or 120); not a + release-blocker. Queued follow-up name if needed: + `tune-search-rate-limit`. + - **`generate` kept at 5/hour (parent suggested 3).** + `pages/api/user/avatar/generate.js` calls DiceBear (free public + API), not OpenAI / Replicate / Stability — cost is Vercel blob + storage + DiceBear-side throttling, not per-call $. 5/hour is + generous enough that a user trying 3-4 seeds doesn't hit the + wall, strict enough that an accidental render loop still trips + inside the first minute. If we ever switch generators to a paid + provider, the same one-line `LIMITER_CONFIG` edit drops it back + to 3 or lower. +- **Decision 4 — Two-extractor shape with defensive THROW + (architect-self-ratified).** `extractIpIdentifier(req)` + (module-private; renamed from the pre-refactor `extractIdentifier`) + and `extractUserIdentifier(userId)` (module-private; new). The + user extractor **THROWS** with + `'[rate-limit] extractUserIdentifier called without an + authenticated userId. Place the rate-limit gate AFTER the auth + check, never before.'` when `userId` is `null` / `undefined` / + `''` / `NaN`. Surfaces gate-ordering bugs **at dev time** rather + than silently falling back to IP and converting a per-user limit + into a per-IP limit (which would lock other household members out + for one user's behavior — the exact regression flagged in the + convoy file's § Known constraints). Numeric `0` is intentionally + accepted (returns `'user:0'`) — defensively forward-compatible if + a future schema introduces user ID 0. +- **Decision 5 — Uniform 429 response (architect-self-ratified).** + Single `'Too many attempts. Try again later.'` message across all + five classes, matching `login.js` + `register.js` verbatim. + `Retry-After` calculation is `Math.ceil((reset - Date.now()) / + 1000)`; status code is `429`. Per-class variation (e.g., *"Too + many search requests"*) was considered and rejected because it + would fingerprint to an attacker which routes have which limits + + windows, making it easier to craft a request pattern that avoids + 429 on the more-permissive routes while still abusing the + less-permissive ones. +- **Decision 6 — No new vitest or playwright tests + (architect-self-ratified).** Same reasoning as `cors-tighten` + Decision D4. Per-route handler tests are deferred to the queued + `fill-vitest-handler-coverage` convoy. The fail-loud-in-prod + predicate in `lib/rate-limit.js::init()` is the integration test + — if `KV_REST_API_*` is unset on a deployed env, every gated route + fails closed on the first call. Architect-time investigation + corrected a stale claim in the convoy file's § Known constraints + (the line *"auth-utils tests run through the same module"*): `rg + 'rate-limit|@upstash' test/` returns zero matches, so the lib + refactor is strictly safer than the convoy file implied — there + is no transitive vitest path to break. + +### As-shipped surface + +The shape splits cleanly into four layers (mirrors `cors-tighten`'s +24-source-files / 1-CI-job pattern split, but with one extra layer +because of the atomic admin UI fix and the rule extension): + +1. **One `lib/rate-limit.js` refactor.** Single-class auth-only + limiter → 5-class `Map` with distinct Redis + prefixes (`tcgvault:auth`, `tcgvault:search`, `tcgvault:upload`, + `tcgvault:generate`, `tcgvault:import`). +90 / -23 net (90 lines + added, 23 lines reshaped — the existing `init()`, + `extractIdentifier`, `checkAuthRateLimit` functions are + conceptually preserved but restructured to share infrastructure + across all five classes). Brief 4's `checkAuthRateLimit(req)` + return shape is **byte-identical** post-refactor (`{ allowed, + remaining, reset }`); `login.js` + `register.js` were not + touched and continue to work unchanged. +2. **Six new route-gate additions** (lib refactor delivers the + helpers; routes call them): + - `pages/api/users/search.js` — `checkSearchRateLimit(req)` after + the existing inline JWT verify. IP-keyed. +7 lines. + - `pages/api/cards/search.js` — `checkSearchRateLimit(req)` at top + of handler after the method check, before the 240-line SQL + god-function (which stays byte-identical per § Files explicitly + out of scope). IP-keyed, anonymous-by-design. +7 lines. + - `pages/api/user/avatar.js` — `checkUploadRateLimit(req, + user.userId)` after `getUserFromRequest`, before the POST/DELETE + method branching (so the limiter fires before + `parseMultipartFormData` consumes the 5MB body). User-keyed. +7 + lines. + - `pages/api/user/avatar/generate.js` — + `checkGenerateRateLimit(req, user.userId)` after + `getUserFromRequest`, before the user-data SQL query. + User-keyed. +7 lines. + - `pages/api/cards/import-mtg.js` — + `getUserFromRequest` + `if (user.role !== 'admin') return 403` + + `checkImportRateLimit(req, user.userId)` before the existing + `try` block. Closes the publicly-callable anonymous-abuse + vector. +16 lines. + - `pages/api/cards/import-pokemon.js` — same shape as + `import-mtg.js`. +16 lines. + - `pages/api/cards/import-lorcana.js` — same shape as + `import-mtg.js`; gated defensively despite zero current + frontend callers. +16 lines. +3. **One atomic admin UI fix.** `pages/admin/card-import.js` adds + `'Authorization': \`Bearer ${localStorage.getItem('auth_token')}\`` + to the import fetch's headers (one-line addition; the 309-line + god-component is otherwise byte-identical). **This was the + architect's critical pre-brief discovery and the reason Decision + 1 routes back to the operator** — gating the import APIs without + this matching client fetch fix would have closed P0 #6 but + introduced an immediate 401 on every "Import Cards" click, + producing a visible UX regression on the only live admin tooling + that exercises the gated routes. Shipping the gate + the client + fix in the same PR keeps the convoy atomic; PR review enforced + the dependency explicitly. +4. **One `.cursor/rules/api-routes.mdc` § Rate limiting extension.** + Replaced the previous auth-only § with the per-class table + the + verbatim call shape + gate-ordering rules (method check first; + auth before any user-keyed limiter; IP-keyed gate placement is + flexible; admin-role check goes between auth and rate-limit for + the import routes) + identifier-extraction documentation + + uniform 429 response shape + fail-closed env-var contract + + fail-open Upstash-outage behavior. Implementer landed this in + the same PR; doc-writer pass verified the extension is complete + and made no further touch-ups (see § What did NOT change below). + +### As-shipped metrics + +Diff size (per `git show --stat 708ef45`): + +- **12 files modified, +1612 / -23.** Note that the 1612-addition + figure is dominated by `.convoys/add-rate-limiting.md` (676 lines) + and `.convoys/add-rate-limiting/brief-1-extend-rate-limit-and-wire-routes.md` + (751 lines), which the squash includes because the architect commit + preceded the implementer commit on the same branch. The actual + source-file diff is much smaller: + - `lib/rate-limit.js`: +90 / -23 (the lib refactor). + - `.cursor/rules/api-routes.mdc`: +41 (the § Rate limiting extension). + - 6 route files under `pages/api/`: +69 lines total (3× +16 for + import routes, 4× +7 for search/avatar/generate/users-search). + - `pages/admin/card-import.js`: +1 (the Bearer-header addition). + +Post-merge CI run 26382185019 + subsequent runs on `main`: + +- **`Playwright smoke` — PASS in 59s, 3/3 tests in 3.8s** against the + post-rate-limit Vercel preview. Same three checks (`home redirects + or renders without 5xx` ✓ 431ms / `sign-in page renders` ✓ 331ms / + `public health endpoint responds` ✓ 193ms) — all green. **Critical + cross-validation:** smoke calls `/api/health` once per run (well + below the search limiter's 60/min ceiling), and the home + sign-in + routes don't touch any of the 6 newly-gated endpoints, so smoke + does NOT 429 against the new search class. The cross-validation + finding accumulates: smoke test 2 (`'sign-in page renders'`) still + passes against the post-CORS + 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 auth surface stayed + stable under sweeping changes, and the same 3-test smoke spec + defended it every time. +- **`forbidden-cors-headers` (from `cors-tighten`)** — PASS. None of + the 6 route edits introduced an `Access-Control-Allow-*` header + (the convoy is purely additive of rate-limit gate code; CORS + surface was not touched). The grep stays clean. +- **`forbidden-endpoints` (from `fix-auth-bypass` Brief 3)** — PASS. + No new `pages/api/test-*.js` or other deleted-endpoint shapes + reintroduced. +- **`Unit tests (vitest)`** — PASS, 21/21 in 27s. No new tests, no + removed tests; the existing `lib/auth-secret.test.js` (3) + + `lib/permission-middleware.test.js` (8) + + `pages/api/auth-utils.test.js` (5) + + `components/Layout.test.js` (5) suites are all unaffected. Decision + 6's architect-time verification (`rg 'rate-limit|@upstash' test/` + returns zero matches) confirmed at merge. +- **`Lint`** — 128 problems (lint baseline preserved, no + regression). The lib refactor + the 6 route edits + the admin UI + one-liner introduced zero new lint problems; the `|| true` + wrapper in `.github/workflows/ci.yml` was a no-op for this convoy. +- **`Screenshot diff`** — `continue-on-error: true` swallow per + `adopt-playwright-smoke` Decision 4 (no baseline committed yet); + the documented Decision-4 end state. Triggered on PR #20 because + the `paths:` filter `pages/**` matches the 6 route edits under + `pages/api/`; same minor false-positive as PR #19, tracked by the + queued `tighten-visual-diff-path-filter` follow-up. +- **All other gates (`Schema map up to date`, `Aggregate gate`)** — + green. + +### Cross-validation finding: smoke spec defends the rate-limit +surface (organically) + +The 3-test smoke spec was authored by `adopt-playwright-smoke` (PR +#18) against an un-rate-limited preview, with no foresight about +this convoy's gating. Post-merge run 26382185019 confirms the spec +**still passes against the post-rate-limit preview** — the home +route, the `/login` route, and `/api/health` don't touch any of the +six newly-gated endpoints, and `/api/health` is anonymous / +unrate-limited so the smoke flow doesn't bump up against the search +class's 60/min ceiling. The cross-validation lineage accumulates: + +- PR #15 (`fix-layout-default-user`, `ca302a8`) introduced the + `Sign in` CTA that smoke test 2 asserts + on. +- PR #19 (`cors-tighten`, `da50d78`) removed wildcard CORS from 24 + handlers; smoke test 2 still passed against the post-CORS + preview. +- PR #20 (`add-rate-limiting`, `708ef45`) — this convoy — wired 6 new + route gates; smoke test 2 still passes. + +Smoke is doing real work: it has now defended the auth surface +against three sweeping changes without anyone having to write a +single dedicated test. P0 #7's resolved state, P0 #5's resolved +state, and now P0 #6's resolved state are all backed by a live CI +signal — not just a vitest assertion. + +### Operator action required going forward + +**None.** No new env vars (Upstash `KV_REST_API_URL` / +`KV_REST_API_TOKEN` were already auto-provisioned via the Vercel +Marketplace integration for Brief 4). No new dependencies +(`@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0` already +installed). No new secrets, no infra changes, no CI gates to enable +or disable. The fail-loud-in-prod predicate in +`lib/rate-limit.js::init()` is self-defending: if a future deploy +unsets either env var, every gated route fails closed on the first +call (`throw new Error('[rate-limit] Upstash not configured...')`), +which surfaces immediately as a 500 in the Vercel logs rather than +silently disabling brute-force protection. + +If a follow-up tuning need surfaces (search 60/min too tight, generate +5/hour too tight, etc.), the fix is a single-line `LIMITER_CONFIG` +edit. See `tune-search-rate-limit` (not yet queued; surface only if +real users 429) and the broader `tiered-rate-limits` (P3 polish, +when a premium-tier scheme exists) for the longer view. + +### What did NOT change + +Audit trail of files explicitly NOT touched by this convoy, despite +sitting near the rate-limit surface: + +- **`pages/api/auth/login.js`, `pages/api/auth/register.js`** — + Brief 4 contract preserved. Both files continue to call + `checkAuthRateLimit(req)` against the refactored lib; byte-identical + post-merge. Manual verification at architect time: the 6th-attempt + 429 path still fires correctly. +- **`lib/permission-middleware.js`, `lib/auth-secret.js`, + `pages/api/auth-utils.js`** — auth surface untouched. No `null`-vs- + synthetic-admin regression risk; `test/lib/permission-middleware.test.js`'s + negative regression test still defends Gotcha #2. +- **`package.json`, `package-lock.json`** — zero dep additions, zero + version bumps. `@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0` + pinned from Brief 4. +- **`.github/workflows/*.yml`** — no new CI gate. Per-class + rate-limit wiring isn't grep-checkable; the fail-loud-in-prod + predicate is the integration test (see Decision 6). +- **`test/**`** — Decision 6, no new vitest or playwright specs. + 21/21 still green; smoke 3/3 still green. +- **`pages/api/cards/search.js`'s SQL** — the 240-line god-function + with 7+ conditional `SELECT * FROM cards WHERE …` branches was + explicitly out of scope per `.convoys/add-rate-limiting.md` § + Out of scope. Queued as `god-function-split` / + `refactor-cards-search-sql` (not yet queued in `ship-readiness.md` + — surface when the convoy is sized). +- **`pages/api/user/avatar.js`'s `parseMultipartFormData` body + streaming** — the 5MB multipart body is consumed via + `req.on('data')` before any rate-limit gate can short-circuit, so + an attacker can still exhaust the 5MB body per 429. Queued as + `harden-multipart-parser` (not yet in `ship-readiness.md` — surface + if a real abuse incident occurs). The brief's gate-ordering places + the limiter BEFORE the method branches that call + `parseMultipartFormData`, so when that future hardening lands, the + gate ordering is already correct. +- **`scripts/import-*.js`** — standalone CLI scripts independent of + the API routes; not in scope per § Files explicitly out of scope. + An operator running `node scripts/import-mtg.js` directly bypasses + the rate-limit + admin-role gate entirely (which is the intended + flow — local admin work is unthrottled). +- **`AGENTS.md`'s Gotcha #5 (deleted `setup-database.js`) + Gotcha + #8 (Layout default-user)** — both still RESOLVED, both unchanged. + This convoy's doc-writer pass updates Gotcha #12 (rate-limit + env-var contract) to reflect the 5-class reality, but Gotcha #5 + and #8 are independent of this convoy. +- **Auth wrapper extraction (`withAdmin(handler)`)** — flagged in + `.cursor/rules/auth-and-permissions.mdc` (*"check user.role === + 'admin' directly; consider extracting `withAdmin()` if a third call + site appears"*). The three import routes are the third+fourth+fifth + call sites in the codebase, but extracting the wrapper is its own + scope — for this convoy, the inline `if (user.role !== 'admin') + return 403` shape was preserved across all three. Surface as a + follow-up convoy if a sixth call site appears or if a reviewer + flags the inline shape as a maintainability concern; for now, the + uniform inline check across the three import routes is consistent + with the rest of the codebase. diff --git a/.convoys/ship-readiness.md b/.convoys/ship-readiness.md index 0979636..9d30c63 100644 --- a/.convoys/ship-readiness.md +++ b/.convoys/ship-readiness.md @@ -15,7 +15,7 @@ Code graph: 122 files, 628 nodes, 5602 edges, 11 communities. Indexed by `user-c ## Status summary (as of 2026-05-24) -**P0 ship-blockers: 7 of 8 RESOLVED; 1 remains.** +**P0 ship-blockers: 8 of 8 RESOLVED. Launch-readiness P0 checklist is empty.** | Item | Status | Convoy | | --- | --- | --- | @@ -24,15 +24,17 @@ Code graph: 122 files, 628 nodes, 5602 edges, 11 communities. Indexed by `user-c | P0 #3 — Default admin credentials in seed | **RESOLVED** 2026-05-23 | `drop-public-setup` (`ff80753` + `b63b509`) | | P0 #4 — Dev-only test endpoints | **RESOLVED** 2026-05-23 | `fix-auth-bypass` Brief 3 (`fc0dd73`) | | P0 #5 — Wildcard CORS on API surface | **RESOLVED** 2026-05-24 | `fix-auth-bypass` Brief 4 (`297afca`) + `cors-tighten` (`da50d78`) | -| P0 #6 — No rate limiting | **PARTIAL** 2026-05-23 | `fix-auth-bypass` Brief 4 (login + register only); queued `add-rate-limiting` for the rest | +| P0 #6 — No rate limiting | **RESOLVED** 2026-05-24 | `fix-auth-bypass` Brief 4 (`297afca`, login + register) + `add-rate-limiting` (`708ef45`, the remaining surface) | | P0 #7 — Layout default-prop leaks email | **RESOLVED** 2026-05-24 | `fix-layout-default-user` (`ca302a8`) | | P0 #8 — Next.js 15.4.3 vulnerable version | **RESOLVED** 2026-05-23 | `bump-next-js` (`e57ea17`) | -**Only P0 #6 remains open** — extending `lib/rate-limit.js` to -`/api/users/search`, `/api/cards/search`, all `/api/cards/import-*`, -and `/api/user/avatar*` (queued as `add-rate-limiting`, launch -sequence step 4 below). That convoy is the last P0 milestone before -the security gate closes. +**Milestone reached 2026-05-24:** `add-rate-limiting` (PR #20, squash +commit `708ef45`) closed P0 #6 — the last open P0 — flipping the +ship-blocker set from 7/8 to **8/8 RESOLVED**. The security gate is +closed; remaining launch work is P1 quality bar (single SQL client, +single auth provider, lint baseline cleanup, brand decision, test +coverage expansion) plus the P2 / P3 polish lanes in this file's +Queued convoys section. None of those are P0 ship-blockers. ## P0 — ship-blockers (security) @@ -113,13 +115,40 @@ These MUST land before any anonymous traffic touches the production URL. - **Operator action required going forward:** **none.** No env vars to seed, no secrets to rotate, no infra changes. The `forbidden-cors-headers` job is self-contained (plain bash grep on the runner); future PRs that accidentally re-scaffold a wildcard CORS header will fail the build with a file-and-line pointer to the offending line. - **Owns:** `role-implementer`. -### 6. No rate limiting anywhere — **PARTIAL 2026-05-23** +### 6. No rate limiting anywhere — **RESOLVED 2026-05-24** *(milestone — last P0 closed)* -- **Partially resolved by:** `fix-auth-bypass` Brief 4, commit `297afca` (PR #9). Login + register only; the rest of the listed endpoints are **deferred** to the queued `add-rate-limiting` convoy. +- **Resolved by:** `fix-auth-bypass` Brief 4, commit `297afca` (PR #9, login + register only) + `add-rate-limiting`, squash commit `708ef45` (PR #20, the remaining surface + 3-import-route gating + 1 atomic admin UI fix + 1 rule extension). - **Impact:** Login endpoint accepts unlimited attempts; card-search endpoint can be hammered; image upload endpoints can be exhausted. The `pages/api/cards/import-*.js` endpoints externally hit Scryfall/Pokémon APIs with no caller throttling. - **Fix:** Adopt `@upstash/ratelimit` (free tier covers a small launch) or Vercel's built-in middleware-based rate limiting. Apply to: `/api/auth/login`, `/api/auth/register`, `/api/users/search`, `/api/cards/search`, all `/api/cards/import-*`, and `/api/user/avatar*` (upload). -- **As-shipped:** `lib/rate-limit.js` (new) provides `checkAuthRateLimit(req)` via `@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0` (5 attempts / 15-min sliding window per IP). Wired into login + register. Env vars are `KV_REST_API_URL` / `KV_REST_API_TOKEN` (auto-provisioned by Vercel's Upstash Marketplace integration — note this is a rename from the brief's original `UPSTASH_REDIS_REST_*` spec; see `.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md` § Post-merge addendum). Fails closed in prod when env vars are unset; warn-and-no-ops in dev. Search / import / avatar endpoints are unchanged. -- **Owns:** `role-architect` (pattern) → `role-implementer` (per-route). +- **As-shipped (Brief 4, 2026-05-23):** `lib/rate-limit.js` (new) provides `checkAuthRateLimit(req)` via `@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0` (5 attempts / 15-min sliding window per IP). Wired into login + register. Env vars are `KV_REST_API_URL` / `KV_REST_API_TOKEN` (auto-provisioned by Vercel's Upstash Marketplace integration — note this is a rename from the brief's original `UPSTASH_REDIS_REST_*` spec; see `.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md` § Post-merge addendum). Fails closed in prod when env vars are unset; warn-and-no-ops in dev. Search / import / avatar endpoints were unchanged at that point (the deferred surface that `add-rate-limiting` then closed). +- **As-shipped (`add-rate-limiting`, 2026-05-24, squash commit `708ef45`, PR #20, architect-commit `60b842e`, implementer-commit `51a3a97`):** + 1. **`lib/rate-limit.js` refactored** from a single auth-only `Ratelimit` instance into a `Map` cache with one shared Redis client and **5 `Ratelimit` instances** (one per class, distinct Redis prefix). Five exported functions: `checkAuthRateLimit(req)` (Brief 4 contract preserved byte-identical), `checkSearchRateLimit(req)`, `checkUploadRateLimit(req, userId)`, `checkGenerateRateLimit(req, userId)`, `checkImportRateLimit(req, userId)`. Internal `check(className, identifier)` shared helper. `LIMITER_CONFIG` is a top-level `const` map of `{ limit, window, prefix }` per class; adding a sixth class is a one-line addition + one new exported function. + 2. **6 routes newly gated** with the appropriate per-class limiter at the correct ordering (auth before user-keyed limiter; method check first): + + | Limiter | Limit/window | Key | Routes | + | --- | --- | --- | --- | + | `checkAuthRateLimit` (unchanged from Brief 4) | 5 / 15 min | IP | `auth/login.js`, `auth/register.js` | + | `checkSearchRateLimit` (new) | 60 / 1 min | IP | `users/search.js`, `cards/search.js` | + | `checkUploadRateLimit` (new) | 10 / 1 hour | user | `user/avatar.js` | + | `checkGenerateRateLimit` (new) | 5 / 1 hour | user | `user/avatar/generate.js` | + | `checkImportRateLimit` (new) | 5 / 1 hour | user (admin-only) | `cards/import-mtg.js`, `cards/import-pokemon.js`, `cards/import-lorcana.js` | + + 3. **`extractUserIdentifier(userId)` THROWS** on `null` / `undefined` / `''` / `NaN` (Decision 4 defensive shape). Surfaces gate-ordering bugs at dev time rather than silently falling back to IP and converting a per-user limit into a per-IP limit — which would lock other household members out for one user's behavior. Numeric `0` is intentionally accepted (returns `'user:0'`) for forward-compat. + 4. **Three `pages/api/cards/import-*.js` routes newly auth-gated.** Each grew `getUserFromRequest` + `if (user.role !== 'admin') return 403` + `checkImportRateLimit(req, user.userId)` before the existing `try` block. Closes the publicly-callable anonymous-abuse vector the architect's pre-brief audit flagged (each handler hits Scryfall / Pokémon-TCG / Lorcana APIs and performs UPSERTs into `cards` with no caller throttling pre-convoy). Lorcana was gated defensively despite zero current frontend callers — see § Queued convoys for the `delete-dead-lorcana-import` cleanup follow-up. + 5. **Atomic admin UI fix in `pages/admin/card-import.js`.** Added `'Authorization': \`Bearer ${localStorage.getItem('auth_token')}\`` to the import fetch's headers (one-line addition). **This was the architect's critical pre-brief discovery and the reason Decision 1 routed back to the operator** — gating the import APIs without this matching client fetch fix would have closed P0 #6 but introduced an immediate 401 on every "Import Cards" click, producing a visible UX regression on the only live admin tooling that exercises the gated routes. Shipping the API gate + the client fix in the same atomic PR is what made Decision 1 Option A viable. + 6. **`.cursor/rules/api-routes.mdc` § Rate limiting extended** with the per-class table + verbatim call shape + gate-ordering rules (method check first; auth before any user-keyed limiter; admin-role check goes between auth and rate-limit for the import routes) + identifier-extraction documentation + uniform 429 response shape + fail-closed env-var contract + fail-open Upstash-outage behavior. Doc-writer pass verified the implementer's extension is complete; no further touch-ups needed. + 7. **All six architect decisions ratified at gate 1.** D1 operator-ratified (Option A — gate all three import routes plus the atomic admin UI fix); D2-D6 architect-self-ratified per the precedent established by `cors-tighten` D2-D5 and `fix-vercel-deployment-protection-in-ci` A/B/D (hybrid named-limiter shape; per-class limit values with tuning evidence; two-extractor shape with defensive THROW; uniform 429 message; no new vitest / playwright specs in this convoy). + 8. **Diff: 12 files, +1612 / -23 in the squash.** The 1612-addition figure is dominated by the architect's convoy + brief files (676 + 751 lines) which the squash includes because the architect commit preceded the implementer commit on the same branch. Actual source-file diff is much smaller: `lib/rate-limit.js` +90/-23 (lib refactor); `.cursor/rules/api-routes.mdc` +41 (rule extension); 6 route files +69 total (3× +16 for import routes, 4× +7 for search/avatar/generate/users-search); `pages/admin/card-import.js` +1 (Bearer-header addition). +- **As-shipped metrics (post-merge run 26382185019 + subsequent runs):** + - `Playwright smoke` — PASS in **59s, 3/3 tests in 3.8s** against the post-rate-limit Vercel preview (home redirects ✓ 431ms / sign-in page renders ✓ 331ms / `/api/health` ✓ 193ms). **Critical cross-validation:** smoke calls `/api/health` once per run (well below the new search class's 60/min ceiling), and the home + sign-in routes don't touch any of the 6 newly-gated endpoints — so the new search limiter does NOT 429 the smoke spec. The cross-validation lineage now accumulates across three convoys: smoke test 2 still passes against post-PR-#15 Layout default-user + post-PR-#19 CORS-tighten + post-PR-#20 rate-limiting — the same 3-test spec has defended the auth surface through three sweeping changes without anyone writing a dedicated test. + - `forbidden-cors-headers` (from `cors-tighten`) — PASS. The convoy is purely additive of rate-limit gate code; no CORS headers were reintroduced. + - `forbidden-endpoints` (from `fix-auth-bypass` Brief 3) — PASS. No new `pages/api/test-*.js` or deleted-endpoint shapes reintroduced. + - `Unit tests (vitest)` — PASS, **21/21 in 27s**. Decision 6 (no new vitest specs) verified at architect time (`rg 'rate-limit|@upstash' test/` returns zero matches; the existing 21 specs don't transitively import `lib/rate-limit.js`, so the lib refactor was strictly safer than the convoy file's stale § Known constraints implied). + - `Lint` — 128 problems (baseline preserved, no regression). Zero new lint problems from the lib refactor, the 6 route edits, or the admin UI one-liner. + - `Screenshot diff` — `continue-on-error: true` swallow per `adopt-playwright-smoke` Decision 4 (no baseline committed yet). Triggered on PR #20 because the `paths:` filter `pages/**` matches the 6 route edits under `pages/api/`; same minor false-positive as PR #19, tracked by the queued `tighten-visual-diff-path-filter` follow-up. Not a regression. + - All other gates (`Schema map up to date`, `Aggregate gate`) — green. +- **Operator action required going forward:** **none.** All Upstash env vars (`KV_REST_API_URL` / `KV_REST_API_TOKEN`) were already auto-provisioned via the Vercel Marketplace integration for Brief 4. No new secrets, no infra changes, no CI gates to enable. The fail-loud-in-prod predicate in `lib/rate-limit.js::init()` is self-defending: if a future deploy unsets either env var, every gated route fails closed on the first call (`throw new Error('[rate-limit] Upstash not configured...')`). If a follow-up tuning need surfaces (search 60/min too tight, generate 5/hour too tight), the fix is a single-line `LIMITER_CONFIG` edit; surface as `tune-search-rate-limit` or `tiered-rate-limits` only if real users 429. +- **Owns:** `role-architect` (pattern + Decision 1 routing) → `role-implementer` (per-route). ### 7. Layout default-prop leaks maintainer email — **RESOLVED 2026-05-24** @@ -308,7 +337,7 @@ Each phase is one Conductor-created convoy. Don't run more than two in parallel 2. **`drop-public-setup`** (P0 #3, #4). One PR. Trivial; do as a hotfix. 3. **`fix-layout-default-user`** (P0 #7). One PR. Trivial. 3.5. **`fix-lint-baseline`** (P1 #11.5). 2-4 PRs via multitask. Closes the lint gate (drops `continue-on-error`). -4. **`add-rate-limiting`** (P0 #6 full). One PR. Adds @upstash/ratelimit + applies to listed routes. +4. **`add-rate-limiting`** (P0 #6 full). One PR. Adds @upstash/ratelimit + applies to listed routes. **RESOLVED 2026-05-24** — PR #20 squash `708ef45`; closes P0 #6 (last open P0), flipping the ship-blocker set to **8/8 RESOLVED**. 5 named limiters (auth/search/upload/generate/import), 6 routes newly gated + the 3 import routes auth-gated atomically with a `pages/admin/card-import.js` Bearer-header fix. Smoke 3/3 green in 3.8s post-merge — confirms the new 60/min search limiter doesn't 429 the smoke spec. See § Queued convoys and P0 #6 above for the full as-shipped block. 5. **`pick-a-name`** (P1 #12). Human decision first, then one or two PRs. 6. **`adopt-vitest`** (P1 #10 step 1). One PR. Enables testing every future change. 7. **`migration-tool`** (P1 #11). One PR. Backfill + first new migration. @@ -326,12 +355,15 @@ Total: ~14 convoys to get from current state to public-launch-ready. Estimate 4- Follow-ups surfaced mid-convoy or mid-PR that didn't fit the original launch sequence but need to land before public traffic. Listed in priority order; not all will be P0/P1 — most are CI / DX / hygiene polish. - **`rotate-default-admin`** (priority: P2 hygiene). Operator-rotation script for envs that ran `setup-neon-db.js` before `drop-public-setup` and still carry the weak `admin123` bcrypt hash. Surfaced in P0 #3 § Operator caveat. Optional: do nothing if no audit finds a deployed env with the weak hash. -- **`add-rate-limiting`** (priority: P1 quality, also listed in launch sequence step 4). Extend `lib/rate-limit.js` to `/api/users/search`, `/api/cards/search`, all `/api/cards/import-*`, and `/api/user/avatar*`. Login + register already wired in `fix-auth-bypass` Brief 4. -- **`tighten-visual-diff-path-filter`** (priority: P3 polish). Tighten `.github/workflows/visual-diff.yml`'s `paths:` filter from `pages/**` to something like `pages/!(api)/**` so API-only PRs don't trigger the visual workflow. Surfaced on PR #19 (`cors-tighten`) which was API-only but triggered `Screenshot diff` anyway, wasting ~55s of CI time. Low priority — works as designed today, just chatty. One-line YAML tweak; verify the negated-glob syntax against GitHub Actions' [path-filter semantics](https://docs.github.com/en/actions/using-workflows/triggering-a-workflow#filter-pattern-cheat-sheet) before merging (the `!()` extglob may need to be expressed differently if Actions uses minimatch without extglob support). +- **`delete-dead-lorcana-import`** (priority: P3 polish). Delete `pages/api/cards/import-lorcana.js` (and possibly `scripts/import-lorcana.js`) if Lorcana stays out of the admin UI's `` only offers `'mtg'` and `'pokemon'`. The route is gated defensively (auth + admin-role + rate-limit) as part of PR #20 so the future Lorcana admin UI path inherits protection automatically, but if Lorcana is never wired in, this is the cleanup convoy. Strictly easier than gating-then-deleting because the gating shape is uniform across all three import routes today (mtg + pokemon + lorcana); a future cleanup only needs to delete the lorcana file + remove the `'lorcana'` enum option from `.cursor/rules/api-routes.mdc`'s import-routes table. Do nothing if Lorcana support gets wired into the admin UI in a feature convoy; cancel the entry then. +- **`tighten-visual-diff-path-filter`** (priority: P3 polish). Tighten `.github/workflows/visual-diff.yml`'s `paths:` filter from `pages/**` to something like `pages/!(api)/**` so API-only PRs don't trigger the visual workflow. Surfaced on PR #19 (`cors-tighten`) which was API-only but triggered `Screenshot diff` anyway, wasting ~55s of CI time. PR #20 (`add-rate-limiting`) re-triggered the same false-positive — same shape, same swallow via `continue-on-error: true`. Low priority — works as designed today, just chatty. One-line YAML tweak; verify the negated-glob syntax against GitHub Actions' [path-filter semantics](https://docs.github.com/en/actions/using-workflows/triggering-a-workflow#filter-pattern-cheat-sheet) before merging (the `!()` extglob may need to be expressed differently if Actions uses minimatch without extglob support). - **`purge-weak-creds-from-helpers`** (priority: P2 hygiene). Sweep `scripts/reset-db.js`, `scripts/create-test-users.js`, and `TESTING_GUIDE.md` for the literal `admin@tcgvault.com` / `admin123` references. May fold into `pick-a-name` since the email itself is changing. - **`single-auth-provider`** (priority: P1 quality, also listed as launch sequence step 9). Collapse `lib/auth-context.js` + `lib/admin-auth.js` into `lib/use-auth.js`. Surfaced again as a follow-up in P0 #7 § Flagged-but-deferred (4 pages still import the legacy `useAuth`). - **`cleanup-mobile-nav-dead-props`** (priority: P3 polish). `components/MobileNavigation.js` accepts a dead `user` prop; remove it. Surfaced in P0 #7 § Flagged-but-deferred. May fold into `god-component-split` (P2 #13) if that lands first. - **`bump-eslint-10`** (priority: P2 hygiene; upstream-blocked). Bump ESLint from v9 to v10 once `typescript-eslint` ships a v10-tested release and `eslint-config-next` bundles it. Surfaced in `.convoys/bump-next-js.md` § Decisions D. +- **`harden-multipart-parser`** (priority: P2 quality). Surfaced 2026-05-24 in `add-rate-limiting` § Risk list. `pages/api/user/avatar.js`'s `parseMultipartFormData` consumes the 5MB multipart body via `req.on('data')` before any response is sent, so an attacker can still exhaust the 5MB body even on a 429 path from the new `checkUploadRateLimit` gate. Real defense requires moving the parse into a separate edge function or using `read-up-to` semantics. Not a release-blocker — the gate-ordering in PR #20 places the limiter BEFORE the method branches that call `parseMultipartFormData`, so when this hardening lands, the gate ordering is already correct. Surface as P1 only if a real abuse incident occurs. +- **`god-function-split` / `refactor-cards-search-sql`** (priority: P2 refactor). Surfaced 2026-05-24 in `add-rate-limiting` § Files explicitly out of scope. `pages/api/cards/search.js` has a 240-line god-function shape with 7+ conditional `SELECT * FROM cards WHERE …` branches; the PR #20 rate-limit gate sits at the top of the handler and leaves the SQL byte-identical. Splitting is its own scope (probably one convoy per branch group with `slice_dependencies:` for safe multitask fan-out). Not security-critical; deferred to the P2 lane. +- **`withAdmin(handler)` wrapper extraction** (priority: P3 polish / DX). Surfaced 2026-05-24 in `add-rate-limiting` Decision 1 + § What did NOT change. `.cursor/rules/auth-and-permissions.mdc` notes *"check `user.role === 'admin'` directly; consider extracting `withAdmin()` if a third call site appears"* — the three `cards/import-*.js` routes are the third+fourth+fifth call sites in the codebase, but PR #20 kept the inline shape for uniformity across the three import routes and for the convoy's atomic-close-P0-#6 goal. A future convoy can extract `withAdmin(handler)` to `lib/permission-middleware.js` (or wherever the architect decides) and sweep all 5 admin-role check sites onto it. Pure refactor; no security delta either way. - **`seed-visual-baselines-on-linux`** (priority: P2 CI infra; **operator action required**). Generate Linux baselines for `tests/visual/__screenshots__/` in the `mcr.microsoft.com/playwright:v1.60.0-noble` Docker image and commit them in a small follow-up PR. Mac-generated baselines would silently overwrite Linux CI baselines because `playwright.config.js`'s custom `snapshotPathTemplate` has no `{platform}` token (Risk R3 + Boot-the-brief Finding 7 in `.convoys/adopt-playwright-smoke.md`). Until this PR lands, every `Screenshot diff` run on a PR touching `pages/**` / `components/**` / `styles/**` / Tailwind/PostCSS config fails at the test step and posts a "Visual Diff — view run" comment with empty artifacts — that's the documented Decision-4 end state of `adopt-playwright-smoke`, not a regression. One small PR with just the PNG baseline(s). Surfaced 2026-05-24 as the follow-up to `adopt-playwright-smoke` (PR #18). - **`adopt-playwright-smoke`** (priority: P1 quality, also listed as launch sequence step 10 / P1 #10 step 2) — **RESOLVED 2026-05-24**. - **Resolved by:** squash commit `7b6f751` (PR #18, architect-commit `3ac527e`, implementer-commit `c72d006`). Brief 1 shipped as planned with two small lint-baseline-preserving deviations from the brief's verbatim shape (documented in the convoy file's § As-shipped). diff --git a/.cursor/rules/api-routes.mdc b/.cursor/rules/api-routes.mdc index 12eabaf..c834ca3 100644 --- a/.cursor/rules/api-routes.mdc +++ b/.cursor/rules/api-routes.mdc @@ -103,7 +103,7 @@ await logCollectionActivity(collectionId, userId, 'card_added', { cardId, quanti ## 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 }`. +`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 | | --- | --- | --- | --- | --- | --- | diff --git a/AGENTS.md b/AGENTS.md index 468120f..9dfa564 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -60,6 +60,22 @@ Code graph is indexed by `user-code-review-graph` MCP (122 files, 628 nodes, 560 - **#11 — Turbopack is now the default bundler.** `next dev` and `next build` use Turbopack by default in Next.js 16. The fallback per command is `--webpack` (e.g. `next build --webpack`). We have no custom `webpack:` block in `next.config.js`, no custom loaders/aliases, and no Sass tilde imports, so Turbopack should "just work" — but if a build/runtime regression appears, reproduce on both bundlers before deciding whether to revert or pin a script to webpack. Do not pre-emptively switch to `--webpack`. - **#12 — Rate-limit env vars are `KV_REST_API_URL` / `KV_REST_API_TOKEN`, not `UPSTASH_REDIS_REST_*`.** `lib/rate-limit.js` reads the Vercel Upstash Marketplace integration's auto-provisioned names. Three other Upstash-shaped vars exist in the Vercel-managed env (`KV_URL`, `REDIS_URL`, `KV_REST_API_READ_ONLY_TOKEN`) but our `@upstash/redis` REST client does not use them — do not wire to them. In prod, the rate-limit module **fails closed** if either of the two REST vars is missing (a single failed login is a better outcome than silently disabling brute-force protection). In dev / test, it warn-and-continues as a no-op so local work is unaffected when Upstash isn't wired up. + **Milestone — `add-rate-limiting` convoy (squash `708ef45`, PR #20, 2026-05-24) closed P0 #6 — all 8 P0s now RESOLVED.** The lib refactored from a single auth-only limiter to **5 named limiters** with a `Map` cache (one shared Redis client, five `Ratelimit` instances, distinct Redis prefix per class). The five exports + their use cases: + + | Helper | Class | Limit/window | Key | Routes | + | --- | --- | --- | --- | --- | + | `checkAuthRateLimit(req)` | `auth` | 5 / 15 min | IP | `/api/auth/login`, `/api/auth/register` (Brief 4 contract; byte-identical return shape preserved) | + | `checkSearchRateLimit(req)` | `search` | 60 / 1 min | IP | `/api/users/search`, `/api/cards/search` | + | `checkUploadRateLimit(req, userId)` | `upload` | 10 / 1 hour | user | `/api/user/avatar` | + | `checkGenerateRateLimit(req, userId)` | `generate` | 5 / 1 hour | user | `/api/user/avatar/generate` | + | `checkImportRateLimit(req, userId)` | `import` | 5 / 1 hour | user (admin-only) | `/api/cards/import-mtg`, `/api/cards/import-pokemon`, `/api/cards/import-lorcana` | + + All five return the same `{ allowed, remaining, reset }` shape; on `!allowed`, set `Retry-After: Math.ceil((reset - Date.now()) / 1000)` and return 429 with the uniform message `'Too many attempts. Try again later.'` (per-class variation would fingerprint the limits to an attacker — explicitly rejected). + + **Defensive THROW pattern.** `extractUserIdentifier(userId)` THROWS with a named error when `userId` is `null` / `undefined` / `''` / `NaN`. Surfaces gate-ordering bugs at dev time rather than silently falling back to IP and converting a per-user limit into a per-IP limit (which would lock household members out for one user's behavior). Numeric `0` is intentionally accepted (returns `'user:0'`) for forward-compat. **Gate-ordering rule: per-user rate-limit gates (`upload`, `generate`, `import`) MUST sit AFTER the auth check.** For the three `/api/cards/import-*` routes, the ordering is also `auth → admin-role check (403 if not admin) → rate-limit`; the admin-role check sits between auth and rate-limit. IP-keyed gates (`auth`, `search`) can sit anywhere after the method check. + + Adding a sixth class is a one-line `LIMITER_CONFIG` addition + one new exported function (no `init()` restructuring needed). Tuning an existing class is a one-line `LIMITER_CONFIG` edit. The full verbatim call shape + gate-ordering rules + identifier-extraction documentation live in `.cursor/rules/api-routes.mdc` § Rate limiting. + ## 5. Running locally - **Runtime:** Node 20 (Vercel default).