deckhearth/.convoys/fix-auth-bypass.md
Randall Stillwell c7ad0ffa29 docs: post-convoy cleanup for fix-auth-bypass
Closes out the fix-auth-bypass convoy (PRs #6–#11, merged through
1629afb) on the docs side. Code already on main; this PR is docs only.

Updates:

AGENTS.md
  - §1 auth bullet refreshed (auth-secret SoT, 24h TTL, no synthetic
    admin, login/register rate limit)
  - §3 conventions point at lib/auth-secret.js + lib/rate-limit.js
  - §4 gotchas #2/#3/#5 converted to "Resolved" notes in place
    (NOT renumbered, to preserve cross-references)
  - new #12 documents the KV_REST_API_* env-var convention
  - §5 setup list adds the rate-limit env vars
  - §6 testing rewritten for Vitest (16 unit tests, blocking CI gate)

.cursor/rules/auth-and-permissions.mdc
  - canonical-surface table gains lib/auth-secret.js + lib/rate-limit.js
  - token model now 24h (was 7d) with fail-loud explanation
  - server-side authorization patterns lead with null → 401 contract

.cursor/rules/api-routes.mdc
  - removes the "CRITICAL — known bug" callout (resolved by Brief 2)
  - adds a "Rate limiting" section with verbatim shape + env-var notes
  - "Dev/test endpoints" → "Removed" historical note so future agents
    searching for test-db understand why it's gone

.convoys/fix-auth-bypass.md (restored — was on convoy branch only)
  - frontmatter → status: shipped
  - new "Convoy outcome" section: briefs + commits + resolved gotchas,
    R1-R12 risk walk, env-var-rename deviation record, queued follow-up
    convoys, lessons learned

.convoys/fix-auth-bypass/brief-{1..5}-*.md (restored from convoy branch)
  - audit-trail completeness; convoy plan references them by name
  - brief 4 additionally updated: UPSTASH_REDIS_REST_* → KV_REST_API_*
    across init rules, smoke, pre-deploy checklist
  - brief 4 has a new "Post-merge addendum" explaining the rename

.convoys/ship-readiness.md
  - P0 #1, #2, #4 → RESOLVED with merge-commit citations
  - P0 #5 (CORS), #6 (rate limit) → PARTIAL with deferral pointers
    (cors-tighten and add-rate-limiting convoys)
  - each item gains an "As-shipped" line for self-containment

README.md
  - Next.js 15 → 16, TypeScript claim corrected to JS-with-devDep
  - auth + rate-limit + testing bullets updated
  - env-var template extended with KV_REST_API_*
  - deleted dev-endpoints note added to the API list
  - "Default Admin Account" section LEFT ALONE — drop-public-setup territory

Verified: build exit 0 (with JWT_SECRET set), 16/16 vitest tests pass,
lint baseline unchanged (128/81/47).

Convoy: fix-auth-bypass / role-doc-writer (closeout)
Co-authored-by: Cursor <cursoragent@cursor.com>
2026-05-23 12:22:50 -05:00

38 KiB
Raw Blame History

name classification success_metric skip status created shipped
fix-auth-bypass server-only getUserFromRequest returns null for missing tokens; no API route accepts unauthenticated requests; CI green.
ia
ux
visual
a11y
design
shipped 2026-05-22 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 46 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.signgenerateToken. 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.020.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)

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:

/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:

/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 (R1R12)

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 7d24h 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
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-tightenpages/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 27.

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 (~1020 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.