From a8bab93270b46f1e4812593ded1dd30e21f411c1 Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Fri, 22 May 2026 23:19:03 -0500 Subject: [PATCH] convoy: scope fix-auth-bypass (P0 #1, #2, #4, #5, #6 partial) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Conductor output for the first real convoy after the agent-pipeline bootstrap. Closes P0 ship-blockers #1, #2, #4, #5 and the auth-route slice of #6 from .convoys/ship-readiness.md. Classification: server-only Skip: ia, ux, visual, a11y, design Next role: role-architect The convoy is unflagged (auth fixes don't ship behind a feature flag). Architect produces a slice plan with explicit slice_dependencies so implementers can /multitask the disjoint briefs (dev-endpoint delete + CORS tighten + rate-limit wiring) once the central JWT secret helper lands. Out of scope here (own convoys): - P0 #3 default admin creds + README → drop-public-setup - P0 #7 Layout default-prop email leak → fix-layout-default-user - P0 #6 full (search/import/upload) → add-rate-limiting Convoy file: .convoys/fix-auth-bypass.md Analytics: emitted via scripts/log-convoy-event.sh Co-authored-by: Cursor --- .convoys/fix-auth-bypass.md | 101 ++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 .convoys/fix-auth-bypass.md diff --git a/.convoys/fix-auth-bypass.md b/.convoys/fix-auth-bypass.md new file mode 100644 index 0000000..e3f6945 --- /dev/null +++ b/.convoys/fix-auth-bypass.md @@ -0,0 +1,101 @@ +--- +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: open +created: 2026-05-22 +--- + +# 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-`: `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.