rstillwell/deckhearth
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

docs: post-convoy cleanup for add-rate-limiting — MILESTONE, last P0 closed

Browse source 
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<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 (squash 708ef45, 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>
This commit is contained in:
Randall Stillwell 2026-05-24 23:09:07 -05:00
parent 708ef45a96
commit 7832e03dec
4 changed files with 423 additions and 16 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

361
.convoys/add-rate-limiting.md
View file

@ -14,8 +14,9 @@ skip:
- role-a11y-auditor - role-a11y-auditor
- role-ux-reviewer - role-ux-reviewer
- role-ia-architect - role-ia-architect
status: in-progress status: shipped
created: 2026-05-24 created: 2026-05-24
shipped: 2026-05-24
parent: ship-readiness parent: ship-readiness
addresses: P0 #6 (PARTIAL → RESOLVED) — the LAST open P0 addresses: P0 #6 (PARTIAL → RESOLVED) — the LAST open P0
depends_on: depends_on:
@ -674,3 +675,361 @@ slice_dependencies:
One brief, no `/multitask` fan-out — the conductor dispatches a single One brief, no `/multitask` fan-out — the conductor dispatches a single
implementer. implementer.
## As-shipped
Shipped 2026-05-24 as squash commit `708ef45` (PR #20, architect-commit
`60b842e`, implementer-commit `51a3a97`). **This is the milestone
convoy — it closes the LAST open P0 (P0 #6) and flips
`.convoys/ship-readiness.md`'s § Status summary from "7 of 8 RESOLVED;
1 remains" to "8 of 8 RESOLVED. Launch-readiness P0 checklist is
empty."** Brief 1 shipped as planned — single lib refactor + six
route-gate additions + one admin UI atomic fix + one rule extension,
no scope expansions beyond the architect-ratified Decision 1, and the
empirical CI metrics + the cross-validation finding + the
"D3 tuning-evidence" rationale captured here so the next architect /
reviewer has the audit trail.
### Decisions ratified at gate 1
All six decisions landed verbatim. Only D1 required operator gate-1
sign-off (significant scope expansion to admin-only enforcement on the
import surface plus an atomic admin UI touch); D2-D6 are
architect-self-ratifiable per the precedent established by
`cors-tighten` Decisions D2-D5 and
`fix-vercel-deployment-protection-in-ci` Decisions A/B/D.
- **Decision 1 — Option A (operator-ratified).** Gate all three
`pages/api/cards/import-*.js` routes in this convoy with
`getUserFromRequest` + `if (user.role !== 'admin') return 403` +
`checkImportRateLimit(req, user.userId)`. **And** atomically update
`pages/admin/card-import.js` to send
`'Authorization': \`Bearer ${localStorage.getItem('auth_token')}\``
on its import fetch — without this, the API gating would have
immediately broken every "Import Cards" click in the admin UI. The
architect's pre-brief investigation flagged this as the critical
hidden coupling: gating the import APIs without the matching client
fetch fix would have closed P0 #6 but introduced a visible UX
regression on the only live admin tooling that depends on it. Lorcana
was gated defensively despite having zero current frontend callers
(`pages/admin/card-import.js`'s `<select>` 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<className, Ratelimit>` 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<className, Ratelimit>` 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
`<Link href="/login">Sign in</Link>` 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.

60
.convoys/ship-readiness.md
View file

@ -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) ## 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 | | 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 #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 #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 #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 #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`) | | 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 **Milestone reached 2026-05-24:** `add-rate-limiting` (PR #20, squash
`/api/users/search`, `/api/cards/search`, all `/api/cards/import-*`, commit `708ef45`) closed P0 #6 — the last open P0 — flipping the
and `/api/user/avatar*` (queued as `add-rate-limiting`, launch ship-blocker set from 7/8 to **8/8 RESOLVED**. The security gate is
sequence step 4 below). That convoy is the last P0 milestone before closed; remaining launch work is P1 quality bar (single SQL client,
the security gate closes. 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) ## 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. - **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`. - **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. - **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). - **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. - **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).
- **Owns:** `role-architect` (pattern) → `role-implementer` (per-route). - **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<className, Ratelimit>` 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** ### 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. 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. **`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`). 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. 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. 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. 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. 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. - **`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. - **`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 `<select>` permanently. Surfaced 2026-05-24 in `add-rate-limiting` Decision 1: the architect ran `rg 'import-lorcana' pages/ components/` and found zero frontend callers — `pages/admin/card-import.js`'s `<select>` 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. 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). - **`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. - **`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`). - **`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. - **`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. - **`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). - **`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**. - **`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). - **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).

2
.cursor/rules/api-routes.mdc
View file

@ -103,7 +103,7 @@ await logCollectionActivity(collectionId, userId, 'card_added', { cardId, quanti
## Rate limiting ## 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 | | Class | Limit | Window | Key | Used by | Helper |
| --- | --- | --- | --- | --- | --- | | --- | --- | --- | --- | --- | --- |

16
AGENTS.md
View file

@ -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`. - **#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. - **#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<className, Ratelimit>` 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 ## 5. Running locally
- **Runtime:** Node 20 (Vercel default). - **Runtime:** Node 20 (Vercel default).