deckhearth/.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md

---
convoy: fix-auth-bypass
brief_number: 4
depends_on: [1]
files:
  - package.json
  - package-lock.json
  - lib/rate-limit.js
  - pages/api/auth/login.js
  - pages/api/auth/register.js
cross_brief_commitments:
  - brief: 1
    description: |
      Brief 1 already removed the `JWT_SECRET` literal and routed login.js +
      register.js through `auth-utils.generateToken`. This brief preserves those
      changes; do NOT reintroduce inline `jwt.sign` or `JWT_SECRET` references.
  - brief: 5
    description: |
      Brief 5 (vitest) modifies `package.json` and `package-lock.json` after
      this brief. If Brief 5 lands first by accident, this brief's implementer
      MUST rebase on Brief 5's lockfile rather than regenerate from scratch.
      The sequenced order is Brief 4 → Brief 5; the convoy's slice_dependencies
      enforces this.
---

# Brief 4: Tighten the public auth surface (CORS + rate limit)

## Goal (1 sentence)

Drop the wide-open `Access-Control-Allow-Origin: '*'` header from `/api/auth/login` and `/api/auth/register`, and rate-limit both endpoints to 5 attempts per 15 minutes per IP via `@upstash/ratelimit` (with a graceful no-op fallback in non-production environments where Upstash isn't configured).

## Files in scope (do not edit anything else)

- `package.json` — modified (add `@upstash/ratelimit`, `@upstash/redis`)
- `package-lock.json` — modified (regenerated by `npm install`)
- `lib/rate-limit.js` — **new**
- `pages/api/auth/login.js` — modified
- `pages/api/auth/register.js` — modified

## Conventions to follow

- `.cursor/rules/auth-and-permissions.mdc` § "Token model" — auth flow shape stays unchanged. Only the request-acceptance gate (CORS, rate limit) changes.
- `.cursor/rules/api-routes.mdc` § "Method gating" + "Error handling" — the rate-limit check goes inside the existing `try`/`catch`, after the method gate, before the body parsing.
- `.cursor/rules/no-go-zones.mdc` — do not touch `pages/api/auth/verify.js`, `pages/api/favorites.js`, `pages/api/users/search.js`, or any other auth-adjacent file. The CORS sweep on the rest of the API is `add-rate-limiting` / future scope.
- `package.json` formatting: 2-space indent, alphabetical key order within `dependencies` / `devDependencies` (match the existing block from Brief 1's bump-next-js work).
- `lib/rate-limit.js` ESM export, kebab-case file name, 2-space indent, no top-level side effects beyond a const init.

## Acceptance criteria

### `package.json` changes

- [ ] `dependencies` gains `"@upstash/ratelimit": "^2.0.8"`. (Verified at architect time: `npm view @upstash/ratelimit version` → `2.0.8`. Peer dep: `@upstash/redis: ^1.34.3`.)
- [ ] `dependencies` gains `"@upstash/redis": "^1.38.0"`. (Verified at architect time: `npm view @upstash/redis version` → `1.38.0`. Satisfies `@upstash/ratelimit@2.0.8`'s peer-dep range `^1.34.3`. The only direct dep `@upstash/redis` itself pulls in is `uncrypto@^0.1.3`.)
- [ ] No other `dependencies` change. No `devDependencies` change in this brief (vitest is Brief 5).
- [ ] No `engines` block change. Both packages are pure-JS ESM with Node `>=18` requirements; tcg-vault runs Node 20 on Vercel.

### `package-lock.json` changes

- [ ] Regenerated via `npm install` (no hand edits).
- [ ] `npm ls @upstash/ratelimit` reports a single `2.0.x` version. No duplicates.
- [ ] `npm ls @upstash/redis` reports a single `1.38.x` version.
- [ ] `npm install` exits cleanly with no `ERESOLVE` errors and no `npm warn deprecated` for either package.

### `lib/rate-limit.js` (new)

- [ ] File exports a single async function `checkAuthRateLimit(req)` that returns `{ allowed: boolean, remaining: number, reset: number }`.
- [ ] On first call, the module initializes a singleton `Ratelimit` instance lazily. **Do not initialize at module top level** — top-level `new Redis(...)` would throw at import time in environments without Upstash env vars (including local dev where the developer hasn't onboarded Upstash yet, and any test that imports `pages/api/auth/login.js` transitively).
- [ ] Initialization rules:
  - If `process.env.KV_REST_API_URL` and `process.env.KV_REST_API_TOKEN` are both set: construct `new Redis({ url, token })` and `new Ratelimit({ redis, limiter: Ratelimit.slidingWindow(5, '15 m'), prefix: 'tcgvault:auth' })`. (Env-var names match Vercel's Upstash Marketplace integration; see Post-merge addendum.)
  - If either env var is missing AND `process.env.NODE_ENV === 'production'`: **throw at first call** with a message naming both env vars. (Fail-closed in prod — better to error a single login attempt than silently disable rate limiting.)
  - If either env var is missing AND `NODE_ENV !== 'production'`: log one `console.warn` ("`[rate-limit] KV_REST_API_URL / KV_REST_API_TOKEN not set — rate limiting disabled (dev/test only)`"), cache a no-op limiter (return `{ allowed: true, remaining: Infinity, reset: 0 }` from `checkAuthRateLimit`).
- [ ] IP extraction:

```js
const xff = req.headers['x-forwarded-for'];
const firstHop = Array.isArray(xff) ? xff[0] : xff?.split(',')[0]?.trim();
const identifier = firstHop || req.socket?.remoteAddress || 'anonymous';
```

  Use `identifier` as the rate-limit key. Do NOT use `req.body.email` (an attacker can rotate emails) or `req.headers.authorization` (login is unauthenticated by design — the header is absent).
- [ ] On Upstash quota error or network failure inside `ratelimit.limit(...)`: catch and **fail-open** (return `{ allowed: true, ... }`) with a single `console.error('[rate-limit]', err)`. Reasoning: a hard outage at Upstash should not lock everyone out of login. Brute-force protection lives behind defense-in-depth (Vercel firewall, future fail2ban-style lockout). Document this trade-off in a comment.
- [ ] No default export. Only the named `checkAuthRateLimit` export.
- [ ] No top-level `await` (Next.js Pages Router serverless bundler handles ESM, but module-init time is the wrong place for I/O — keep it lazy).

### `pages/api/auth/login.js`

- [ ] **Drop CORS-`*`.** Remove lines 9-17 (the `setHeader('Access-Control-Allow-Origin', '*')` and friends, plus the OPTIONS preflight). Same-origin requests on Vercel work without explicit CORS headers — the browser doesn't preflight a same-origin POST.
  - If a future cross-origin client appears (e.g. a separate marketing-site origin), pin via `process.env.PUBLIC_FRONTEND_ORIGIN`. **Do NOT add this conditionally now** — adding the env-var path "just in case" creates a code path no test will cover, and the current `tcg-vault` deploy is single-origin Vercel. The `add-rate-limiting` convoy or a follow-up `cors-tighten` convoy can add it when it actually has a consumer.
- [ ] **No OPTIONS handler.** With CORS-* gone, OPTIONS preflight isn't relevant for same-origin POST. If the front-end ever sends a preflight (it shouldn't on same-origin), Next.js will route it to this handler, which will hit the `if (req.method !== 'POST')` 405 branch — that's the correct response.
- [ ] **Add the rate-limit gate** between the method check and the body parsing. Verbatim shape:

```js
import { checkAuthRateLimit } from '../../../lib/rate-limit.js';
// ... existing imports stay ...

export default async function handler(req, res) {
  if (req.method !== 'POST') {
    return res.status(405).json({ error: 'Method not allowed' });
  }

  const { allowed, reset } = await checkAuthRateLimit(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 {
    // ... existing body unchanged ...
  } catch (error) {
    console.error('Login error:', error);
    res.status(500).json({ error: 'Internal server error' });
  }
}
```

  Note `Retry-After` is in seconds, and `reset` from `@upstash/ratelimit` is a Unix-ms timestamp (per the SDK's `Ratelimit.limit` return shape).
- [ ] Brief 1 already replaced `jwt.sign(...)` with `generateToken(user)`. **Preserve that.** Do not reintroduce inline `jwt.sign` or `JWT_SECRET` references.
- [ ] Brief 1 already removed `import jwt from 'jsonwebtoken'`. Keep it removed.

### `pages/api/auth/register.js`

- [ ] Same CORS removal as login.js (drop the four `setHeader` calls + OPTIONS preflight at lines 10-18).
- [ ] Same rate-limit gate, same shape, between method check and `try`. The 429 response shape and `Retry-After` header are identical.
- [ ] Same import path: `'../../../lib/rate-limit.js'`. Verify by reading the existing `'../../../lib/slug-utils.js'` import on line 4.
- [ ] Brief 1's `generateToken` call is preserved.

### Smoke (manual)

- [ ] In `.env.local`, set `KV_REST_API_URL` and `KV_REST_API_TOKEN` (if you have an Upstash free-tier account or have pulled them down from Vercel via `vercel env pull`). If you don't, leave both unset — the warn-and-continue branch should fire, and login still works.
- [ ] `npm run dev`; submit invalid login 6 times in quick succession (each with a typo). Expect: first 5 return 401, 6th returns 429 with `Retry-After` header. (Skipped if Upstash isn't configured.)
- [ ] Submit a valid login. Expect: token returned. (Successful logins also count against the limit per the sliding-window algo — that's intentional; a credential-stuffing attacker can't dodge by knowing one valid pair.)
- [ ] Open dev tools → network tab on the login submit. Confirm there is **no** `Access-Control-Allow-Origin` response header. Confirm there is **no** preflight `OPTIONS` request.
- [ ] Verify same behavior on `/api/auth/register`.
- [ ] Vercel preview deploy succeeds with both env vars unset → expect `npm run build` to succeed (lazy init means no import-time throw).

### Pre-deploy checklist (call out in the PR description)

- [ ] **Before merging to `main`, confirm `KV_REST_API_URL` and `KV_REST_API_TOKEN` are present in the Vercel project settings (Production + Preview environments).** These are **auto-provisioned** the moment the Vercel Upstash Marketplace integration is enabled on the project — no manual paste-the-token step. (You can verify locally with `vercel env ls` or by inspecting Vercel's project → Settings → Environment Variables.) Without them, the prod auth endpoints will throw on first login attempt (intentional fail-closed). Free-tier Upstash Redis via the Marketplace is sufficient (10k commands/day; rate-limit traffic is single-digit commands per request).
- [ ] Add a note to `.env.local.example` (if it exists; otherwise to AGENTS.md "Running locally" — but defer to doc-writer pass).

### Out of scope

- [ ] No CORS / rate-limit on `pages/api/auth/verify.js`. (`verify.js` is a GET on token presence; rate-limiting it would bounce legitimate page loads. The CORS-* on it is a smaller risk, deferred to `cors-tighten` or `add-rate-limiting`.)
- [ ] No CORS / rate-limit on `pages/api/favorites.js`, `pages/api/users/search.js`, `pages/api/cards/import-*.js`, avatar upload, etc. → `add-rate-limiting` convoy.
- [ ] No `withRateLimit(handler)` higher-order wrapper. The two endpoints in scope justify inline; a wrapper is premature abstraction until there are 3+ call sites.
- [ ] No middleware-based rate limit (Next.js `middleware.js`). Pages Router with serverless functions doesn't share the Edge runtime cleanly with `@upstash/ratelimit`'s default Node-fetch path. Inline is simpler.
- [ ] No `withCollectionPermission`-style wrapper change.
- [ ] No `AGENTS.md` / `.cursor/rules/auth-and-permissions.mdc` updates — doc-writer pass.

## Rationale (≤3 sentences)

Wrapping login + register with rate limiting closes the credential-stuffing window before public launch (P0 #6 partial), and dropping CORS-* removes a class of CSRF vectors that the wild-card header was masking (P0 #4). Choosing `@upstash/ratelimit` over a DIY-Postgres alternative respects the convoy's "no schema changes" rule, and choosing serverless-native over an in-memory limiter respects the Vercel deployment model (each cold start would otherwise reset its own counter). Bundling CORS and rate-limit into one brief — rather than splitting them across Brief 4 + Brief 5 as the convoy file initially suggested — avoids two PRs editing the same two handler files in sequence.

## Post-merge addendum (2026-05-23)

Added retroactively by `role-doc-writer` during convoy close-out. The brief as originally written specified `UPSTASH_REDIS_REST_URL` / `UPSTASH_REDIS_REST_TOKEN` — the two env-var names baked into `@upstash/redis`'s generic README examples — and the verbatim shape above still reflects that. **The shipped code in `lib/rate-limit.js` uses `KV_REST_API_URL` / `KV_REST_API_TOKEN` instead.** This addendum records the deviation so future readers don't mistake the brief's original shape for the as-built behavior.

**What changed and why.** Mid-implementation, the implementer surfaced that this project already runs on Vercel's Upstash Marketplace integration, which auto-provisions a Redis instance under a project-scoped credential set named `KV_*` (alongside `KV_URL`, `REDIS_URL`, and `KV_REST_API_READ_ONLY_TOKEN`). Aliasing those to a new `UPSTASH_REDIS_REST_*` pair would have required either (a) a manual paste-the-token step on every environment (Production, Preview, Local) or (b) a duplicate set of env vars pointing at the same Upstash instance. Neither was worth the friction; the Marketplace's own naming is the lower-coordination path.

**How the change was approved.** Implementer paused, surfaced the discrepancy upward via parent-agent interrupt, parent agent approved the rename to `KV_REST_API_*` ("ship what Vercel hands you"), and the implementer continued with the renamed pair. The convoy file's "Pre-merge env-var checklist" line ("UPSTASH_REDIS_REST_URL / UPSTASH_REDIS_REST_TOKEN") was not updated in the implementation PR — the close-out doc-writer pass (this addendum + the matching edit to `.convoys/fix-auth-bypass.md` § "Convoy outcome") is where the canonical record lives.

**Not in scope for this brief.** The other three `KV_*`-prefixed vars Vercel exposes (`KV_URL`, `REDIS_URL`, `KV_REST_API_READ_ONLY_TOKEN`) are intentionally unused. `@upstash/redis`'s REST client reads only `KV_REST_API_URL` + `KV_REST_API_TOKEN`; the others are for the Redis-protocol client (`@upstash/redis/cloudflare` / `ioredis`) or for read-only consumers. Do not wire them up unless a downstream library specifically requires one.

**Verbatim shape correction.** The "Initialization rules" snippet above has been updated in-place to read `KV_REST_API_URL` / `KV_REST_API_TOKEN`. The "Smoke (manual)" and "Pre-deploy checklist" sections have been updated to match. Any other documentation that still mentions `UPSTASH_REDIS_REST_*` for this project (search-and-replace target) is stale.