convoy(fix-auth-bypass): architect plan + 5 briefs (Wave A/B/C dispatch)
Architect pass for P0 security-critical convoy (closes ship-blockers #1, #2, #4, #5, #6 partial). Produces 5 briefs with explicit slice_dependencies for /multitask fan-out. Decomposition: - Brief 1: Central JWT secret helper + 24h token TTL (8 files, ~120 LOC) - Brief 2: Remove the synthetic-admin bypass (2 files, ~25 LOC net negative) - Brief 3: Delete 4 dev-only endpoints + CI guard (6 files, ~30 LOC) - Brief 4: Tighten auth surface — CORS + rate limit (5 files, ~150 LOC) - Brief 5: Install vitest + auth tests + re-enable CI test job (8 files, ~280 LOC) Total estimate: ~600 LOC across 5 PRs. All under 400-LOC budget. Wave A (parallel from t=0): Briefs 1 + 3 (disjoint files) Wave B (parallel after Brief 1): Briefs 2 + 4 (disjoint subsets of Brief 1's exports) Wave C (after Briefs 2 + 4): Brief 5 alone (lockfile sequencing + functional dep on Brief 2's null contract) Architect's calls (3 decisions documented in convoy file Decisions log): - Token TTL = 24h (matches current login.js UX; security-conservative) - Rate-limit = @upstash/ratelimit@^2.0.8 + @upstash/redis@^1.38.0 (DIY-Postgres needs schema change OOS; DIY-memory broken on Vercel cold starts; next-rate-limit is stale) - Vitest in this convoy (not split to adopt-vitest); pinned to ^3.2.4 to dodge vitest@4's non-optional vite peer dep Boot-the-brief findings (9 verifications, 0 revisions): - 24/24 getUserFromRequest callers already handle null correctly — Brief 2 is safer than the convoy file predicted - 7 JWT_SECRET literal sites match AGENTS.md gotcha #3 exactly - Dev endpoints have zero runtime references (only doc references) — safe to delete - Cross-brief commitments declared in both directions for every Brief-1 -> {2,4,5} pair Risk list: 12 risks documented (R1-R12). Headlines: - R1: JWT_SECRET fail-loud throws may break unexpected import chains - R3: existing tokens stop verifying once literal fallback removed (one-time "log back in" pre-launch is acceptable) - R5-R6: rate-limit IP extraction + Upstash quota; fail-open mitigation - R10: JWT_SECRET rotation now requires a deploy (no silent fallback) Pre-merge env-var checklist (user action required before Brief 4 ships): - UPSTASH_REDIS_REST_URL (new — Vercel project settings) - UPSTASH_REDIS_REST_TOKEN (new — Vercel project settings) - JWT_SECRET (verify already set — no fallback any more) Awaiting human gate 1 (plan approval) before implementers run. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Randall Stillwell
parent
ebd4fd1397
commit
1667b87ee3
6 changed files with 999 additions and 0 deletions
|
|
@ -99,3 +99,204 @@ 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."*
|
> *"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.
|
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.
|
||||||
|
|
|
||||||
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.
|
||||||
152
.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md
Normal file
152
.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md
Normal file
|
|
@ -0,0 +1,152 @@
|
||||||
|
---
|
||||||
|
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.UPSTASH_REDIS_REST_URL` and `process.env.UPSTASH_REDIS_REST_TOKEN` are both set: construct `new Redis({ url, token })` and `new Ratelimit({ redis, limiter: Ratelimit.slidingWindow(5, '15 m'), prefix: 'tcgvault:auth' })`.
|
||||||
|
- 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] Upstash not configured — 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 `UPSTASH_REDIS_REST_URL` and `UPSTASH_REDIS_REST_TOKEN` (if you have an Upstash free-tier account). 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`, set `UPSTASH_REDIS_REST_URL` and `UPSTASH_REDIS_REST_TOKEN` in the Vercel project settings (Production + Preview environments).** Without these, the prod auth endpoints will throw on first login attempt (intentional fail-closed). Free-tier Upstash Redis 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.
|
||||||
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.
|
||||||
Loading…
Reference in a new issue