docs: post-convoy cleanup for fix-auth-bypass #12
11 changed files with 1283 additions and 25 deletions
375
.convoys/fix-auth-bypass.md
Normal file
375
.convoys/fix-auth-bypass.md
Normal file
|
|
@ -0,0 +1,375 @@
|
|||
---
|
||||
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.
|
||||
179
.convoys/fix-auth-bypass/brief-1-central-jwt-secret-helper.md
Normal file
179
.convoys/fix-auth-bypass/brief-1-central-jwt-secret-helper.md
Normal file
|
|
@ -0,0 +1,179 @@
|
|||
---
|
||||
convoy: fix-auth-bypass
|
||||
brief_number: 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
|
||||
cross_brief_commitments:
|
||||
- brief: 2
|
||||
description: |
|
||||
Brief 2 modifies `lib/permission-middleware.js` (replaces the synthetic-admin
|
||||
fallback in `getUserFromRequest`) and `pages/api/auth/verify.js` (removes the
|
||||
no-token admin-fetch branch). This brief MUST land first, because Brief 2
|
||||
relies on the `JWT_SECRET` import already being in place.
|
||||
- brief: 4
|
||||
description: |
|
||||
Brief 4 modifies `pages/api/auth/login.js` and `pages/api/auth/register.js`
|
||||
(drops `Access-Control-Allow-Origin: '*'`, wraps with rate limiter). This
|
||||
brief MUST land first, because Brief 4 builds on the post-refactor login /
|
||||
register handlers (no `JWT_SECRET` literal, `generateToken` from `auth-utils`).
|
||||
- brief: 5
|
||||
description: |
|
||||
Brief 5 (vitest + tests) imports `JWT_SECRET` and `JWT_TOKEN_TTL` from
|
||||
`lib/auth-secret.js` in test setup. This brief MUST land first.
|
||||
---
|
||||
|
||||
# Brief 1: Central JWT secret helper + 24h token TTL
|
||||
|
||||
## Goal (1 sentence)
|
||||
|
||||
Create `lib/auth-secret.js` as the single source of truth for `JWT_SECRET` (fail-loud at module load if unset) and `JWT_TOKEN_TTL = '24h'`, then refactor the 7 files currently embedding `process.env.JWT_SECRET || '…'` literals to import from it.
|
||||
|
||||
## Files in scope (do not edit anything else)
|
||||
|
||||
- `lib/auth-secret.js` — **new**
|
||||
- `lib/permission-middleware.js` — modified (literal → import)
|
||||
- `pages/api/auth-utils.js` — modified (literal → import; `'7d'` → `JWT_TOKEN_TTL`)
|
||||
- `pages/api/auth/login.js` — modified (literal → import; inline `jwt.sign(...)` → `generateToken(user)` from `auth-utils`; drop now-unused `jwt` import)
|
||||
- `pages/api/auth/register.js` — modified (same as login)
|
||||
- `pages/api/auth/verify.js` — modified (literal → import). **Do NOT remove the no-token admin-fetch branch here** — that's Brief 2's scope. Just swap the secret literal for the import.
|
||||
- `pages/api/favorites.js` — modified (literal → import)
|
||||
- `pages/api/users/search.js` — modified (literal → import)
|
||||
|
||||
## Conventions to follow
|
||||
|
||||
- `.cursor/rules/auth-and-permissions.mdc` § "Token model" — JWT model + signing surface.
|
||||
- `.cursor/rules/api-routes.mdc` § "Authentication & Authorization" — handler shape stays the same; only the secret source changes.
|
||||
- `.cursor/rules/no-go-zones.mdc` — do not edit any file outside `files:` above. In particular: no edits to `lib/auth-context.js`, `lib/admin-auth.js`, `lib/use-auth.js`, `lib/database.js`, `pages/_app.js`, or any UI file. Auth-context cleanup is the future `single-auth-provider` convoy.
|
||||
- `package.json` formatting: 2-space indent, `"type": "module"` is set — use ES module imports throughout.
|
||||
- Existing `import` style in `pages/api/auth-utils.js`: relative paths, no aliases. Match.
|
||||
- No `engines` block change.
|
||||
- No new dependencies in `package.json`. (Brief 4 adds `@upstash/ratelimit`; Brief 5 adds `vitest`. This brief adds nothing.)
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
### `lib/auth-secret.js` (new)
|
||||
|
||||
- [ ] File contains exactly two named exports: `JWT_SECRET` and `JWT_TOKEN_TTL`.
|
||||
- [ ] `JWT_SECRET` reads `process.env.JWT_SECRET`. If unset OR empty string, the module **throws at import time** with a clear, actionable message that names the env var and points at `.env.local`. Verbatim shape (or near-verbatim — the message body can be reworded but the shape must be):
|
||||
|
||||
```js
|
||||
const JWT_SECRET = process.env.JWT_SECRET;
|
||||
|
||||
if (!JWT_SECRET) {
|
||||
throw new Error(
|
||||
'JWT_SECRET environment variable is not set. ' +
|
||||
'Set it in .env.local for local dev, or in the Vercel project settings for deploys. ' +
|
||||
'Generate a strong secret with: openssl rand -hex 32'
|
||||
);
|
||||
}
|
||||
|
||||
export { JWT_SECRET };
|
||||
export const JWT_TOKEN_TTL = '24h';
|
||||
```
|
||||
|
||||
- [ ] **No fallback string literal.** A previous fallback `'your-secret-key-change-in-production'` is what we are explicitly removing — do not reintroduce it under any condition.
|
||||
- [ ] No length check (a length check is tempting but not required by the convoy and risks breaking existing valid-but-shorter dev secrets in `.env.local`; defer to a future hardening pass).
|
||||
- [ ] No default export.
|
||||
- [ ] No top-level side effects beyond the throw on missing env (no `console.log`, no `dotenv.config()` — Next.js loads `.env.local` automatically, and tests load env via `test/setup.js` in Brief 5).
|
||||
|
||||
### `pages/api/auth-utils.js`
|
||||
|
||||
- [ ] Line 4 (`const JWT_SECRET = process.env.JWT_SECRET || 'your-secret-key';`) **deleted**.
|
||||
- [ ] Add at top of file: `import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js';`
|
||||
- [ ] `generateToken(user)` returns `jwt.sign({...}, JWT_SECRET, { expiresIn: JWT_TOKEN_TTL })` — the literal `'7d'` is replaced. **This is the canonical token-minting function.**
|
||||
- [ ] `verifyToken(token)` continues to call `jwt.verify(token, JWT_SECRET)` (no expiry param needed on verify).
|
||||
- [ ] No other behavior change. `hashPassword`, `verifyPassword`, `isAdmin`, `getUserById` are untouched.
|
||||
|
||||
### `pages/api/auth/login.js`
|
||||
|
||||
- [ ] Line 5 (`const JWT_SECRET = process.env.JWT_SECRET || 'your-secret-key-change-in-production';`) **deleted**.
|
||||
- [ ] Replace `import jwt from 'jsonwebtoken';` (line 2) with `import { generateToken } from '../../auth-utils.js';`. The path is `pages/api/auth/login.js` → `pages/api/auth-utils.js`, so relative import is `../auth-utils.js`. Verify by reading line 4 of `pages/api/auth/register.js` for the existing relative-import pattern (`'../../../lib/slug-utils.js'`).
|
||||
- [ ] Replace the inline JWT mint:
|
||||
|
||||
```js
|
||||
// before (lines 51-55)
|
||||
const token = jwt.sign(
|
||||
{ userId: user.id, email: user.email, role: user.role },
|
||||
JWT_SECRET,
|
||||
{ expiresIn: '24h' }
|
||||
);
|
||||
|
||||
// after
|
||||
const token = generateToken({ id: user.id, email: user.email, role: user.role });
|
||||
```
|
||||
|
||||
Note the param shape change: `generateToken` reads `user.id` (not `user.userId`), per the existing implementation in `auth-utils.js`.
|
||||
- [ ] **No CORS change here.** Brief 4 will tighten `Access-Control-Allow-Origin: '*'`. Leave it alone in this brief.
|
||||
- [ ] **No rate-limit wiring here.** Brief 4 wraps with `@upstash/ratelimit`. Leave the handler shape alone.
|
||||
|
||||
### `pages/api/auth/register.js`
|
||||
|
||||
- [ ] Line 6 (`const JWT_SECRET = process.env.JWT_SECRET || 'your-secret-key-change-in-production';`) **deleted**.
|
||||
- [ ] Replace `import jwt from 'jsonwebtoken';` with `import { generateToken } from '../auth-utils.js';`. Relative path: `pages/api/auth/register.js` → `pages/api/auth-utils.js` is `'../auth-utils.js'`.
|
||||
- [ ] Replace the inline JWT mint at lines 142-147 with `const token = generateToken({ id: user.id, email: user.email, role: user.role });`
|
||||
- [ ] Same CORS / rate-limit hands-off rule as login.
|
||||
|
||||
### `pages/api/auth/verify.js`
|
||||
|
||||
- [ ] Line 4 literal **deleted**.
|
||||
- [ ] Add `import { JWT_SECRET } from '../../../lib/auth-secret.js';` at top. Path correctness: `pages/api/auth/verify.js` → `lib/auth-secret.js` is `'../../../lib/auth-secret.js'`.
|
||||
- [ ] Keep `jwt.verify(token, JWT_SECRET)` inline (do not refactor to call `verifyToken` from `auth-utils.js` — that would change error semantics, and Brief 2 is already going to touch this file. Keep this brief mechanical).
|
||||
- [ ] **Do NOT remove the no-token admin-fetch branch (lines 25-39).** That is Brief 2's job. Touching it here splits the security fix across two PRs unnecessarily.
|
||||
|
||||
### `pages/api/favorites.js`
|
||||
|
||||
- [ ] Line 4 literal **deleted**.
|
||||
- [ ] Add `import { JWT_SECRET } from '../../lib/auth-secret.js';` at top. Path: `pages/api/favorites.js` → `lib/auth-secret.js` is `'../../lib/auth-secret.js'`.
|
||||
- [ ] Keep `jwt.verify(token, JWT_SECRET)` inline. No other change.
|
||||
|
||||
### `pages/api/users/search.js`
|
||||
|
||||
- [ ] Line 4 literal **deleted**.
|
||||
- [ ] Add `import { JWT_SECRET } from '../../../lib/auth-secret.js';` at top. Path: `pages/api/users/search.js` → `lib/auth-secret.js` is `'../../../lib/auth-secret.js'`.
|
||||
- [ ] Keep `jwt.verify(token, JWT_SECRET)` inline. No other change.
|
||||
|
||||
### `lib/permission-middleware.js`
|
||||
|
||||
- [ ] Line 4 literal **deleted**.
|
||||
- [ ] Add `import { JWT_SECRET } from './auth-secret.js';` at top.
|
||||
- [ ] **Keep the rest of `getUserFromRequest` unchanged in this brief.** The synthetic-admin fallback removal is Brief 2's job.
|
||||
- [ ] `withCollectionPermission`, `checkCollectionPermission`, `logCollectionActivity` are untouched.
|
||||
|
||||
### Repo-wide grep verification (run before opening PR)
|
||||
|
||||
- [ ] `rg "process\.env\.JWT_SECRET" --type js` returns **zero hits** in `lib/`, `pages/`. (Hits in `.convoys/`, `.cursor/`, `AGENTS.md`, `docs/` are documentation references — leave them alone in this brief.)
|
||||
- [ ] `rg "your-secret-key" --type js` returns zero hits.
|
||||
- [ ] `rg "'7d'" --type js pages/api/auth-utils.js` returns zero hits (replaced by `JWT_TOKEN_TTL`).
|
||||
- [ ] `rg "'24h'" --type js pages/api/auth/` returns zero hits (replaced via `generateToken`).
|
||||
|
||||
### Smoke (manual, no test runner yet — Brief 5 adds vitest)
|
||||
|
||||
Document that you ran these in the PR description (not enforced in CI):
|
||||
|
||||
- [ ] `npm run lint` exits 0 (or matches the existing baseline — pre-existing errors are fine, no new ones).
|
||||
- [ ] `npm run dev` boots; visit `http://localhost:3000/login`; submit valid credentials; observe that `localStorage.auth_token` is set and decoding the token shows `exp - iat ≈ 86400` (24h, not 7 days).
|
||||
- [ ] Temporarily unset `JWT_SECRET` in `.env.local` and run `npm run dev`. Confirm the server logs the thrown error and the page returns 500. **Re-set `JWT_SECRET` before opening the PR.**
|
||||
- [ ] `npm run build` succeeds. Vercel's preview deploy on the PR is green.
|
||||
|
||||
### Out of scope (do not do these)
|
||||
|
||||
- [ ] No edit to `pages/_app.js`, `lib/auth-context.js`, `lib/admin-auth.js`, `lib/use-auth.js`. Client-side context cleanup is the future `single-auth-provider` convoy.
|
||||
- [ ] No edit to `AGENTS.md` or `.cursor/rules/auth-and-permissions.mdc`. Doc-writer pass updates these after the convoy lands.
|
||||
- [ ] No removal of the synthetic-admin fallback in `getUserFromRequest` — Brief 2.
|
||||
- [ ] No removal of the no-token admin branch in `verify.js` — Brief 2.
|
||||
- [ ] No CORS changes — Brief 4.
|
||||
- [ ] No rate-limit wiring — Brief 4.
|
||||
- [ ] No test files — Brief 5.
|
||||
- [ ] No deletion of `pages/api/test-*.js`, `pages/api/simple.js`, `pages/api/setup-database.js` — Brief 3.
|
||||
|
||||
## Rationale (≤3 sentences)
|
||||
|
||||
Centralizing `JWT_SECRET` removes 7 copies of the fallback literal in one PR, making the eventual fail-closed runtime behavior trivial to audit. Co-locating `JWT_TOKEN_TTL` in the same module canonicalizes 24h (matching current `login.js` behavior, which is what existing users have been getting) and resolves the silent inconsistency between `auth-utils.generateToken` (`'7d'`) and `login.js` (`'24h'`). Routing `login.js` and `register.js` through `auth-utils.generateToken` removes a second, drift-prone JWT-mint call site; the alternative — leaving inline `jwt.sign` everywhere — would make the next refactor more painful for no gain.
|
||||
131
.convoys/fix-auth-bypass/brief-2-remove-admin-bypass.md
Normal file
131
.convoys/fix-auth-bypass/brief-2-remove-admin-bypass.md
Normal file
|
|
@ -0,0 +1,131 @@
|
|||
---
|
||||
convoy: fix-auth-bypass
|
||||
brief_number: 2
|
||||
depends_on: [1]
|
||||
files:
|
||||
- lib/permission-middleware.js
|
||||
- pages/api/auth/verify.js
|
||||
cross_brief_commitments:
|
||||
- brief: 1
|
||||
description: |
|
||||
Brief 1 already replaced the `JWT_SECRET` literal in both files with imports
|
||||
from `lib/auth-secret.js`. This brief preserves those imports and only
|
||||
removes the synthetic-admin fallback shapes.
|
||||
- brief: 5
|
||||
description: |
|
||||
Brief 5 (vitest) writes the unit tests that prove `getUserFromRequest`
|
||||
returns `null` for the four shapes (missing header, malformed token,
|
||||
expired token, valid token-but-no-user-row). The behavior is implemented
|
||||
here; the harness lands in Brief 5.
|
||||
---
|
||||
|
||||
# Brief 2: Remove the synthetic-admin bypass
|
||||
|
||||
## Goal (1 sentence)
|
||||
|
||||
Make `lib/permission-middleware.js::getUserFromRequest` return `null` for any unauthenticated request, and make `pages/api/auth/verify.js` return 401 instead of fetching `admin@tcgvault.com` when no Bearer token is present.
|
||||
|
||||
## Files in scope (do not edit anything else)
|
||||
|
||||
- `lib/permission-middleware.js` — modified
|
||||
- `pages/api/auth/verify.js` — modified
|
||||
|
||||
## Conventions to follow
|
||||
|
||||
- `.cursor/rules/auth-and-permissions.mdc` § "Server-side authorization patterns" — `if (!user) return res.status(401)` pattern. The 24 callers of `getUserFromRequest` already follow this; we just need to make the helper actually emit `null`.
|
||||
- `.cursor/rules/api-routes.mdc` § "Error handling" — keep the `try/catch` wrapper in place; do not throw out of the handler.
|
||||
- `.cursor/rules/no-go-zones.mdc` — do not touch any other file.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
### `lib/permission-middleware.js::getUserFromRequest`
|
||||
|
||||
- [ ] **Delete lines 14-17** of the post-Brief-1 file (the `console.warn` and the synthetic admin return). Replace with a plain `return null`. Verbatim shape:
|
||||
|
||||
```js
|
||||
// before (post-Brief-1, with literal already gone):
|
||||
if (!authHeader || !authHeader.startsWith('Bearer ')) {
|
||||
// For development, return user ID 1 if no token (should be removed in production)
|
||||
console.warn('⚠️ Development mode: Using fallback user authentication');
|
||||
return { userId: 1, email: 'admin@tcgvault.com', role: 'admin' };
|
||||
}
|
||||
|
||||
// after:
|
||||
if (!authHeader || !authHeader.startsWith('Bearer ')) {
|
||||
return null;
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] No `console.warn`. No comment-out. No env-gate (`NODE_ENV === 'development'`). The fallback is gone, period. If a developer needs an authenticated session locally, they log in.
|
||||
- [ ] The rest of `getUserFromRequest` (token verify, DB lookup, error catch) is unchanged.
|
||||
- [ ] The catch block at lines 38-41 stays:
|
||||
|
||||
```js
|
||||
} catch (error) {
|
||||
console.error('Error getting user from request:', error);
|
||||
return null;
|
||||
}
|
||||
```
|
||||
|
||||
This means JWT verification errors (expired, malformed, bad signature) AND DB errors all collapse to `null`. The 401 vs 500 distinction is left to callers (currently every caller treats `null` as 401, which is correct for an auth helper).
|
||||
|
||||
- [ ] No change to `checkCollectionPermission`, `withCollectionPermission`, `checkRolePermission`, or `logCollectionActivity`.
|
||||
|
||||
### `pages/api/auth/verify.js`
|
||||
|
||||
- [ ] **Delete lines 25-39** of the post-Brief-1 file (the `// For development, return admin user if no token provided` block and the `SELECT … WHERE email = 'admin@tcgvault.com'` query). Replace with an immediate 401:
|
||||
|
||||
```js
|
||||
// before:
|
||||
if (!authHeader || !authHeader.startsWith('Bearer ')) {
|
||||
// For development, return admin user if no token provided
|
||||
// In production, this should return 401
|
||||
const result = await sql`
|
||||
SELECT id, email, role, created_at
|
||||
FROM users
|
||||
WHERE email = 'admin@tcgvault.com'
|
||||
`;
|
||||
|
||||
if (result.rows.length > 0) {
|
||||
return res.status(200).json(result.rows[0]);
|
||||
} else {
|
||||
return res.status(401).json({ error: 'No admin user found' });
|
||||
}
|
||||
}
|
||||
|
||||
// after:
|
||||
if (!authHeader || !authHeader.startsWith('Bearer ')) {
|
||||
return res.status(401).json({ error: 'Authentication required' });
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] No env-gate. No comment-out.
|
||||
- [ ] The rest of `verify.js` (CORS headers, OPTIONS preflight, method gate, JWT verify, DB lookup) is unchanged. CORS tightening is Brief 4's scope and only covers `login.js` + `register.js`, NOT `verify.js` (out of scope per the convoy).
|
||||
- [ ] The error-message wording matches the existing convention: `{ error: 'Authentication required' }`. Do not invent a new shape.
|
||||
|
||||
### Caller spot-check (do this before opening the PR)
|
||||
|
||||
The convoy claims "30+ handlers depend on `getUserFromRequest`." Re-verify by running the following (results captured at architect time on 2026-05-23 — `master` revision `ebd4fd1`; if the count drifts, list the new files in the PR description):
|
||||
|
||||
- [ ] `rg "getUserFromRequest" pages/api --type js -l | wc -l` → 24 files (one of which is `permission-middleware.js`'s import-bookkeeping artifact, leave the count as-is).
|
||||
- [ ] `rg "if \(!user\)" pages/api --type js -A 1` (with `-A 1`) — every match must be followed by `return res.status(401).json({ error: 'Authentication required' });` or a similar 401. If any caller has a different shape (e.g. `if (!user) return res.status(403)`, or `if (user) ...` inverted, or no null guard at all), **stop and re-architect**: that caller would need behavioral changes, and this convoy explicitly does not touch caller code.
|
||||
- [ ] One file is known to use optional-chaining instead of an early 401 — `pages/api/collections/[identifier].js` uses `user?.userId` because it allows anonymous access to public collections. **This is intentional** and stays correct under the fix (when `user` is `null`, `user?.userId` is `undefined`, the public-collection branch still works). Do not "fix" it.
|
||||
|
||||
### Smoke (manual)
|
||||
|
||||
- [ ] `npm run dev`; with no `Authorization` header, hit `curl http://localhost:3000/api/user/profile` → expect HTTP 401 with body `{"error":"Authentication required"}`. (Pre-fix: returns the admin user's profile.)
|
||||
- [ ] Same with `curl http://localhost:3000/api/auth/verify` → expect HTTP 401. (Pre-fix: returns admin user data.)
|
||||
- [ ] Log in via the UI; observe the dashboard loads (the helper still works for valid tokens).
|
||||
- [ ] Log out; observe the dashboard redirects to `/login` (the helper now correctly returns `null`).
|
||||
|
||||
### Out of scope
|
||||
|
||||
- [ ] No edits to any of the 24 callers — they already handle `null` correctly.
|
||||
- [ ] No CORS changes (Brief 4).
|
||||
- [ ] No rate-limit (Brief 4).
|
||||
- [ ] No tests — Brief 5 ships them.
|
||||
- [ ] No `AGENTS.md` / `.cursor/rules/*.mdc` updates — doc-writer pass.
|
||||
|
||||
## Rationale (≤3 sentences)
|
||||
|
||||
This is the convoy's actual security fix — removing the synthetic admin makes 24 currently-broken handlers correct in one ~6-line change. Folding `verify.js`'s parallel bug (the no-token branch fetches `admin@tcgvault.com` directly from the DB) into the same brief keeps "the auth helper returns null" and "the verify endpoint returns 401" coupled, since both have to land before any unauthenticated request can be safely served. Splitting them risks a deploy ordering where one is fixed and the other isn't — exactly the inconsistency that lets a P0 ship-blocker survive.
|
||||
125
.convoys/fix-auth-bypass/brief-3-delete-dev-endpoints.md
Normal file
125
.convoys/fix-auth-bypass/brief-3-delete-dev-endpoints.md
Normal file
|
|
@ -0,0 +1,125 @@
|
|||
---
|
||||
convoy: fix-auth-bypass
|
||||
brief_number: 3
|
||||
depends_on: []
|
||||
files:
|
||||
- .github/workflows/ci.yml
|
||||
- README.md
|
||||
deletes:
|
||||
- pages/api/simple.js
|
||||
- pages/api/test-auth.js
|
||||
- pages/api/test-db.js
|
||||
- pages/api/setup-database.js
|
||||
---
|
||||
|
||||
# Brief 3: Delete dev-only API endpoints + add CI guard
|
||||
|
||||
## Goal (1 sentence)
|
||||
|
||||
Delete the four unauthenticated dev endpoints currently shipped to prod (`/api/simple`, `/api/test-auth`, `/api/test-db`, `/api/setup-database`) and add a CI grep step that fails the build if anyone re-introduces them.
|
||||
|
||||
## Files in scope (do not edit anything else)
|
||||
|
||||
- `pages/api/simple.js` — **deleted**
|
||||
- `pages/api/test-auth.js` — **deleted**
|
||||
- `pages/api/test-db.js` — **deleted**
|
||||
- `pages/api/setup-database.js` — **deleted**
|
||||
- `.github/workflows/ci.yml` — modified (new job)
|
||||
- `README.md` — modified (one-line removal)
|
||||
|
||||
## Conventions to follow
|
||||
|
||||
- `.cursor/rules/api-routes.mdc` § "Dev/test endpoints" — these files are explicitly called out as dev-only and slated for deletion. This brief executes that.
|
||||
- `.cursor/rules/no-go-zones.mdc` — none of these four files appear in the no-go list (they are not in `scripts/add-*` or any "append-only / historical" set). They are explicitly listed in the api-routes rule as "should be deleted."
|
||||
- `.github/workflows/ci.yml` formatting: 2-space indent, jobs go under the existing `jobs:` map, match the style of `lint:` and `schema-map-fresh:`.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
### Deletions
|
||||
|
||||
- [ ] `pages/api/simple.js` removed via `git rm`.
|
||||
- [ ] `pages/api/test-auth.js` removed via `git rm`.
|
||||
- [ ] `pages/api/test-db.js` removed via `git rm`.
|
||||
- [ ] `pages/api/setup-database.js` removed via `git rm`.
|
||||
- [ ] No grep hits for any of these paths anywhere in `pages/`, `components/`, `lib/`, or `scripts/`. Run before the PR:
|
||||
|
||||
```bash
|
||||
rg "/api/(simple|test-auth|test-db|setup-database)" --type js
|
||||
rg "(setup-database|test-auth|test-db|api/simple)" pages components lib scripts
|
||||
```
|
||||
|
||||
Expected: zero hits in source. Doc references in `.cursor/rules/api-routes.mdc`, `AGENTS.md`, `.convoys/`, `docs/` are out of scope (doc-writer cleans them up later).
|
||||
|
||||
### `README.md`
|
||||
|
||||
- [ ] Remove the line `- \`GET /api/test-db\` - Database connection test` (currently line 79). If the surrounding API list is short and now incomplete, leave it as-is — the doc-writer pass will rewrite that section.
|
||||
|
||||
### `.github/workflows/ci.yml`
|
||||
|
||||
- [ ] Add a new job `forbidden-endpoints` after `schema-map-fresh:`. Verbatim shape:
|
||||
|
||||
```yaml
|
||||
forbidden-endpoints:
|
||||
name: No dev endpoints in pages/api
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- name: Fail if dev endpoints re-appear under pages/api/
|
||||
run: |
|
||||
BAD_PATHS=(
|
||||
"pages/api/simple.js"
|
||||
"pages/api/test-auth.js"
|
||||
"pages/api/test-db.js"
|
||||
"pages/api/setup-database.js"
|
||||
)
|
||||
FOUND=()
|
||||
for path in "${BAD_PATHS[@]}"; do
|
||||
if [ -f "$path" ]; then
|
||||
FOUND+=("$path")
|
||||
fi
|
||||
done
|
||||
# Also flag any new pages/api/test-*.js the explicit list missed.
|
||||
while IFS= read -r path; do
|
||||
FOUND+=("$path")
|
||||
done < <(find pages/api -maxdepth 4 -type f -name 'test-*.js' 2>/dev/null || true)
|
||||
if [ ${#FOUND[@]} -gt 0 ]; then
|
||||
echo "::error::Forbidden dev endpoints present in pages/api/. Delete them or move to scripts/."
|
||||
for path in "${FOUND[@]}"; do
|
||||
echo "::error file=${path}::Forbidden dev endpoint."
|
||||
done
|
||||
exit 1
|
||||
fi
|
||||
echo "OK: no forbidden dev endpoints under pages/api/."
|
||||
```
|
||||
|
||||
- [ ] The job runs on `pull_request` and `push` (it inherits the workflow-level `on:` triggers — no per-job `on:` block needed).
|
||||
- [ ] No new `concurrency:` block (the workflow-level `concurrency:` is already set).
|
||||
- [ ] No `if:` conditional that lets this job skip on docs-only PRs. The check is fast (a `find` + 4 `[ -f ]` calls) and skipping it would defeat the purpose.
|
||||
- [ ] The job is **blocking** — no `|| true` wrapper, no `::warning` fallback. (Lint has the wrapper because of the documented `fix-lint-baseline` debt; this job is not subject to that.)
|
||||
|
||||
### Smoke
|
||||
|
||||
- [ ] After deleting the files, `npm run build` succeeds (no broken imports — these endpoints are unreferenced, verified in the architect's audit).
|
||||
- [ ] `git grep -l 'api/simple\|test-auth\|test-db\|setup-database' pages components lib` returns no source files (only docs).
|
||||
- [ ] Locally, simulate the CI guard:
|
||||
|
||||
```bash
|
||||
bash -c '
|
||||
BAD_PATHS=("pages/api/simple.js" "pages/api/test-auth.js" "pages/api/test-db.js" "pages/api/setup-database.js")
|
||||
FOUND=(); for p in "${BAD_PATHS[@]}"; do [ -f "$p" ] && FOUND+=("$p"); done
|
||||
[ ${#FOUND[@]} -eq 0 ] && echo OK || { echo "FAIL: ${FOUND[@]}"; exit 1; }
|
||||
'
|
||||
```
|
||||
|
||||
Expect `OK`. Then create a temporary `pages/api/test-fake.js` (matches `test-*.js` glob) and re-run — expect `FAIL`. Delete the temp file before opening the PR.
|
||||
|
||||
### Out of scope
|
||||
|
||||
- [ ] No `pages/api/cards/import-*.js` deletion or gating. Those are admin-imports with rate-limit concerns; `add-rate-limiting` convoy.
|
||||
- [ ] No `pages/api/auth/*` changes — Brief 1 + Brief 2 + Brief 4 cover those.
|
||||
- [ ] No README rewrite of the API list — doc-writer pass.
|
||||
- [ ] No new test files — Brief 5.
|
||||
|
||||
## Rationale (≤3 sentences)
|
||||
|
||||
These four files are the highest-impact deletions in the convoy: `pages/api/setup-database.js` is a public unauthenticated POST that triggers DDL, and the other three leak DB / auth internals to anyone who hits them. The CI guard is cheap insurance — without it, a future agent following an outdated tutorial could re-introduce `pages/api/test-db.js` in good faith. Keeping this brief tiny (deletions + one CI job + one README line) means it can ship in parallel with Briefs 1, 2, and 4 with no merge-conflict risk.
|
||||
164
.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md
Normal file
164
.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md
Normal file
|
|
@ -0,0 +1,164 @@
|
|||
---
|
||||
convoy: fix-auth-bypass
|
||||
brief_number: 4
|
||||
depends_on: [1]
|
||||
files:
|
||||
- package.json
|
||||
- package-lock.json
|
||||
- lib/rate-limit.js
|
||||
- pages/api/auth/login.js
|
||||
- pages/api/auth/register.js
|
||||
cross_brief_commitments:
|
||||
- brief: 1
|
||||
description: |
|
||||
Brief 1 already removed the `JWT_SECRET` literal and routed login.js +
|
||||
register.js through `auth-utils.generateToken`. This brief preserves those
|
||||
changes; do NOT reintroduce inline `jwt.sign` or `JWT_SECRET` references.
|
||||
- brief: 5
|
||||
description: |
|
||||
Brief 5 (vitest) modifies `package.json` and `package-lock.json` after
|
||||
this brief. If Brief 5 lands first by accident, this brief's implementer
|
||||
MUST rebase on Brief 5's lockfile rather than regenerate from scratch.
|
||||
The sequenced order is Brief 4 → Brief 5; the convoy's slice_dependencies
|
||||
enforces this.
|
||||
---
|
||||
|
||||
# Brief 4: Tighten the public auth surface (CORS + rate limit)
|
||||
|
||||
## Goal (1 sentence)
|
||||
|
||||
Drop the wide-open `Access-Control-Allow-Origin: '*'` header from `/api/auth/login` and `/api/auth/register`, and rate-limit both endpoints to 5 attempts per 15 minutes per IP via `@upstash/ratelimit` (with a graceful no-op fallback in non-production environments where Upstash isn't configured).
|
||||
|
||||
## Files in scope (do not edit anything else)
|
||||
|
||||
- `package.json` — modified (add `@upstash/ratelimit`, `@upstash/redis`)
|
||||
- `package-lock.json` — modified (regenerated by `npm install`)
|
||||
- `lib/rate-limit.js` — **new**
|
||||
- `pages/api/auth/login.js` — modified
|
||||
- `pages/api/auth/register.js` — modified
|
||||
|
||||
## Conventions to follow
|
||||
|
||||
- `.cursor/rules/auth-and-permissions.mdc` § "Token model" — auth flow shape stays unchanged. Only the request-acceptance gate (CORS, rate limit) changes.
|
||||
- `.cursor/rules/api-routes.mdc` § "Method gating" + "Error handling" — the rate-limit check goes inside the existing `try`/`catch`, after the method gate, before the body parsing.
|
||||
- `.cursor/rules/no-go-zones.mdc` — do not touch `pages/api/auth/verify.js`, `pages/api/favorites.js`, `pages/api/users/search.js`, or any other auth-adjacent file. The CORS sweep on the rest of the API is `add-rate-limiting` / future scope.
|
||||
- `package.json` formatting: 2-space indent, alphabetical key order within `dependencies` / `devDependencies` (match the existing block from Brief 1's bump-next-js work).
|
||||
- `lib/rate-limit.js` ESM export, kebab-case file name, 2-space indent, no top-level side effects beyond a const init.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
### `package.json` changes
|
||||
|
||||
- [ ] `dependencies` gains `"@upstash/ratelimit": "^2.0.8"`. (Verified at architect time: `npm view @upstash/ratelimit version` → `2.0.8`. Peer dep: `@upstash/redis: ^1.34.3`.)
|
||||
- [ ] `dependencies` gains `"@upstash/redis": "^1.38.0"`. (Verified at architect time: `npm view @upstash/redis version` → `1.38.0`. Satisfies `@upstash/ratelimit@2.0.8`'s peer-dep range `^1.34.3`. The only direct dep `@upstash/redis` itself pulls in is `uncrypto@^0.1.3`.)
|
||||
- [ ] No other `dependencies` change. No `devDependencies` change in this brief (vitest is Brief 5).
|
||||
- [ ] No `engines` block change. Both packages are pure-JS ESM with Node `>=18` requirements; tcg-vault runs Node 20 on Vercel.
|
||||
|
||||
### `package-lock.json` changes
|
||||
|
||||
- [ ] Regenerated via `npm install` (no hand edits).
|
||||
- [ ] `npm ls @upstash/ratelimit` reports a single `2.0.x` version. No duplicates.
|
||||
- [ ] `npm ls @upstash/redis` reports a single `1.38.x` version.
|
||||
- [ ] `npm install` exits cleanly with no `ERESOLVE` errors and no `npm warn deprecated` for either package.
|
||||
|
||||
### `lib/rate-limit.js` (new)
|
||||
|
||||
- [ ] File exports a single async function `checkAuthRateLimit(req)` that returns `{ allowed: boolean, remaining: number, reset: number }`.
|
||||
- [ ] On first call, the module initializes a singleton `Ratelimit` instance lazily. **Do not initialize at module top level** — top-level `new Redis(...)` would throw at import time in environments without Upstash env vars (including local dev where the developer hasn't onboarded Upstash yet, and any test that imports `pages/api/auth/login.js` transitively).
|
||||
- [ ] Initialization rules:
|
||||
- If `process.env.KV_REST_API_URL` and `process.env.KV_REST_API_TOKEN` are both set: construct `new Redis({ url, token })` and `new Ratelimit({ redis, limiter: Ratelimit.slidingWindow(5, '15 m'), prefix: 'tcgvault:auth' })`. (Env-var names match Vercel's Upstash Marketplace integration; see Post-merge addendum.)
|
||||
- If either env var is missing AND `process.env.NODE_ENV === 'production'`: **throw at first call** with a message naming both env vars. (Fail-closed in prod — better to error a single login attempt than silently disable rate limiting.)
|
||||
- If either env var is missing AND `NODE_ENV !== 'production'`: log one `console.warn` ("`[rate-limit] KV_REST_API_URL / KV_REST_API_TOKEN not set — rate limiting disabled (dev/test only)`"), cache a no-op limiter (return `{ allowed: true, remaining: Infinity, reset: 0 }` from `checkAuthRateLimit`).
|
||||
- [ ] IP extraction:
|
||||
|
||||
```js
|
||||
const xff = req.headers['x-forwarded-for'];
|
||||
const firstHop = Array.isArray(xff) ? xff[0] : xff?.split(',')[0]?.trim();
|
||||
const identifier = firstHop || req.socket?.remoteAddress || 'anonymous';
|
||||
```
|
||||
|
||||
Use `identifier` as the rate-limit key. Do NOT use `req.body.email` (an attacker can rotate emails) or `req.headers.authorization` (login is unauthenticated by design — the header is absent).
|
||||
- [ ] On Upstash quota error or network failure inside `ratelimit.limit(...)`: catch and **fail-open** (return `{ allowed: true, ... }`) with a single `console.error('[rate-limit]', err)`. Reasoning: a hard outage at Upstash should not lock everyone out of login. Brute-force protection lives behind defense-in-depth (Vercel firewall, future fail2ban-style lockout). Document this trade-off in a comment.
|
||||
- [ ] No default export. Only the named `checkAuthRateLimit` export.
|
||||
- [ ] No top-level `await` (Next.js Pages Router serverless bundler handles ESM, but module-init time is the wrong place for I/O — keep it lazy).
|
||||
|
||||
### `pages/api/auth/login.js`
|
||||
|
||||
- [ ] **Drop CORS-`*`.** Remove lines 9-17 (the `setHeader('Access-Control-Allow-Origin', '*')` and friends, plus the OPTIONS preflight). Same-origin requests on Vercel work without explicit CORS headers — the browser doesn't preflight a same-origin POST.
|
||||
- If a future cross-origin client appears (e.g. a separate marketing-site origin), pin via `process.env.PUBLIC_FRONTEND_ORIGIN`. **Do NOT add this conditionally now** — adding the env-var path "just in case" creates a code path no test will cover, and the current `tcg-vault` deploy is single-origin Vercel. The `add-rate-limiting` convoy or a follow-up `cors-tighten` convoy can add it when it actually has a consumer.
|
||||
- [ ] **No OPTIONS handler.** With CORS-* gone, OPTIONS preflight isn't relevant for same-origin POST. If the front-end ever sends a preflight (it shouldn't on same-origin), Next.js will route it to this handler, which will hit the `if (req.method !== 'POST')` 405 branch — that's the correct response.
|
||||
- [ ] **Add the rate-limit gate** between the method check and the body parsing. Verbatim shape:
|
||||
|
||||
```js
|
||||
import { checkAuthRateLimit } from '../../../lib/rate-limit.js';
|
||||
// ... existing imports stay ...
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'POST') {
|
||||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkAuthRateLimit(req);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
try {
|
||||
// ... existing body unchanged ...
|
||||
} catch (error) {
|
||||
console.error('Login error:', error);
|
||||
res.status(500).json({ error: 'Internal server error' });
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Note `Retry-After` is in seconds, and `reset` from `@upstash/ratelimit` is a Unix-ms timestamp (per the SDK's `Ratelimit.limit` return shape).
|
||||
- [ ] Brief 1 already replaced `jwt.sign(...)` with `generateToken(user)`. **Preserve that.** Do not reintroduce inline `jwt.sign` or `JWT_SECRET` references.
|
||||
- [ ] Brief 1 already removed `import jwt from 'jsonwebtoken'`. Keep it removed.
|
||||
|
||||
### `pages/api/auth/register.js`
|
||||
|
||||
- [ ] Same CORS removal as login.js (drop the four `setHeader` calls + OPTIONS preflight at lines 10-18).
|
||||
- [ ] Same rate-limit gate, same shape, between method check and `try`. The 429 response shape and `Retry-After` header are identical.
|
||||
- [ ] Same import path: `'../../../lib/rate-limit.js'`. Verify by reading the existing `'../../../lib/slug-utils.js'` import on line 4.
|
||||
- [ ] Brief 1's `generateToken` call is preserved.
|
||||
|
||||
### Smoke (manual)
|
||||
|
||||
- [ ] In `.env.local`, set `KV_REST_API_URL` and `KV_REST_API_TOKEN` (if you have an Upstash free-tier account or have pulled them down from Vercel via `vercel env pull`). If you don't, leave both unset — the warn-and-continue branch should fire, and login still works.
|
||||
- [ ] `npm run dev`; submit invalid login 6 times in quick succession (each with a typo). Expect: first 5 return 401, 6th returns 429 with `Retry-After` header. (Skipped if Upstash isn't configured.)
|
||||
- [ ] Submit a valid login. Expect: token returned. (Successful logins also count against the limit per the sliding-window algo — that's intentional; a credential-stuffing attacker can't dodge by knowing one valid pair.)
|
||||
- [ ] Open dev tools → network tab on the login submit. Confirm there is **no** `Access-Control-Allow-Origin` response header. Confirm there is **no** preflight `OPTIONS` request.
|
||||
- [ ] Verify same behavior on `/api/auth/register`.
|
||||
- [ ] Vercel preview deploy succeeds with both env vars unset → expect `npm run build` to succeed (lazy init means no import-time throw).
|
||||
|
||||
### Pre-deploy checklist (call out in the PR description)
|
||||
|
||||
- [ ] **Before merging to `main`, confirm `KV_REST_API_URL` and `KV_REST_API_TOKEN` are present in the Vercel project settings (Production + Preview environments).** These are **auto-provisioned** the moment the Vercel Upstash Marketplace integration is enabled on the project — no manual paste-the-token step. (You can verify locally with `vercel env ls` or by inspecting Vercel's project → Settings → Environment Variables.) Without them, the prod auth endpoints will throw on first login attempt (intentional fail-closed). Free-tier Upstash Redis via the Marketplace is sufficient (10k commands/day; rate-limit traffic is single-digit commands per request).
|
||||
- [ ] Add a note to `.env.local.example` (if it exists; otherwise to AGENTS.md "Running locally" — but defer to doc-writer pass).
|
||||
|
||||
### Out of scope
|
||||
|
||||
- [ ] No CORS / rate-limit on `pages/api/auth/verify.js`. (`verify.js` is a GET on token presence; rate-limiting it would bounce legitimate page loads. The CORS-* on it is a smaller risk, deferred to `cors-tighten` or `add-rate-limiting`.)
|
||||
- [ ] No CORS / rate-limit on `pages/api/favorites.js`, `pages/api/users/search.js`, `pages/api/cards/import-*.js`, avatar upload, etc. → `add-rate-limiting` convoy.
|
||||
- [ ] No `withRateLimit(handler)` higher-order wrapper. The two endpoints in scope justify inline; a wrapper is premature abstraction until there are 3+ call sites.
|
||||
- [ ] No middleware-based rate limit (Next.js `middleware.js`). Pages Router with serverless functions doesn't share the Edge runtime cleanly with `@upstash/ratelimit`'s default Node-fetch path. Inline is simpler.
|
||||
- [ ] No `withCollectionPermission`-style wrapper change.
|
||||
- [ ] No `AGENTS.md` / `.cursor/rules/auth-and-permissions.mdc` updates — doc-writer pass.
|
||||
|
||||
## Rationale (≤3 sentences)
|
||||
|
||||
Wrapping login + register with rate limiting closes the credential-stuffing window before public launch (P0 #6 partial), and dropping CORS-* removes a class of CSRF vectors that the wild-card header was masking (P0 #4). Choosing `@upstash/ratelimit` over a DIY-Postgres alternative respects the convoy's "no schema changes" rule, and choosing serverless-native over an in-memory limiter respects the Vercel deployment model (each cold start would otherwise reset its own counter). Bundling CORS and rate-limit into one brief — rather than splitting them across Brief 4 + Brief 5 as the convoy file initially suggested — avoids two PRs editing the same two handler files in sequence.
|
||||
|
||||
## Post-merge addendum (2026-05-23)
|
||||
|
||||
Added retroactively by `role-doc-writer` during convoy close-out. The brief as originally written specified `UPSTASH_REDIS_REST_URL` / `UPSTASH_REDIS_REST_TOKEN` — the two env-var names baked into `@upstash/redis`'s generic README examples — and the verbatim shape above still reflects that. **The shipped code in `lib/rate-limit.js` uses `KV_REST_API_URL` / `KV_REST_API_TOKEN` instead.** This addendum records the deviation so future readers don't mistake the brief's original shape for the as-built behavior.
|
||||
|
||||
**What changed and why.** Mid-implementation, the implementer surfaced that this project already runs on Vercel's Upstash Marketplace integration, which auto-provisions a Redis instance under a project-scoped credential set named `KV_*` (alongside `KV_URL`, `REDIS_URL`, and `KV_REST_API_READ_ONLY_TOKEN`). Aliasing those to a new `UPSTASH_REDIS_REST_*` pair would have required either (a) a manual paste-the-token step on every environment (Production, Preview, Local) or (b) a duplicate set of env vars pointing at the same Upstash instance. Neither was worth the friction; the Marketplace's own naming is the lower-coordination path.
|
||||
|
||||
**How the change was approved.** Implementer paused, surfaced the discrepancy upward via parent-agent interrupt, parent agent approved the rename to `KV_REST_API_*` ("ship what Vercel hands you"), and the implementer continued with the renamed pair. The convoy file's "Pre-merge env-var checklist" line ("UPSTASH_REDIS_REST_URL / UPSTASH_REDIS_REST_TOKEN") was not updated in the implementation PR — the close-out doc-writer pass (this addendum + the matching edit to `.convoys/fix-auth-bypass.md` § "Convoy outcome") is where the canonical record lives.
|
||||
|
||||
**Not in scope for this brief.** The other three `KV_*`-prefixed vars Vercel exposes (`KV_URL`, `REDIS_URL`, `KV_REST_API_READ_ONLY_TOKEN`) are intentionally unused. `@upstash/redis`'s REST client reads only `KV_REST_API_URL` + `KV_REST_API_TOKEN`; the others are for the Redis-protocol client (`@upstash/redis/cloudflare` / `ioredis`) or for read-only consumers. Do not wire them up unless a downstream library specifically requires one.
|
||||
|
||||
**Verbatim shape correction.** The "Initialization rules" snippet above has been updated in-place to read `KV_REST_API_URL` / `KV_REST_API_TOKEN`. The "Smoke (manual)" and "Pre-deploy checklist" sections have been updated to match. Any other documentation that still mentions `UPSTASH_REDIS_REST_*` for this project (search-and-replace target) is stale.
|
||||
211
.convoys/fix-auth-bypass/brief-5-vitest-and-auth-tests.md
Normal file
211
.convoys/fix-auth-bypass/brief-5-vitest-and-auth-tests.md
Normal file
|
|
@ -0,0 +1,211 @@
|
|||
---
|
||||
convoy: fix-auth-bypass
|
||||
brief_number: 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
|
||||
cross_brief_commitments:
|
||||
- brief: 1
|
||||
description: |
|
||||
Brief 1 created `lib/auth-secret.js` with the `JWT_SECRET` fail-loud
|
||||
throw. Brief 5's `test/setup.js` MUST set `process.env.JWT_SECRET` to
|
||||
a stable test value BEFORE any test file imports any auth code, or
|
||||
every test crashes at module load.
|
||||
- brief: 2
|
||||
description: |
|
||||
Brief 2 fixed `getUserFromRequest` to return `null` for unauthenticated
|
||||
requests. Brief 5's `permission-middleware.test.js` exists to lock that
|
||||
behavior in. If Brief 2 is reverted or partially regressed, these tests
|
||||
MUST fail.
|
||||
- brief: 4
|
||||
description: |
|
||||
Brief 4 added `package.json` + `package-lock.json` changes for
|
||||
`@upstash/ratelimit`. Brief 5 stacks vitest + vite + (transitively
|
||||
installed) onto the same lockfile. If Brief 4 has not landed when
|
||||
Brief 5 starts, the implementer MUST rebase / coordinate the lockfile
|
||||
regen. Slice_dependencies enforces the order.
|
||||
---
|
||||
|
||||
# Brief 5: Install vitest + write the auth unit tests + re-enable the CI test job
|
||||
|
||||
## Goal (1 sentence)
|
||||
|
||||
Install `vitest@^3.2.4`, write unit tests that lock in the post-Brief-2 behavior of `getUserFromRequest` (null for missing/malformed/expired tokens; user object for valid tokens) plus thin coverage of `auth-utils.generateToken` / `verifyToken`, and re-enable the disabled `test:` job in `.github/workflows/ci.yml`.
|
||||
|
||||
## Files in scope (do not edit anything else)
|
||||
|
||||
- `package.json` — modified (add vitest as devDep, add `test` and `test:run` scripts)
|
||||
- `package-lock.json` — modified (regenerated)
|
||||
- `vitest.config.js` — **new**
|
||||
- `test/setup.js` — **new** (sets test env, mocks `@vercel/postgres`)
|
||||
- `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` — modified (uncomment + adjust the disabled `test:` job)
|
||||
|
||||
## Conventions to follow
|
||||
|
||||
- `.cursor/rules/no-go-zones.mdc` — do not edit any auth source file (those are Briefs 1 / 2 / 4). Tests **read** the source; they do not modify it.
|
||||
- `.cursor/rules/auth-and-permissions.mdc` is the contract under test — every assertion in these tests should map to a bullet in that rule.
|
||||
- `package.json` formatting: 2-space indent, alphabetical key order within `devDependencies`. New `scripts` keys go alphabetically among existing keys.
|
||||
- ESM throughout (`"type": "module"` is set). All test files use `import`.
|
||||
- File naming: `*.test.js` (vitest's default `include` pattern).
|
||||
- **Plain JavaScript only.** Do not add a `tsconfig.json`. Do not use `.ts` files. Do not import `@types/*` packages. The repo is JavaScript-only; the existing `typescript@^5.9.3` devDep is purely a transitive requirement of `eslint-config-next@16` and is NOT a language switch (per AGENTS.md and the bump-next-js retro).
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
### `package.json` changes
|
||||
|
||||
- [ ] `devDependencies` gains `"vitest": "^3.2.4"`. (Verified at architect time: vitest@3.2.4 has `vite` as a regular dependency, not a peer dependency, so we do **not** need to install Vite separately. vitest@4.x requires `vite ^6 || ^7 || ^8` as a non-optional peer — that's why we pin to v3.)
|
||||
- [ ] No `vite` direct devDep. (vitest@3 bundles vite transitively.)
|
||||
- [ ] No `@types/node` or any `@types/*` package — JS-only.
|
||||
- [ ] No `@vitest/ui`, `@vitest/coverage-v8`, `happy-dom`, `jsdom` — none needed for unit tests of pure-Node modules.
|
||||
- [ ] `scripts` gains:
|
||||
- `"test": "vitest"` (watch mode, dev convenience)
|
||||
- `"test:run": "vitest run"` (single-pass, CI mode)
|
||||
- [ ] `scripts` does NOT gain a `test:ui` or `test:coverage` script in this brief — those are follow-up.
|
||||
|
||||
### `package-lock.json` changes
|
||||
|
||||
- [ ] Regenerated via `npm install`.
|
||||
- [ ] `npm ls vitest` reports a single `3.2.x` version.
|
||||
- [ ] `npm ls vite` reports a single `5.x`, `6.x`, or `7.x` version (vitest@3.2.4's regular dep range is `^5.0.0 || ^6.0.0 || ^7.0.0-0`; the locked version depends on what npm resolves at install time).
|
||||
- [ ] `npm install` exits cleanly with no `ERESOLVE` errors. **`npm warn deprecated` lines are tolerated** for transitive deps (vitest's tree pulls in `glob@7` and `inflight` historically). If the warnings are loud, capture them in the PR description but don't block.
|
||||
|
||||
### `vitest.config.js` (new)
|
||||
|
||||
- [ ] ESM (`export default`), 2-space indent.
|
||||
- [ ] Verbatim shape:
|
||||
|
||||
```js
|
||||
import { defineConfig } from 'vitest/config';
|
||||
|
||||
export default defineConfig({
|
||||
test: {
|
||||
environment: 'node',
|
||||
globals: false,
|
||||
setupFiles: ['./test/setup.js'],
|
||||
include: ['test/**/*.test.js'],
|
||||
testTimeout: 5000,
|
||||
},
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] No `coverage:` block. No `pool:` override. No `transform:` config (vitest's default Vite-based transform handles `.js` ESM out of the box).
|
||||
|
||||
### `test/setup.js` (new)
|
||||
|
||||
- [ ] Sets stable test env BEFORE any module is imported elsewhere. Verbatim shape:
|
||||
|
||||
```js
|
||||
process.env.JWT_SECRET = 'test-secret-for-vitest-only-do-not-use-in-prod';
|
||||
process.env.NODE_ENV = 'test';
|
||||
```
|
||||
|
||||
- [ ] **Do NOT set `UPSTASH_REDIS_REST_URL` / `UPSTASH_REDIS_REST_TOKEN`.** The rate-limit module's no-op fallback fires under `NODE_ENV !== 'production'` with Upstash unset. If a future test wants to assert rate-limit behavior, it can mock `@upstash/ratelimit` per-test.
|
||||
- [ ] No `dotenv` import. Vitest does not automatically read `.env.local`, and we do not want production secrets leaking into test runs.
|
||||
|
||||
### `test/lib/auth-secret.test.js` (new)
|
||||
|
||||
Cover the two exports and the import-time throw.
|
||||
|
||||
- [ ] `import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js'` succeeds when `process.env.JWT_SECRET` is set (it is, via `test/setup.js`).
|
||||
- [ ] `JWT_SECRET` equals the value set in `test/setup.js`.
|
||||
- [ ] `JWT_TOKEN_TTL` equals `'24h'`.
|
||||
- [ ] **Import-time throw test:** use `vi.resetModules()` + `vi.stubEnv('JWT_SECRET', '')` + `await expect(import('../../lib/auth-secret.js')).rejects.toThrow(/JWT_SECRET/)`. Then `vi.unstubAllEnvs()` to restore. (Verbatim pattern lives in vitest docs §"Mocking → Environment Variables"; the test must use `await import(...)` because static `import` resolves at file-parse time and would crash the test runner.)
|
||||
- [ ] Test count: 3.
|
||||
|
||||
### `test/lib/permission-middleware.test.js` (new)
|
||||
|
||||
This is the core security test. Lock in Brief 2's behavior.
|
||||
|
||||
- [ ] `vi.mock('@vercel/postgres', () => ({ sql: vi.fn() }))` at the top of the file. The `sql` mock returns `Promise.resolve({ rows: [...] })` per-test, allowing each test to set the user-row shape it expects.
|
||||
- [ ] Helper to mint a valid token in tests:
|
||||
|
||||
```js
|
||||
import jwt from 'jsonwebtoken';
|
||||
import { JWT_SECRET } from '../../lib/auth-secret.js';
|
||||
|
||||
function makeToken(payload, opts = {}) {
|
||||
return jwt.sign(payload, JWT_SECRET, { expiresIn: opts.expiresIn ?? '1h' });
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] Test cases (each maps to a bullet in `.cursor/rules/auth-and-permissions.mdc` § "Token model"):
|
||||
- `returns null when Authorization header is missing` — `getUserFromRequest({ headers: {} })` resolves to `null`. **No DB query is made** (assert `sql` mock not called).
|
||||
- `returns null when Authorization header is not Bearer` — `{ headers: { authorization: 'Basic foo' } }` resolves to `null`.
|
||||
- `returns null when token is malformed` — `{ headers: { authorization: 'Bearer not-a-jwt' } }` resolves to `null`.
|
||||
- `returns null when token signature uses a wrong secret` — sign a payload with `'other-secret'`, expect `null`.
|
||||
- `returns null when token is expired` — sign with `expiresIn: '-1s'`, expect `null`.
|
||||
- `returns null when token is valid but user-row is missing` — set `sql` to return `{ rows: [] }`, expect `null`.
|
||||
- `returns user object when token is valid and user-row exists` — set `sql` to return `{ rows: [{ id: 42, email: 'a@b.c', role: 'user' }] }`. Expect `{ userId: 42, email: 'a@b.c', role: 'user' }`. Note the `userId` (not `id`) field name — that is the helper's documented contract.
|
||||
- [ ] **Negative regression test (Brief 2 lock):** confirm the helper does NOT return the synthetic admin shape `{ userId: 1, email: 'admin@tcgvault.com', role: 'admin' }` when no header is present. This is a smoke against the bug specifically.
|
||||
- [ ] Test count: 8.
|
||||
|
||||
### `test/api/auth-utils.test.js` (new)
|
||||
|
||||
Thin coverage of the JWT-mint contract.
|
||||
|
||||
- [ ] `vi.mock('../../lib/database.js', () => ({ db: { query: vi.fn() } }))` — `auth-utils.js` imports `db`, but the tests only exercise `generateToken` / `verifyToken`, which don't touch the DB. The mock just satisfies the import.
|
||||
- [ ] Test cases:
|
||||
- `generateToken issues a token whose expiry is 24h from now (±5s tolerance)` — decode the token, check `decoded.exp - decoded.iat === 86400`.
|
||||
- `generateToken includes userId, email, role from the user arg` — decode, assert payload.
|
||||
- `verifyToken returns the payload for a valid token`.
|
||||
- `verifyToken returns null for a malformed token`.
|
||||
- `verifyToken returns null for a token signed with a different secret`.
|
||||
- [ ] Test count: 5.
|
||||
|
||||
### `.github/workflows/ci.yml`
|
||||
|
||||
The current file has the `test:` job commented out at lines 87-103. Re-enable it. Verbatim replacement for that block:
|
||||
|
||||
```yaml
|
||||
test:
|
||||
name: Unit tests (vitest)
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- uses: actions/setup-node@v4
|
||||
with:
|
||||
node-version: ${{ env.NODE_VERSION }}
|
||||
cache: npm
|
||||
- run: npm ci
|
||||
- run: npm run test:run
|
||||
env:
|
||||
JWT_SECRET: ci-secret-only-for-tests-do-not-use-in-prod
|
||||
```
|
||||
|
||||
- [ ] **No** `POSTGRES_URL` env in CI. The unit tests mock `@vercel/postgres`; they don't need a real connection. Setting it to a fake value would mask import-time validation that may exist in `lib/database.js`.
|
||||
- [ ] **No** `UPSTASH_REDIS_REST_*` envs. Tests don't exercise the rate-limit module.
|
||||
- [ ] The job is **blocking** (no `|| true`, no `::warning`).
|
||||
- [ ] Concurrency is inherited from the workflow level; no per-job override.
|
||||
- [ ] Remove the trailing comment block at the bottom of the file (the `# test:` placeholder lines 87-103). They become real lines now.
|
||||
- [ ] Update the workflow header comment (lines 11-13) to remove the "tcg-vault has no test runner installed yet" note.
|
||||
|
||||
### Smoke (manual)
|
||||
|
||||
- [ ] `npm install` from a clean tree succeeds.
|
||||
- [ ] `npm run test:run` runs all 16 tests and exits 0.
|
||||
- [ ] `npm run test` (watch mode) shows the same 16 tests passing on save.
|
||||
- [ ] **Failure-mode smoke:** temporarily revert one line of Brief 2's fix (e.g. add back the `return { userId: 1, ... }` synthetic admin in `getUserFromRequest`). Run `npm run test:run`. Expect: `permission-middleware.test.js`'s "returns null when Authorization header is missing" test FAILS. Restore Brief 2 before opening the PR.
|
||||
- [ ] Push to a draft PR and confirm the GitHub Actions `test` job runs and is green.
|
||||
|
||||
### Out of scope
|
||||
|
||||
- [ ] No tests for `lib/rate-limit.js` (Brief 4). The graceful-fallback branch is hard to test cleanly without an Upstash mock; defer to a follow-up.
|
||||
- [ ] No tests for `pages/api/auth/login.js` / `register.js` integration paths (would require fluent HTTP-handler mocking; defer to a follow-up Playwright / supertest convoy).
|
||||
- [ ] No tests for `withCollectionPermission`, `checkCollectionPermission`, `logCollectionActivity`. This convoy is scoped to the auth-bypass surface; collection-permission tests are their own follow-up.
|
||||
- [ ] No `tsconfig.json` or `.ts` files. JS-only, per AGENTS.md.
|
||||
- [ ] No coverage report or coverage gate. Follow-up convoy.
|
||||
- [ ] No Playwright / E2E. Follow-up convoy (`adopt-playwright`).
|
||||
|
||||
## Rationale (≤3 sentences)
|
||||
|
||||
Bringing vitest forward by one slot in the launch sequence is justified by the security blast radius of an auth refactor — the alternative is shipping Brief 2 untested and waiting for the test-runner convoy to backfill, which leaves `getUserFromRequest`'s null-return contract unenforced for an unknown number of PRs. Pinning vitest to v3.2.4 (rather than the latest v4.1.7) avoids the non-optional `vite` peer-dep that v4 introduced, keeping the devDep set minimal for a JS-only repo. Mocking `@vercel/postgres` in unit tests rather than spinning up a real Postgres in CI keeps the test job under 30 seconds end-to-end and avoids the operational cost of a CI-only DB.
|
||||
|
|
@ -17,16 +17,19 @@ Code graph: 122 files, 628 nodes, 5602 edges, 11 communities. Indexed by `user-c
|
|||
|
||||
These MUST land before any anonymous traffic touches the production URL.
|
||||
|
||||
### 1. `getUserFromRequest` returns a hardcoded admin when no Bearer token is present
|
||||
### 1. `getUserFromRequest` returns a hardcoded admin when no Bearer token is present — **RESOLVED 2026-05-23**
|
||||
|
||||
- **Resolved by:** `fix-auth-bypass` Brief 2, commit `258e479` (PR #8). Follow-up Brief 6 hotfix `1fca3aa` added explicit 401 guards to the cards-collection POST/PUT/DELETE branches that previously masked the bug as 500s.
|
||||
- **File:** `lib/permission-middleware.js` lines 13-17.
|
||||
- **Impact:** Every API route that calls `getUserFromRequest` (30+ handlers — see `user-code-review-graph` cross-community edges from `api-handler` → `lib-admin`) accepts unauthenticated requests as admin user 1.
|
||||
- **Repro:** `curl https://<host>/api/collections` with no `Authorization` header returns admin's collections.
|
||||
- **Fix:** Delete lines 13-17. Return `null` when no Bearer token. Update every caller to handle `null` properly (most already do; the broken fallback was masking the right path).
|
||||
- **As-shipped:** The helper now returns `null` for any unauthenticated request. 16 unit tests in `test/lib/permission-middleware.test.js` lock in the contract (including a negative regression against the old synthetic-admin shape). `pages/api/auth/verify.js` returns 401 on the no-token branch instead of fetching the seed admin row.
|
||||
- **Owns:** `role-architect` + `role-implementer` (one PR; small surface area in the helper, callers already check `!user`).
|
||||
|
||||
### 2. JWT_SECRET hardcoded fallback in 7 files
|
||||
### 2. JWT_SECRET hardcoded fallback in 7 files — **RESOLVED 2026-05-23**
|
||||
|
||||
- **Resolved by:** `fix-auth-bypass` Brief 1, commit `4a10dce` (PR #7).
|
||||
- **Files:**
|
||||
- `pages/api/auth-utils.js` (`'your-secret-key'`)
|
||||
- `pages/api/auth/login.js`, `pages/api/auth/register.js`, `pages/api/auth/verify.js`
|
||||
|
|
@ -35,6 +38,7 @@ These MUST land before any anonymous traffic touches the production URL.
|
|||
- **Impact:** If `JWT_SECRET` env var is unset (e.g. preview/staging misconfig), tokens are signed with `'your-secret-key-change-in-production'` — an attacker can sign their own admin token in 5 seconds.
|
||||
- **Fix:** Centralize JWT_SECRET access in one helper that `throw`s at module load if `process.env.JWT_SECRET` is unset. Every other file imports from there.
|
||||
- **Bonus:** Token expiry is inconsistent (`/api/auth/login.js` uses 24h, `pages/api/auth-utils.js` uses 7d). Pick one.
|
||||
- **As-shipped:** `lib/auth-secret.js` is the single source of truth and throws at module load if `JWT_SECRET` is unset. Canonical TTL is `JWT_TOKEN_TTL = '24h'`. All 7 literal fallback sites are converted to import-and-throw. `test/lib/auth-secret.test.js` (3 tests) covers the fail-loud path.
|
||||
- **Owns:** `role-architect` + `role-implementer`.
|
||||
|
||||
### 3. Default admin credentials in seed + README
|
||||
|
|
@ -50,24 +54,30 @@ These MUST land before any anonymous traffic touches the production URL.
|
|||
3. Strip the admin password from README — replace with "run `npm run setup-db` and follow the prompt".
|
||||
- **Owns:** `role-implementer`.
|
||||
|
||||
### 4. Dev-only test endpoints shipped to production
|
||||
### 4. Dev-only test endpoints shipped to production — **RESOLVED 2026-05-23**
|
||||
|
||||
- **Resolved by:** `fix-auth-bypass` Brief 3, commit `fc0dd73` (PR #6).
|
||||
- **Files:** `pages/api/simple.js`, `pages/api/test-auth.js`, `pages/api/test-db.js`, `pages/api/setup-database.js`.
|
||||
- **Impact:** Unknown — depends on what they expose. `/api/test-db` likely returns the DB connection string; `/api/test-auth` may leak token-handling details.
|
||||
- **Fix:** Delete all four. Add a CI grep that fails the build if any file matching `pages/api/(test-|simple|setup-)*.js` exists.
|
||||
- **As-shipped:** All four files deleted. `.github/workflows/ci.yml` has a new `forbidden-endpoints` job (blocking) that fails the build if any of the four paths reappear OR if a new `pages/api/test-*.js` file is added. Local simulation in the implementer PR confirmed clean → OK, with `test-fake.js` → FAIL, post-cleanup → OK.
|
||||
- **Owns:** `role-implementer`.
|
||||
|
||||
### 5. CORS `Access-Control-Allow-Origin: *` on auth endpoints
|
||||
### 5. CORS `Access-Control-Allow-Origin: *` on auth endpoints — **PARTIAL 2026-05-23**
|
||||
|
||||
- **Partially resolved by:** `fix-auth-bypass` Brief 4, commit `297afca` (PR #9). Login + register only; `pages/api/auth/verify.js` is **deferred** to the queued `cors-tighten` follow-up convoy.
|
||||
- **Files:** at minimum `pages/api/auth/login.js`, `pages/api/auth/register.js`, `pages/api/setup-database.js` (verify others).
|
||||
- **Impact:** Any origin can submit credentials. Combined with the no-rate-limit problem below, credential stuffing is wide open.
|
||||
- **Fix:** Set `Access-Control-Allow-Origin` to the literal frontend origin (`https://tcgvault.com` / preview domain), or remove the header entirely if the API and the frontend are same-origin (they are, on Vercel).
|
||||
- **As-shipped:** `pages/api/auth/login.js` and `pages/api/auth/register.js` drop the four `setHeader` calls + the OPTIONS preflight handler. `pages/api/setup-database.js` was deleted entirely by Brief 3. `pages/api/auth/verify.js` still has the wildcard header — see follow-up convoy `cors-tighten`.
|
||||
- **Owns:** `role-implementer`.
|
||||
|
||||
### 6. No rate limiting anywhere
|
||||
### 6. No rate limiting anywhere — **PARTIAL 2026-05-23**
|
||||
|
||||
- **Partially resolved by:** `fix-auth-bypass` Brief 4, commit `297afca` (PR #9). Login + register only; the rest of the listed endpoints are **deferred** to the queued `add-rate-limiting` convoy.
|
||||
- **Impact:** Login endpoint accepts unlimited attempts; card-search endpoint can be hammered; image upload endpoints can be exhausted. The `pages/api/cards/import-*.js` endpoints externally hit Scryfall/Pokémon APIs with no caller throttling.
|
||||
- **Fix:** Adopt `@upstash/ratelimit` (free tier covers a small launch) or Vercel's built-in middleware-based rate limiting. Apply to: `/api/auth/login`, `/api/auth/register`, `/api/users/search`, `/api/cards/search`, all `/api/cards/import-*`, and `/api/user/avatar*` (upload).
|
||||
- **As-shipped:** `lib/rate-limit.js` (new) provides `checkAuthRateLimit(req)` via `@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0` (5 attempts / 15-min sliding window per IP). Wired into login + register. Env vars are `KV_REST_API_URL` / `KV_REST_API_TOKEN` (auto-provisioned by Vercel's Upstash Marketplace integration — note this is a rename from the brief's original `UPSTASH_REDIS_REST_*` spec; see `.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md` § Post-merge addendum). Fails closed in prod when env vars are unset; warn-and-no-ops in dev. Search / import / avatar endpoints are unchanged.
|
||||
- **Owns:** `role-architect` (pattern) → `role-implementer` (per-route).
|
||||
|
||||
### 7. Layout default-prop leaks maintainer email
|
||||
|
|
|
|||
|
|
@ -36,7 +36,7 @@ export default withCollectionPermission('viewer')(handler);
|
|||
// 'viewer' | 'editor' | 'owner' — checks owner OR is_public OR explicit collection_permissions row
|
||||
```
|
||||
|
||||
**CRITICAL — known bug:** `getUserFromRequest` currently has a development-mode fallback that returns a hardcoded admin user when no Bearer token is present. Until that's fixed, callers MUST also assert `req.headers.authorization` exists when the route is sensitive (admin operations, deletes). See `AGENTS.md` Gotcha #2.
|
||||
`getUserFromRequest` returns `null` for any unauthenticated request (missing header, malformed token, wrong signature, expired token, unknown user id). The early `if (!user) return res.status(401)` pattern in the snippet above is the canonical guard for every authenticated route. (The pre-`fix-auth-bypass` synthetic-admin fallback for missing tokens has been removed — see `AGENTS.md` Gotcha #2 for the audit trail.)
|
||||
|
||||
## Request validation
|
||||
|
||||
|
|
@ -101,6 +101,39 @@ import { logCollectionActivity } from '../../lib/permission-middleware';
|
|||
await logCollectionActivity(collectionId, userId, 'card_added', { cardId, quantity });
|
||||
```
|
||||
|
||||
## Dev/test endpoints
|
||||
## Rate limiting
|
||||
|
||||
`pages/api/simple.js`, `pages/api/test-auth.js`, `pages/api/test-db.js`, `pages/api/setup-database.js` — these are dev-only endpoints currently shipped to prod. Don't add more. Existing ones should be deleted or admin-gated before public launch.
|
||||
`/api/auth/login` and `/api/auth/register` are wrapped with a 5-attempt / 15-minute sliding window via `lib/rate-limit.js`. New endpoints on the public auth surface (or anywhere brute-force / credential-stuffing matters) should follow the same shape:
|
||||
|
||||
```js
|
||||
import { checkAuthRateLimit } from '../../../lib/rate-limit.js';
|
||||
|
||||
export default async function handler(req, res) {
|
||||
if (req.method !== 'POST') {
|
||||
return res.status(405).json({ error: 'Method not allowed' });
|
||||
}
|
||||
|
||||
const { allowed, reset } = await checkAuthRateLimit(req);
|
||||
if (!allowed) {
|
||||
res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000));
|
||||
return res.status(429).json({ error: 'Too many attempts. Try again later.' });
|
||||
}
|
||||
|
||||
try {
|
||||
// ... handler body ...
|
||||
} catch (err) {
|
||||
// ...
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Notes:
|
||||
|
||||
- Gate sits between the method check and the body. It MUST be inside the `try`/`catch` if you want Upstash errors to bubble — but `checkAuthRateLimit` already swallows them and fails-open, so the placement above is fine.
|
||||
- Identifier is the first hop in `x-forwarded-for` (Vercel's edge); do NOT key off `req.body.email` (rotates) or `req.headers.authorization` (login is unauthenticated by design).
|
||||
- Env vars are `KV_REST_API_URL` + `KV_REST_API_TOKEN` (auto-provisioned by Vercel's Upstash Marketplace integration). In prod, missing either var is a **fail-closed throw** on the first call — set them in Vercel project settings before merging anything that imports `lib/rate-limit.js`. In dev, the module warn-and-no-ops so local work isn't blocked.
|
||||
- The current scope is just the two auth endpoints. Sweeping the rest of the API (`/api/users/search`, `/api/cards/import-*`, avatar upload) is the queued `add-rate-limiting` convoy — follow the same pattern there.
|
||||
|
||||
## Dev/test endpoints (removed)
|
||||
|
||||
The four endpoints `pages/api/simple.js`, `pages/api/test-auth.js`, `pages/api/test-db.js`, and `pages/api/setup-database.js` used to exist as unauthenticated dev / diagnostic routes. They were **deleted** by `fix-auth-bypass` Brief 3 (commit `fc0dd73`) and `.github/workflows/ci.yml`'s `forbidden-endpoints` job now fails the build if any of them are re-introduced, or if any new file matching `pages/api/test-*.js` is added. **Do not re-create these files.** If a future agent searches for `test-db` or `setup-database` and finds them missing, this section is the explanation — diagnostics belong outside the public API surface (a CLI script, an admin-gated route, or `npm run` task).
|
||||
|
|
|
|||
|
|
@ -14,7 +14,9 @@ There are three parallel client-side auth implementations and one server-side he
|
|||
| Server: extract user from request | `lib/permission-middleware.js::getUserFromRequest` |
|
||||
| Server: gate a collection route | `lib/permission-middleware.js::withCollectionPermission` |
|
||||
| Server: log collection mutation | `lib/permission-middleware.js::logCollectionActivity` |
|
||||
| Server: token / password primitives | `pages/api/auth-utils.js` (`generateToken`, `verifyToken`, `hashPassword`, `verifyPassword`) |
|
||||
| Server: JWT secret + canonical TTL | `lib/auth-secret.js` (`JWT_SECRET`, `JWT_TOKEN_TTL`) — single source of truth, fail-loud on unset env |
|
||||
| Server: token / password primitives | `pages/api/auth-utils.js` (`generateToken`, `verifyToken`, `hashPassword`, `verifyPassword`) — reads secret + TTL from `lib/auth-secret.js` |
|
||||
| Server: rate-limit auth endpoints | `lib/rate-limit.js::checkAuthRateLimit` (5 attempts / 15 min sliding window via `@upstash/ratelimit`) |
|
||||
| Client: hook | `lib/use-auth.js::useAuth` |
|
||||
| Client: route protection | `components/ProtectedRoute.js` |
|
||||
| Client: admin route protection | `components/AdminProtected.js` |
|
||||
|
|
@ -29,11 +31,11 @@ A convoy is planned to collapse these three into one provider + one hook.
|
|||
## Token model
|
||||
|
||||
- JWT in localStorage under the key `auth_token`.
|
||||
- Signed with `JWT_SECRET` (HS256), 7-day expiry, payload `{ userId, email, role }`.
|
||||
- Signed with `JWT_SECRET` (HS256), **24-hour expiry** (canonical `JWT_TOKEN_TTL = '24h'` from `lib/auth-secret.js`), payload `{ userId, email, role }`.
|
||||
- Sent on every authenticated fetch as `Authorization: Bearer <token>`.
|
||||
- Verified server-side with `jsonwebtoken.verify(token, JWT_SECRET)`.
|
||||
- Verified server-side with `jsonwebtoken.verify(token, JWT_SECRET)` (or `verifyToken` from `pages/api/auth-utils.js`).
|
||||
|
||||
**JWT_SECRET MUST be set in the deploy environment.** Seven files default it to a string literal if unset; that defeats signing.
|
||||
**`JWT_SECRET` MUST be set in the deploy environment.** `lib/auth-secret.js` is the only place either `JWT_SECRET` or `JWT_TOKEN_TTL` is defined; the module **throws at import time** if `process.env.JWT_SECRET` is unset, which fails the request loudly rather than silently signing with a literal fallback. (The legacy `'your-secret-key-change-in-production'` fallback was duplicated across 7 files pre-`fix-auth-bypass`; that whole pattern is gone — do not reintroduce it.)
|
||||
|
||||
## Roles
|
||||
|
||||
|
|
@ -50,6 +52,17 @@ When introducing a new permission tier, update both `checkRolePermission`'s hier
|
|||
|
||||
## Server-side authorization patterns
|
||||
|
||||
`getUserFromRequest` returns `{ userId, email, role }` for a valid Bearer token, or `null` for any other case (missing header, malformed token, wrong signature, expired token, unknown user id). **`null` means 401** — always early-return before doing anything that depends on the user:
|
||||
|
||||
```js
|
||||
const user = await getUserFromRequest(req);
|
||||
if (!user) return res.status(401).json({ error: 'Authentication required' });
|
||||
```
|
||||
|
||||
There is **no synthetic-admin fallback** for missing tokens. The old dev-mode behavior of returning user 1 as admin is gone (resolved by `fix-auth-bypass` Brief 2, commit `258e479`). Do not reintroduce it under any framing — `test/lib/permission-middleware.test.js` has a negative regression test that will fail if the synthetic-admin shape comes back.
|
||||
|
||||
From there:
|
||||
|
||||
- **Owner-only** (delete, settings): inside the handler, `if (user.userId !== resource.user_id) return res.status(403)`.
|
||||
- **Editor-or-owner**: use `withCollectionPermission('editor')`.
|
||||
- **Public read**: use `withCollectionPermission('viewer')` — handles `is_public` and explicit-permission case.
|
||||
|
|
|
|||
25
AGENTS.md
25
AGENTS.md
|
|
@ -10,7 +10,7 @@ A web app for managing trading-card-game collections (Magic, Pokémon, Lorcana).
|
|||
|
||||
- **Framework:** Next.js 16 (Pages router) + React 18, JavaScript (not TypeScript — see Gotcha #9)
|
||||
- **Data:** Neon Postgres, accessed two different ways — `@neondatabase/serverless` (`lib/database.js`) AND raw `@vercel/postgres` (`pages/api/**`). Pick ONE; see Gotcha #1.
|
||||
- **Auth:** Custom JWT (jsonwebtoken + bcryptjs), token stored in `localStorage`, sent as `Authorization: Bearer …`. No NextAuth.
|
||||
- **Auth:** Custom JWT (jsonwebtoken + bcryptjs), token stored in `localStorage`, sent as `Authorization: Bearer …`. No NextAuth. The secret + canonical 24h TTL come from `lib/auth-secret.js` (single source of truth; throws at module load if `JWT_SECRET` is unset). `getUserFromRequest` returns `null` for unauthenticated requests — no synthetic admin fallback — and login + register are rate-limited (5 attempts / 15 min via `@upstash/ratelimit`). The seed admin row (`admin@tcgvault.com` / `admin123`) still ships in `scripts/setup-neon-db.js`; see Gotcha #4.
|
||||
- **UI:** Tailwind CSS + custom CSS variables for theming (light/dark via `lib/theme-context.js`)
|
||||
- **Hosting:** Vercel (`vercel.json`, `.vercel/` present)
|
||||
|
||||
|
|
@ -31,9 +31,11 @@ Code graph is indexed by `user-code-review-graph` MCP (122 files, 628 nodes, 560
|
|||
|
||||
## 3. Key conventions
|
||||
|
||||
- **Auth (server):** `import { getUserFromRequest } from '../../lib/permission-middleware'` → returns `{ userId, email, role }` or `null`. **IMPORTANT: the current implementation returns a hardcoded admin user when no Bearer token is present — treat that as a known prod bug, do NOT copy the pattern.**
|
||||
- **Auth (server):** `import { getUserFromRequest } from '../../lib/permission-middleware'` → returns `{ userId, email, role }` or `null`. `null` means "send 401" — always early-return when the user is null before doing any work that depends on their identity.
|
||||
- **Auth (client):** `import { useAuth } from '../lib/use-auth'`. Avoid `lib/auth-context.js` and `lib/admin-auth.js` for new code — they are legacy parallel implementations.
|
||||
- **Auth helper (JWT only):** `import { ... } from '../../lib/api/auth-utils'` (`generateToken`, `verifyToken`, `hashPassword`, `verifyPassword`).
|
||||
- **JWT secret + TTL:** `import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js'`. This is the only place either value is defined; do not reintroduce literal fallbacks. `JWT_TOKEN_TTL = '24h'` is canonical.
|
||||
- **Auth helper (token mint / verify / password hash):** `import { ... } from '../../pages/api/auth-utils'` (`generateToken`, `verifyToken`, `hashPassword`, `verifyPassword`). Reads the secret + TTL from `lib/auth-secret.js` under the hood.
|
||||
- **Rate limiting:** `import { checkAuthRateLimit } from '../../lib/rate-limit.js'` for any new auth-surface endpoint (`/api/auth/login` + `/api/auth/register` already wired). Returns `{ allowed, remaining, reset }`; on `!allowed` return 429 with a `Retry-After` header. See `.cursor/rules/api-routes.mdc` § "Rate limiting" for the verbatim shape.
|
||||
- **Permission gate for collection routes:** wrap handlers with `withCollectionPermission('viewer' | 'editor' | 'owner')` from `lib/permission-middleware.js`.
|
||||
- **DB access:** Use **tagged-template** style — `import { sql } from '@vercel/postgres'`. Avoid the legacy `lib/database.js` `db.query(string, params)` API; its parameter interpolation uses `sql.unsafe` and is a SQL-injection vector.
|
||||
- **Activity logging:** `logCollectionActivity(collectionId, userId, action, details)` — call it from any handler that mutates a collection.
|
||||
|
|
@ -45,26 +47,31 @@ Code graph is indexed by `user-code-review-graph` MCP (122 files, 628 nodes, 560
|
|||
## 4. Common gotchas
|
||||
|
||||
- **#1 — Two SQL clients live in parallel.** `@neondatabase/serverless` (used by `lib/database.js`) and `@vercel/postgres` (used by most `pages/api/**` handlers). New code: prefer `@vercel/postgres` tagged templates. Migration to a single client is tracked in `.convoys/`.
|
||||
- **#2 — `getUserFromRequest` has a dev fallback shipped to prod.** When no Bearer token is present it returns user 1 as admin. This is a critical security issue, NOT a feature. Don't rely on it; treat unauthenticated requests as 401.
|
||||
- **#3 — JWT_SECRET default is hardcoded across 7 files.** If `process.env.JWT_SECRET` is unset, tokens are signed with `'your-secret-key-change-in-production'`. The Vercel project MUST set `JWT_SECRET`; CI/staging too.
|
||||
- **#4 — Default admin credentials are in the seed.** `admin@tcgvault.com` / `admin123` from `scripts/setup-neon-db.js`. Change the password immediately after running setup.
|
||||
- **#5 — `pages/api/setup-database.js` is a public endpoint.** Anyone hitting it triggers DB DDL. Either delete or gate behind admin auth before public launch.
|
||||
- **#2 — `getUserFromRequest` synthetic-admin fallback. RESOLVED** by `fix-auth-bypass` Brief 2 (commit `258e479`). The helper now returns `null` for unauthenticated requests; `pages/api/auth/verify.js` returns 401 on the no-token branch. The 16 unit tests in `test/lib/permission-middleware.test.js` lock in the contract, including a negative regression against the old synthetic-admin shape. Entry kept (not renumbered) to preserve the audit trail and stable cross-references.
|
||||
- **#3 — JWT_SECRET hardcoded across 7 files. RESOLVED** by `fix-auth-bypass` Brief 1 (commit `4a10dce`). `lib/auth-secret.js` is now the single source of truth and throws at module load when `JWT_SECRET` is unset. Canonical TTL is `JWT_TOKEN_TTL = '24h'`. The `'your-secret-key-change-in-production'` literal is gone from all 7 sites; CI lint passes against the post-fix tree. Entry kept (not renumbered) to preserve cross-references.
|
||||
- **#4 — Default admin credentials are in the seed.** `admin@tcgvault.com` / `admin123` from `scripts/setup-neon-db.js`. Change the password immediately after running setup. Tracked by the queued `drop-public-setup` convoy.
|
||||
- **#5 — `pages/api/setup-database.js` public endpoint. RESOLVED** by `fix-auth-bypass` Brief 3 (commit `fc0dd73`). The file is deleted along with the other three dev endpoints (`/api/simple`, `/api/test-auth`, `/api/test-db`), and `.github/workflows/ci.yml`'s new `forbidden-endpoints` job fails the build if any of them are re-introduced (or if a new `pages/api/test-*.js` file appears). Entry kept (not renumbered) to preserve cross-references.
|
||||
- **#6 — Migrations are bare scripts.** `scripts/add-*.js` and `scripts/fix-*.js` are run-once jobs with no idempotency tracking. Adopt `node-pg-migrate`, `kysely`, or `drizzle-kit` before more schema changes.
|
||||
- **#7 — Dual `is_public` semantics.** Collections and decks both have `is_public` columns; check which controls discovery vs. anonymous read in the relevant route.
|
||||
- **#8 — Layout has hardcoded default user.** `Layout({ user = { email: 'me@randallstillwell.com', role: 'user' } })`. Anything rendering Layout without passing `user` will impersonate the maintainer. Pass `user` explicitly from every page.
|
||||
- **#9 — `typescript` is a devDep, but the source is still JavaScript-only.** `package.json` lists `typescript@^5.9.3` purely so `eslint-config-next@16`'s bundled `typescript-eslint` chain can satisfy its hard `require('typescript')` at module load (the `peerDependenciesMeta.typescript.optional: true` flag in `eslint-config-next` only suppresses npm's install-time warning, not the runtime require). There is no `tsconfig.json`, no `.ts`/`.tsx` files, and no `// @ts-check` directives. Do not rename `.js` files to `.ts` or add a `tsconfig.json` without an explicit convoy decision — TypeScript adoption is its own scope. See `.convoys/bump-next-js.md` § Decisions C.
|
||||
- **#10 — ESLint pinned to v9 (maintenance), not v10 (latest).** `devDependencies.eslint` is `^9.39.4` even though `latest` is `10.4.0`. We tried v10 and `npm run lint` crashed with `TypeError: scopeManager.addGlobals is not a function` because `eslint-config-next@16`'s bundled `typescript-eslint@8.x` predates ESLint v10's redesigned global-ingestion path. Reverted to v9 under Decision D. **Do NOT bump ESLint independently** — wait for the queued `bump-eslint-10` follow-up convoy, which is upstream-blocked until `typescript-eslint` ships a v10-tested release that `eslint-config-next` bundles. See `.convoys/bump-next-js.md` § Decisions D + "Follow-up convoys queued".
|
||||
- **#11 — Turbopack is now the default bundler.** `next dev` and `next build` use Turbopack by default in Next.js 16. The fallback per command is `--webpack` (e.g. `next build --webpack`). We have no custom `webpack:` block in `next.config.js`, no custom loaders/aliases, and no Sass tilde imports, so Turbopack should "just work" — but if a build/runtime regression appears, reproduce on both bundlers before deciding whether to revert or pin a script to webpack. Do not pre-emptively switch to `--webpack`.
|
||||
- **#12 — Rate-limit env vars are `KV_REST_API_URL` / `KV_REST_API_TOKEN`, not `UPSTASH_REDIS_REST_*`.** `lib/rate-limit.js` reads the Vercel Upstash Marketplace integration's auto-provisioned names. Three other Upstash-shaped vars exist in the Vercel-managed env (`KV_URL`, `REDIS_URL`, `KV_REST_API_READ_ONLY_TOKEN`) but our `@upstash/redis` REST client does not use them — do not wire to them. In prod, the rate-limit module **fails closed** if either of the two REST vars is missing (a single failed login is a better outcome than silently disabling brute-force protection). In dev / test, it warn-and-continues as a no-op so local work is unaffected when Upstash isn't wired up.
|
||||
|
||||
## 5. Running locally
|
||||
|
||||
- **Runtime:** Node 20 (Vercel default).
|
||||
- **Setup:** `npm install`, copy `.env.local` template (POSTGRES_URL + JWT_SECRET + RESEND_API_KEY + BLOB_READ_WRITE_TOKEN), then `npm run setup-db` once.
|
||||
- **Setup:** `npm install`, copy `.env.local` template (POSTGRES_URL + JWT_SECRET + RESEND_API_KEY + BLOB_READ_WRITE_TOKEN; optionally KV_REST_API_URL + KV_REST_API_TOKEN to exercise the rate limiter locally — without them, `lib/rate-limit.js` warn-and-no-ops in dev), then `npm run setup-db` once.
|
||||
- **Dev server:** `npm run dev` → http://localhost:3000.
|
||||
|
||||
## 6. Testing
|
||||
|
||||
- **Runner:** None yet. Adding `vitest` + `@playwright/test` is in `.convoys/`. Until then: manual smoke per `TESTING_GUIDE.md`.
|
||||
- **Unit-test runner:** `vitest@^3.2.4` (installed via `fix-auth-bypass` Brief 5, commit `1629afb`). `npm test` for watch mode; `npm run test:run` for the CI / single-shot mode. Config in `vitest.config.js`, setup in `test/setup.js` (sets `JWT_SECRET` + `NODE_ENV=test` before any module loads). Specs live under `test/` mirroring source layout (`test/lib/*.test.js`, `test/api/*.test.js`).
|
||||
- **Coverage today:** 16 unit tests covering the post-`fix-auth-bypass` auth surface — `lib/auth-secret.js` (3), `lib/permission-middleware.js::getUserFromRequest` (8, incl. a negative regression against the old synthetic-admin shape — Gotcha #2), and `pages/api/auth-utils.js` (5). These tests lock in the contracts established by Briefs 1 and 2; do not weaken them when refactoring auth.
|
||||
- **CI:** the `test:` job in `.github/workflows/ci.yml` runs `npm run test:run` on every PR and push to `main` and is **blocking** (no `|| true`, no `continue-on-error`). A red test job blocks merge.
|
||||
- **E2E / smoke runner:** `@playwright/test` is **still pending** — queued for the `adopt-playwright-smoke` convoy (see `.convoys/ship-readiness.md` § Proposed launch sequence step 10). Until it lands, `preview-smoke.yml` and `visual-diff.yml` are no-ops on the smoke side.
|
||||
- **Manual QA:** `TESTING_GUIDE.md` still applies for surfaces not yet covered by automated tests (UI flows, scanner camera path, import jobs).
|
||||
|
||||
## 7. Deployment
|
||||
|
||||
|
|
|
|||
18
README.md
18
README.md
|
|
@ -13,10 +13,12 @@ A modern trading card game collection manager built with Next.js and Neon Databa
|
|||
|
||||
## 🛠️ Tech Stack
|
||||
|
||||
- **Frontend**: Next.js 15, React 18, TypeScript
|
||||
- **Frontend**: Next.js 16 (Pages router), React 18, JavaScript (TypeScript is a devDep only — see `AGENTS.md` Gotcha #9)
|
||||
- **Backend**: Next.js API Routes
|
||||
- **Database**: Neon PostgreSQL (serverless)
|
||||
- **Authentication**: JWT with bcrypt
|
||||
- **Authentication**: JWT with bcrypt (24-hour expiry; `lib/auth-secret.js` is the single source of truth for `JWT_SECRET`)
|
||||
- **Rate limiting**: `@upstash/ratelimit` on `/api/auth/login` + `/api/auth/register` (5 attempts / 15 min per IP)
|
||||
- **Testing**: Vitest (unit); Playwright queued
|
||||
- **Styling**: Tailwind CSS
|
||||
- **Deployment**: Vercel
|
||||
|
||||
|
|
@ -38,11 +40,17 @@ A modern trading card game collection manager built with Next.js and Neon Databa
|
|||
cp .env.example .env.local
|
||||
```
|
||||
|
||||
Update `.env.local` with your Neon database URL:
|
||||
Update `.env.local` with your Neon database URL and a real JWT secret:
|
||||
```env
|
||||
POSTGRES_URL="postgresql://your-username:your-password@your-host/your-database"
|
||||
JWT_SECRET="your-super-secret-jwt-key"
|
||||
JWT_SECRET="<generate with: openssl rand -hex 32>"
|
||||
# Optional — exercise the rate limiter locally. Without them, `lib/rate-limit.js`
|
||||
# warn-and-no-ops in dev. In production these are auto-provisioned by the
|
||||
# Vercel Upstash Marketplace integration.
|
||||
KV_REST_API_URL="https://<your-upstash-host>.upstash.io"
|
||||
KV_REST_API_TOKEN="<your-upstash-rest-token>"
|
||||
```
|
||||
`JWT_SECRET` is **required** — `lib/auth-secret.js` throws at import time if it's unset.
|
||||
|
||||
4. **Set up the database**
|
||||
```bash
|
||||
|
|
@ -77,6 +85,8 @@ The application uses the following tables:
|
|||
### Health Check
|
||||
- `GET /api/health` - Application health
|
||||
|
||||
> **Note:** Earlier revisions of this README also listed `GET /api/test-db` (and three other unauthenticated dev endpoints: `/api/simple`, `/api/test-auth`, `/api/setup-database`). All four were deleted in `fix-auth-bypass` Brief 3 (commit `fc0dd73`) and CI now blocks their reintroduction. Don't recreate them.
|
||||
|
||||
## 🚀 Deployment
|
||||
|
||||
This app is configured for deployment on Vercel:
|
||||
|
|
|
|||
Loading…
Reference in a new issue