deckhearth/.convoys/fix-auth-bypass.md

---
name: fix-auth-bypass
classification: server-only
success_metric: getUserFromRequest returns null for missing tokens; no API route accepts unauthenticated requests; CI green.
skip:
  - ia
  - ux
  - visual
  - a11y
  - design
status: shipped
created: 2026-05-22
shipped: 2026-05-23
---

# Convoy: fix-auth-bypass

Closes P0 ship-blockers **#1, #2, #4, #5, and #6 (partial)** from `.convoys/ship-readiness.md`. This is the very first real convoy after the bootstrap and gates the rest of the launch sequence — until it lands, every other production-bound PR is paused.

## Why

The current `lib/permission-middleware.js::getUserFromRequest` returns a hardcoded admin user (`{ id: 1, role: 'admin', email: 'admin@tcgvault.com' }`) when no `Authorization` header is present. Every API route that calls it (30+ handlers per `user-code-review-graph`) therefore accepts unauthenticated requests **as admin**. Combined with:

- A weak fallback `JWT_SECRET` (`'your-secret-key-change-in-production'`) duplicated across 7 files,
- Four dev-only endpoints (`/api/simple`, `/api/test-auth`, `/api/test-db`, `/api/setup-database`) shipped in `pages/api/`,
- `Access-Control-Allow-Origin: *` on auth endpoints,
- Zero rate limiting on login,

…the production URL is effectively wide-open. **No anonymous traffic can touch the live site until this convoy ships.**

Success looks like:

1. `getUserFromRequest` returns `null` when there is no Bearer token. Period. No callers receive a synthetic admin.
2. There is exactly one source of truth for the JWT secret. If `process.env.JWT_SECRET` is unset, the server fails to boot with a clear error — not a silent fallback.
3. The four dev endpoints are gone, and CI fails the build if they reappear.
4. The login + register endpoints respond only to the production frontend origin (or no CORS header at all on same-origin Vercel deploy).
5. Login + register are rate-limited (the bare minimum of P0 #6; the rest is `add-rate-limiting`).
6. CI is green (lint + the new auth tests).

## Scope

**In:**

- `lib/permission-middleware.js` — remove hardcoded admin fallback; return `null` on missing/invalid token.
- New `lib/auth-secret.js` (or named equivalent — Architect to confirm) — single export of `JWT_SECRET`, throws at module load if unset.
- Refactor `pages/api/auth-utils.js`, `pages/api/auth/login.js`, `pages/api/auth/register.js`, `pages/api/auth/verify.js`, `pages/api/favorites.js`, `pages/api/users/search.js`, and `lib/permission-middleware.js` to import from the new secret helper. Remove all `process.env.JWT_SECRET || '…'` literals.
- Reconcile token expiry inconsistency (login = 24h, auth-utils = 7d). Pick one — Architect's call; record in `.cursor/rules/auth-and-permissions.mdc`.
- Delete `pages/api/simple.js`, `pages/api/test-auth.js`, `pages/api/test-db.js`, `pages/api/setup-database.js`.
- Add a CI grep step to `.github/workflows/ci.yml` that fails the build if `pages/api/test-*`, `pages/api/simple.js`, or `pages/api/setup-database.js` ever re-appear.
- Tighten `Access-Control-Allow-Origin` on `pages/api/auth/login.js` and `pages/api/auth/register.js`. Default: drop the header entirely (same-origin on Vercel). Fallback: pin to a `process.env.PUBLIC_FRONTEND_ORIGIN` env var.
- Adopt `@upstash/ratelimit` (or equivalent — Architect's pick) and apply to `/api/auth/login` and `/api/auth/register` only. **Other endpoints listed in P0 #6 (search, imports, avatar upload) are deferred to the `add-rate-limiting` convoy.**
- Add unit tests for `getUserFromRequest`: missing header → `null`, malformed token → `null`, valid token → user object, expired token → `null`. Architect to decide whether to land this with `vitest` now or defer to the `adopt-vitest` convoy. **Default recommendation: install vitest in this convoy.** The blast radius of an auth refactor justifies bringing the test runner forward by one slot in the launch sequence.

**Out (deferred to their own convoys):**

- P0 #3 (default admin creds + README) → `drop-public-setup`.
- P0 #7 (Layout default-prop leaks maintainer email) → `fix-layout-default-user`.
- P0 #6 (full) — rate limit on search / import / upload routes → `add-rate-limiting`.
- Any auth-context client-side cleanup (`lib/auth-context.js` vs `lib/admin-auth.js` vs `lib/use-auth.js`) → `single-auth-provider`.
- The `lib/database.js` vs `@vercel/postgres` reconciliation → `single-sql-client`.

**Hard "do not touch" in this convoy:**

- No UI files. No `components/`, no `pages/*.js` that aren't under `pages/api/`. If a UI file appears in a brief, kick it back to Architect.
- No schema changes. No SQL migrations. (`scripts/setup-neon-db.js` is read-only here.)
- No new feature flags. The flag wrapper exists (`lib/flags/index.js`) but this convoy is unflagged — auth fixes don't ship behind a flag.

## Roles invoked

Per `server-only` classification (skip: `ia, ux, visual, a11y, design`):

1. **role-architect** — produces a slice plan with explicit `slice_dependencies:`. Expect 4–6 briefs (auth-secret helper, getUserFromRequest fix + caller audit, dev-endpoint removal + CI guard, CORS tighten, rate-limit pattern + login/register wiring, tests). Architect must declare which briefs are parallel-safe.
2. **role-implementer** — runs one brief at a time, except where Architect marks `depends_on: []` and `files:` are disjoint. Then `/multitask` can fan out (see dispatch below).
3. **role-reviewer** — single-pass after the PR drafts. **Design-system-auditor and a11y-auditor are skipped** for this convoy — there is no UI surface to audit. Reviewer covers correctness, security regressions, and test coverage.
4. **role-doc-writer** — last. Updates `.cursor/rules/auth-and-permissions.mdc` (canonical secret helper, chosen expiry, rate-limit pattern), `AGENTS.md` "Common gotchas" section (remove items that are no longer gotchas), and `docs/SCHEMA_MAP.md` only if any DB read pattern changed (it shouldn't).

**Multitask dispatch recommendation** (Cursor 3.2 `/multitask`): after Architect publishes briefs with `depends_on: []` and disjoint `files:`, the user may dispatch implementers in parallel. Typical safe fan-out for this convoy:

- Group `audit-fix-auth-bypass-<pr>`: `role-reviewer` only (no design / a11y).
- Implementer fan-out: only if Architect explicitly marks briefs as parallel-safe. The auth-secret helper brief must complete first; everything else depends on it. So realistic fan-out is post-secret-helper: dev-endpoint deletion + CORS tighten + rate-limit wiring in parallel; `getUserFromRequest` fix runs alongside but its tests block on the secret helper landing first.

## Todos

High-level checklist for the next role to refine. Each becomes a brief under `.convoys/fix-auth-bypass/brief-N-*.md`.

- [ ] **Brief 1 — Central JWT secret helper.** Create `lib/auth-secret.js`, fail-loud on missing env. Decide canonical token TTL.
- [ ] **Brief 2 — Remove the admin bypass.** Fix `getUserFromRequest`; audit every caller (`user-code-review-graph` query: incoming edges to `lib-admin::getUserFromRequest`). Add unit tests covering missing/invalid/expired/valid token paths.
- [ ] **Brief 3 — Delete dev-only endpoints.** Remove four files; add CI guard.
- [ ] **Brief 4 — Tighten auth CORS.** Drop `Access-Control-Allow-Origin: *` on login + register. Add same-origin fallback via env var.
- [ ] **Brief 5 — Rate-limit login + register.** Install `@upstash/ratelimit` (or Architect-chosen alternative). Wire to login + register only. Defer the full sweep to `add-rate-limiting`.
- [ ] **Brief 6 — Test harness (provisional).** Install `vitest`, write the `getUserFromRequest` suite, re-enable the `test:` job in `.github/workflows/ci.yml`. Architect to confirm whether this is in-scope here or split to `adopt-vitest`.
- [ ] **Doc-writer pass.** Update auth rules + AGENTS.md gotchas.

## Hand-off

**Next role: `role-architect`.**

To run it in a new chat, paste:

> *"Run role-architect on convoy `fix-auth-bypass`. Read `.convoys/fix-auth-bypass.md` for scope and todos, then produce a slice plan with explicit `slice_dependencies:`. Output briefs to `.convoys/fix-auth-bypass/brief-N-*.md`. Flag which briefs are parallel-safe so the user can `/multitask` implementers."*

Conductor exits here. Human-in-the-loop gate: review the convoy file, confirm the scope split, then start the Architect.

## Architecture

Architect: `role-architect`. Date: 2026-05-23. Convoy decomposed into **5 briefs** (down from the conductor's 6 candidates — Brief 4 "CORS tighten" and Brief 5 "rate-limit" are merged into a single Brief 4 because they share `pages/api/auth/login.js` + `pages/api/auth/register.js` and would otherwise serialize against each other).

### File plan

| File | Action | Brief | Purpose |
| --- | --- | --- | --- |
| `lib/auth-secret.js` | new | 1 | Single source of truth for `JWT_SECRET` (fail-loud) + canonical `JWT_TOKEN_TTL = '24h'`. |
| `lib/permission-middleware.js` | modified ×2 | 1, 2 | Brief 1 swaps the `JWT_SECRET` literal for an import; Brief 2 removes the synthetic-admin fallback in `getUserFromRequest`. |
| `pages/api/auth-utils.js` | modified | 1 | Literal → import; `'7d'` → `JWT_TOKEN_TTL`. Becomes the canonical `generateToken` / `verifyToken` site. |
| `pages/api/auth/login.js` | modified ×2 | 1, 4 | Brief 1: literal → import, inline `jwt.sign` → `generateToken`. Brief 4: drop CORS-`*`, add rate-limit gate. |
| `pages/api/auth/register.js` | modified ×2 | 1, 4 | Same as login. |
| `pages/api/auth/verify.js` | modified ×2 | 1, 2 | Brief 1: literal → import. Brief 2: remove the no-token admin-fetch branch (returns 401 instead). |
| `pages/api/favorites.js` | modified | 1 | Literal → import. |
| `pages/api/users/search.js` | modified | 1 | Literal → import. |
| `pages/api/simple.js` | **deleted** | 3 | Dev endpoint, unauthenticated, no runtime references. |
| `pages/api/test-auth.js` | **deleted** | 3 | Dev endpoint, leaks token-handling internals. |
| `pages/api/test-db.js` | **deleted** | 3 | Dev endpoint, exposes DB connection metadata. |
| `pages/api/setup-database.js` | **deleted** | 3 | Public unauthenticated DDL trigger. |
| `lib/rate-limit.js` | new | 4 | Lazy-init `@upstash/ratelimit` wrapper with prod fail-closed + dev no-op fallback. |
| `package.json` | modified ×2 | 4, 5 | Brief 4: add `@upstash/ratelimit` + `@upstash/redis`. Brief 5: add `vitest` devDep + `test` / `test:run` scripts. |
| `package-lock.json` | modified ×2 | 4, 5 | Regenerated by `npm install` in each. |
| `vitest.config.js` | new | 5 | Node env, `test/**/*.test.js`, `test/setup.js` setupFile. |
| `test/setup.js` | new | 5 | Sets `JWT_SECRET=test-…` and `NODE_ENV=test` before any module loads. |
| `test/lib/auth-secret.test.js` | new | 5 | 3 tests: exports + fail-loud throw. |
| `test/lib/permission-middleware.test.js` | new | 5 | 8 tests covering Brief 2's null-return contract (incl. negative regression against the synthetic-admin shape). |
| `test/api/auth-utils.test.js` | new | 5 | 5 tests covering `generateToken` / `verifyToken` round-trip + 24h TTL. |
| `.github/workflows/ci.yml` | modified ×2 | 3, 5 | Brief 3: add `forbidden-endpoints` job (blocking). Brief 5: re-enable the disabled `test:` job, remove the "no test runner" comment header. |
| `README.md` | modified | 3 | Remove the `GET /api/test-db` line from the API list. |

Note the ×2 markers — those files have two briefs editing them in sequence. The slice_dependencies graph below sequences them so no two parallel writers ever target the same file.

### API surface

No new routes. Modified routes:

| Method | Path | Auth | Brief | Notes |
| --- | --- | --- | --- | --- |
| `POST` | `/api/auth/login` | none (auth-emitting) | 1, 4 | Brief 1: token-mint refactor (no behavior change). Brief 4: drops CORS-`*`, adds rate-limit (5/15min/IP). On limit: 429 + `Retry-After` header. |
| `POST` | `/api/auth/register` | none | 1, 4 | Same as login. |
| `GET` | `/api/auth/verify` | Bearer (now required) | 1, 2 | Brief 1: secret-import refactor. Brief 2: returns 401 instead of fetching `admin@tcgvault.com` when no Bearer header. |
| `GET / POST / DELETE` | `/api/favorites` | Bearer | 1 | Secret-import refactor only. |
| `GET` | `/api/users/search` | Bearer | 1 | Secret-import refactor only. |

Deleted routes (no replacement, no redirect):

| Method | Path | Brief |
| --- | --- | --- |
| `GET / POST` | `/api/simple` | 3 |
| `GET` | `/api/test-auth` | 3 |
| `GET` | `/api/test-db` | 3 |
| `POST` | `/api/setup-database` | 3 |

Request validation: no new schema validator (no zod/yup) added in this convoy — the existing manual validation in each handler stays. Validator adoption is its own future convoy.

### Schema diff

**No schema change.** No SQL migration. No edits to `scripts/setup-neon-db.js` or `docs/SCHEMA_MAP.md`. The convoy is hard-scoped against schema changes.

The seed user (`admin@tcgvault.com`, password `admin123`) is **not** removed by this convoy — that is the future `drop-public-setup` convoy. Brief 2 only stops `verify.js` from auto-fetching that row; the row itself remains.

### Test plan

- **Brief 5** ships the harness (vitest@^3.2.4, plain JS) and 16 unit tests:
  - 3 tests: `lib/auth-secret.js` (exports + fail-loud throw on missing env).
  - 8 tests: `lib/permission-middleware.js::getUserFromRequest` (missing header / non-Bearer / malformed / wrong-secret / expired / valid-but-no-row / valid + happy-path / negative regression against synthetic-admin shape).
  - 5 tests: `pages/api/auth-utils.js` (`generateToken` 24h TTL + payload + `verifyToken` round-trip + bad-signature + malformed).
- **No integration tests** (`pages/api/auth/login.js` end-to-end). Deferred to a follow-up convoy that adopts `supertest` or Playwright.
- **No tests for `lib/rate-limit.js`.** The lazy-init + fail-open + fail-closed branches need an Upstash mock; deferred to a follow-up.
- **CI integration:** Brief 5 re-enables `.github/workflows/ci.yml`'s `test:` job (commented out at lines 87-103 today). The job runs on every PR and push to `main`, blocking on failure.
- **Existing test files to use as examples:** none — this is the first test infra in the repo. The closest reference is the `bump-next-js` convoy retro, which documents the JS-only constraint.

### Risk list

This is a security-critical convoy; the risks are higher than `bump-next-js`.

- **R1 — JWT_SECRET fail-loud breaks anything that imports `lib/auth-secret.js` at module-load time without the env var set.** Includes: any future test, any future `npm run setup-db` or import script that transitively imports auth code, and any new `pages/_app.js`-time import. **Mitigation:** none of the in-scope auth files are imported at build time (Pages Router serverless functions are imported per-request); `next build` should not trip the throw. **Verification:** Brief 1's smoke step explicitly tests `npm run dev` with `JWT_SECRET` unset and confirms the error message is clear. Brief 5's `test/setup.js` sets `JWT_SECRET` before any test imports auth code.
- **R2 — Removing the synthetic-admin fallback may break a caller that secretly relies on it.** **Mitigation:** Brief 2 spot-checks all 24 callers; the architect verified that 23/24 use `if (!user) return 401` and the 1 exception (`pages/api/collections/[identifier].js`) uses `user?.userId` optional-chaining and works correctly when `user` is `null`. **Residual risk:** any caller added between architect's audit (commit `ebd4fd1`) and Brief 2's merge could regress. Mitigated by including the spot-check command in Brief 2's acceptance criteria so the implementer re-runs the grep at PR open time.
- **R3 — Existing logged-in users hold tokens signed against the literal fallback secret (`'your-secret-key-change-in-production'`).** Once Brief 1 lands and `JWT_SECRET` is required to be set in prod, those tokens stop verifying because `jwt.verify(token, REAL_SECRET)` will reject them. **Mitigation:** the deploy plan should announce a "you'll need to log back in" notice. There is no graceful migration; the alternative (accept either secret for a transition window) is exactly the bypass we are trying to remove. The blast-radius is acceptable because the user base is currently small (pre-launch).
- **R4 — Token TTL change from `7d` (in `auth-utils.generateToken`) to `24h`.** No user is currently affected because `auth-utils.generateToken` was not in the call path — `login.js` and `register.js` did inline `jwt.sign`. **Net effect:** users continue to get the 24h tokens they already had; the TTL drift in `auth-utils` is fixed in the same direction.
- **R5 — Rate-limit picks the wrong identifier on Vercel.** `req.headers['x-forwarded-for']` is set by Vercel's proxy and includes a chain when behind multiple hops; the first IP is the client. **Mitigation:** Brief 4 specifies the first-hop extraction explicitly. **Residual risk:** if Vercel ever changes its forwarding chain, the limit-key changes too. **Verification:** the smoke step in Brief 4 confirms the rate-limit fires on a real Vercel preview.
- **R6 — Upstash quota exhaustion.** Free tier is 10k commands/day. Each login costs ~1 command (sliding-window read+write batched). At 10k logins/day the limiter starts failing. **Mitigation:** Brief 4's `lib/rate-limit.js` fail-opens on Upstash error (single `console.error`). Defense-in-depth via Vercel firewall is a future hardening pass.
- **R7 — `package.json` / `package-lock.json` merge conflicts between Brief 4 and Brief 5.** Both touch the lockfile. **Mitigation:** slice_dependencies sequences Brief 5 after Brief 4 (`depends_on: [1, 2, 4]`); the implementer for Brief 5 rebases onto Brief 4's main commit, not onto pre-Brief-4 main.
- **R8 — `@upstash/ratelimit@2.0.8` introduces a transitive that conflicts with our existing `@vercel/postgres@0.10.0` or `@neondatabase/serverless@1.0.1`.** **Mitigation:** the architect ran `npm view @upstash/ratelimit dependencies` and `npm view @upstash/redis dependencies` (sole new transitives: `uncrypto@^0.1.3`, `crypto-js`-style one-file modules). No overlap with the existing tree. **Residual risk:** `npm install` could surface a peer-dep warning we missed. Brief 4 acceptance criterion makes the implementer report the install output.
- **R9 — vitest@3.2.4 transitively pulls in `vite@5/6/7`, which has a Node engines requirement of `^20.19 || >=22.12`.** Vercel's CI runs Node 20 (set in `ci.yml`'s `NODE_VERSION: '20'`, which `actions/setup-node@v4` resolves to the latest 20.x patch — currently `>=20.19`). **Verification:** the existing `bump-next-js` convoy's brief #1 already documents this constraint and Vercel's runtime satisfies it. Local-dev developers on Node 20.0–20.18 will see vitest fail at install time; mitigation is to bump local Node to 20.19+, which is already the existing recommendation.
- **R10 — JWT-secret rotation is now coupled to a redeploy.** Pre-fix, rotating the env var was a no-op (the fallback string was used regardless). Post-fix, an unset env var means the server refuses to boot. **Mitigation:** documented in Brief 4's pre-deploy checklist; the fix is to set `JWT_SECRET` in Vercel before merging.
- **R11 — Test-mock drift.** `test/lib/permission-middleware.test.js` mocks `@vercel/postgres`. If a future convoy migrates the file to `@neondatabase/serverless` or another client, the mock won't fire and tests pass without exercising the real path. **Mitigation:** the mock target is documented in Brief 5's acceptance criteria; the future-migration convoy must also update the mock.
- **R12 — CI guard regex misses a renamed dev endpoint.** Brief 3's `forbidden-endpoints` job checks 4 explicit paths plus `find pages/api -name 'test-*.js'`. If someone re-introduces a dev endpoint as `pages/api/debug.js` or `pages/api/internal/health.js`, the guard misses it. **Mitigation:** intentional — the guard is a regression-prevention belt for the four known files, not a general "no dev endpoints" policy. Adding a stricter check (e.g. require all public endpoints to import an auth helper) is a future hardening convoy.

### Decomposition

| Brief # | Title | Files | Depends on | Estimated PR size |
| --- | --- | --- | --- | --- |
| 1 | Central JWT secret helper + 24h token TTL | `lib/auth-secret.js` (new), `lib/permission-middleware.js`, `pages/api/auth-utils.js`, `pages/api/auth/login.js`, `pages/api/auth/register.js`, `pages/api/auth/verify.js`, `pages/api/favorites.js`, `pages/api/users/search.js` | — | ~120 LOC (mostly mechanical import refactor across 7 files + 12-line new helper) |
| 2 | Remove the synthetic-admin bypass | `lib/permission-middleware.js`, `pages/api/auth/verify.js` | 1 | ~25 LOC (net-negative; deletes the dev-fallback branches) |
| 3 | Delete dev-only endpoints + CI guard | `.github/workflows/ci.yml`, `README.md` (modified); `pages/api/simple.js`, `pages/api/test-auth.js`, `pages/api/test-db.js`, `pages/api/setup-database.js` (deleted) | — | ~30 LOC (one CI job + 4 deletions + 1 README line) |
| 4 | Tighten the public auth surface (CORS + rate limit) | `package.json`, `package-lock.json`, `lib/rate-limit.js` (new), `pages/api/auth/login.js`, `pages/api/auth/register.js` | 1 | ~150 LOC (rate-limit module ~70, two handler edits ~40, package.json/lock ~40) |
| 5 | Install vitest + auth tests + re-enable CI test job | `package.json`, `package-lock.json`, `vitest.config.js` (new), `test/setup.js` (new), `test/lib/auth-secret.test.js` (new), `test/lib/permission-middleware.test.js` (new), `test/api/auth-utils.test.js` (new), `.github/workflows/ci.yml` | 1, 2, 4 | ~280 LOC (16 test cases dominate; vitest config + setup + CI YAML are small) |

All five briefs are under the 400-LOC budget. Brief 5 is the largest by LOC but the lowest by complexity (test boilerplate).

### Slice dependencies (multitask-ready)

```yaml
slice_dependencies:
  - brief: 1
    depends_on: []
    files:
      - lib/auth-secret.js
      - lib/permission-middleware.js
      - pages/api/auth-utils.js
      - pages/api/auth/login.js
      - pages/api/auth/register.js
      - pages/api/auth/verify.js
      - pages/api/favorites.js
      - pages/api/users/search.js
  - brief: 2
    depends_on: [1]
    files:
      - lib/permission-middleware.js
      - pages/api/auth/verify.js
  - brief: 3
    depends_on: []
    files:
      - .github/workflows/ci.yml
      - README.md
      - pages/api/simple.js
      - pages/api/test-auth.js
      - pages/api/test-db.js
      - pages/api/setup-database.js
  - brief: 4
    depends_on: [1]
    files:
      - package.json
      - package-lock.json
      - lib/rate-limit.js
      - pages/api/auth/login.js
      - pages/api/auth/register.js
  - brief: 5
    depends_on: [1, 2, 4]
    files:
      - package.json
      - package-lock.json
      - vitest.config.js
      - test/setup.js
      - test/lib/auth-secret.test.js
      - test/lib/permission-middleware.test.js
      - test/api/auth-utils.test.js
      - .github/workflows/ci.yml
```

**Multitask fan-out plan (3 waves):**

1. **Wave A (concurrent):** Briefs **1** + **3**. Files are completely disjoint. Two implementers can run side-by-side.
2. **Wave B (concurrent, after Brief 1 merges):** Briefs **2** + **4**. Both depend on Brief 1's secret-helper landing first. Their `files:` sets overlap only on files Brief 1 already published, and they touch disjoint subsets of those files (Brief 2 → `permission-middleware.js` + `verify.js`; Brief 4 → `login.js` + `register.js`).
3. **Wave C (single, after Brief 2 + Brief 4 merge):** Brief **5**. `depends_on: [1, 2, 4]` because the tests cover Brief 2's behavior and the lockfile sits on top of Brief 4's `npm install`.

`/multitask` dispatch suggestion when the human approves the plan:

```text
/multitask
- impl-1: role-implementer brief=1 from .convoys/fix-auth-bypass/brief-1-central-jwt-secret-helper.md
- impl-3: role-implementer brief=3 from .convoys/fix-auth-bypass/brief-3-delete-dev-endpoints.md
```

Then after Wave A merges:

```text
/multitask
- impl-2: role-implementer brief=2 from .convoys/fix-auth-bypass/brief-2-remove-admin-bypass.md
- impl-4: role-implementer brief=4 from .convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md
```

Then Brief 5 alone.

### Architect's calls (decisions made during this pass)

- **Token TTL: 24h.** Matches current `login.js` user experience (no session-length regression for existing users) and is the more security-conservative choice over the unused `auth-utils.generateToken`'s `'7d'` default. Codified as `JWT_TOKEN_TTL = '24h'` in `lib/auth-secret.js`.
- **Rate-limit library: `@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0`.** DIY-Postgres was rejected (would require schema changes — out of scope). DIY-in-memory was rejected (broken on Vercel cold starts). `next-rate-limit` was rejected (stale, in-memory, same cold-start issue). `@upstash/ratelimit` is the only mature serverless-native option. Cost: free tier (10k commands/day) is sufficient for current traffic.
- **Brief 6 (vitest): in-scope, not split.** The convoy file's default recommendation stands — auth's blast radius justifies bringing the test runner forward by one slot. Pinned to vitest@^3.2.4 (not v4) because v4 makes `vite` a non-optional peer-dep, which would inflate this JS-only repo's dep tree without benefit. **Renumbered as Brief 5** in the final decomposition.
- **Briefs 4 + 5 from the original conductor draft (CORS + rate-limit) merged into a single Brief 4.** Both edit `pages/api/auth/login.js` and `pages/api/auth/register.js`. Splitting them would force serial execution; merging them ships the public-auth-surface tightening as one cohesive PR.
- **Brief 6 from the original draft (vitest) is now Brief 5.** Total brief count: 5.
- **`pages/api/auth/verify.js` CORS is NOT tightened in this convoy.** Convoy explicitly scopes Brief 4 to login + register. Verify-CORS is deferred to `cors-tighten` or `add-rate-limiting`. Documented as out-of-scope in Brief 2 and Brief 4.

### Boot-the-brief findings

The architect ran the verification pass before declaring complete. Findings:

1. **`@upstash/ratelimit@2.0.8` peer dep verified.** `npm view @upstash/ratelimit peerDependencies` → `{ '@upstash/redis': '^1.34.3' }`. Pin both `@upstash/ratelimit@^2.0.8` and `@upstash/redis@^1.38.0` in Brief 4's package.json change. Confirmed that `@upstash/redis@1.38.0` falls within the peer range.
2. **`@upstash/redis@1.38.0` transitive surface verified.** Sole production dep: `uncrypto@^0.1.3` (a single-file polyfill for Node's `webcrypto` — pure-JS, ~50 SLOC). No conflict with the existing dep tree.
3. **vitest@4 vs vitest@3 peer-dep delta.** vitest@4.1.7 lists `vite` as a non-optional peer dep (range `^6 || ^7 || ^8`); vitest@3.2.4 lists `vite` as a regular dep (range `^5 || ^6 || ^7`). For a JS-only repo with no Vite plugins, v3.2.4 is strictly easier — no extra `vite` install, no peer-dep conflict. Brief 5 pins `vitest@^3.2.4`. Documented in Brief 5's acceptance criterion + rationale.
4. **vitest@3's vite dep has Node `^20.19 || >=22.12`.** Vercel CI's `setup-node@v4` with `node-version: '20'` resolves to latest 20.x patch (currently 20.19+); satisfies the requirement. Local-dev users on Node <20.19 will need to upgrade — already the recommendation per the bump-next-js retro.
5. **Caller audit of `getUserFromRequest`.** Architect ran `rg "getUserFromRequest" pages/api --type js -l` → 24 files. Sampled 22 of them with `rg "if \(!user\)" pages/api --type js -A 1` and confirmed all 22 use the `if (!user) return res.status(401)` pattern. The 23rd (`pages/api/community/collections.js`) and 24th (verified in spot-check above) use the same pattern. The one exception is `pages/api/collections/[identifier].js` which uses `user?.userId` optional-chaining — confirmed correct under the post-Brief-2 null return. **No caller code change is needed in this convoy.**
6. **JWT_SECRET literal sites confirmed: 7 files.** Matches AGENTS.md gotcha #3 exactly: `lib/permission-middleware.js`, `pages/api/auth-utils.js`, `pages/api/auth/login.js`, `pages/api/auth/register.js`, `pages/api/auth/verify.js`, `pages/api/favorites.js`, `pages/api/users/search.js`. Brief 1's grep verification will guarantee all 7 are converted.
7. **Dev endpoints have no runtime references.** `rg "/api/(simple|test-auth|test-db|setup-database)"` returns hits only in docs (`.cursor/rules/api-routes.mdc`, `AGENTS.md`, `.convoys/`, `README.md`) and one CODEOWNERS line. Safe to delete; the README line is also removed in Brief 3.
8. **Cross-brief commitments documented in both directions.** Brief 1 declares commitments to Briefs 2, 4, 5. Briefs 2, 4, 5 each declare reciprocal commitments back to Brief 1. Brief 5 also declares a commitment from Brief 2 (test coverage of Brief 2's null-return contract) and a coordination note from Brief 4 (lockfile sequencing). All round-trip.
9. **No verbatim code-shape mismatches found.** The proposed `lib/auth-secret.js`, `lib/rate-limit.js`, `vitest.config.js`, and CI YAML shapes were checked against the actual installed `package.json`, the existing CI workflow's `lint:` job style, and the `@upstash/ratelimit` README's verbatim `Ratelimit.slidingWindow(N, '<duration>')` API. No discrepancies.

No brief was revised during the Boot-the-brief pass — all proposed shapes survived first-contact verification.

## Convoy outcome

Closed out 2026-05-23 by `role-doc-writer` after the final brief merged to `main` (PR #11, commit `1629afb`). The convoy completed its declared success criteria: `getUserFromRequest` returns `null` for missing tokens, every API route on the auth surface refuses unauthenticated requests, login + register are rate-limited, and CI is green with a blocking `test:` job.

### What shipped

| Brief | Title | Merge commit | Files | Outcome |
| --- | --- | --- | --- | --- |
| 3 | Delete dev endpoints + CI guard | `fc0dd73` | 4 deletions in `pages/api/`; `.github/workflows/ci.yml` (new `forbidden-endpoints` job); `README.md` | Wave A, parallel with Brief 1. Resolves AGENTS.md Gotcha #5. |
| 1 | Central JWT secret helper + 24h TTL | `4a10dce` | `lib/auth-secret.js` (new); 7 sites of literal-fallback removal | Wave A. Resolves AGENTS.md Gotcha #3. |
| 2 | Remove synthetic-admin bypass | `258e479` | `lib/permission-middleware.js`, `pages/api/auth/verify.js` | Wave B, parallel with Brief 4. **Keystone fix** — resolves AGENTS.md Gotcha #2. |
| 4 | Tighten public auth surface (CORS + rate limit) | `297afca` | `lib/rate-limit.js` (new); `pages/api/auth/login.js`, `pages/api/auth/register.js`; `package.json` + `package-lock.json` | Wave B. Drops `Access-Control-Allow-Origin: '*'` from login + register; 5/15min sliding window via `@upstash/ratelimit`. |
| 6 (hotfix) | Cards-collection 401 guards | `1fca3aa` | `pages/api/collections/[identifier]/cards.js` | Out-of-plan follow-up to Brief 2 — three `if (!user) return 401` guards added to the POST/PUT/DELETE branches that previously relied on a 500 cascade. |
| 5 | Vitest harness + 16 auth unit tests + re-enable CI test job | `1629afb` | `vitest.config.js`, `test/setup.js`, `test/lib/*.test.js`, `test/api/*.test.js`; `package.json` + `package-lock.json`; `.github/workflows/ci.yml` | Wave C (last). Locks in Brief 1 + Brief 2 contracts. |

Total: 5 planned briefs + 1 hotfix, all merged across PRs #6–#11. The convoy file's "Decomposition" table predicted ~600 LOC; actuals were within budget on every brief.

### AGENTS.md gotchas resolved

- **#2 — `getUserFromRequest` synthetic-admin fallback** → resolved by Brief 2 (`258e479`). Converted to a "Resolved" note in `AGENTS.md` (not renumbered) so cross-references stay valid.
- **#3 — JWT_SECRET hardcoded across 7 files** → resolved by Brief 1 (`4a10dce`). Converted to a "Resolved" note in `AGENTS.md`.
- **#5 — `pages/api/setup-database.js` public endpoint** → resolved by Brief 3 (`fc0dd73`). Converted to a "Resolved" note in `AGENTS.md`; the CI `forbidden-endpoints` job prevents regression.

Gotchas #1, #4, #6, #7, #8, #9, #10, #11 are unchanged (see those entries in `AGENTS.md` for status). A new Gotcha #12 was added covering the `KV_REST_API_*` env-var convention (see "Mid-flight deviation" below).

### Mid-flight deviation: env-var rename

The convoy plan and Brief 4 originally specified `UPSTASH_REDIS_REST_URL` / `UPSTASH_REDIS_REST_TOKEN` — the names `@upstash/redis`'s generic README example uses. Mid-implementation, the Brief 4 implementer surfaced that the project already runs on Vercel's Upstash Marketplace integration, which auto-provisions Redis credentials under `KV_REST_API_URL` / `KV_REST_API_TOKEN`. A parent-agent interrupt approved the rename ("ship what Vercel hands you"), and the implementer continued with the Marketplace-native names.

Net effect: zero manual env-var paste step on any environment; cleanup of three otherwise-redundant `KV_*` shadows that would have pointed at the same Upstash instance under different keys. See `.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md` § "Post-merge addendum" for the full record.

### Risk-list status (R1–R12)

Walking each risk from § Architecture → Risk list:

| Risk | Status | Note |
| --- | --- | --- |
| R1 — `JWT_SECRET` fail-loud breaks unexpected importers | **Addressed** | Brief 1 verified `next dev` + `next build` both clear with `JWT_SECRET` set; Brief 5's `test/setup.js` sets it before any auth import. No regression observed. |
| R2 — Caller relies on synthetic-admin fallback | **Addressed** | All 24 `getUserFromRequest` callers verified to handle `null` correctly (23 use `if (!user) return 401`; one uses `user?.userId` optional-chaining). Plus the Brief 6 hotfix added explicit null guards to the cards-collection routes that were silently 500'ing. |
| R3 — Existing tokens stop verifying | **Addressed (accepted)** | One-time "you'll need to log back in" is the expected outcome. Acceptable pre-launch. No graceful migration was offered (it would have meant accepting both secrets, which is the bypass we removed). |
| R4 — Token TTL drift `7d` → `24h` | **Addressed** | Codified as `JWT_TOKEN_TTL = '24h'` in `lib/auth-secret.js`; no user impact because the `7d` site (`auth-utils.generateToken`) wasn't in the call path pre-fix. |
| R5 — Rate-limit picks wrong identifier on Vercel | **Addressed** | First-hop XFF extraction implemented per the spec; Brief 4's smoke confirmed real-Vercel preview behavior. |
| R6 — Upstash quota exhaustion | **Partial** | `lib/rate-limit.js` fail-opens on Upstash error per spec. Quota monitoring + defense-in-depth (Vercel firewall) is **deferred** to a future hardening pass. |
| R7 — `package.json` / `package-lock.json` merge conflicts | **Addressed** | Wave-C sequencing (Brief 5 after Brief 4) prevented any actual conflict. |
| R8 — `@upstash/ratelimit` transitive surprises | **Addressed** | `npm install` clean; `npm ls @upstash/ratelimit` and `npm ls @upstash/redis` each report a single version. No `ERESOLVE`. |
| R9 — `vitest@3.2.4` Node engines constraint | **Addressed** | Vercel CI Node 20.x satisfies `^20.19 || >=22.12`. Local dev requirement (Node 20.19+) was already documented in the `bump-next-js` retro. |
| R10 — `JWT_SECRET` rotation now requires a deploy | **Addressed (accepted)** | Documented in Brief 1's pre-deploy checklist + `AGENTS.md` § 1. The fail-loud throw is the desired property. |
| R11 — Test-mock drift if SQL client migrates | **Deferred** | Documented in Brief 5's acceptance criteria; the future `single-sql-client` convoy must update `test/lib/permission-middleware.test.js`'s `@vercel/postgres` mock. |
| R12 — CI guard misses a renamed dev endpoint | **Partial (accepted)** | Intentional — `forbidden-endpoints` is a regression-prevention belt for the four known files, not a general "no dev endpoints" policy. A stricter check is deferred to a future hardening pass. |

No risks fired during the convoy. The closest call was R2 — the cards-collection routes (`pages/api/collections/[identifier]/cards.js` POST/PUT/DELETE) had a pre-existing reliance on the synthetic admin that surfaced as 500s after Brief 2 instead of clean 401s. The Brief 6 hotfix addressed it in `1fca3aa`, before the convoy closed.

### Follow-up convoys queued

These are tracked here (rather than only in `.convoys/ship-readiness.md`) because they are direct continuations of the auth surface the convoy hardened:

- **`expand-auth-tests`** — rate-limit unit tests (mock `@upstash/redis`), login/register integration tests covering CORS-gone + 429 path, broader Vitest coverage of the `withCollectionPermission` wrapper. Brief 5 explicitly deferred these.
- **`cors-tighten`** — `pages/api/auth/verify.js` still ships `Access-Control-Allow-Origin: '*'` (Brief 4 was scoped to login + register only, per § Architecture). Also covers any other CORS-`*` sites elsewhere in `pages/api/`.
- **`drop-public-setup`** — closes Gotcha #4 (default admin credentials in seed) plus any residual README copy on admin bootstrap.
- **`fix-layout-default-user`** — closes Gotcha #8 (Layout default user impersonates maintainer).
- **`single-sql-client`** — closes Gotcha #1 (two SQL clients in parallel). Deferred from this convoy's scope ("Out") per the original plan.
- **`migration-tool`** — closes Gotcha #6 (bare migration scripts). Unscoped, still open.
- **`dual-is-public`** — closes Gotcha #7 (dual `is_public` semantics on collections + decks). Unscoped, still open.

The full launch sequence remains as enumerated in `.convoys/ship-readiness.md` § "Proposed launch sequence" — `fix-auth-bypass` is step 1; the follow-ups above are interleaved across steps 2–7.

### Lessons learned

- **Worktree-based parallel implementer dispatch worked well.** Wave A (Briefs 1 + 3) and Wave B (Briefs 2 + 4) each ran two implementers concurrently in separate worktrees with zero merge conflict. The architect's `slice_dependencies` table was an unusually accurate guide — the `files:` sets really were disjoint within each wave. Worth keeping as the default decomposition discipline.
- **Mid-flight interrupt for the env-var rename was the right call.** Stopping the Brief 4 implementer, raising the discrepancy to the parent agent, and continuing with the corrected names cost ~5 minutes and avoided shipping a duplicate env-var pair plus a Vercel-onboarding wiki page. The convoy's audit trail (this section + the brief's "Post-merge addendum") preserves the reasoning for future agents.
- **Per-brief PRs gave better review density than the single-PR pattern from `bump-next-js`.** Each PR was small enough that human review fit in one sitting (~10–20 min), and a single brief's diff was easy to reason about in isolation. The trade-off (5+ PRs instead of 1) was worth it for a security-critical convoy where review attention is the limiting factor.