feat(security): rate-limit search/upload/import + gate import routes (P0 #6 - closes last P0)
Closes P0 #6 from PARTIAL to RESOLVED. 8/8 P0s now closed. Extends lib/rate-limit.js from single-class to 5 named limiters (auth/search/upload/generate/import). Atomically gates the 3 import routes (auth + admin-role check + rate limit) and fixes pages/admin/card-import.js's missing Bearer header in the same commit (architect's critical discovery: API gating alone would have broken the admin UI). Per Decision 1 Option A. 10 files +185/-23. Local: lint 128 baseline, vitest 21/21. CI: Playwright smoke 3/3 in 3.8s, forbidden-cors-headers pass, all gates green. PR #20 architect-commit60b842e, implementer-commit51a3a97. Brief 4's login.js + register.js byte-identical.
This commit is contained in:
varutasu
GitHub
parent
f8b2cd85bc
commit
708ef45a96
12 changed files with 1612 additions and 23 deletions
676
.convoys/add-rate-limiting.md
Normal file
676
.convoys/add-rate-limiting.md
Normal file
|
|
@ -0,0 +1,676 @@
|
|||
---
|
||||
name: add-rate-limiting
|
||||
classification: convoy
|
||||
success_metric: |
|
||||
All endpoints listed in P0 #6 of `.convoys/ship-readiness.md` carry
|
||||
a rate-limit gate appropriate to their abuse class (search /
|
||||
upload / import / admin). The card-import endpoints are no longer
|
||||
anonymous-callable. `lib/rate-limit.js` exposes named limiters
|
||||
per route class with documented limits + windows + key-extraction
|
||||
strategies. P0 #6 flips PARTIAL → RESOLVED, closing the last
|
||||
open P0 ship-blocker.
|
||||
skip:
|
||||
- role-design-system-auditor
|
||||
- role-a11y-auditor
|
||||
- role-ux-reviewer
|
||||
- role-ia-architect
|
||||
status: in-progress
|
||||
created: 2026-05-24
|
||||
parent: ship-readiness
|
||||
addresses: P0 #6 (PARTIAL → RESOLVED) — the LAST open P0
|
||||
depends_on:
|
||||
- fix-auth-bypass (Brief 4 shipped lib/rate-limit.js + auth-only limiter)
|
||||
- cors-tighten (just closed P0 #5; not a hard dep but the API surface is now CORS-clean)
|
||||
---
|
||||
|
||||
# Convoy: add-rate-limiting
|
||||
|
||||
Extend `lib/rate-limit.js` and wire the result into the remaining
|
||||
abusable endpoints. This is the final P0 — when it lands, the
|
||||
launch-readiness ship-blocker list is empty (all 8 of 8 RESOLVED).
|
||||
|
||||
## Why now
|
||||
|
||||
`fix-auth-bypass` Brief 4 shipped the rate-limit infrastructure
|
||||
(`lib/rate-limit.js`, `@upstash/ratelimit@^2.0.8`,
|
||||
`@upstash/redis@^1.38.0`, Vercel-Upstash Marketplace env vars
|
||||
`KV_REST_API_URL` / `KV_REST_API_TOKEN`) and wired it into the
|
||||
two auth endpoints (`login`, `register`) with a 5-attempts /
|
||||
15-minute sliding window. The remaining abusable surface was
|
||||
intentionally deferred to this convoy.
|
||||
|
||||
The "Queued convoys" entry in `.convoys/ship-readiness.md` lists
|
||||
the surface as:
|
||||
|
||||
- `/api/users/search`
|
||||
- `/api/cards/search`
|
||||
- All `/api/cards/import-*`
|
||||
- `/api/user/avatar*` (upload)
|
||||
|
||||
Parent's pre-architect audit at convoy creation surfaced a
|
||||
**critical secondary finding** beyond "missing rate limit":
|
||||
|
||||
**`pages/api/cards/import-mtg.js`, `import-pokemon.js`,
|
||||
`import-lorcana.js` have ZERO authentication checks.** They are
|
||||
publicly callable, they hit Scryfall / Pokémon-TCG / Lorcana APIs
|
||||
with no caller throttling AND no auth, and they perform UPSERTs
|
||||
into the `cards` table. An attacker can:
|
||||
|
||||
1. Trigger expensive external API calls from your IP (Scryfall +
|
||||
Pokémon-TCG have published rate limits; you'd hit them with no
|
||||
recourse, getting your IP throttled at the source).
|
||||
2. Cause unbounded DB writes (each import inserts hundreds-to-thousands
|
||||
of rows; `card.id` lookups deduplicate but the INSERT path runs
|
||||
in a tight per-card loop).
|
||||
3. Even with a generous per-IP rate limit (5 req / hour), with
|
||||
1000 IPs the math is 120,000 imports / day — enough to exhaust
|
||||
Vercel function quotas and Neon row budgets.
|
||||
|
||||
The rate-limit alone is insufficient. **Auth-gate-then-rate-limit
|
||||
is the correct shape.** Whether to add auth gates to the import
|
||||
routes in this convoy (vs. spinning out a separate
|
||||
`gate-import-routes` convoy) is **architect Decision 1** below.
|
||||
Parent recommends in-scope; expansion is mid-convoy precedent
|
||||
established by `drop-public-setup` (Brief 2's CJS/ESM expansion)
|
||||
and other prior convoys.
|
||||
|
||||
## Scope
|
||||
|
||||
### Confirmed in-scope (regardless of architect Decision 1)
|
||||
|
||||
- **`lib/rate-limit.js`** — refactor to expose multiple named
|
||||
limiters (one per route class), preserving the existing
|
||||
`checkAuthRateLimit` export for `login` / `register` backwards
|
||||
compatibility. New named limiters per Decision 2 below.
|
||||
- **`pages/api/users/search.js`** — add rate-limit gate after the
|
||||
existing JWT verification. Limit per Decision 2.
|
||||
- **`pages/api/cards/search.js`** — add rate-limit gate at top of
|
||||
handler (this endpoint is anonymous-by-design; rate-limit keyed
|
||||
by IP).
|
||||
- **`pages/api/user/avatar.js`** — add rate-limit gate after the
|
||||
existing `getUserFromRequest` check. Limit keyed by `user.userId`
|
||||
(authenticated; per-user makes more sense than per-IP for an
|
||||
upload endpoint where a household might share an IP).
|
||||
- **`pages/api/user/avatar/generate.js`** — same shape as
|
||||
`avatar.js`. Per-user, stricter than avatar.js (AI generation
|
||||
is more expensive than blob upload).
|
||||
- **`.cursor/rules/api-routes.mdc`** § Rate limiting — update the
|
||||
existing § with the new per-class pattern + each named limiter's
|
||||
use case. The current § shows only the auth pattern.
|
||||
|
||||
### Scope-expansion-candidate (architect Decision 1)
|
||||
|
||||
- **`pages/api/cards/import-mtg.js`** — add `getUserFromRequest`
|
||||
check at top of handler; return 403 if not admin (`user.role !==
|
||||
'admin'`); then rate-limit gate (per-user, very strict — these
|
||||
hit external APIs).
|
||||
- **`pages/api/cards/import-pokemon.js`** — same shape.
|
||||
- **`pages/api/cards/import-lorcana.js`** — same shape.
|
||||
- (If Decision 1 = Option A "in scope": ship in this PR. If Option
|
||||
B "spin out": queue `gate-import-routes` follow-up convoy and
|
||||
mark P0 #6 RESOLVED-with-caveat at convoy close.)
|
||||
|
||||
### Out of scope
|
||||
|
||||
- **Global IP-based backstop limiter** (e.g., 1000 req / min per
|
||||
IP across all routes via Next.js middleware). Useful but
|
||||
separate scope; `add-global-rate-limit-middleware` would be its
|
||||
own convoy.
|
||||
- **Rate-limit headers on success responses**
|
||||
(`X-RateLimit-Remaining`, `X-RateLimit-Reset`). The existing
|
||||
`checkAuthRateLimit` returns `remaining` + `reset` in its
|
||||
result object but `login.js` doesn't propagate them on
|
||||
success; just on 429. Honoring the existing convention.
|
||||
- **`@upstash/ratelimit` version bump** — pin stays at `^2.0.8`
|
||||
from Brief 4. Bumping is its own convoy.
|
||||
- **Per-route handler unit tests** (still deferred to the
|
||||
queued `fill-vitest-handler-coverage` convoy — same reasoning
|
||||
as `cors-tighten`).
|
||||
- **Refactoring the existing `cards/search.js` SQL** (the file
|
||||
has a known god-function shape with 7+ conditional SQL
|
||||
branches; that's `god-function-split` scope, not here).
|
||||
- **Verifying the avatar `parseMultipartFormData` body parser**
|
||||
is rate-limit-safe (the body is read in `req.on('data')` before
|
||||
any rate-limit gate could short-circuit; an attacker can still
|
||||
exhaust the 5MB body even on a 429 path). That's a separate
|
||||
body-streaming-defense concern (`harden-multipart-parser`) and
|
||||
not in scope here.
|
||||
|
||||
## Operator action required
|
||||
|
||||
**None.** All Upstash env vars (`KV_REST_API_URL` /
|
||||
`KV_REST_API_TOKEN`) are already auto-provisioned via the Vercel
|
||||
Marketplace integration (seeded for `fix-auth-bypass` Brief 4).
|
||||
No new secrets, no new dependencies (the `@upstash/ratelimit` +
|
||||
`@upstash/redis` packages are already installed).
|
||||
|
||||
## Decisions to ratify with operator (architect routes)
|
||||
|
||||
1. **Scope expansion — auth gates on `pages/api/cards/import-*.js`
|
||||
in this convoy?**
|
||||
- **Option A — In scope (parent recommends).** Add
|
||||
`getUserFromRequest` + admin-role check + rate-limit gate to
|
||||
all 3 import routes in this PR. ~30 LOC across 3 files.
|
||||
Closes the full P0 #6 attack surface in one shot. Precedent:
|
||||
`drop-public-setup` Brief 2 (CJS/ESM expansion).
|
||||
- **Option B — Spin out.** Stay narrow on the 4 originally-listed
|
||||
routes; queue `gate-import-routes` as a separate convoy. Mark
|
||||
P0 #6 RESOLVED-with-caveat noting the import-route auth gap.
|
||||
- **Option C — Hybrid.** Add the rate-limit gate to import
|
||||
routes now (with a clear `TODO: requires-auth` comment),
|
||||
gate auth later. Worst of both worlds — leaves an
|
||||
anonymous-callable abusive endpoint live with only IP-based
|
||||
throttling.
|
||||
- Architect investigates: confirm the import routes are
|
||||
intended to be admin-only (per AGENTS.md / scripts/ folder
|
||||
conventions), confirm Decision-A scope size is bounded, route
|
||||
back recommendation.
|
||||
|
||||
2. **Named-limiter shape in `lib/rate-limit.js`.** The existing
|
||||
`checkAuthRateLimit(req)` function has the limit + window
|
||||
hardcoded. To support per-class limits cleanly, two main
|
||||
patterns:
|
||||
- **(a) Named-export functions.** Each route class gets its
|
||||
own exported function: `checkSearchRateLimit`,
|
||||
`checkUploadRateLimit`, `checkImportRateLimit`. Internal
|
||||
`init()` builds one `Ratelimit` instance per class, cached
|
||||
under different Redis prefixes. Verbose but very explicit at
|
||||
each call site.
|
||||
- **(b) Generic `checkRateLimit(req, options)`.** Single
|
||||
exported function takes a config object. Calling code
|
||||
becomes `await checkRateLimit(req, { class: 'search' })` or
|
||||
similar. Less verbose at the lib but more at each call site,
|
||||
and the per-route limits are documented in the lib module
|
||||
rather than implicit in the function name.
|
||||
- **(c) Hybrid.** Keep `checkAuthRateLimit` (used by login +
|
||||
register; high-stakes contract; don't break). Add named
|
||||
functions for the other 3 classes (search, upload, import).
|
||||
Best of both — preserves Brief 4's contract, names new
|
||||
classes explicitly.
|
||||
- Parent recommends (c). Architect ratifies after reading
|
||||
`lib/rate-limit.js` shape.
|
||||
|
||||
3. **Per-class limit values.** Reference table (architect's
|
||||
to tune):
|
||||
- **`auth`** (existing) — 5 / 15min, IP-keyed. (Don't change.)
|
||||
- **`search`** (new) — recommend 30 / 1min, IP-keyed.
|
||||
Legitimate users type-ahead-search; 30/min is generous but
|
||||
stops a scraper in its tracks within 2-3s. Same IP-keying
|
||||
as auth.
|
||||
- **`upload`** (new — avatar.js, avatar/generate.js) —
|
||||
recommend 10 / 1hour, user-keyed. Avatar uploads are rare;
|
||||
10/hour catches accidental loops without blocking a user
|
||||
who hits "save" three times by accident. User-keyed because
|
||||
the user is authenticated and a household IP shouldn't
|
||||
punish other users.
|
||||
- **`generate`** (new — avatar/generate.js specifically) —
|
||||
recommend 3 / 1hour, user-keyed. AI generation costs money;
|
||||
stricter than plain upload.
|
||||
- **`import`** (new — cards/import-*.js) — recommend 5 / 1hour,
|
||||
user-keyed (if Decision 1 = Option A). Admin-triggered, hits
|
||||
external APIs with their own rate limits; 5/hour is plenty.
|
||||
4. **Identifier extraction.** `lib/rate-limit.js`'s
|
||||
`extractIdentifier()` currently always extracts the first
|
||||
`x-forwarded-for` IP. For per-user limiters (upload, generate,
|
||||
import), we need a `user_id`-based extractor. Three options:
|
||||
- **(a) Two extractors.** `extractIpIdentifier` (existing) +
|
||||
`extractUserIdentifier(req, userId)` (new). Each limiter
|
||||
calls the right one.
|
||||
- **(b) Single extractor takes an `opts.user` flag.** Less
|
||||
code but harder to read.
|
||||
- **(c) Caller passes `key` directly.** Each handler calls
|
||||
`checkUploadRateLimit(req, { key: user.userId })` and the lib
|
||||
just trusts it.
|
||||
- Parent recommends (a) — explicit, two small functions,
|
||||
hard to misuse.
|
||||
|
||||
5. **429 response shape.** Match the existing
|
||||
`login.js`/`register.js` pattern verbatim:
|
||||
```js
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
```
|
||||
Architect: should the error message vary per class (e.g., "Too
|
||||
many search requests" vs "Too many uploads") or stay uniform?
|
||||
Recommend uniform — minimizes attacker-fingerprinting of which
|
||||
routes have which limits. Architect ratifies.
|
||||
|
||||
6. **Test coverage in this convoy.** No new per-route handler
|
||||
tests (per Decision-4-equivalent from `cors-tighten`; same
|
||||
deferred-to-`fill-vitest-handler-coverage` reasoning). The
|
||||
Playwright smoke spec doesn't exercise any of these routes,
|
||||
so no smoke coverage either. **The fail-loud-in-prod
|
||||
semantics in `lib/rate-limit.js`'s `init()` function are the
|
||||
integration test** — if the env vars are unset on a deployed
|
||||
env, every limit-gated route fails closed on first call.
|
||||
|
||||
## Known constraints
|
||||
|
||||
- **`lib/rate-limit.js` already has the right architecture** for
|
||||
multiple limiters (lazy `init()` returns a `cached` object;
|
||||
refactor pattern is to make `cached` a Map<className,
|
||||
Ratelimit> instead of a single instance). Don't rewrite from
|
||||
scratch; extend.
|
||||
- **Fail-closed semantics in prod must be preserved.** Brief 4's
|
||||
`init()` throws when env vars are unset and `NODE_ENV ===
|
||||
'production'`. New limiters must inherit this — a misconfigured
|
||||
prod env should fail loud on every call, not silently disable
|
||||
all rate limiting.
|
||||
- **Card-search is anonymous-by-design** — do NOT add a
|
||||
`getUserFromRequest` check. The IP-keyed limit is the correct
|
||||
defense (search is a public catalogue feature).
|
||||
- **User-keyed limits require an authenticated request.** The
|
||||
rate-limit gate MUST sit AFTER the auth check in the handler
|
||||
body. Wrong order = security regression (anonymous user bypasses
|
||||
the gate because there's no `userId` to key on, the extractor
|
||||
falls back to IP, and the per-user limit becomes per-IP — same
|
||||
household IP could be locked out for a different user's
|
||||
behavior).
|
||||
- **Brief 4's `lib/rate-limit.js` is unit-tested transitively**
|
||||
via the existing vitest suite (none of the 21 tests
|
||||
specifically test rate-limit, but the auth-utils tests run
|
||||
through the same module). Don't break those.
|
||||
- **`@upstash/ratelimit` supports multiple shapes**
|
||||
(`slidingWindow`, `fixedWindow`, `tokenBucket`); Brief 4 used
|
||||
`slidingWindow`. New limiters should follow the same shape
|
||||
unless the architect has a specific reason for a different
|
||||
algorithm per class.
|
||||
- **Per-route Pattern checks** — the 6 (or 9 if Decision 1 =
|
||||
Option A) target files have different existing structures:
|
||||
- `users/search.js`: auth check inside try/catch on JWT;
|
||||
rate-limit goes after JWT verify, inside the existing try
|
||||
- `cards/search.js`: no auth; rate-limit goes at top of
|
||||
handler after the method check
|
||||
- `user/avatar.js`: auth check inside the outer try (line
|
||||
16-19); rate-limit goes after auth, before the method
|
||||
branching at line 21
|
||||
- `user/avatar/generate.js`: similar shape to avatar.js
|
||||
- `cards/import-*.js`: NO auth currently. Decision 1 dictates
|
||||
the shape — either add auth-then-rate-limit, or rate-limit-only
|
||||
(Option C, not recommended)
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
The convoy is shippable when ALL of the following hold:
|
||||
|
||||
1. `lib/rate-limit.js` exposes the named limiters per Decision 2.
|
||||
Backwards-compat: `checkAuthRateLimit` still works for
|
||||
`login.js` / `register.js` (do NOT break Brief 4's contract).
|
||||
2. All 4 originally-listed endpoints (`users/search`,
|
||||
`cards/search`, `user/avatar`, `user/avatar/generate`) carry
|
||||
the appropriate rate-limit gate at the correct ordering
|
||||
(after auth if applicable; per Decision 4 keying).
|
||||
3. If Decision 1 = Option A: all 3 import endpoints carry both
|
||||
the auth gate (admin-role check) + the import rate-limit gate.
|
||||
4. `npm run lint` exit code matches baseline (still 128 problems;
|
||||
do NOT regress).
|
||||
5. `npm run test:run` (vitest) still passes 21/21 (no regression
|
||||
on the existing auth-utils tests that transitively load
|
||||
`lib/rate-limit.js`).
|
||||
6. `npm run test:smoke` (via CI on the PR) still passes 3/3
|
||||
against the Vercel preview — proves the live login → verify
|
||||
→ /api/health flow doesn't accidentally get rate-limited
|
||||
(the smoke spec hits each endpoint once per run, well below
|
||||
any limit).
|
||||
7. `.cursor/rules/api-routes.mdc` § Rate limiting updated to
|
||||
document each new limiter's use case + the per-class limits
|
||||
+ the user-keyed vs IP-keyed convention.
|
||||
8. Bypass-secret-leak check: zero matches of any
|
||||
`KV_REST_API_*` or `UPSTASH_*` env-var values in any CI log.
|
||||
(Auto-masked by GitHub Actions; verify post-merge.)
|
||||
|
||||
## Anything flagged but not acted on (in advance)
|
||||
|
||||
- **Global IP backstop limiter via Next.js middleware** —
|
||||
out of scope; queue `add-global-rate-limit-middleware` if a
|
||||
future audit shows non-listed routes being abused.
|
||||
- **Body-streaming defense for avatar uploads** — the 5MB
|
||||
multipart body is consumed before any rate-limit gate can
|
||||
short-circuit. Real defense requires moving the parse into a
|
||||
separate edge function or using `read-up-to` semantics; queue
|
||||
`harden-multipart-parser` if it ever surfaces in a real abuse
|
||||
incident.
|
||||
- **Per-tier user limits** — premium users might get higher
|
||||
search/upload limits. Queue `tiered-rate-limits` when a
|
||||
tiering scheme exists (today there are only `admin` and
|
||||
default roles).
|
||||
- **Rate-limit metrics dashboard** — Upstash exposes per-key hit
|
||||
counts; surfacing them in an admin dashboard would catch
|
||||
abuse early. Queue `rate-limit-observability` post-launch.
|
||||
- **Card-search SQL god-function** — known shape issue,
|
||||
separate convoy (`god-function-split` or
|
||||
`refactor-cards-search-sql`); do NOT touch in this convoy.
|
||||
|
||||
## Decisions (post-architect)
|
||||
|
||||
Six decisions ratified by `role-architect` on 2026-05-24. **Only Decision 1
|
||||
requires operator gate-1 sign-off** (significant scope expansion to admin-only
|
||||
import-route enforcement + admin UI source touch); the other five are
|
||||
architect-self-ratifiable per the precedent established by `cors-tighten`
|
||||
Decision D2-D5 + `fix-vercel-deployment-protection-in-ci` Decisions A/B/D.
|
||||
|
||||
### Decision 1 — Scope expansion: gate `pages/api/cards/import-*.js` in this convoy (**OPERATOR-RATIFIABLE**)
|
||||
|
||||
**Ratified: Option A — gate all three import routes with auth + admin-role + import rate-limit, AND fix the admin UI to send the Bearer token.**
|
||||
|
||||
Architect investigation results:
|
||||
|
||||
- **`pages/api/cards/import-mtg.js`** — LIVE admin tooling. Called by
|
||||
`pages/admin/card-import.js` line 39-49 (the `<select>` defaults to
|
||||
`'mtg'`). Currently fetched WITHOUT an `Authorization` header.
|
||||
- **`pages/api/cards/import-pokemon.js`** — LIVE admin tooling. Same
|
||||
admin UI fetch path; the `<select>` second option is `'pokemon'`.
|
||||
- **`pages/api/cards/import-lorcana.js`** — DEAD in frontend. Architect
|
||||
ran `rg 'import-lorcana' pages/ components/` and the file has zero
|
||||
frontend callers; the admin UI's `<select>` only offers `'mtg'` and
|
||||
`'pokemon'`. The standalone `scripts/import-lorcana.js` exists as an
|
||||
independent CLI path. Gating defensively is the chosen path because:
|
||||
(a) future Lorcana admin UI work inherits protection automatically,
|
||||
(b) the diff is uniform (3 routes get identical treatment), (c) a
|
||||
future cleanup convoy can delete the route if it stays unused — and
|
||||
deletion is strictly easier than gating-then-deleting because Option A
|
||||
preserves all the import infrastructure.
|
||||
|
||||
**Critical scope expansion required by Option A:**
|
||||
`pages/admin/card-import.js` currently calls the import APIs without an
|
||||
`Authorization` header (line 43-49). The moment the import APIs gain
|
||||
`getUserFromRequest`, the admin UI begins returning 401 on every import
|
||||
click — visible UX regression. The brief therefore includes a one-line
|
||||
edit to `pages/admin/card-import.js` adding
|
||||
`'Authorization': \`Bearer ${localStorage.getItem('auth_token')}\``
|
||||
to the fetch's headers. This is the minimum touch; the 309-line
|
||||
god-component is otherwise byte-identical.
|
||||
|
||||
**Rejected alternatives:**
|
||||
- **Option B (spin out `gate-import-routes`).** Would leave a publicly-callable
|
||||
abuse vector live for at least one more convoy cycle; closes P0 #6 only
|
||||
partially. Rejected.
|
||||
- **Option C (hybrid — rate-limit-only without auth).** Worst of both worlds:
|
||||
anonymous abuse + only IP-based throttling. Rejected per the convoy spec.
|
||||
- **Option D-full (delete all three import routes).** Would break the admin
|
||||
UI immediately and require either deleting `pages/admin/card-import.js` too
|
||||
or restructuring it to call something else. Wider scope than gating; defeats
|
||||
the convoy's "close P0 #6 in one shot" success metric.
|
||||
- **Option D-partial (delete only `import-lorcana.js`, gate mtg+pokemon).**
|
||||
Considered. Slightly smaller attack surface, but introduces a non-uniform
|
||||
pattern (two gated routes + one deleted route) that complicates the
|
||||
reviewer's mental model. Lorcana support is a documented game in
|
||||
`AGENTS.md` § 1; deleting the API forecloses the trivial path to a future
|
||||
Lorcana admin UI. Rejected as marginal; queue `delete-dead-lorcana-import`
|
||||
as a follow-up convoy if Lorcana never gets wired into the admin UI.
|
||||
|
||||
**Operator action**: ratify Option A or counter-propose. Architect proceeds
|
||||
to brief generation assuming Option A; if operator counters before
|
||||
implementer dispatch, the brief is revised in place.
|
||||
|
||||
### Decision 2 — Named-limiter shape in `lib/rate-limit.js` (architect-self-ratifiable)
|
||||
|
||||
**Ratified: Option (c) hybrid — preserve `checkAuthRateLimit(req)`, add four named functions.**
|
||||
|
||||
The verbatim new module shape is specified in
|
||||
`.convoys/add-rate-limiting/brief-1-extend-rate-limit-and-wire-routes.md`
|
||||
§ `lib/rate-limit.js` (modified). Key design choices:
|
||||
|
||||
- `Map<className, Ratelimit>` instance cache (one Redis client, five
|
||||
Ratelimit instances, distinct prefix per class). Each `Ratelimit`
|
||||
construction is a cheap object wrap around the shared Redis client; no
|
||||
separate REST connection per class.
|
||||
- Five exported functions: `checkAuthRateLimit(req)`,
|
||||
`checkSearchRateLimit(req)`, `checkUploadRateLimit(req, userId)`,
|
||||
`checkGenerateRateLimit(req, userId)`, `checkImportRateLimit(req, userId)`.
|
||||
Internal `check(className, identifier)` shared helper.
|
||||
- `extractIpIdentifier(req)` is module-private (renamed from the
|
||||
pre-refactor `extractIdentifier` — that legacy name is gone but the
|
||||
behavior is identical).
|
||||
- `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).
|
||||
- Brief 4's `checkAuthRateLimit(req)` return shape is **byte-identical**
|
||||
post-refactor (`{ allowed, remaining, reset }`) — Brief 4 contract
|
||||
preserved.
|
||||
|
||||
### Decision 3 — Per-class limit values (architect-self-ratifiable)
|
||||
|
||||
**Ratified with tuning evidence:**
|
||||
|
||||
| Class | Limit | Window | Key | Rationale |
|
||||
| --- | --- | --- | --- | --- |
|
||||
| `auth` | 5 | 15 min | IP | Brief 4 unchanged. |
|
||||
| `search` | **60** | 1 min | IP | **Raised from parent's 30.** `components/ShareModal.js::handleSearch` (lines 56-77) fires on every keystroke with no debounce; typing a 17-char email = 16 requests in <5s, which would 429 at 30/1min on a single legitimate user entry. 60/1min covers a realistic burst and still stops a scraper. |
|
||||
| `upload` | 10 | 1 hour | user | Avatar uploads are rare; 10/hour catches accidental loops. User-keyed because a household IP shouldn't punish other users. |
|
||||
| `generate` | **5** | 1 hour | user | **Raised from parent's 3.** `pages/api/user/avatar/generate.js` calls DiceBear (free public API), not OpenAI/Replicate; cost is Vercel blob + DiceBear-side throttling, not per-call $. 5/hour still catches loops without blocking a user trying 3-4 seeds. |
|
||||
| `import` | 5 | 1 hour | user | Admin-only via Decision 1; hits Scryfall/Pokémon-TCG/Lorcana APIs with their own rate limits. 5/hour is plenty for the actual import workflow (one set per click; admin won't import 5 sets per hour in normal operation). |
|
||||
|
||||
If the operator wants different numbers, the change is a single-line edit
|
||||
to `LIMITER_CONFIG` in `lib/rate-limit.js` — call out in PR review.
|
||||
|
||||
### Decision 4 — Identifier extraction shape (architect-self-ratifiable)
|
||||
|
||||
**Ratified: Option (a) — two extractors, `extractUserIdentifier` THROWS on missing userId.**
|
||||
|
||||
Verbatim shape in the brief. Key safety property:
|
||||
`extractUserIdentifier(userId)` THROWS the message
|
||||
`'[rate-limit] extractUserIdentifier called without an authenticated userId. Place the rate-limit gate AFTER the auth check, never before.'`
|
||||
when `userId` is `null`, `undefined`, `''`, or `NaN`. This 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 `.convoys/add-rate-limiting.md` § Known constraints.
|
||||
|
||||
Numeric `0` is intentionally NOT in the throw conditional — if a future
|
||||
schema introduces user ID 0 the limiter still keys correctly as
|
||||
`'user:0'`. There is no current user with ID 0 in the `users` table; the
|
||||
check is defensively forward-compatible.
|
||||
|
||||
### Decision 5 — 429 response shape (architect-self-ratifiable)
|
||||
|
||||
**Ratified: uniform message across all five classes** — `'Too many attempts. Try again later.'`.
|
||||
|
||||
Matches `login.js` + `register.js` verbatim. Per-class variation (e.g.,
|
||||
`'Too many search requests'` vs `'Too many upload requests'`) was
|
||||
considered and rejected: per-class messages 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. Single uniform message =
|
||||
attacker doesn't know what they hit.
|
||||
|
||||
`Retry-After` calculation is also identical:
|
||||
`Math.ceil((reset - Date.now()) / 1000)`. Status code is `429`.
|
||||
|
||||
### Decision 6 — Test coverage in this convoy (architect-self-ratifiable)
|
||||
|
||||
**Ratified: no new vitest or playwright tests.**
|
||||
|
||||
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 first call.
|
||||
|
||||
**Correction to the convoy file's "Known constraints" claim:** the file
|
||||
states *"Brief 4's `lib/rate-limit.js` is unit-tested transitively via
|
||||
the existing vitest suite (none of the 21 tests specifically test
|
||||
rate-limit, but the auth-utils tests run through the same module)."*
|
||||
This is stale. Architect ran `rg 'rate-limit|@upstash' test/` → zero
|
||||
matches. `test/api/auth-utils.test.js` only imports
|
||||
`pages/api/auth-utils.js` + `lib/auth-secret.js` (with a mock for
|
||||
`lib/database.js`); it does NOT load the auth handlers or
|
||||
`lib/rate-limit.js`. The lib refactor is therefore strictly safer than
|
||||
the convoy file implies — there is no transitive test path to break.
|
||||
Decision 6 still holds.
|
||||
|
||||
## Architecture
|
||||
|
||||
### File plan
|
||||
|
||||
| File | Action | Purpose |
|
||||
| --- | --- | --- |
|
||||
| `lib/rate-limit.js` | modified | Refactor from single auth-only limiter into Map-of-named-limiters with five classes. Preserves Brief 4's `checkAuthRateLimit(req)` contract; adds four new named exports. |
|
||||
| `pages/api/users/search.js` | modified | Add `checkSearchRateLimit(req)` gate after the inline JWT verify, before query-length validation. IP-keyed. |
|
||||
| `pages/api/cards/search.js` | modified | Add `checkSearchRateLimit(req)` gate at top of handler, after method check, before the 240-line SQL god-function (which stays byte-identical). IP-keyed, anonymous-by-design. |
|
||||
| `pages/api/user/avatar.js` | modified | Add `checkUploadRateLimit(req, user.userId)` after the existing `getUserFromRequest` check, before the POST/DELETE branching. User-keyed; gate fires before `parseMultipartFormData` body parse. |
|
||||
| `pages/api/user/avatar/generate.js` | modified | Add `checkGenerateRateLimit(req, user.userId)` after the existing `getUserFromRequest` check, before the user-data SQL query. User-keyed; calls DiceBear (free), not a paid AI service. |
|
||||
| `pages/api/cards/import-mtg.js` | modified | Add `getUserFromRequest` + `if (user.role !== 'admin') return 403` + `checkImportRateLimit(req, user.userId)` before the existing `try` block. Closes the publicly-callable anonymous-abuse vector. |
|
||||
| `pages/api/cards/import-pokemon.js` | modified | Same shape as `import-mtg.js`. Live admin tooling per architect investigation. |
|
||||
| `pages/api/cards/import-lorcana.js` | modified | Same shape as `import-mtg.js`. Gated defensively despite zero current frontend callers — future Lorcana admin UI inherits protection. |
|
||||
| `pages/admin/card-import.js` | modified | **Scope expansion for D1.** Add `'Authorization': \`Bearer ${localStorage.getItem('auth_token')}\`` to the import fetch's headers (line 45-47). One-line addition; 309-line god-component otherwise byte-identical. |
|
||||
| `.cursor/rules/api-routes.mdc` | modified | Replace § Rate limiting (lines 104-135) with the new per-class table + verbatim call shape + gate-ordering rules + identifier-extraction documentation. Every other section byte-identical. |
|
||||
|
||||
### API surface
|
||||
|
||||
No new routes. No request/response shape changes on the 7 gated routes
|
||||
(429 is added to the possible status codes for each). The only public
|
||||
surface change is:
|
||||
|
||||
- `/api/cards/import-mtg` POST: now requires `Authorization: Bearer <admin-token>`. Returns 401 (no token), 403 (non-admin), 429 (rate-limited), or the existing 200/400/500 contract.
|
||||
- `/api/cards/import-pokemon` POST: same.
|
||||
- `/api/cards/import-lorcana` POST: same.
|
||||
- `/api/users/search` GET: adds 429 to the possible status codes (existing 401/400/200/500 unchanged).
|
||||
- `/api/cards/search` GET: adds 429 to the possible status codes (existing 200/500 unchanged; no auth either pre- or post-edit).
|
||||
- `/api/user/avatar` POST/DELETE: adds 429 (existing 401/400/200/500 unchanged).
|
||||
- `/api/user/avatar/generate` POST: adds 429 (existing 401/404/200/500 unchanged).
|
||||
|
||||
All 429 responses set `Retry-After: <seconds>` and return
|
||||
`{ "error": "Too many attempts. Try again later." }`.
|
||||
|
||||
### Schema diff
|
||||
|
||||
**No schema changes.** No new Postgres tables, columns, or indexes. The
|
||||
limiter state lives in Upstash Redis (managed); the existing
|
||||
`KV_REST_API_URL` / `KV_REST_API_TOKEN` env vars (auto-provisioned by
|
||||
Vercel's Upstash Marketplace integration) cover all five classes.
|
||||
|
||||
### Test plan
|
||||
|
||||
Per Decision 6, **no new vitest or playwright specs are added in this convoy**.
|
||||
|
||||
Existing test coverage that must still pass post-refactor:
|
||||
|
||||
- `test/lib/auth-secret.test.js` (3 tests) — unrelated to rate-limit.
|
||||
- `test/lib/permission-middleware.test.js` (8 tests) — unrelated.
|
||||
- `test/api/auth-utils.test.js` (5 tests) — unrelated (does NOT import the auth handlers, verified at architect time).
|
||||
- `test/components/Layout.test.js` (5 tests) — unrelated.
|
||||
|
||||
Total: 21/21 must still pass. `npm run test:run` is BLOCKING in CI per
|
||||
the `test:` job in `.github/workflows/ci.yml`.
|
||||
|
||||
Playwright smoke (3/3, runs against Vercel preview):
|
||||
- `home redirects or renders without 5xx` — anonymous GET on `/`; not in scope.
|
||||
- `sign-in page renders` — anonymous GET on `/login`; not in scope.
|
||||
- `public health endpoint responds` — anonymous GET on `/api/health`; not in scope.
|
||||
|
||||
None of the smoke tests exercise any of the 7 gated endpoints, so no
|
||||
smoke regression risk. Smoke run must stay 3/3 green.
|
||||
|
||||
Manual verification (run locally pre-PR, paste output in PR description):
|
||||
documented in the brief's § Manual verification.
|
||||
|
||||
### Risk list
|
||||
|
||||
1. **Brief 4 contract regression on `checkAuthRateLimit(req)`.** If the
|
||||
refactor changes the return shape from `{ allowed, remaining, reset }`,
|
||||
`pages/api/auth/login.js` + `pages/api/auth/register.js` break silently
|
||||
(destructuring undefined). Mitigation: the brief locks the return shape
|
||||
as byte-identical; manual verification exercises the 6th-attempt 429
|
||||
path on `/api/auth/login`.
|
||||
|
||||
2. **Per-class Redis prefix collision.** If two `LIMITER_CONFIG` entries
|
||||
accidentally share a prefix (typo, copy-paste), a search hit eats from
|
||||
the auth budget for the same IP. Mitigation: brief acceptance criterion
|
||||
explicitly checks `rg "tcgvault:" lib/rate-limit.js | sort -u` returns
|
||||
five distinct lines.
|
||||
|
||||
3. **Gate-ordering reversal for user-keyed limiters.** If an implementer
|
||||
places `checkUploadRateLimit(req, user.userId)` BEFORE the
|
||||
`getUserFromRequest` check, every anonymous request throws (via
|
||||
`extractUserIdentifier`'s null guard). Mitigation: the throw IS the
|
||||
defensive signal — it surfaces as a dev-time 500 immediately rather than
|
||||
a silent security regression. The brief's `## Cross-file checks` calls
|
||||
out the order-check explicitly.
|
||||
|
||||
4. **Admin UI broken on first run if `pages/admin/card-import.js` Bearer
|
||||
fix doesn't land.** Adding `getUserFromRequest` to the import APIs
|
||||
without the matching admin-UI fix produces an immediate 401 on the next
|
||||
"Import Cards" click. Mitigation: brief includes both edits as a single
|
||||
atomic change; PR review enforces atomicity (acceptance criterion calls
|
||||
the dependency out explicitly).
|
||||
|
||||
5. **Search 60/1min still too low for rapid typists.** If 60/min ends up
|
||||
too restrictive in production, surface as a `tune-search-rate-limit`
|
||||
follow-up convoy; the fix is a single LIMITER_CONFIG edit (60 → 90 or
|
||||
120). Not a release-blocker — the failure mode is a 429 with a
|
||||
`Retry-After: 60` header, which the front-end can display as
|
||||
"searching too fast, try again in a moment".
|
||||
|
||||
6. **`extractUserIdentifier` throws on edge-case userIds.** Numeric `0` is
|
||||
intentionally accepted (returns `'user:0'`). Empty-string userId throws.
|
||||
If a future auth refactor changes the `userId` type to a UUID string,
|
||||
the empty-string guard still works. If it changes to an opaque `object`,
|
||||
the throw fires (correct — we don't want to key off an object).
|
||||
|
||||
7. **Upstash outage fails-open.** Brief 4's design choice carries through:
|
||||
on `ratelimit.limit(...)` failure, return `{ allowed: true, ... }`. A
|
||||
hard Upstash outage during an attack would defeat the limiter for the
|
||||
duration of the outage. Mitigation: defense-in-depth (Vercel firewall,
|
||||
future fail2ban-style lockout) and Upstash's published SLA. Not
|
||||
addressable inside this convoy.
|
||||
|
||||
8. **Body-stream bypass on avatar.js.** `parseMultipartFormData` consumes
|
||||
the 5MB body via `req.on('data')` before the response is sent, so an
|
||||
attacker can still exhaust the 5MB ceiling per 429. This is explicitly
|
||||
out-of-scope (`harden-multipart-parser` convoy). The brief's gate
|
||||
ordering places the limiter BEFORE the method branches (which call
|
||||
`parseMultipartFormData`), so when that future hardening lands, the
|
||||
gate ordering is already correct.
|
||||
|
||||
### Verbatim new `lib/rate-limit.js` shape
|
||||
|
||||
Specified in full in
|
||||
`.convoys/add-rate-limiting/brief-1-extend-rate-limit-and-wire-routes.md`
|
||||
§ `lib/rate-limit.js` (modified). Implementer has zero design discretion.
|
||||
|
||||
## Decomposition
|
||||
|
||||
| Brief # | Title | Files | Depends on | Estimated PR size |
|
||||
| --- | --- | --- | --- | --- |
|
||||
| 1 | Extend `lib/rate-limit.js` to named per-class limiters + wire into the remaining abusable endpoints + gate the import routes | `lib/rate-limit.js`, `pages/api/users/search.js`, `pages/api/cards/search.js`, `pages/api/user/avatar.js`, `pages/api/user/avatar/generate.js`, `pages/api/cards/import-mtg.js`, `pages/api/cards/import-pokemon.js`, `pages/api/cards/import-lorcana.js`, `pages/admin/card-import.js`, `.cursor/rules/api-routes.mdc` | — | ~180 LOC across 10 files (lib refactor ~65 lines net add; 7 route edits ~6-16 lines each; admin UI +1 line; rules doc ~70 lines + / ~32 lines -) |
|
||||
|
||||
**Brief count: 1.** Splitting into two briefs (one for lib refactor, one
|
||||
for per-route wiring) was considered and rejected:
|
||||
|
||||
- Per-route wiring **depends on** the lib refactor (the new exports
|
||||
don't exist until Brief 1 lands), so parallelism gain via
|
||||
`/multitask` is zero — Brief 2 would have `depends_on: [1]` and run
|
||||
serially anyway.
|
||||
- Single brief presents the reviewer with one coherent change instead
|
||||
of two related-but-fragmented PRs.
|
||||
- Cross-brief commitment overhead (Brief 1 ships stub limiters, Brief 2
|
||||
resolves the wiring) creates exactly the kind of forward-declaration
|
||||
scaffolding the `role-architect` Boot-the-brief check exists to
|
||||
prevent.
|
||||
|
||||
The single brief is well-bounded at ~180 LOC across 10 files (mostly
|
||||
small additions). No single file gets more than ~70 lines of edit; the
|
||||
average file edit is ~12 lines.
|
||||
|
||||
### Slice dependencies (multitask-ready)
|
||||
|
||||
```yaml
|
||||
slice_dependencies:
|
||||
- brief: 1
|
||||
depends_on: []
|
||||
files:
|
||||
- lib/rate-limit.js
|
||||
- pages/api/users/search.js
|
||||
- pages/api/cards/search.js
|
||||
- pages/api/user/avatar.js
|
||||
- pages/api/user/avatar/generate.js
|
||||
- pages/api/cards/import-mtg.js
|
||||
- pages/api/cards/import-pokemon.js
|
||||
- pages/api/cards/import-lorcana.js
|
||||
- pages/admin/card-import.js
|
||||
- .cursor/rules/api-routes.mdc
|
||||
```
|
||||
|
||||
One brief, no `/multitask` fan-out — the conductor dispatches a single
|
||||
implementer.
|
||||
|
|
@ -0,0 +1,751 @@
|
|||
---
|
||||
convoy: add-rate-limiting
|
||||
brief_number: 1
|
||||
depends_on: []
|
||||
files:
|
||||
- lib/rate-limit.js
|
||||
- pages/api/users/search.js
|
||||
- pages/api/cards/search.js
|
||||
- pages/api/user/avatar.js
|
||||
- pages/api/user/avatar/generate.js
|
||||
- pages/api/cards/import-mtg.js
|
||||
- pages/api/cards/import-pokemon.js
|
||||
- pages/api/cards/import-lorcana.js
|
||||
- pages/admin/card-import.js
|
||||
- .cursor/rules/api-routes.mdc
|
||||
---
|
||||
|
||||
# Brief 1: Extend `lib/rate-limit.js` to named per-class limiters + wire into the remaining abusable endpoints + gate the import routes
|
||||
|
||||
## Goal (1 sentence)
|
||||
|
||||
Refactor `lib/rate-limit.js` from a single auth-only limiter into a Map-of-named-limiters shape (preserving the `checkAuthRateLimit(req)` contract that `login.js` + `register.js` depend on per Brief 4), add four new named exports — `checkSearchRateLimit(req)`, `checkUploadRateLimit(req, userId)`, `checkGenerateRateLimit(req, userId)`, `checkImportRateLimit(req, userId)` — wire each into the appropriate handler at the documented gate-ordering (auth before rate-limit for user-keyed classes), add `getUserFromRequest` + admin-role check + import rate-limit to the three currently-anonymous `pages/api/cards/import-*.js` routes, fix `pages/admin/card-import.js` to send the Bearer token the newly-gated import routes require, and extend `.cursor/rules/api-routes.mdc` § Rate limiting with the per-class pattern + a per-class limit table.
|
||||
|
||||
## Files in scope (do not edit anything else)
|
||||
|
||||
The 10 files listed in `files:` above (all modified, no new files, no deletions).
|
||||
|
||||
**Files explicitly out of scope** (do not touch even if it seems related):
|
||||
|
||||
- `pages/api/auth/login.js`, `pages/api/auth/register.js` — already wired by `fix-auth-bypass` Brief 4. **Verify post-edit that they still work** (call `checkAuthRateLimit(req)` against the refactored module), but do NOT modify them.
|
||||
- `lib/permission-middleware.js`, `lib/auth-secret.js`, `pages/api/auth-utils.js` — auth surface is untouched by this convoy.
|
||||
- `package.json`, `package-lock.json` — `@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0` are already installed (Brief 4). No new dependencies. No version bumps.
|
||||
- `AGENTS.md` — Gotcha #12 documents the env-var requirement; the **doc-writer pass at convoy close** will update the gotcha to reflect the new per-class limits. Do NOT preempt that edit here.
|
||||
- `.github/workflows/ci.yml` — no new CI gate is added. The `forbidden-endpoints` + `forbidden-cors-headers` jobs already defend the API surface; per-class rate-limit wiring isn't grep-checkable.
|
||||
- `test/**` — no new per-route handler tests in this convoy (Decision 6 below). Vitest 21/21 must still pass with no spec changes.
|
||||
- `tests/smoke/**`, `tests/visual/**` — smoke + visual suites don't exercise any of these endpoints; do NOT modify.
|
||||
- `pages/api/cards/search.js`'s SQL — the file has a known god-function shape with 7+ conditional SQL branches (`SELECT * FROM cards WHERE …` repeated). That's `god-function-split` / `refactor-cards-search-sql` scope, NOT here. Do NOT touch any of the SQL branches; only add the rate-limit gate at the top.
|
||||
- `pages/api/user/avatar.js`'s `parseMultipartFormData` body-streaming behavior — the 5MB multipart body is consumed before any rate-limit gate could short-circuit, meaning an attacker can still exhaust the 5MB body even on a 429 path. That's `harden-multipart-parser` scope (queued as a follow-up); the gate-ordering in this brief is purely defensive (rate-limit BEFORE the method branches so the gate fires before the body parse).
|
||||
- `scripts/import-*.js` — standalone scripts independent of the API routes; do NOT touch.
|
||||
- Any other `pages/api/**/*.js` file. The convoy scope is the 7 surfaces listed in `.convoys/add-rate-limiting.md` § Scope.
|
||||
|
||||
## Conventions to follow
|
||||
|
||||
### Decisions from the convoy file (cite when implementing)
|
||||
|
||||
- **D1 (operator-ratified):** Option A — gate all three `pages/api/cards/import-*.js` routes in this convoy with `getUserFromRequest` + admin-role check + per-user rate-limit. ALSO update `pages/admin/card-import.js` to send the `Authorization: Bearer ${localStorage.getItem('auth_token')}` header on the import fetch (necessary scope expansion — without it, the gated APIs immediately break the admin UI). Lorcana is gated defensively even though no current frontend caller exists; future cleanup convoy can delete if it stays unused.
|
||||
- **D2 (architect-self-ratified):** Hybrid named-limiter shape — preserve `checkAuthRateLimit(req)` (Brief 4 contract, used by login + register), add four new named functions (`checkSearchRateLimit`, `checkUploadRateLimit`, `checkGenerateRateLimit`, `checkImportRateLimit`). Internal `Map<className, Ratelimit>` cache, distinct Redis prefix per class.
|
||||
- **D3 (architect-self-ratified):** Per-class limits — `auth` 5/15min IP (unchanged), `search` 60/1min IP, `upload` 10/1hour user, `generate` 5/1hour user, `import` 5/1hour user. Search raised from parent's 30 because `components/ShareModal.js`'s `handleSearch` fires on every keystroke (no debounce); typing a 17-char email = 17 requests in <5s, which would 429 at 30/1min. Generate raised from parent's 3 because `pages/api/user/avatar/generate.js` calls DiceBear (free public API), not OpenAI/Replicate; cost is just Vercel blob storage + DiceBear-side throttling.
|
||||
- **D4 (architect-self-ratified):** Two-extractor shape — `extractIpIdentifier(req)` (existing) + `extractUserIdentifier(userId)` (new). `extractUserIdentifier` **throws** when `userId` is null/undefined/'' (defensive — if a future handler accidentally calls a user-keyed limiter before the auth check, the throw surfaces the misordering immediately rather than silently falling back to IP and quietly converting a per-user limit into a per-IP limit, which would lock out other household members for one user's behavior). Documented in the verbatim shape below.
|
||||
- **D5 (architect-self-ratified):** Uniform 429 message — `'Too many attempts. Try again later.'` matches `login.js` + `register.js` verbatim. Per-class variation would fingerprint which routes have which limits to an attacker.
|
||||
- **D6 (architect-self-ratified):** No new per-route handler tests in this convoy. Deferred to queued `fill-vitest-handler-coverage` (same reasoning as `cors-tighten` Decision D4). Vitest 21/21 MUST still pass after the lib refactor — verified at architect time that no current vitest spec transitively imports `lib/rate-limit.js` (only `login.js` + `register.js` import it, and neither is covered by vitest; the convoy file's claim that "auth-utils tests transitively load this module" is stale).
|
||||
|
||||
### Repo conventions (cite + match)
|
||||
|
||||
- **`.cursor/rules/no-go-zones.mdc`.** None of the 10 source files are listed under no-go zones. The "Card-import jobs" entry warns *"Don't run them ad-hoc against prod data; use staging"* — this brief gates them with admin-role enforcement which **directly answers** that no-go-zones warning (only admins can trigger imports; non-admins get 403).
|
||||
- **`.cursor/rules/api-routes.mdc` § Rate limiting.** The existing pattern documents the `auth` class (login/register). This brief extends it with the four new classes; the verbatim updated content is in the Acceptance criteria § for `.cursor/rules/api-routes.mdc` below. Keep the existing § Authentication, § Request validation, § Method gating, § Error handling, § Database access, § Response shape, § Activity logging, § Dev/test endpoints, and § CORS subsections byte-identical — only § Rate limiting changes.
|
||||
- **`.cursor/rules/auth-and-permissions.mdc`.** Admin-role check uses `if (user.role !== 'admin')` directly (per the rule's "Admin-only" pattern: *"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 `withAdmin()` is its own convoy — for this brief, inline the check.
|
||||
- **Brief 4 precedent shape (`.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md`).** The verbatim 429 response shape comes from there:
|
||||
```js
|
||||
const { allowed, reset } = await checkXxxRateLimit(req[, userId]);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
```
|
||||
Apply this shape at each call site. **Do not deviate** — same error message, same `Retry-After` calculation, same status code.
|
||||
- **`@upstash/ratelimit` per-class prefix isolation.** Each class gets a distinct Redis key prefix (`tcgvault:auth`, `tcgvault:search`, `tcgvault:upload`, `tcgvault:generate`, `tcgvault:import`). Without distinct prefixes, hits on one class would consume the budget of another (e.g., a search hit would eat the auth budget for the same IP). Verified against `@upstash/ratelimit@2.0.8`'s `prefix:` option which scopes all keys with the given string.
|
||||
- **Lazy `init()` + fail-closed-in-prod / warn-and-noop-in-dev.** Both behaviors carry through unchanged from Brief 4. New limiters inherit them via the shared `init()` function. Do NOT reintroduce module-top-level `new Redis(...)` — it would throw at import time in any environment without `KV_REST_API_URL` / `KV_REST_API_TOKEN`, breaking local dev, vitest, and Vercel build-time bundling.
|
||||
- **`@vercel/postgres` tagged-templates only.** None of the per-route edits touch SQL. (`cards/search.js` is excluded from SQL refactoring per § Files explicitly out of scope.)
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
### `lib/rate-limit.js` (modified)
|
||||
|
||||
Replace the current 70-line module with the verbatim shape below. The diff is mostly net-additive (~70 lines added, ~5 lines reshaped); the existing `init()`, `extractIdentifier()`, and `checkAuthRateLimit()` functions are conceptually preserved but restructured to share infrastructure across all five classes.
|
||||
|
||||
**Verbatim new module shape:**
|
||||
|
||||
```js
|
||||
import { Ratelimit } from '@upstash/ratelimit';
|
||||
import { Redis } from '@upstash/redis';
|
||||
|
||||
// Per-class limiter configuration. Distinct Redis prefix per class is
|
||||
// REQUIRED — without it, a search-class hit would consume the auth-class
|
||||
// budget for the same identifier. `slidingWindow` chosen across all
|
||||
// classes to match Brief 4's existing algorithm; switching to
|
||||
// `tokenBucket` per-class would be its own convoy.
|
||||
const LIMITER_CONFIG = {
|
||||
auth: { limit: 5, window: '15 m', prefix: 'tcgvault:auth' },
|
||||
search: { limit: 60, window: '1 m', prefix: 'tcgvault:search' },
|
||||
upload: { limit: 10, window: '1 h', prefix: 'tcgvault:upload' },
|
||||
generate: { limit: 5, window: '1 h', prefix: 'tcgvault:generate' },
|
||||
import: { limit: 5, window: '1 h', prefix: 'tcgvault:import' },
|
||||
};
|
||||
|
||||
// Lazy singleton. Module-load init would throw in environments without
|
||||
// Upstash env vars (local dev pre-onboarding, tests that transitively
|
||||
// import the auth handlers, Vercel build-time bundling). Defer
|
||||
// construction until the first request actually arrives.
|
||||
let cached = null;
|
||||
|
||||
function init() {
|
||||
// Env-var names match Vercel's Upstash Marketplace integration, which
|
||||
// auto-provisions KV_REST_API_URL and KV_REST_API_TOKEN. See
|
||||
// https://upstash.com/docs/redis/howto/vercelintegration. Single-source-
|
||||
// of-truth — do NOT alias to UPSTASH_REDIS_REST_*.
|
||||
const url = process.env.KV_REST_API_URL;
|
||||
const token = process.env.KV_REST_API_TOKEN;
|
||||
|
||||
if (url && token) {
|
||||
const redis = new Redis({ url, token });
|
||||
const instances = new Map();
|
||||
for (const [name, cfg] of Object.entries(LIMITER_CONFIG)) {
|
||||
instances.set(
|
||||
name,
|
||||
new Ratelimit({
|
||||
redis,
|
||||
limiter: Ratelimit.slidingWindow(cfg.limit, cfg.window),
|
||||
prefix: cfg.prefix,
|
||||
})
|
||||
);
|
||||
}
|
||||
return { mode: 'live', instances };
|
||||
}
|
||||
|
||||
if (process.env.NODE_ENV === 'production') {
|
||||
// Fail-closed in production. A single failed login is a better outcome
|
||||
// than silently disabling brute-force protection on the live site.
|
||||
throw new Error(
|
||||
'[rate-limit] Upstash not configured. Set KV_REST_API_URL and KV_REST_API_TOKEN in the deployment environment (auto-provisioned by the Vercel Upstash Marketplace integration) before serving auth traffic.'
|
||||
);
|
||||
}
|
||||
|
||||
console.warn(
|
||||
'[rate-limit] KV_REST_API_URL / KV_REST_API_TOKEN not set — rate limiting disabled (dev/test only)'
|
||||
);
|
||||
return { mode: 'noop' };
|
||||
}
|
||||
|
||||
function extractIpIdentifier(req) {
|
||||
const xff = req.headers?.['x-forwarded-for'];
|
||||
const firstHop = Array.isArray(xff) ? xff[0] : xff?.split(',')[0]?.trim();
|
||||
return firstHop || req.socket?.remoteAddress || 'anonymous';
|
||||
}
|
||||
|
||||
// THROWS on missing userId. Per-user limiters MUST sit AFTER the auth
|
||||
// check in the handler body — silently falling back to IP here would
|
||||
// convert a per-user limit into a per-IP limit, locking out other
|
||||
// household members for one user's behavior. The throw surfaces the
|
||||
// misordering immediately during development rather than at first
|
||||
// production incident.
|
||||
function extractUserIdentifier(userId) {
|
||||
if (
|
||||
userId === null ||
|
||||
userId === undefined ||
|
||||
userId === '' ||
|
||||
(typeof userId === 'number' && Number.isNaN(userId))
|
||||
) {
|
||||
throw new Error(
|
||||
'[rate-limit] extractUserIdentifier called without an authenticated userId. Place the rate-limit gate AFTER the auth check, never before.'
|
||||
);
|
||||
}
|
||||
return `user:${userId}`;
|
||||
}
|
||||
|
||||
async function check(className, identifier) {
|
||||
if (!cached) {
|
||||
cached = init();
|
||||
}
|
||||
|
||||
if (cached.mode === 'noop') {
|
||||
return { allowed: true, remaining: Infinity, reset: 0 };
|
||||
}
|
||||
|
||||
const limiter = cached.instances.get(className);
|
||||
if (!limiter) {
|
||||
throw new Error(`[rate-limit] Unknown limiter class: ${className}`);
|
||||
}
|
||||
|
||||
try {
|
||||
const { success, remaining, reset } = await limiter.limit(identifier);
|
||||
return { allowed: success, remaining, reset };
|
||||
} catch (err) {
|
||||
// Fail-open on Upstash outage. A hard outage at the rate-limit backend
|
||||
// should not lock the entire user base out. Brute-force protection
|
||||
// lives behind defense-in-depth (Vercel firewall, etc.).
|
||||
console.error('[rate-limit]', err);
|
||||
return { allowed: true, remaining: Infinity, reset: 0 };
|
||||
}
|
||||
}
|
||||
|
||||
export async function checkAuthRateLimit(req) {
|
||||
return check('auth', extractIpIdentifier(req));
|
||||
}
|
||||
|
||||
export async function checkSearchRateLimit(req) {
|
||||
return check('search', extractIpIdentifier(req));
|
||||
}
|
||||
|
||||
export async function checkUploadRateLimit(req, userId) {
|
||||
return check('upload', extractUserIdentifier(userId));
|
||||
}
|
||||
|
||||
export async function checkGenerateRateLimit(req, userId) {
|
||||
return check('generate', extractUserIdentifier(userId));
|
||||
}
|
||||
|
||||
export async function checkImportRateLimit(req, userId) {
|
||||
return check('import', extractUserIdentifier(userId));
|
||||
}
|
||||
```
|
||||
|
||||
Acceptance:
|
||||
|
||||
- [ ] File ends up as the verbatim shape above (whitespace and comments preserved). 2-space indent. ESM. No default export.
|
||||
- [ ] **`checkAuthRateLimit(req)` return shape is byte-identical to Brief 4's** — `{ allowed: boolean, remaining: number, reset: number }`. `login.js` + `register.js` MUST continue to work without any change to their import or call shape.
|
||||
- [ ] No top-level `await`. No module-load `new Redis(...)`. The `cached = null` declaration is the only top-level side effect.
|
||||
- [ ] `LIMITER_CONFIG` keys are exactly `auth`, `search`, `upload`, `generate`, `import` — five entries, no more, no less.
|
||||
- [ ] Each `LIMITER_CONFIG[*].prefix` is unique and follows the `tcgvault:<class>` pattern.
|
||||
- [ ] `extractUserIdentifier(userId)` THROWS the documented error message on `null`, `undefined`, empty string, or `NaN`. (Numeric `0` is technically valid — there's no user with ID 0 in the schema, but the check is defensive against future ID types; the conditional explicitly does NOT throw on `0` because `0 === null` is false and `0 === undefined` is false. This is intentional — if a future change introduces user ID 0 the limiter still keys correctly.)
|
||||
- [ ] `check('unknown-class', ...)` throws `[rate-limit] Unknown limiter class: unknown-class` (defensive; should never fire in shipped code).
|
||||
- [ ] The five exported `check*RateLimit` functions are the ONLY exports. No legacy `extractIdentifier` re-export — it's been renamed to `extractIpIdentifier` and is module-private.
|
||||
|
||||
### `pages/api/users/search.js` (modified)
|
||||
|
||||
Add a new import and a new rate-limit gate after the JWT verify success, before the query-length validation. The route uses inline `jwt.verify` (not `getUserFromRequest`) but that doesn't matter — the search class is **IP-keyed**, not user-keyed, so the gate doesn't need the user id.
|
||||
|
||||
**Verbatim post-edit shape:**
|
||||
|
||||
```js
|
||||
import { sql } from '@vercel/postgres';
|
||||
import jwt from 'jsonwebtoken';
|
||||
import { JWT_SECRET } from '../../../lib/auth-secret.js';
|
||||
import { checkSearchRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'GET') {
|
||||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
// Verify authentication
|
||||
const authHeader = req.headers.authorization;
|
||||
if (!authHeader || !authHeader.startsWith('Bearer ')) {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
|
||||
const token = authHeader.substring(7);
|
||||
try {
|
||||
jwt.verify(token, JWT_SECRET);
|
||||
} catch (error) {
|
||||
return res.status(401).json({ error: 'Invalid token' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkSearchRateLimit(req);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
const { q: query } = req.query;
|
||||
|
||||
if (!query || query.length < 2) {
|
||||
return res.status(400).json({ error: 'Query must be at least 2 characters' });
|
||||
}
|
||||
|
||||
try {
|
||||
// Search users by email (partial match)
|
||||
const result = await sql`
|
||||
SELECT id, email, role, created_at
|
||||
FROM users
|
||||
WHERE email ILIKE ${`%${query}%`}
|
||||
ORDER BY email
|
||||
LIMIT 10
|
||||
`;
|
||||
|
||||
res.status(200).json({
|
||||
users: result.rows
|
||||
});
|
||||
|
||||
} catch (error) {
|
||||
console.error('User search error:', error);
|
||||
res.status(500).json({ error: 'Internal server error' });
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Acceptance:
|
||||
|
||||
- [ ] One new import line: `import { checkSearchRateLimit } from '../../../lib/rate-limit.js';` (relative path matches the existing `../../../lib/auth-secret.js` precedent on line 3).
|
||||
- [ ] Gate sits between the JWT-verify try/catch (lines 17-21) and the query-length validation (line 25). NOT inside the JWT try block.
|
||||
- [ ] Net diff: +1 import, +5 lines (the gate block), 0 deletions, 0 reorderings.
|
||||
|
||||
### `pages/api/cards/search.js` (modified)
|
||||
|
||||
The route is anonymous-by-design (cards are a public catalogue). Gate at the very top of the handler, after the method check, before the existing `try` block. **IP-keyed.**
|
||||
|
||||
**Verbatim post-edit shape (top of file only):**
|
||||
|
||||
```js
|
||||
import { sql } from '@vercel/postgres';
|
||||
import { checkSearchRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'GET') {
|
||||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkSearchRateLimit(req);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
try {
|
||||
const {
|
||||
query = '',
|
||||
// ... rest of the file unchanged ...
|
||||
```
|
||||
|
||||
Acceptance:
|
||||
|
||||
- [ ] One new import line. Relative path `'../../../lib/rate-limit.js'`.
|
||||
- [ ] Gate sits between the method check (lines 4-6) and the `try` block (current line 8).
|
||||
- [ ] **The 240-line SQL god-function inside the try block is BYTE-IDENTICAL post-edit.** Do NOT touch any of the 7 conditional SQL branches, the filter object, the response shape, or the closing `catch`. The only diff is +1 import and +5 lines for the gate block.
|
||||
- [ ] Do NOT add `getUserFromRequest` to this route. It's anonymous-by-design per the convoy file's "Known constraints" § *"Card-search is anonymous-by-design — do NOT add a `getUserFromRequest` check. The IP-keyed limit is the correct defense (search is a public catalogue feature)."*
|
||||
|
||||
### `pages/api/user/avatar.js` (modified)
|
||||
|
||||
The handler has a structure with NO top-level method gate; method-branches inside the outer try block. Auth check sits inside the try (lines 16-19). Gate goes AFTER the auth check, BEFORE the method-branching (`if (req.method === 'POST')` at line 21), so both the POST upload AND the DELETE branches inherit the limit. **User-keyed**, passing `user.userId`.
|
||||
|
||||
**Verbatim post-edit shape (auth + gate region only):**
|
||||
|
||||
```js
|
||||
import { put, del } from '@vercel/blob';
|
||||
import { sql } from '@vercel/postgres';
|
||||
import { getUserFromRequest } from '../../../lib/permission-middleware';
|
||||
import { checkUploadRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export const config = {
|
||||
api: {
|
||||
bodyParser: {
|
||||
sizeLimit: '5mb',
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
export default async function handler(req, res) {
|
||||
try {
|
||||
// Get authenticated user
|
||||
const user = await getUserFromRequest(req);
|
||||
if (!user) {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkUploadRateLimit(req, user.userId);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
if (req.method === 'POST') {
|
||||
// Handle avatar upload
|
||||
// ... rest of the file unchanged ...
|
||||
```
|
||||
|
||||
Acceptance:
|
||||
|
||||
- [ ] One new import line: `import { checkUploadRateLimit } from '../../../lib/rate-limit.js';`.
|
||||
- [ ] Gate sits between the `if (!user)` 401 (line 17-19) and the `if (req.method === 'POST')` branch (line 21).
|
||||
- [ ] Gate fires BEFORE `parseMultipartFormData(req)` runs. The body-streaming bypass concern (5MB consumed before the gate) is acknowledged out-of-scope (see § Files explicitly out of scope) — but the gate ordering itself MUST be correct so that future hardening of the body parser doesn't need to also reorder the gate.
|
||||
- [ ] Net diff: +1 import, +5 lines, 0 deletions. The POST branch, DELETE branch, helper functions (`parseMultipartFormData`, `deleteOldAvatar`), and the `config` export are byte-identical.
|
||||
|
||||
### `pages/api/user/avatar/generate.js` (modified)
|
||||
|
||||
The handler HAS a top-level method gate (`if (req.method !== 'POST')` at line 6). Auth check sits inside the try block (lines 12-15). Gate goes AFTER the auth check, BEFORE the SQL query that fetches user data (line 18). **User-keyed**, passing `user.userId`.
|
||||
|
||||
**Verbatim post-edit shape (top of handler only):**
|
||||
|
||||
```js
|
||||
import { put } from '@vercel/blob';
|
||||
import { sql } from '@vercel/postgres';
|
||||
import { getUserFromRequest } from '../../../../lib/permission-middleware';
|
||||
import { checkGenerateRateLimit } from '../../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'POST') {
|
||||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
try {
|
||||
// Get authenticated user
|
||||
const user = await getUserFromRequest(req);
|
||||
if (!user) {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkGenerateRateLimit(req, user.userId);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
// Get user information for avatar generation
|
||||
const userResult = await sql`
|
||||
SELECT email, first_name, last_name, username FROM users WHERE id = ${user.userId}
|
||||
`;
|
||||
// ... rest of the file unchanged ...
|
||||
```
|
||||
|
||||
Acceptance:
|
||||
|
||||
- [ ] One new import line: `import { checkGenerateRateLimit } from '../../../../lib/rate-limit.js';` (note FOUR `../` levels — this file is at `pages/api/user/avatar/generate.js`).
|
||||
- [ ] Gate sits between the `if (!user)` 401 (lines 13-15) and the user-data SQL query (current line 18).
|
||||
- [ ] Net diff: +1 import, +5 lines, 0 deletions.
|
||||
|
||||
### `pages/api/cards/import-mtg.js` (modified)
|
||||
|
||||
Currently has NO auth, NO rate-limit. Add three gates in order: method check (already present), auth check (NEW), admin-role check (NEW), import rate-limit (NEW). **User-keyed**, passing `user.userId`.
|
||||
|
||||
**Verbatim post-edit shape (top of handler only):**
|
||||
|
||||
```js
|
||||
import { sql } from '@vercel/postgres';
|
||||
import { getUserFromRequest } from '../../../lib/permission-middleware';
|
||||
import { checkImportRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'POST') {
|
||||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
const user = await getUserFromRequest(req);
|
||||
if (!user) {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
if (user.role !== 'admin') {
|
||||
return res.status(403).json({ error: 'Admin access required' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkImportRateLimit(req, user.userId);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
try {
|
||||
const { setCode } = req.body;
|
||||
// ... rest of the file unchanged ...
|
||||
```
|
||||
|
||||
Acceptance:
|
||||
|
||||
- [ ] Two new import lines (one for `getUserFromRequest`, one for `checkImportRateLimit`). Relative paths `'../../../lib/permission-middleware'` and `'../../../lib/rate-limit.js'` — verified at architect time against the directory depth.
|
||||
- [ ] All three gates sit BEFORE the existing `try` block (current line 8). Order: method → auth → admin → rate-limit.
|
||||
- [ ] Net diff: +2 imports, +14 lines, 0 deletions. The Scryfall fetch + INSERT loop + response shape are byte-identical.
|
||||
|
||||
### `pages/api/cards/import-pokemon.js` (modified)
|
||||
|
||||
Same shape as `import-mtg.js` — three new gates added before the existing `try` block (current line 50). The `delay` + `fetchWithRetry` helpers above the handler stay unchanged.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- [ ] Two new import lines, same paths as `import-mtg.js`.
|
||||
- [ ] All three gates sit BEFORE the `try` block (current line 50), AFTER the method check (current lines 46-48).
|
||||
- [ ] Net diff: +2 imports, +14 lines, 0 deletions. The `fetchWithRetry` + `delay` helpers + Pokemon-TCG fetch + INSERT loop + response shape are byte-identical.
|
||||
|
||||
### `pages/api/cards/import-lorcana.js` (modified)
|
||||
|
||||
Same shape as `import-mtg.js` — three new gates added before the existing `try` block (current line 50). Despite having NO frontend caller today (architect-verified: `rg 'import-lorcana' pages/ components/` returns zero matches in source code), gate defensively so a future Lorcana admin UI addition inherits the protection automatically. The `delay` + `fetchWithRetry` helpers above the handler stay unchanged.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- [ ] Two new import lines, same paths as `import-mtg.js`.
|
||||
- [ ] All three gates sit BEFORE the `try` block (current line 50), AFTER the method check (current lines 46-48).
|
||||
- [ ] Net diff: +2 imports, +14 lines, 0 deletions.
|
||||
|
||||
### `pages/admin/card-import.js` (modified — scope expansion for D1)
|
||||
|
||||
The admin UI currently calls `fetch(endpoint, { method: 'POST', headers: { 'Content-Type': 'application/json' } })` with NO Authorization header (line 43-49). Adding `getUserFromRequest` to the import APIs would 401 the admin UI on first run. Add the Bearer token to the fetch call. **This is the only edit to this file** — do NOT refactor the 309-line god-component, do NOT add Lorcana to the `<select>`, do NOT touch the `popularSets` or any other UI logic.
|
||||
|
||||
**Verbatim post-edit shape (fetch call only):**
|
||||
|
||||
```js
|
||||
const response = await fetch(endpoint, {
|
||||
method: 'POST',
|
||||
headers: {
|
||||
'Content-Type': 'application/json',
|
||||
'Authorization': `Bearer ${localStorage.getItem('auth_token')}`,
|
||||
},
|
||||
body: JSON.stringify({ setCode: setCode.trim() }),
|
||||
});
|
||||
```
|
||||
|
||||
Acceptance:
|
||||
|
||||
- [ ] Net diff: +1 line (the `Authorization` header entry inside the `headers:` object on line 45-47). 0 deletions.
|
||||
- [ ] The `'auth_token'` localStorage key matches every other authenticated fetch in the codebase (`components/ShareModal.js` line 44 + 67, etc.). Do NOT use a different key.
|
||||
- [ ] The 309-line component otherwise stays byte-identical. No refactor of the `<AdminProtected>` wrapper, the `dynamic(... { ssr: false })` export, the `useState` block, the popular-sets grid, or the result-display logic.
|
||||
|
||||
### `.cursor/rules/api-routes.mdc` (modified)
|
||||
|
||||
Replace the existing § "Rate limiting" subsection (lines 104-135 in the current file). Keep every other section byte-identical. The updated subsection:
|
||||
|
||||
````markdown
|
||||
## Rate limiting
|
||||
|
||||
`lib/rate-limit.js` exposes five named limiters, one per route class. Each named export takes `req` (and `userId` for user-keyed classes) and returns `{ allowed, remaining, reset }`.
|
||||
|
||||
| Class | Limit | Window | Key | Used by | Helper |
|
||||
| --- | --- | --- | --- | --- | --- |
|
||||
| `auth` | 5 | 15 min | IP | `/api/auth/login`, `/api/auth/register` | `checkAuthRateLimit(req)` |
|
||||
| `search` | 60 | 1 min | IP | `/api/users/search`, `/api/cards/search` | `checkSearchRateLimit(req)` |
|
||||
| `upload` | 10 | 1 hour | user | `/api/user/avatar` | `checkUploadRateLimit(req, userId)` |
|
||||
| `generate` | 5 | 1 hour | user | `/api/user/avatar/generate` | `checkGenerateRateLimit(req, userId)` |
|
||||
| `import` | 5 | 1 hour | user | `/api/cards/import-mtg`, `/api/cards/import-pokemon`, `/api/cards/import-lorcana` | `checkImportRateLimit(req, userId)` |
|
||||
|
||||
**Verbatim call shape** (identical across all five classes — only the helper name and the optional `userId` argument differ):
|
||||
|
||||
```js
|
||||
import { checkSearchRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'GET') {
|
||||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
// For user-keyed classes, auth check goes HERE first; see "Gate ordering" below.
|
||||
|
||||
const { allowed, reset } = await checkSearchRateLimit(req);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
try {
|
||||
// ... handler body ...
|
||||
} catch (err) {
|
||||
// ...
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Gate ordering rules:**
|
||||
|
||||
1. **Method check first.** Reject the wrong verb with 405 before doing any limiter work.
|
||||
2. **Auth check before any user-keyed limiter.** `extractUserIdentifier(userId)` THROWS when `userId` is null/undefined/empty (defensive). For `upload`, `generate`, and `import`, the handler MUST call `getUserFromRequest(req)` (or equivalent JWT verification) and confirm a non-null user BEFORE calling the limiter. Wrong order = anonymous user bypasses (the THROW surfaces immediately during dev; do not catch and silently fall back to IP).
|
||||
3. **For IP-keyed limiters (`auth`, `search`), gate placement is flexible** — either at the top of the handler (after the method check) or after a separate auth check that the route happens to also have (e.g. `users/search` JWT-verifies before rate-limiting, both are correct). The limiter only needs `req` for IP extraction.
|
||||
4. **Admin-role check, if applicable, goes between auth and rate-limit.** Used by all three `/api/cards/import-*` routes: `if (user.role !== 'admin') return res.status(403).json({ error: 'Admin access required' })` sits between the `if (!user)` 401 and the import rate-limit call.
|
||||
|
||||
**Identifier extraction:**
|
||||
|
||||
- `extractIpIdentifier(req)` (module-private) — first hop in `x-forwarded-for` (Vercel's edge), falling back to `req.socket.remoteAddress`, falling back to the literal `'anonymous'`. Do NOT key off `req.body.email` (rotates) or `req.headers.authorization` (unauthenticated endpoints don't have one).
|
||||
- `extractUserIdentifier(userId)` (module-private) — formats as `user:${userId}`. Throws on null/undefined/empty/NaN to surface gate-ordering bugs at dev time rather than silently falling back to IP and creating a per-IP-not-per-user limit.
|
||||
|
||||
**Env vars (unchanged from Brief 4):** `KV_REST_API_URL` + `KV_REST_API_TOKEN` (auto-provisioned by Vercel's Upstash Marketplace integration). In prod, missing either var is a **fail-closed throw** on the first call. In dev / test, the module warn-and-no-ops so local work isn't blocked. See `AGENTS.md` Gotcha #12 for the full env-var contract.
|
||||
|
||||
**429 response shape is uniform across all five classes.** Same error message (`'Too many attempts. Try again later.'`) and same `Retry-After` header calculation. Per-class variation would fingerprint the limits to an attacker.
|
||||
|
||||
**Fail-open on Upstash outage.** A network failure inside `ratelimit.limit(...)` returns `{ allowed: true, remaining: Infinity, reset: 0 }` with a single `console.error('[rate-limit]', err)`. Reasoning: a hard Upstash outage should not lock the entire user base out of every gated route. Brute-force / abuse protection lives behind defense-in-depth (Vercel firewall, future fail2ban-style lockout).
|
||||
````
|
||||
|
||||
Acceptance:
|
||||
|
||||
- [ ] Replace lines 104-135 (the existing § "Rate limiting" subsection — starts with `## Rate limiting` and ends right before `## Dev/test endpoints (removed)`). Use the verbatim content above.
|
||||
- [ ] Every OTHER section in the file stays byte-identical. No edits to § Authentication & Authorization, § Request validation, § Method gating, § Error handling, § Database access, § Response shape, § Activity logging, § Dev/test endpoints, or § CORS.
|
||||
- [ ] The Markdown table renders cleanly (5 columns: Class | Limit | Window | Key | Used by | Helper — 6 columns actually, count the pipes; ensure alignment).
|
||||
- [ ] No mention of the now-stale `Sweeping the rest of the API ... is the queued add-rate-limiting convoy` line — that sentence in the current rule gets replaced by the full new content.
|
||||
|
||||
### Cross-file checks
|
||||
|
||||
- [ ] **`npm run lint` exit code unchanged.** The current baseline is `✖ 128 problems (81 errors, 47 warnings)` (per `bump-next-js` Decision D + `fix-lint-baseline` tracking). Each per-route edit is an import + a small gate block — no new `react-hooks/*` paths, no new unused vars, no new `no-img-element` triggers. If the count grows, investigate before commit.
|
||||
- [ ] **`npm run test:run` (vitest) passes 21/21.** No test file is touched in this convoy. The lib refactor preserves `checkAuthRateLimit(req)`'s return shape so any indirect dependency is irrelevant; architect verified at brief time that no current vitest spec actually imports `lib/rate-limit.js` (the convoy file's stale claim about transitive loading is corrected in Decision 6).
|
||||
- [ ] **`npm run build` exit 0.** Turbopack compile time should be unchanged. The 10 modified files still compile to the same shape.
|
||||
- [ ] **`npm run test:smoke` against the Vercel preview passes 3/3.** None of the 3 smoke tests (`'home redirects or renders without 5xx'`, `'sign-in page renders'`, `'public health endpoint responds'`) hit any of the 7 gated endpoints, so no smoke regression. Verify in CI on PR push.
|
||||
- [ ] **Repo-wide grep clean.** After the sweep:
|
||||
```bash
|
||||
rg "checkAuthRateLimit" pages/api/
|
||||
```
|
||||
Expected: 2 matches (`login.js` + `register.js`) — same as before this convoy.
|
||||
|
||||
```bash
|
||||
rg "checkSearchRateLimit|checkUploadRateLimit|checkGenerateRateLimit|checkImportRateLimit" pages/api/
|
||||
```
|
||||
Expected: 6 matches total (search-2, upload-1, generate-1, import-3 — matching the 7 surfaces; users/search counts as search-1, cards/search counts as search-2).
|
||||
|
||||
- [ ] **Per-class prefix uniqueness check.**
|
||||
```bash
|
||||
rg "tcgvault:" lib/rate-limit.js | sort -u
|
||||
```
|
||||
Expected: 5 distinct lines, one per class (`tcgvault:auth`, `tcgvault:search`, `tcgvault:upload`, `tcgvault:generate`, `tcgvault:import`). No duplicates.
|
||||
|
||||
- [ ] **Auth-then-rate-limit ordering check** for user-keyed routes. In `import-mtg.js`, `import-pokemon.js`, `import-lorcana.js`, `avatar.js`, `avatar/generate.js`: visually confirm via `git diff` that `getUserFromRequest` (or equivalent) appears BEFORE the `check*RateLimit` call. If the order is reversed, the `extractUserIdentifier` throw fires on every anonymous request — the test would surface as a 500 in dev, but better to never ship that shape.
|
||||
|
||||
## Manual verification (in addition to CI on push)
|
||||
|
||||
Run these in order. Paste relevant output (with secrets redacted) into the PR description.
|
||||
|
||||
- [ ] **Local dev boot.**
|
||||
```bash
|
||||
npm run dev
|
||||
```
|
||||
Expected: clean boot, no `[rate-limit]` warn-spam at startup (the limiter is lazy-init; no warn until first gated call). If `KV_REST_API_*` env vars are absent in `.env.local`, the first request to ANY gated route will emit one `[rate-limit] KV_REST_API_URL / KV_REST_API_TOKEN not set — rate limiting disabled (dev/test only)` line — that's correct dev-mode behavior.
|
||||
|
||||
- [ ] **Auth limiter regression check** — Brief 4's contract must be preserved.
|
||||
```bash
|
||||
for i in 1 2 3 4 5 6; do
|
||||
curl -sS -o /dev/null -w "POST /api/auth/login attempt $i: %{http_code}\n" \
|
||||
-X POST -H "Content-Type: application/json" \
|
||||
-d '{"email":"nobody@example.com","password":"wrong"}' \
|
||||
http://localhost:3000/api/auth/login
|
||||
done
|
||||
```
|
||||
Expected (only meaningful with `KV_REST_API_*` set):
|
||||
- Attempts 1-5: `401`
|
||||
- Attempt 6: `429` with `Retry-After` header
|
||||
|
||||
Without Upstash configured locally, all 6 will return `401` — the dev-mode no-op limiter — and that's also correct. Either outcome confirms `checkAuthRateLimit` still works through the refactored module.
|
||||
|
||||
- [ ] **Search limiter (anonymous, IP-keyed).** Without Upstash, this should never 429 in dev:
|
||||
```bash
|
||||
for i in $(seq 1 5); do
|
||||
curl -sS -o /dev/null -w "GET /api/cards/search: %{http_code}\n" \
|
||||
"http://localhost:3000/api/cards/search?query=test"
|
||||
done
|
||||
```
|
||||
Expected: `200` each call (dev-mode noop). The gate is wired but won't fire without Upstash. To exercise the live path, set `KV_REST_API_*` and burst >60 in <60s.
|
||||
|
||||
- [ ] **User-keyed limiter — verify gate-ordering throws on misuse.** This is a one-shot sanity check that the `extractUserIdentifier` throw fires when called pre-auth. Boot dev, then:
|
||||
```bash
|
||||
node -e "
|
||||
const { checkUploadRateLimit } = require('./lib/rate-limit.js');
|
||||
checkUploadRateLimit({ headers: {} }, null).catch(err => {
|
||||
console.log('OK - throw fired:', err.message.startsWith('[rate-limit] extractUserIdentifier'));
|
||||
});
|
||||
"
|
||||
```
|
||||
Expected: `OK - throw fired: true`. (If you get an `ERR_REQUIRE_ESM` error, use `node --experimental-vm-modules` or write a tiny `.mjs` wrapper — the module is ESM. The point is the throw, not the invocation shape.)
|
||||
|
||||
- [ ] **Admin-only enforcement on import routes** — anonymous → 401, authenticated-non-admin → 403, authenticated-admin → 200 (or whatever the import returns).
|
||||
```bash
|
||||
# 1. Anonymous:
|
||||
curl -sS -o /dev/null -w "anonymous import-mtg: %{http_code}\n" \
|
||||
-X POST -H "Content-Type: application/json" \
|
||||
-d '{"setCode":"neo"}' \
|
||||
http://localhost:3000/api/cards/import-mtg
|
||||
# Expected: 401
|
||||
|
||||
# 2. Auth'd non-admin (use a regular user's token):
|
||||
curl -sS -o /dev/null -w "user import-mtg: %{http_code}\n" \
|
||||
-X POST -H "Content-Type: application/json" \
|
||||
-H "Authorization: Bearer <regular-user-token>" \
|
||||
-d '{"setCode":"neo"}' \
|
||||
http://localhost:3000/api/cards/import-mtg
|
||||
# Expected: 403
|
||||
|
||||
# 3. Auth'd admin: (optional — actually triggers Scryfall fetch + DB writes; skip
|
||||
# unless you're staging-pointed and want to exercise the full happy path):
|
||||
curl -sS -w "admin import-mtg: %{http_code}\n" \
|
||||
-X POST -H "Content-Type: application/json" \
|
||||
-H "Authorization: Bearer <admin-token>" \
|
||||
-d '{"setCode":"neo"}' \
|
||||
http://localhost:3000/api/cards/import-mtg
|
||||
# Expected: 200 with {imported, skipped, total}
|
||||
```
|
||||
|
||||
Repeat for `import-pokemon` and `import-lorcana`. The first two checks (anonymous + non-admin) are the meaningful security check; the admin check is optional smoke and SHOULD ONLY run against a staging DB per `.cursor/rules/no-go-zones.mdc`.
|
||||
|
||||
- [ ] **Admin UI smoke** — log in as admin in the browser, visit `/admin/card-import`, type a set code (e.g. `neo` for MTG), click "Import Cards". Expected: the request succeeds (or returns whatever Scryfall would return). If the request 401s, the `pages/admin/card-import.js` Bearer-token edit didn't land — check the browser's network tab for the Authorization header on the POST.
|
||||
|
||||
- [ ] **Avatar upload smoke** — log in, visit `/profile` (or wherever the avatar uploader lives), upload an image. Expected: success. Then submit the form 11 times in <1 hour to verify the gate fires (with Upstash configured); without Upstash, no 429 in dev.
|
||||
|
||||
- [ ] **Vitest pass count unchanged.**
|
||||
```bash
|
||||
npm run test:run 2>&1 | tail -5
|
||||
```
|
||||
Expected: `Tests 21 passed (21)`. If the count or any individual test changes, the lib refactor broke a contract — investigate immediately. Most likely culprit: `checkAuthRateLimit`'s return shape drifted from `{ allowed, remaining, reset }`.
|
||||
|
||||
- [ ] **Diff hygiene.** `git diff main..HEAD --stat` should show:
|
||||
- `lib/rate-limit.js`: ~70 lines + / ~5 lines - (net add of ~65 lines).
|
||||
- 7 source-file additions (~14 lines + / ~0 lines - each): `users/search.js` (~6/0), `cards/search.js` (~6/0), `user/avatar.js` (~6/0), `user/avatar/generate.js` (~6/0), `cards/import-mtg.js` (~16/0), `cards/import-pokemon.js` (~16/0), `cards/import-lorcana.js` (~16/0).
|
||||
- `pages/admin/card-import.js`: +1 / 0 lines.
|
||||
- `.cursor/rules/api-routes.mdc`: ~70 lines + / ~32 lines - (replacing the existing § Rate limiting subsection).
|
||||
- No whitespace-only changes elsewhere.
|
||||
|
||||
## Boot-the-brief findings (preempted by the architect; do not re-investigate)
|
||||
|
||||
### Finding 1 — `pages/admin/card-import.js` does NOT currently send the Bearer token
|
||||
|
||||
Architect read the file at brief time (309 lines). Line 43-49 calls `fetch(endpoint, { method: 'POST', headers: { 'Content-Type': 'application/json' } })` with NO `Authorization` header. Without the fix in this brief, the moment the import APIs gain `getUserFromRequest`, the admin UI starts returning 401 on every import attempt. This is the documented scope expansion under Decision 1 — it's not optional, the brief is internally consistent only with both edits (API gate + admin UI fix) shipping together.
|
||||
|
||||
### Finding 2 — `import-lorcana.js` has zero frontend callers
|
||||
|
||||
Architect ran `rg 'import-lorcana' pages/ components/` and only `pages/admin/card-import.js` matched — but that match is in a comment / file-listing context, not a code-execution call (the `<select>` in card-import.js only has `mtg` and `pokemon` options). So gating Lorcana is purely defensive. A future cleanup convoy may delete `pages/api/cards/import-lorcana.js` if it's never wired up; for now, gating with the same shape as mtg/pokemon is the smaller-diff path.
|
||||
|
||||
### Finding 3 — `components/ShareModal.js`'s user-search has NO debounce
|
||||
|
||||
Architect read `components/ShareModal.js::handleSearch` (lines 56-77). It calls `fetch('/api/users/search?q=...')` on every keystroke when `query.length >= 2`. Typing a 17-char email like `alice@example.com` fires 16 requests within ~3 seconds (one per char after the 2-char minimum). Parent's recommendation of `search: 30 / 1min` would 429 on a single legitimate email entry. **Tuned up to 60 / 1min** in Decision 3 to fit the realistic burst pattern without blocking the search-as-you-type UX. A future client-side fix (adding debounce in ShareModal) would let us re-tighten this; queue as `debounce-share-modal-search` if it surfaces.
|
||||
|
||||
### Finding 4 — `pages/api/user/avatar/generate.js` uses DiceBear, not a paid AI service
|
||||
|
||||
Architect read the file (133 lines). It calls `https://api.dicebear.com/7.x/${avatarStyle}/svg?...` — free public API for SVG initials avatars. No OpenAI / Anthropic / Replicate cost. The "cost" of abuse is Vercel blob storage (the generated SVG gets `put()` into blob storage on every successful call) + DiceBear's own rate-limiting if we hammer them. **Tuned generate up to 5 / 1hour** from parent's 3 — still catches accidental loops (user mashing "regenerate avatar" button) without blocking legitimate "I want to try 4 different seeds" workflow.
|
||||
|
||||
### Finding 5 — `pages/api/cards/search.js` is a 240-line SQL god-function (do NOT refactor)
|
||||
|
||||
Architect read the file in full. Lines 40-186 are seven conditional SQL branches plus a fallback JS-filter path. Already flagged in `.convoys/ship-readiness.md` as `god-function-split` / `refactor-cards-search-sql` scope. The brief is explicit: only add the rate-limit gate at the top, do NOT touch any SQL. The implementer MUST resist the urge to "clean up while I'm in here" — that's a separate convoy with its own architect pass.
|
||||
|
||||
### Finding 6 — `pages/api/user/avatar.js` has NO top-level method gate; method-branches inside the outer `try`
|
||||
|
||||
Architect read the file (209 lines). Handler structure is `try { getUserFromRequest; if POST {...} else if DELETE {...} else 405 }` — the method check is the LAST branch, after both POST and DELETE bodies. This is unusual but the brief accommodates it by placing the rate-limit gate AFTER the auth check, BEFORE the method-branching. Both POST and DELETE branches inherit the limit. (DELETE is rare — only fires on "remove my avatar" — so the upload limit applying to both is fine.) Do NOT restructure the handler to add a top-level method gate; that's a cosmetic refactor and out of scope.
|
||||
|
||||
### Finding 7 — `pages/api/users/search.js` uses inline `jwt.verify`, not `getUserFromRequest`
|
||||
|
||||
Architect read the file. Lines 17-21 do `const token = authHeader.substring(7); try { jwt.verify(token, JWT_SECRET); } catch { 401; }` — but the verified `decoded` payload is discarded (the route doesn't need the user ID, only the proof of auth). For the IP-keyed search limiter, we don't need the user either — the gate just goes after the JWT-verify catch block, before the query-length validation. **Do NOT refactor to use `getUserFromRequest`** — that's its own auth-surface convoy (queued `single-auth-provider`).
|
||||
|
||||
### Finding 8 — Vitest does NOT currently transitively import `lib/rate-limit.js`
|
||||
|
||||
The convoy file claims `lib/rate-limit.js` is "unit-tested transitively via the existing vitest suite" — that's stale. Architect ran `rg 'rate-limit|@upstash' test/` → zero matches. Only `pages/api/auth/login.js` + `register.js` import `lib/rate-limit.js`, and neither has a vitest spec. The lib refactor is therefore **strictly safer** than the convoy file implies — there's no transitive test path to break. (`test/api/auth-utils.test.js` only imports `pages/api/auth-utils.js` + `lib/auth-secret.js`; no handler imports.) Decision 6 still holds: no NEW tests this convoy.
|
||||
|
||||
### Finding 9 — Test setup file doesn't set `KV_REST_API_*` (intentionally)
|
||||
|
||||
`test/setup.js` sets only `JWT_SECRET` and `NODE_ENV=test`. With `NODE_ENV=test`, `lib/rate-limit.js`'s `init()` falls into the warn-and-noop branch (`NODE_ENV !== 'production'`), so vitest never tries to construct a real Redis client. Even if a future vitest spec adds a handler import, the limiter no-ops in test. This is the correct shape; do NOT add `KV_REST_API_*` to `test/setup.js`.
|
||||
|
||||
### Finding 10 — `parseMultipartFormData` body-streaming is acknowledged out-of-scope but the gate ordering still matters
|
||||
|
||||
`pages/api/user/avatar.js::parseMultipartFormData` consumes the multipart body via `req.on('data')` + `req.on('end')`. If the rate-limit gate were placed AFTER `parseMultipartFormData`, an attacker could flood the 5MB ceiling even on a 429 path. The brief places the gate BEFORE the method-branching (which calls `parseMultipartFormData` inside the POST branch), so the gate fires before the body parse. **This is the correct ordering even though the body-streaming defense is out of scope** — when `harden-multipart-parser` eventually lands, the gate ordering will already be correct and won't need adjustment.
|
||||
|
||||
### Finding 11 — Per-class Redis prefix isolation is required for correctness
|
||||
|
||||
`@upstash/ratelimit@2.0.8`'s `prefix:` option scopes all keys for that limiter. Without distinct prefixes, two limiters sharing a prefix would share a sliding-window counter, meaning a search hit would consume the auth budget for the same identifier (or, for user-keyed classes, a search hit from user X would consume their upload budget). The brief enforces five distinct prefixes (`tcgvault:auth`, `tcgvault:search`, `tcgvault:upload`, `tcgvault:generate`, `tcgvault:import`). Verified at architect time against the @upstash/ratelimit README's prefix semantics.
|
||||
|
||||
## Out of scope (do not do these)
|
||||
|
||||
- [ ] Do NOT add new vitest or playwright tests. Deferred to `fill-vitest-handler-coverage` (Decision 6).
|
||||
- [ ] Do NOT add a `withRateLimit(handler)` higher-order wrapper. The 7 call sites justify inline; a wrapper is premature abstraction.
|
||||
- [ ] Do NOT migrate `lib/rate-limit.js` to Next.js middleware (Edge runtime). Pages Router serverless functions don't share the Edge runtime cleanly with `@upstash/ratelimit`'s default Node-fetch path; inline is simpler.
|
||||
- [ ] Do NOT add rate-limit headers to SUCCESS responses (`X-RateLimit-Remaining`, `X-RateLimit-Reset`). Honoring the existing `login.js` / `register.js` convention — only the 429 path sets `Retry-After`.
|
||||
- [ ] Do NOT vary the 429 error message per class (Decision 5). Uniform message minimizes attacker fingerprinting.
|
||||
- [ ] Do NOT add a global IP-based backstop limiter (Next.js middleware). Queued as `add-global-rate-limit-middleware` if a future audit shows non-listed routes being abused.
|
||||
- [ ] Do NOT touch `pages/api/cards/search.js`'s 240-line SQL god-function. Only add the rate-limit gate at the top.
|
||||
- [ ] Do NOT touch `pages/api/user/avatar.js`'s `parseMultipartFormData`. Body-streaming defense is `harden-multipart-parser` scope.
|
||||
- [ ] Do NOT delete `pages/api/cards/import-lorcana.js`. Gating with the same shape as mtg/pokemon is the chosen path under Decision 1 (Option A applied uniformly to all three).
|
||||
- [ ] Do NOT add Lorcana to the `<select>` in `pages/admin/card-import.js`. The only edit to that file is the Bearer-token addition on line 45-47.
|
||||
- [ ] Do NOT extract a `withAdmin(handler)` wrapper from the three new admin-role checks. The convention rule says to extract when a third call site appears; these ARE the third+fourth+fifth, but extraction is its own auth-surface refactor (queued `single-auth-provider` adjacent). Inline for this convoy.
|
||||
- [ ] Do NOT touch `AGENTS.md` Gotcha #12. Doc-writer pass at convoy close owns the update; preempting here creates merge conflicts.
|
||||
- [ ] Do NOT touch `.github/workflows/ci.yml`. No new CI gate is added in this convoy (per-class rate-limit wiring isn't grep-checkable; the existing `forbidden-endpoints` + `forbidden-cors-headers` jobs suffice for the API surface).
|
||||
- [ ] Do NOT bump `@upstash/ratelimit` or `@upstash/redis` versions. Pins stay at `^2.0.8` and `^1.38.0` from Brief 4.
|
||||
- [ ] Do NOT add `KV_REST_API_*` to `test/setup.js`. The warn-and-noop branch is the correct test behavior.
|
||||
|
||||
## Rationale (≤3 sentences)
|
||||
|
||||
Extending `lib/rate-limit.js` from one auth-only limiter to five named per-class limiters closes the last open P0 (#6 PARTIAL → RESOLVED) by wiring rate-limit + auth+admin gates into the remaining abusable surface; the hybrid named-export shape preserves Brief 4's `checkAuthRateLimit(req)` contract so `login.js` + `register.js` stay untouched. Auth-gating the three `pages/api/cards/import-*.js` routes (currently anonymous, hitting external Scryfall / Pokémon-TCG / Lorcana APIs with no caller throttling AND performing unbounded DB writes) is the security-critical scope expansion under Decision 1; adding the matching Bearer-token send to `pages/admin/card-import.js` is the necessary admin-UI fix to keep the gated APIs callable. Once this lands, the launch-readiness ship-blocker list is empty (8 of 8 RESOLVED), and the per-class shape is documented in `.cursor/rules/api-routes.mdc` for any future route to follow without architect re-derivation.
|
||||
|
|
@ -103,17 +103,29 @@ await logCollectionActivity(collectionId, userId, 'card_added', { cardId, quanti
|
|||
|
||||
## Rate limiting
|
||||
|
||||
`/api/auth/login` and `/api/auth/register` are wrapped with a 5-attempt / 15-minute sliding window via `lib/rate-limit.js`. New endpoints on the public auth surface (or anywhere brute-force / credential-stuffing matters) should follow the same shape:
|
||||
`lib/rate-limit.js` exposes five named limiters, one per route class. Each named export takes `req` (and `userId` for user-keyed classes) and returns `{ allowed, remaining, reset }`.
|
||||
|
||||
| Class | Limit | Window | Key | Used by | Helper |
|
||||
| --- | --- | --- | --- | --- | --- |
|
||||
| `auth` | 5 | 15 min | IP | `/api/auth/login`, `/api/auth/register` | `checkAuthRateLimit(req)` |
|
||||
| `search` | 60 | 1 min | IP | `/api/users/search`, `/api/cards/search` | `checkSearchRateLimit(req)` |
|
||||
| `upload` | 10 | 1 hour | user | `/api/user/avatar` | `checkUploadRateLimit(req, userId)` |
|
||||
| `generate` | 5 | 1 hour | user | `/api/user/avatar/generate` | `checkGenerateRateLimit(req, userId)` |
|
||||
| `import` | 5 | 1 hour | user | `/api/cards/import-mtg`, `/api/cards/import-pokemon`, `/api/cards/import-lorcana` | `checkImportRateLimit(req, userId)` |
|
||||
|
||||
**Verbatim call shape** (identical across all five classes — only the helper name and the optional `userId` argument differ):
|
||||
|
||||
```js
|
||||
import { checkAuthRateLimit } from '../../../lib/rate-limit.js';
|
||||
import { checkSearchRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'POST') {
|
||||
if (req.method !== 'GET') {
|
||||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkAuthRateLimit(req);
|
||||
// For user-keyed classes, auth check goes HERE first; see "Gate ordering" below.
|
||||
|
||||
const { allowed, reset } = await checkSearchRateLimit(req);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
|
|
@ -127,12 +139,23 @@ export default async function handler(req, res) {
|
|||
}
|
||||
```
|
||||
|
||||
Notes:
|
||||
**Gate ordering rules:**
|
||||
|
||||
- Gate sits between the method check and the body. It MUST be inside the `try`/`catch` if you want Upstash errors to bubble — but `checkAuthRateLimit` already swallows them and fails-open, so the placement above is fine.
|
||||
- Identifier is the first hop in `x-forwarded-for` (Vercel's edge); do NOT key off `req.body.email` (rotates) or `req.headers.authorization` (login is unauthenticated by design).
|
||||
- Env vars are `KV_REST_API_URL` + `KV_REST_API_TOKEN` (auto-provisioned by Vercel's Upstash Marketplace integration). In prod, missing either var is a **fail-closed throw** on the first call — set them in Vercel project settings before merging anything that imports `lib/rate-limit.js`. In dev, the module warn-and-no-ops so local work isn't blocked.
|
||||
- The current scope is just the two auth endpoints. Sweeping the rest of the API (`/api/users/search`, `/api/cards/import-*`, avatar upload) is the queued `add-rate-limiting` convoy — follow the same pattern there.
|
||||
1. **Method check first.** Reject the wrong verb with 405 before doing any limiter work.
|
||||
2. **Auth check before any user-keyed limiter.** `extractUserIdentifier(userId)` THROWS when `userId` is null/undefined/empty (defensive). For `upload`, `generate`, and `import`, the handler MUST call `getUserFromRequest(req)` (or equivalent JWT verification) and confirm a non-null user BEFORE calling the limiter. Wrong order = anonymous user bypasses (the THROW surfaces immediately during dev; do not catch and silently fall back to IP).
|
||||
3. **For IP-keyed limiters (`auth`, `search`), gate placement is flexible** — either at the top of the handler (after the method check) or after a separate auth check that the route happens to also have (e.g. `users/search` JWT-verifies before rate-limiting, both are correct). The limiter only needs `req` for IP extraction.
|
||||
4. **Admin-role check, if applicable, goes between auth and rate-limit.** Used by all three `/api/cards/import-*` routes: `if (user.role !== 'admin') return res.status(403).json({ error: 'Admin access required' })` sits between the `if (!user)` 401 and the import rate-limit call.
|
||||
|
||||
**Identifier extraction:**
|
||||
|
||||
- `extractIpIdentifier(req)` (module-private) — first hop in `x-forwarded-for` (Vercel's edge), falling back to `req.socket.remoteAddress`, falling back to the literal `'anonymous'`. Do NOT key off `req.body.email` (rotates) or `req.headers.authorization` (unauthenticated endpoints don't have one).
|
||||
- `extractUserIdentifier(userId)` (module-private) — formats as `user:${userId}`. Throws on null/undefined/empty/NaN to surface gate-ordering bugs at dev time rather than silently falling back to IP and creating a per-IP-not-per-user limit.
|
||||
|
||||
**Env vars (unchanged from Brief 4):** `KV_REST_API_URL` + `KV_REST_API_TOKEN` (auto-provisioned by Vercel's Upstash Marketplace integration). In prod, missing either var is a **fail-closed throw** on the first call. In dev / test, the module warn-and-no-ops so local work isn't blocked. See `AGENTS.md` Gotcha #12 for the full env-var contract.
|
||||
|
||||
**429 response shape is uniform across all five classes.** Same error message (`'Too many attempts. Try again later.'`) and same `Retry-After` header calculation. Per-class variation would fingerprint the limits to an attacker.
|
||||
|
||||
**Fail-open on Upstash outage.** A network failure inside `ratelimit.limit(...)` returns `{ allowed: true, remaining: Infinity, reset: 0 }` with a single `console.error('[rate-limit]', err)`. Reasoning: a hard Upstash outage should not lock the entire user base out of every gated route. Brute-force / abuse protection lives behind defense-in-depth (Vercel firewall, future fail2ban-style lockout).
|
||||
|
||||
## Dev/test endpoints (removed)
|
||||
|
||||
|
|
|
|||
|
|
@ -1,10 +1,23 @@
|
|||
import { Ratelimit } from '@upstash/ratelimit';
|
||||
import { Redis } from '@upstash/redis';
|
||||
|
||||
// Per-class limiter configuration. Distinct Redis prefix per class is
|
||||
// REQUIRED — without it, a search-class hit would consume the auth-class
|
||||
// budget for the same identifier. `slidingWindow` chosen across all
|
||||
// classes to match Brief 4's existing algorithm; switching to
|
||||
// `tokenBucket` per-class would be its own convoy.
|
||||
const LIMITER_CONFIG = {
|
||||
auth: { limit: 5, window: '15 m', prefix: 'tcgvault:auth' },
|
||||
search: { limit: 60, window: '1 m', prefix: 'tcgvault:search' },
|
||||
upload: { limit: 10, window: '1 h', prefix: 'tcgvault:upload' },
|
||||
generate: { limit: 5, window: '1 h', prefix: 'tcgvault:generate' },
|
||||
import: { limit: 5, window: '1 h', prefix: 'tcgvault:import' },
|
||||
};
|
||||
|
||||
// Lazy singleton. Module-load init would throw in environments without
|
||||
// Upstash env vars (local dev pre-onboarding, tests that transitively
|
||||
// import the auth handlers, Vercel build-time bundling). Defer construction
|
||||
// until the first request actually arrives.
|
||||
// import the auth handlers, Vercel build-time bundling). Defer
|
||||
// construction until the first request actually arrives.
|
||||
let cached = null;
|
||||
|
||||
function init() {
|
||||
|
|
@ -17,12 +30,18 @@ function init() {
|
|||
|
||||
if (url && token) {
|
||||
const redis = new Redis({ url, token });
|
||||
const ratelimit = new Ratelimit({
|
||||
const instances = new Map();
|
||||
for (const [name, cfg] of Object.entries(LIMITER_CONFIG)) {
|
||||
instances.set(
|
||||
name,
|
||||
new Ratelimit({
|
||||
redis,
|
||||
limiter: Ratelimit.slidingWindow(5, '15 m'),
|
||||
prefix: 'tcgvault:auth',
|
||||
});
|
||||
return { mode: 'live', ratelimit };
|
||||
limiter: Ratelimit.slidingWindow(cfg.limit, cfg.window),
|
||||
prefix: cfg.prefix,
|
||||
})
|
||||
);
|
||||
}
|
||||
return { mode: 'live', instances };
|
||||
}
|
||||
|
||||
if (process.env.NODE_ENV === 'production') {
|
||||
|
|
@ -39,13 +58,33 @@ function init() {
|
|||
return { mode: 'noop' };
|
||||
}
|
||||
|
||||
function extractIdentifier(req) {
|
||||
function extractIpIdentifier(req) {
|
||||
const xff = req.headers?.['x-forwarded-for'];
|
||||
const firstHop = Array.isArray(xff) ? xff[0] : xff?.split(',')[0]?.trim();
|
||||
return firstHop || req.socket?.remoteAddress || 'anonymous';
|
||||
}
|
||||
|
||||
export async function checkAuthRateLimit(req) {
|
||||
// THROWS on missing userId. Per-user limiters MUST sit AFTER the auth
|
||||
// check in the handler body — silently falling back to IP here would
|
||||
// convert a per-user limit into a per-IP limit, locking out other
|
||||
// household members for one user's behavior. The throw surfaces the
|
||||
// misordering immediately during development rather than at first
|
||||
// production incident.
|
||||
function extractUserIdentifier(userId) {
|
||||
if (
|
||||
userId === null ||
|
||||
userId === undefined ||
|
||||
userId === '' ||
|
||||
(typeof userId === 'number' && Number.isNaN(userId))
|
||||
) {
|
||||
throw new Error(
|
||||
'[rate-limit] extractUserIdentifier called without an authenticated userId. Place the rate-limit gate AFTER the auth check, never before.'
|
||||
);
|
||||
}
|
||||
return `user:${userId}`;
|
||||
}
|
||||
|
||||
async function check(className, identifier) {
|
||||
if (!cached) {
|
||||
cached = init();
|
||||
}
|
||||
|
|
@ -54,16 +93,39 @@ export async function checkAuthRateLimit(req) {
|
|||
return { allowed: true, remaining: Infinity, reset: 0 };
|
||||
}
|
||||
|
||||
const identifier = extractIdentifier(req);
|
||||
const limiter = cached.instances.get(className);
|
||||
if (!limiter) {
|
||||
throw new Error(`[rate-limit] Unknown limiter class: ${className}`);
|
||||
}
|
||||
|
||||
try {
|
||||
const { success, remaining, reset } = await cached.ratelimit.limit(identifier);
|
||||
const { success, remaining, reset } = await limiter.limit(identifier);
|
||||
return { allowed: success, remaining, reset };
|
||||
} catch (err) {
|
||||
// Fail-open on Upstash outage. A hard outage at the rate-limit backend
|
||||
// should not lock the entire user base out of login. Brute-force
|
||||
// protection lives behind defense-in-depth (Vercel firewall, etc.).
|
||||
// should not lock the entire user base out. Brute-force protection
|
||||
// lives behind defense-in-depth (Vercel firewall, etc.).
|
||||
console.error('[rate-limit]', err);
|
||||
return { allowed: true, remaining: Infinity, reset: 0 };
|
||||
}
|
||||
}
|
||||
|
||||
export async function checkAuthRateLimit(req) {
|
||||
return check('auth', extractIpIdentifier(req));
|
||||
}
|
||||
|
||||
export async function checkSearchRateLimit(req) {
|
||||
return check('search', extractIpIdentifier(req));
|
||||
}
|
||||
|
||||
export async function checkUploadRateLimit(req, userId) {
|
||||
return check('upload', extractUserIdentifier(userId));
|
||||
}
|
||||
|
||||
export async function checkGenerateRateLimit(req, userId) {
|
||||
return check('generate', extractUserIdentifier(userId));
|
||||
}
|
||||
|
||||
export async function checkImportRateLimit(req, userId) {
|
||||
return check('import', extractUserIdentifier(userId));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -44,6 +44,7 @@ const CardImport = () => {
|
|||
method: 'POST',
|
||||
headers: {
|
||||
'Content-Type': 'application/json',
|
||||
'Authorization': `Bearer ${localStorage.getItem('auth_token')}`,
|
||||
},
|
||||
body: JSON.stringify({ setCode: setCode.trim() }),
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,4 +1,6 @@
|
|||
import { sql } from '@vercel/postgres';
|
||||
import { getUserFromRequest } from '../../../lib/permission-middleware';
|
||||
import { checkImportRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
// Helper function to delay execution
|
||||
const delay = (ms) => new Promise(resolve => setTimeout(resolve, ms));
|
||||
|
|
@ -47,6 +49,20 @@ export default async function handler(req, res) {
|
|||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
const user = await getUserFromRequest(req);
|
||||
if (!user) {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
if (user.role !== 'admin') {
|
||||
return res.status(403).json({ error: 'Admin access required' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkImportRateLimit(req, user.userId);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
try {
|
||||
const { setCode } = req.body;
|
||||
|
||||
|
|
|
|||
|
|
@ -1,10 +1,26 @@
|
|||
import { sql } from '@vercel/postgres';
|
||||
import { getUserFromRequest } from '../../../lib/permission-middleware';
|
||||
import { checkImportRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'POST') {
|
||||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
const user = await getUserFromRequest(req);
|
||||
if (!user) {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
if (user.role !== 'admin') {
|
||||
return res.status(403).json({ error: 'Admin access required' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkImportRateLimit(req, user.userId);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
try {
|
||||
const { setCode } = req.body;
|
||||
|
||||
|
|
|
|||
|
|
@ -1,4 +1,6 @@
|
|||
import { sql } from '@vercel/postgres';
|
||||
import { getUserFromRequest } from '../../../lib/permission-middleware';
|
||||
import { checkImportRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
// Helper function to delay execution
|
||||
const delay = (ms) => new Promise(resolve => setTimeout(resolve, ms));
|
||||
|
|
@ -47,6 +49,20 @@ export default async function handler(req, res) {
|
|||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
const user = await getUserFromRequest(req);
|
||||
if (!user) {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
if (user.role !== 'admin') {
|
||||
return res.status(403).json({ error: 'Admin access required' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkImportRateLimit(req, user.userId);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
try {
|
||||
const { setCode } = req.body;
|
||||
|
||||
|
|
|
|||
|
|
@ -1,10 +1,17 @@
|
|||
import { sql } from '@vercel/postgres';
|
||||
import { checkSearchRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'GET') {
|
||||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkSearchRateLimit(req);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
try {
|
||||
const {
|
||||
query = '',
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import { put, del } from '@vercel/blob';
|
||||
import { sql } from '@vercel/postgres';
|
||||
import { getUserFromRequest } from '../../../lib/permission-middleware';
|
||||
import { checkUploadRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export const config = {
|
||||
api: {
|
||||
|
|
@ -18,6 +19,12 @@ export default async function handler(req, res) {
|
|||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkUploadRateLimit(req, user.userId);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
if (req.method === 'POST') {
|
||||
// Handle avatar upload
|
||||
const contentType = req.headers['content-type'];
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import { put } from '@vercel/blob';
|
||||
import { sql } from '@vercel/postgres';
|
||||
import { getUserFromRequest } from '../../../../lib/permission-middleware';
|
||||
import { checkGenerateRateLimit } from '../../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'POST') {
|
||||
|
|
@ -14,6 +15,12 @@ export default async function handler(req, res) {
|
|||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkGenerateRateLimit(req, user.userId);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
// Get user information for avatar generation
|
||||
const userResult = await sql`
|
||||
SELECT email, first_name, last_name, username FROM users WHERE id = ${user.userId}
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import { sql } from '@vercel/postgres';
|
||||
import jwt from 'jsonwebtoken';
|
||||
import { JWT_SECRET } from '../../../lib/auth-secret.js';
|
||||
import { checkSearchRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'GET') {
|
||||
|
|
@ -20,6 +21,12 @@ export default async function handler(req, res) {
|
|||
return res.status(401).json({ error: 'Invalid token' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkSearchRateLimit(req);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
const { q: query } = req.query;
|
||||
|
||||
if (!query || query.length < 2) {
|
||||
|
|
|
|||
Loading…
Reference in a new issue