a8bab93270
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 <cursoragent@cursor.com>
101 lines
8.2 KiB
Markdown
101 lines
8.2 KiB
Markdown
---
|
||
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-<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.
|