12 changed files with 2781 additions and 661 deletions
370
.convoys/bump-next-js.md
Normal file
370
.convoys/bump-next-js.md
Normal file
|
|
@ -0,0 +1,370 @@
|
|||
---
|
||||
name: bump-next-js
|
||||
classification: feature
|
||||
success_metric: "npm install next@16.2.6 ships, Vercel deploys complete, no runtime regressions in dev or build."
|
||||
skip:
|
||||
- ia
|
||||
- ux
|
||||
- flag
|
||||
status: open
|
||||
created: 2026-05-22
|
||||
---
|
||||
|
||||
# Convoy: bump-next-js
|
||||
|
||||
Closes P0 ship-blocker **#8** from `.convoys/ship-readiness.md`. Highest-priority convoy in the launch sequence — promoted to slot 0 because Vercel is currently refusing to deploy any branch (including `main`) until Next.js is bumped, which makes every downstream `preview-smoke` / `visual-diff` gate non-functional.
|
||||
|
||||
## Why
|
||||
|
||||
Vercel's platform-level security gate is blocking every deployment with `"Vulnerable version of Next.js detected, please update immediately"`. The lockfile currently resolves `next@15.4.3`; latest is `16.2.6`. The build itself completes (Vercel CLI confirms `Build Completed in /vercel/output [29s]`), but the deployment is rejected before going live.
|
||||
|
||||
Concrete impact, as of 2026-05-22:
|
||||
|
||||
- **The last successful deploy on `main` was 2025-08-01.** Production is stale.
|
||||
- **Preview deployments are unavailable** on every PR. `preview-smoke.yml` and `visual-diff.yml` have nothing to point at, so they fail-quiet on every PR.
|
||||
- **PR #1 (the bootstrap PR) cannot validate its own L3 visual gates** because of this.
|
||||
|
||||
This convoy unblocks the entire launch sequence. Until it ships, the other 13 convoys are running half-blind. Success looks like:
|
||||
|
||||
1. `package.json` declares `"next": "^16.2.6"` (or whatever the architect picks — see scope).
|
||||
2. `package-lock.json` regenerated.
|
||||
3. `npm run dev` boots without warnings about deprecated APIs.
|
||||
4. `npm run build` exits 0 with no breaking-change errors.
|
||||
5. A PR opened from a feature branch produces a **successful** Vercel preview deploy.
|
||||
6. `preview-smoke` and `visual-diff` workflows have a live URL to hit (they'll still fail on missing `@playwright/test` until `adopt-vitest` lands, but the Vercel half is no longer broken).
|
||||
7. CI green: lint passes (wrapper is in place from bootstrap), aggregate gate passes.
|
||||
|
||||
## Scope
|
||||
|
||||
**In:**
|
||||
|
||||
- Bump `next` from `15.4.3` to `16.2.6` in `package.json` + `package-lock.json`.
|
||||
- Bump `eslint-config-next` from `15.4.2` to a matching `16.x` release to keep the lint config aligned with the framework.
|
||||
- Audit Next.js 15 → 16 migration guide ([blog](https://nextjs.org/blog/next-16), [upgrade guide](https://nextjs.org/docs/app/building-your-application/upgrading)) and identify which surfaces in `tcg-vault` are affected. Educated guess at affected paths (validate during architect):
|
||||
- `next.config.js` — the `images.domains` field has been deprecated for several major versions; if Next 16 drops it, migrate to `images.remotePatterns`.
|
||||
- `next/image` usage across `pages/cards.js`, `pages/card/[id].js`, `components/CollectionSelectionModal.js`, `components/ManaSymbols.js`, `components/UploadImageModal.js` — verify props are still supported.
|
||||
- Pages Router specifics — Pages Router is intentionally more stable than App Router across major bumps, but `getServerSideProps` / `getStaticProps` semantics may have edge-case changes.
|
||||
- API routes — `req` / `res` API stays stable in Pages Router; should be a no-op surface.
|
||||
- Middleware — `tcg-vault` has no `middleware.js` currently; nothing to migrate.
|
||||
- Update `AGENTS.md` "Tech stack quick reference" to bump the Next.js version string.
|
||||
- Validate via local `npm run build`, then push to confirm Vercel preview deploys successfully.
|
||||
|
||||
**Out (deferred to their own convoys):**
|
||||
|
||||
- **React 18 → 19 upgrade.** `next@16` peer-deps accept `react@^18.2.0 || ^19.0.0`. Current `react@18.3.1` is in range. A React 19 bump is its own convoy (`bump-react`) because of compiler / Suspense / `use()` API changes.
|
||||
- **App Router migration.** `tcg-vault` is on Pages Router. Migrating to App Router is a multi-month effort and outside this convoy.
|
||||
- **Test runner adoption** (`adopt-vitest` / `adopt-playwright-smoke`) — those convoys remain queued.
|
||||
- **`.eslintrc.json` rule tuning** — the bootstrap added a stub extending `next/core-web-vitals`. If `eslint-config-next@16` ships new rules that surface additional errors, defer the cleanup to `fix-lint-baseline`.
|
||||
|
||||
**Hard "do not touch" in this convoy:**
|
||||
|
||||
- No auth code (`lib/permission-middleware.js`, `pages/api/auth/`, `pages/api/auth-utils.js`) — that's `fix-auth-bypass`.
|
||||
- No DB code.
|
||||
- No new features or UI changes beyond what's strictly required to keep existing pages rendering after the bump.
|
||||
- No CODEOWNERS / workflow / convoy file edits.
|
||||
- No feature flags. The bump ships unflagged.
|
||||
|
||||
## Roles invoked
|
||||
|
||||
Per `feature` classification with custom skips (`ia, ux, flag`):
|
||||
|
||||
1. **role-architect** — produces a slice plan. Reads the Next 16 migration guide, lists every breaking change that touches `tcg-vault`, decides which need code changes vs. configuration changes vs. no-ops. Output: 1–3 briefs under `.convoys/bump-next-js/brief-N-*.md`. Likely shape:
|
||||
- Brief 1: the bump itself (package.json + lockfile + any required `next.config.js` migration).
|
||||
- Brief 2 (if needed): code changes for any deprecated APIs (e.g. `<Image>` prop rename).
|
||||
- Brief 3 (if needed): visual-diff baseline refresh if rendering changed.
|
||||
2. **role-implementer** — single-writer flow. The bump itself is one file change + lockfile; can't be meaningfully parallelized.
|
||||
3. **Audit fan-out** (`/multitask`, group id `audit-bump-next-js-<pr>`) — runs in parallel after the PR is drafted:
|
||||
- **role-reviewer** — correctness, regression risk
|
||||
- **role-design-system-auditor** — verify CSS / theming / token usage still renders correctly
|
||||
- **role-a11y-auditor** — verify accessibility didn't regress (Next.js 16 may change focus-management defaults)
|
||||
4. **role-doc-writer** — last. Updates `AGENTS.md` "Tech stack" section. Adds an entry to a CHANGELOG if one is started here (it'll be backfilled separately in `launch-polish`).
|
||||
|
||||
## Todos
|
||||
|
||||
High-level checklist for the architect to refine into briefs:
|
||||
|
||||
- [ ] **Brief 1 — Migration audit.** Read the [Next.js 16 release notes](https://nextjs.org/blog/next-16) and [upgrade guide](https://nextjs.org/docs/app/building-your-application/upgrading). Produce a short table: deprecated API → file(s) that use it → migration step. Specifically check: `images.domains` deprecation, `next/font` changes, `next/image` prop changes, any default-runtime changes (edge vs node).
|
||||
- [ ] **Brief 2 — Bump + lockfile.** `npm install next@16.2.6 eslint-config-next@^16`. Commit `package.json` + `package-lock.json`. Verify `npm ls next` shows the new version.
|
||||
- [ ] **Brief 3 — Verify build + dev locally.** `npm run build` must exit 0 with no breaking-change errors. `npm run dev` must boot without deprecation warnings on the routes we ship today. If errors surface, this is where they get fixed.
|
||||
- [ ] **Brief 4 — Vercel preview deploy.** Push the branch and confirm the Vercel deploy completes successfully (status moves from `pending` → `success`, not `Error`). Capture the preview URL in the PR description.
|
||||
- [ ] **Brief 5 — Visual diff baseline.** If `preview-smoke.yml` / `visual-diff.yml` aren't installed yet (they need `@playwright/test`), this brief is informational — flag any obvious visual changes to the reviewer + design-system-auditor. Once `adopt-playwright-smoke` lands, this becomes a real verification step.
|
||||
- [ ] **Doc-writer pass.** Update `AGENTS.md` tech-stack line. Note the bump in the bootstrap PR's "Notes for reviewer" or, if PR #1 has merged by then, open a small standalone docs PR.
|
||||
|
||||
## Hand-off
|
||||
|
||||
**Next role: `role-architect`** (IA + UX are skipped; routing straight to Architect).
|
||||
|
||||
To run it in a new chat, paste:
|
||||
|
||||
> *"Run role-architect on convoy `bump-next-js`. Read `.convoys/bump-next-js.md` for scope and todos, then read the Next.js 15 → 16 upgrade guide and produce a slice plan. Output briefs to `.convoys/bump-next-js/brief-N-*.md`. Mark any briefs that are parallel-safe (probably none — this is mostly a single-writer flow except the audit fan-out). Be conservative about scope creep: if the migration guide flags an API not used in `tcg-vault`, note it in the brief but don't add a 'while we're here' fix."*
|
||||
|
||||
After architect publishes the brief(s), the user runs `role-implementer` serially. Once the PR is drafted, the user uses Cursor 3.2 `/multitask` to dispatch the audit cohort (`reviewer + design-system-auditor + a11y-auditor`) in parallel under group id `audit-bump-next-js-<pr>`.
|
||||
|
||||
Conductor exits here.
|
||||
|
||||
## Architecture
|
||||
|
||||
_Produced by `role-architect` on 2026-05-22 against Next.js 16.2.6 (latest stable; verified via `npm view next version`). Updated 2026-05-23 after four gate-1 scope changes (A, B, C, D — see Decisions log below):_
|
||||
|
||||
- _A: scope expanded to include the ESLint v8 → v9 + flat-config migration so `eslint-config-next` can move to `^16` matching `next`._
|
||||
- _B: pivoted from ESLint v9 to v10 (then-`latest`) after gate-1 re-review of risk R14._
|
||||
- _C: added `typescript@^5.9.3` as a devDep after an implementer escalation surfaced that `eslint-config-next@16`'s `peerDependenciesMeta.typescript.optional: true` annotation does not make `typescript` runtime-optional._
|
||||
- _D: reverted the v10 pivot back to v9.39.4 after a pass-2 implementer escalation showed Risk R15 firing empirically (`TypeError: scopeManager.addGlobals is not a function` from `@typescript-eslint/scope-manager@8.59.4` predating v10 GA). v10 deferred to the upstream-blocked `bump-eslint-10` follow-up convoy._
|
||||
|
||||
### File plan
|
||||
|
||||
| File | Action | Purpose |
|
||||
| --- | --- | --- |
|
||||
| `package.json` | modified | Bump `dependencies.next` from `^15.4.2` to `^16.2.6`. Bump `devDependencies.eslint` from `^8` to `^9.39.4` (npm's `maintenance` dist-tag; per Decisions log entry D, reverted from the v10 pin set under entry B after R15 fired empirically). Bump `devDependencies.eslint-config-next` from `15.4.2` to `^16.2.6` to match `next` — peer-dep `eslint: >=9.0.0` accepts v9.39.4 trivially. **Add `devDependencies.typescript: "^5.9.3"`** (per Decisions log entry C — `eslint-config-next@16` bundles `typescript-eslint`, which hard-requires `typescript` at module load under both v9 and v10; the `peerDependenciesMeta.typescript.optional: true` flag only suppresses npm's install-time warning, not the runtime require). Replace `scripts.lint` from `next lint` to `eslint .` (Next 16 removed the `next lint` command). React, react-dom, and all other packages stay unchanged. |
|
||||
| `package-lock.json` | modified | Regenerated by `npm install`. Reflects the new `next@16.2.6`, `eslint@^9.39.4`, `eslint-config-next@^16.2.6`, and `typescript@^5.9.3` resolutions. Under Decision D the lockfile stays on the v9 dep-tree (`@eslint/eslintrc` is still a v9 transitive dep; the v10 dep-tree changes that would have removed it are deferred to the queued `bump-eslint-10` follow-up convoy). The new `typescript` subtree is small — `typescript` itself has no `dependencies` and no `peerDependencies`. Do not hand-edit. |
|
||||
| `next.config.js` | modified | Migrate `images.domains: [...]` (deprecated in 16, deprecation warning at startup) to `images.remotePatterns: [...]`. Three patterns, one per CDN currently in `images.domains`. |
|
||||
| `.eslintrc.json` | **deleted** | The 40-byte legacy stub (`{"extends": "next/core-web-vitals"}`) is replaced by `eslint.config.mjs` because `eslint-config-next@16` only supports flat config. Leaving both files in place would be a footgun. |
|
||||
| `eslint.config.mjs` | **new** | Flat-config replacement for `.eslintrc.json`. Reproduces the prior `next/core-web-vitals` extends behavior using the verbatim shape from the official Next.js docs (`defineConfig([...nextVitals, globalIgnores([...])])`). `globalIgnores` covers the no-go-zone paths the user specified at gate 1 plus `eslint-config-next`'s documented defaults. |
|
||||
|
||||
Doc-writer's `AGENTS.md` "Tech stack" string update is a separate PR by `role-doc-writer` after this one merges (per the convoy's Roles list).
|
||||
|
||||
### API surface
|
||||
|
||||
**No API changes.** This convoy does not touch `pages/api/**`. The Next.js 16 upgrade guide does not change the Pages-Router `req`/`res` handler signature; `tcg-vault`'s ~30 API handlers all use the legacy `(req, res) => { ... }` shape and continue to work unchanged.
|
||||
|
||||
### Schema diff
|
||||
|
||||
**No schema changes.** This convoy does not touch the database. Neon Postgres + `scripts/setup-neon-db.js` are out of scope.
|
||||
|
||||
### Test plan
|
||||
|
||||
`tcg-vault` has no automated test runner installed yet (vitest + Playwright adoption is tracked under `adopt-vitest` and `adopt-playwright-smoke` convoys). For this convoy:
|
||||
|
||||
- **Manual smoke per `TESTING_GUIDE.md`** is the verification mechanism. Specifically: home (`/`), login (`/login`), signup (`/signup`), browse (`/cards`), and `/collections` must render without runtime errors after the bump.
|
||||
- **`npm run build` exiting 0** is the integration test for the Turbopack default-bundler change. No custom webpack config exists in `next.config.js`, so Turbopack should "just work."
|
||||
- **`npm run lint` running to completion** (regardless of the error count) is the integration test for the ESLint v8 → v9 + flat-config migration (per Decision D — Decision B's pivot to v10 was reverted after R15 fired empirically). The pre-existing baseline of ~100 errors is expected to shift modestly under v9 due to plugin major bumps (`eslint-plugin-react-hooks` v5 → v7, `@next/eslint-plugin-next` 15 → 16) but **not** to shift the way v10 would have (no new `eslint:recommended` rules, no JSX reference tracking, no `no-shadow-restricted-names.reportGlobalThis: true` default — those land later under `bump-eslint-10`). CI's `|| true` wrapper continues to tolerate any non-zero exit. **Counting baseline drift is `fix-lint-baseline`'s job, not this convoy's.** **If `npm run lint` does not run to completion under v9.39.4** — e.g. it crashes with a `TypeError` — see Brief 1's failure-mode classifier under "Local verification." Decision D's expectation is that R15's empirical signature (`scopeManager.addGlobals is not a function`) does NOT recur on v9 because v9 doesn't call `addGlobals`. If a different `TypeError` fires on v9, escalate rather than patching transitive deps.
|
||||
- **Vercel preview deploy reaching Success state** is the end-to-end integration test. The convoy's `success_metric` ("npm install next@16.2.6 ships, Vercel deploys complete, no runtime regressions in dev or build") is exactly this.
|
||||
- **Audit fan-out (`/multitask`, group id `audit-bump-next-js-<pr>`)** is the qualitative gate: `role-reviewer` for correctness, `role-design-system-auditor` for token rendering, `role-a11y-auditor` for focus / scroll-behavior regression. They run AFTER the PR is drafted, not as part of this brief.
|
||||
- Once `adopt-vitest` lands, retrofit a smoke test for `next.config.js` parsing and one for `<img>` (or future `<Image>`) rendering against a fixture page.
|
||||
|
||||
### Risk list
|
||||
|
||||
- **R1: Turbopack-by-default may surface unexpected build/runtime differences vs webpack.** Per gate-1 decision: accept the default. tcg-vault has no `webpack:` block in `next.config.js`, no custom loaders/aliases, no Sass tilde imports, no `resolve.fallback` workarounds. Likelihood of regression: low. **Fallback per command:** `next build --webpack` and `next dev --webpack`. **If a regression appears, the implementer should reproduce on both bundlers** (run the failing flow once with the default, once with `--webpack`) **before deciding whether to revert the bump or pin the script to webpack.** Capture the reproduction in the PR description for `role-reviewer` to triage. Do not pre-emptively add `--webpack` to the scripts.
|
||||
- **R2 — RESOLVED at gate 1, via the A → B → D path.** Originally: "`eslint-config-next` cannot be bumped to `^16` in this convoy." Gate-1 decision A expanded scope to include the ESLint v8 → v9 + flat-config migration. Decision B pivoted from v9 to v10. Decision D reverted v10 → v9.39.4 after R15 fired empirically on the implementer's pass-2 lint run. **Final pins:** `eslint@^9.39.4`, `eslint-config-next@^16.2.6`, `typescript@^5.9.3`, `.eslintrc.json` deleted, `eslint.config.mjs` created. See R12, R13, R14, R15 below for the residual + reinstated risks. The deferred `migrate-to-eslint-flat-config` convoy is **closed before opening** — its work has been folded in. The `bump-eslint-10` follow-up convoy is **queued as upstream-blocked** — see "Follow-up convoys queued" section.
|
||||
- **R3: `next lint` removal hard-breaks `npm run lint`.** Without the `scripts.lint` change, both local devs and CI's `npm run lint --if-present` job would invoke a removed command. Mitigation: change script to `eslint .`. CI's existing `|| true` wrapper continues to tolerate the pre-existing lint baseline (~100 errors, tracked under `fix-lint-baseline`).
|
||||
- **R4: `images.domains` is in `next.config.js` but `next/image` isn't actually used.** Strictly speaking, the migration is preemptive — silences the deprecation warning but adds no functional change. Acceptable: keeps the config valid for the eventual `next/image` adoption. Don't delete the block; that would force re-adding it later.
|
||||
- **R5: `images.minimumCacheTTL` default changed from 60s to 4h.** Behavior change. Not impactful in `tcg-vault` because `next/image` isn't used. No mitigation required; flag here only so future readers don't re-investigate.
|
||||
- **R6: Vercel deploy might fail for an unrelated reason.** The convoy's premise is that the platform-level "Vulnerable version" gate is the sole blocker. If the build itself fails on 16 (e.g. an undocumented Turbopack edge case), the fix lands in this brief. If the failure is environmental (env vars, build settings), escalate — that's a different convoy.
|
||||
- **R7: React 18 stays — intentional.** `next@16` peer-dep accepts `react ^18.2.0 || ^19.0.0`. Current `18.3.1` is in range. **Do not bump React in this convoy.** React 19 has compiler / Suspense / `use()` API changes and is its own convoy (`bump-react`).
|
||||
- **R8: TypeScript >=5.1.0 required by Next 16.** Partially applicable. **`tcg-vault` source code remains plain JavaScript** — no `tsconfig.json`, no `.ts`/`.tsx` files, no source migration in this convoy. **However, per Decision C (2026-05-23), `typescript@^5.9.3` is now installed as a devDep** because `eslint-config-next@16`'s bundled `typescript-eslint` chain hard-`require`s it at module load. The original parenthetical claim — "`eslint-config-next@16` lists `typescript` as an optional peer (`peerDependenciesMeta.typescript.optional: true`) so JS-only consumers are fine" — was wrong: that flag only suppresses npm's install-time warning; the transitive `@typescript-eslint/typescript-estree@8.59.4` (a regular `dependency`, not a peer) does an unconditional `require('typescript')` at module load. See R16 for the full devDep impact analysis.
|
||||
- **R9: Node.js floor — DEFANGED under Decision D.** Originally elevated under Decision B because ESLint v10 raised the floor to `^20.19.0 || ^22.13.0 || >=24`. **Under Decision D's v9.39.4 pin**, the ESLint floor reverts to `^18.18.0 || ^20.9.0 || >=21.1.0` (Next 16 also requires `>=20.9.0` — the same floor). CI's `setup-node@v4` `node-version: '20'`, local `node@22.14.0`, and Vercel's default Node 22 all satisfy with margin to spare. The "moving target on `node-version: '20'`" concern from Decision B is inert under v9. **The constraint will reactivate when `bump-eslint-10` lands**; the queued follow-up convoy should pick up the CI pin question (`node-version: '20.19'` or `'lts/iron'`) at that point.
|
||||
- **R10: `next dev` and `next build` now use separate output dirs (`.next/dev/` vs `.next/`).** `.gitignore` line 28 has `/.next/`, which is a directory rule that covers both subdirs. No `.gitignore` change needed.
|
||||
- **R11: Convoy file's audit list (line 45) is wrong about `next/image` usage.** Pages listed (`pages/cards.js`, `pages/card/[id].js`, etc.) use plain `<img>` tags, not `<Image>`. Architect verified via `rg "from ['\"]next/image['\"]"` — zero hits in `pages/`, `components/`, `lib/`. Surface this to the convoy author so future planning is not based on the same assumption.
|
||||
- **R12 (post-gate-1 expansion; revised under Decision D): ESLint flat-config migration + plugin major bumps will shift the lint baseline modestly.** Drivers under v9.39.4: `eslint-config-next@16.2.6` bundles `eslint-plugin-react-hooks@^7` (vs v5) and `@next/eslint-plugin-next@16` (vs 15.4.2). **The v10-specific drivers from Decision B's wording are deferred to the queued `bump-eslint-10` follow-up** (the three new `eslint:recommended` rules, JSX reference tracking, `no-shadow-restricted-names.reportGlobalThis: true` default). `eslint-env` comments would be errors under v10 — we have zero (`rg "eslint-env"` returned zero hits, ✓), so the `bump-eslint-10` follow-up will not snag here either. **The ~100-error baseline is approximate and will move modestly under v9, more substantially when v10 lands.** CI's `|| true` wrapper tolerates any non-zero exit, so this is non-blocking either way. **Do not "fix while we're here."** `fix-lint-baseline` will reconcile against whichever baseline is current.
|
||||
- **R13: Native flat-config import path is verbatim from Next.js docs — no `FlatCompat` shim added.** Boot-the-brief verified by extracting the published tarball that `eslint-config-next/core-web-vitals` exports a flat-config array (`module.exports = config`). Under Decision D's v9 pin, `@eslint/eslintrc` is still part of v9's own dep tree (v10 dropped it), so the lockfile retains it as a transitive dep — but we still don't import `FlatCompat` from it. If for any reason the native flat-config export resolution fails at install time (e.g. a transitive dep mismatch), the implementer should NOT swap in `@eslint/eslintrc`'s `FlatCompat` — instead, raise it in the PR description and the architect will revisit.
|
||||
- **R14 — REINSTATED under Decision D (2026-05-23).** ESLint v10.4.0 is the current `latest` dist-tag; this convoy pins `eslint@^9.39.4` (the `maintenance` dist-tag) per Decision D until upstream `eslint-config-next` ships a release that bundles a v10-tested `typescript-eslint`. **Tracked under follow-up convoy `bump-eslint-10` (currently upstream-blocked)** — see "Follow-up convoys queued" section. The previous "RESOLVED at gate 1 (Decision B)" framing was correct given Boot-the-brief evidence at the time; Decision D reverses it specifically because empirical lint runs surfaced R15 firing. **Cost of pinning to maintenance:** small. v9.39.4 still receives security backports if any are needed during the window before `bump-eslint-10` lands; the v9 → v10 jump is a single-line `package.json` edit when prerequisites are met (no flat-config edits required — same `defineConfig` + `globalIgnores` shape works on both majors).
|
||||
- **R15 — FIRED EMPIRICALLY (pass-2 implementer run, 2026-05-23); RESOLVED BY DECISION D.** Originally framed as: "`eslint-config-next@16.2.6`'s bundled plugin set was published before ESLint v10 (Oct 2025 vs Feb 2026); v10 runtime compatibility is statically unprovable." **What actually fired** was a different (and worse) failure mode than the originally feared `context.getCwd()` / `SourceCode#getJSDocComment()` deprecated-API removals:
|
||||
- **Crash signature:** `TypeError: scopeManager.addGlobals is not a function`
|
||||
- **Call site:** ESLint v10's `lib/source-code/source-code.js:221` calls `scopeManager.addGlobals(...)` as part of v10's redesigned global-ingestion path.
|
||||
- **Missing-method site:** `@typescript-eslint/scope-manager@8.59.4` (transitive dep of `typescript-eslint@8.59.4`, which `eslint-config-next@16.2.6` bundles as a regular `dependency`) does not implement `addGlobals` on its `ScopeManager` class. The method is a v10-introduced extension; v9 used a different ingestion path that `typescript-eslint@8.x` was authored against.
|
||||
- **Why bundled-plugin set didn't help:** `@typescript-eslint/scope-manager@8.x` was published Oct/Nov 2025; v10 GA was 2026-02-06. `typescript-eslint` has not yet shipped a v10-tested release. The peer-dep range `eslint: >=9.0.0` is technically satisfied by v10, but the runtime compatibility was not.
|
||||
- **Resolution (Decision D):** revert `eslint` to `^9.39.4`. The same `typescript-eslint@8.59.4` works correctly on v9 because v9 doesn't call `addGlobals`. `typescript@^5.9.3` (Decision C) is retained — that install was confirmed correct on pass 2 and is required under both v9 and v10.
|
||||
- **R15 stays in the convoy's risk list as FIRED-RESOLVED** so the historical record is preserved and so the queued `bump-eslint-10` follow-up convoy inherits the diagnostic verbatim. The originally feared deprecated-API removals (`context.getCwd()`, etc.) are still live risks for the eventual v10 cutover, but they did not fire on pass 2 — `addGlobals` fired first.
|
||||
- **R16 (new, post-gate-1 Decision C; status unchanged under Decision D): Adding `typescript` as a devDep brings `typescript@^5.x` and its tooling into the dep tree.** This is universally how JS-only Next.js projects handle `eslint-config-next@16` — the package's `typescript-eslint` transitive dep (specifically `@typescript-eslint/typescript-estree@8.59.4`'s `dist/convert.js:40`) hard-requires `typescript` at runtime despite being flagged `peerDependenciesMeta.optional: true` at the `eslint-config-next` wrapper level (the `optional` annotation only suppresses npm's install-time warning, not the runtime require). **Confirmed correct under Decision D's v9 pin** — pass-2 implementer evidence shows the `typescript` install resolved the original `Cannot find module 'typescript'` crash; the residual `addGlobals` crash was a different failure mode (R15) and is the reason for the v9 revert. No downstream impact expected: `typescript` only runs when lint runs (the JS source code is unchanged, no `tsconfig.json` is created, no `.js` files are renamed); `fix-lint-baseline` and `adopt-vitest` convoys will not be affected. **Engines:** `typescript@5.9.3` requires `node >= 14.17`, well below ESLint v9's `^18.18.0` floor (and v10's `^20.19.0` floor when `bump-eslint-10` lands) — no new Node constraint introduced. **Lockfile impact:** small — `typescript` has no `dependencies` and no `peerDependencies`. **Verified runtime require evidence:** see Brief 1's Boot-the-brief finding #17 for the verbatim 9-site grep of `require('typescript')` in the published `typescript-estree@8.59.4` tarball, all unconditional (no `try/catch`, no dynamic import, no `require.resolve` guard).
|
||||
### Decomposition
|
||||
|
||||
| Brief # | Title | Files | Depends on | Estimated PR size |
|
||||
| --- | --- | --- | --- | --- |
|
||||
| 1 | Bump Next.js to 16.2.6 + migrate `next.config.js`, ESLint flat config, and lint script | `package.json` (mod — `next`, `eslint`, `eslint-config-next` bumps + new `typescript` devDep per Decision C), `package-lock.json` (mod), `next.config.js` (mod), `eslint.config.mjs` (new), `.eslintrc.json` (deleted) | _(none)_ | 5 files touched (3 mod, 1 new, 1 deleted), lockfile regen (large auto-diff). True non-lockfile diff: ~31 LOC (~12 of which is the new `eslint.config.mjs`; +1 LOC for the `typescript` devDep line in `package.json`). |
|
||||
|
||||
**Still one brief, even after four gate-1 scope changes (A, B, C, D).** Both the Next bump and the ESLint migration touch `package.json`, so they cannot run in parallel anyway — keeping them in one brief gives reviewers one PR, one Vercel preview, and one revert boundary if anything regresses. The cumulative expansion adds ~21 LOC (delete a 40-byte file, add a ~12-LOC `eslint.config.mjs`, three devDep changes in `package.json` — two version bumps for `eslint`/`eslint-config-next` and one new line for `typescript@^5.9.3`). Decision D does not change the LOC count — it re-pins an existing line (`devDependencies.eslint`) from `^10.4.0` back to `^9.39.4`, no addition or deletion. Total non-lockfile diff stays well under the 400-LOC guideline. Decisions C and D do not change the brief count, the brief's `files:` set, or the `slice_dependencies` graph — `package.json` and `package-lock.json` were already in scope from the start. Doc-writer's `AGENTS.md` pass remains a separate PR by `role-doc-writer` per the convoy's Roles list.
|
||||
|
||||
The audit fan-out (`role-reviewer` + `role-design-system-auditor` + `role-a11y-auditor`) is **parallel via `/multitask`**, but that's a downstream concern triggered by the conductor after the PR is drafted — not part of the implementer decomposition.
|
||||
|
||||
### Slice dependencies (multitask-ready)
|
||||
|
||||
```yaml
|
||||
slice_dependencies:
|
||||
- brief: 1
|
||||
depends_on: []
|
||||
files:
|
||||
- package.json
|
||||
- package-lock.json
|
||||
- next.config.js
|
||||
- eslint.config.mjs
|
||||
deletes:
|
||||
- .eslintrc.json
|
||||
```
|
||||
|
||||
Single brief, no parallelization opportunity at the implementer stage. The conductor should dispatch `role-implementer` serially (no `/multitask` fan-out for the implementer phase). The audit-cohort fan-out happens later, after PR draft, under group id `audit-bump-next-js-<pr>`.
|
||||
|
||||
## Decisions (post-IA round)
|
||||
|
||||
### A — 2026-05-23: Expand convoy scope to include ESLint v8 → v9 + flat-config migration
|
||||
|
||||
**Context.** During the architect's initial Boot-the-brief check, two findings landed at human gate 1:
|
||||
|
||||
1. `eslint-config-next@16.2.6` requires `eslint >= 9.0.0` (flat config). The convoy file (line 42) prescribed bumping `eslint-config-next` to `^16` "matching next" but didn't account for this peer-dep cliff. The architect's first-pass plan pinned `eslint-config-next@15.4.2` and flagged the deviation.
|
||||
2. `next lint` was removed in Next 16. `package.json`'s `lint` script and CI's `npm run lint` both invoke a removed command in 16.
|
||||
|
||||
**Decision.** Expand this convoy to include the ESLint v9 + flat-config migration, rather than spinning out a separate `migrate-to-eslint-flat-config` convoy. Rationale: both the Next bump and the ESLint migration touch `package.json`, so they cannot ship in parallel anyway; one PR gives reviewers a single revert boundary; the expansion adds only ~20 LOC of non-lockfile diff (delete `.eslintrc.json`, add `eslint.config.mjs`, two devDep version bumps); and `eslint-config-next@16` ships native flat-config exports so no `FlatCompat` shim or `@eslint/eslintrc` install is needed.
|
||||
|
||||
**Specific changes baked into Brief 1:**
|
||||
|
||||
- Bump `devDependencies.eslint` from `^8` to `^9.39.4` (latest 9.x; ESLint v10 was released between convoy authoring and now — see Boot-the-brief #5 — but per this decision we stay on 9.x).
|
||||
- Bump `devDependencies.eslint-config-next` from `15.4.2` to `^16.2.6`.
|
||||
- Change `scripts.lint` from `"next lint"` to `"eslint ."`.
|
||||
- Delete `.eslintrc.json` (40-byte stub: `{"extends": "next/core-web-vitals"}`).
|
||||
- Add `eslint.config.mjs` using the verbatim shape from the [official Next.js docs](https://nextjs.org/docs/app/api-reference/config/eslint): `defineConfig([...nextVitals, globalIgnores([...])])`. Imports come from `eslint/config` (built-in helpers since 9.21.0) and `eslint-config-next/core-web-vitals`.
|
||||
- `globalIgnores` covers `.next/**`, `node_modules/**`, `out/**`, `build/**`, `next-env.d.ts`, and `scripts/migrations/**` per gate-1 instruction.
|
||||
|
||||
**Out-of-scope (still deferred):**
|
||||
|
||||
- Fixing the ~100-error pre-existing lint baseline. Stays under `fix-lint-baseline`. CI's `npm run lint || true` wrapper continues to tolerate non-zero exit; the baseline number will shift with the v9 plugin upgrades but counting that drift is `fix-lint-baseline`'s job.
|
||||
- Bumping ESLint to v10. Surfaced as Risk R14; revisit in a later `bump-eslint-10` convoy if desired.
|
||||
- Bumping React 18 → 19. Stays under `bump-react`.
|
||||
- App Router migration, test-runner adoption, auth fixes, schema migrations — all unchanged from the original convoy scope.
|
||||
|
||||
**Canonical authority.** `tcg-vault` does not maintain `docs/04-architecture/*.md` files, so this Decisions entry IS the canonical authority. Decision recorded in chat on 2026-05-23 between user and `role-architect`. Boot-the-brief recheck performed against this decision before publishing the revised Brief 1.
|
||||
|
||||
**Consequences for downstream roles.**
|
||||
|
||||
- `role-implementer`: must run `npm install next@^16.2.6 eslint@^9.39.4 eslint-config-next@^16.2.6` (the three explicit version pins), then delete `.eslintrc.json`, write `eslint.config.mjs` per the verbatim shape in Brief 1, and update `package.json`'s `scripts.lint`. No mid-flight scope decisions.
|
||||
- `role-reviewer`: includes the ESLint config change in correctness review. Verify `npm run lint` runs (regardless of error count); verify the lockfile diff is consistent with the three version pins.
|
||||
- `role-design-system-auditor` and `role-a11y-auditor`: unchanged. The lint config doesn't affect render output.
|
||||
- `role-doc-writer`: still updates `AGENTS.md` "Tech stack" string (Next.js 15 → 16) in a separate PR. May optionally also update the line that says "JavaScript (not TypeScript)" remains accurate; no change needed there.
|
||||
|
||||
> **Superseded by Decision B (2026-05-23, same day).** The implementer command above changed from `eslint@^9.39.4` to `eslint@^10.4.0`. See entry B below for details.
|
||||
|
||||
### B — 2026-05-23: Pivot ESLint pin from v9.x to v10.x
|
||||
|
||||
**Context.** Decision A (above, same day) expanded scope to include the ESLint v8 → v9 + flat-config migration, with `eslint` pinned to `^9.39.4`. During the architect's Boot-the-brief recheck of A, finding #5 surfaced that **ESLint v10.4.0 had been released to the `latest` dist-tag on 2026-02-06** — between when this convoy was authored (2026-05-22) and when gate 1 was reached (2026-05-23). v9.39.4 had moved to the `maintenance` tag. The architect surfaced this as Risk R14 with a "stay conservative on v9" recommendation. On gate-1 re-review, the user pivoted to v10 to avoid a back-to-back `bump-eslint-10` convoy.
|
||||
|
||||
**Decision.** Pin `devDependencies.eslint` to `^10.4.0` (current `latest`) instead of `^9.39.4`. All other pins from Decision A stand: `next@^16.2.6`, `eslint-config-next@^16.2.6`, `.eslintrc.json` deleted, `eslint.config.mjs` created with the same verbatim shape (no v10-specific signature change in `defineConfig` or `globalIgnores`).
|
||||
|
||||
**Boot-the-brief recheck against v10 (no blocker found):**
|
||||
|
||||
1. **Peer-dep compatibility.** `npm view eslint-config-next@16.2.6 peerDependencies` returns `{"eslint": ">=9.0.0", ...}` with **no `<10` upper bound**. v10 is accepted.
|
||||
2. **Node engine compatibility.** `eslint@10.4.0` engines: `node ^20.19.0 || ^22.13.0 || >=24` (tighter floor than v9's `^18.18.0 || ^20.9.0 || >=21.1.0`). CI's `setup-node@v4` with `node-version: '20'` resolves to latest 20.x ≥ 20.19; local `node@22.14.0` is in `^22.13.0`; Vercel default Node 22 is ≥ 22.13. All ✓. Residual concern (CI's "latest 20.x" is a moving target) is documented as Risk R9; pinning CI to `node-version: '20.19'` would eliminate it but is out of scope per the convoy's "Hard do not touch" list.
|
||||
3. **`eslint/config` exports retained.** Extracted `eslint@10.4.0` tarball, opened `lib/config-api.js`: still re-exports `defineConfig` and `globalIgnores` from `@eslint/config-helpers`. The brief's verbatim shape is unchanged.
|
||||
4. **`eslint-env` comments are errors in v10.** `rg "eslint-env"` returned zero hits in `tcg-vault` source. ✓
|
||||
5. **No App-Router-only or Cache-Components surfaces affected.** v10's `eslint:recommended` updates, JSX reference tracking, and `no-shadow-restricted-names.reportGlobalThis: true` will shift the lint baseline more than v9 would have, but that's `fix-lint-baseline`'s problem (Risk R12, expanded).
|
||||
|
||||
**The one new risk v10 surfaces (R15 in the convoy file's Risk list):** `eslint-config-next@16.2.6` was published before ESLint v10. Its bundled plugin set (`@next/eslint-plugin-next@16.2.6`, `eslint-plugin-react@^7.37.0`, `eslint-plugin-react-hooks@^7.0.0`, `eslint-plugin-import@^2.32.0`, `eslint-plugin-jsx-a11y@^6.10.0`, `typescript-eslint@^8.46.0`) was not statically vetted against v10. If any plugin uses a v9-deprecated API that v10 removed (`context.getCwd()`, `SourceCode#getJSDocComment()`, etc.), `npm run lint` will throw `TypeError`. **Acceptance criterion: `npm run lint` runs to completion.** If it crashes, the implementer escalates and we revert to v9 (one-line change). Cost of being wrong: small.
|
||||
|
||||
**Out-of-scope (still deferred):**
|
||||
|
||||
- All items deferred under Decision A remain deferred.
|
||||
- CI workflow changes (e.g. pinning `node-version: '20.19'` for ESLint v10's stricter floor) — see Risk R9 residual concern. Pickup point: next CI-touching convoy (`adopt-vitest`).
|
||||
- Any `bump-eslint-10` convoy is now **closed before opening** — its work is folded into this one.
|
||||
|
||||
**Canonical authority.** Same as Decision A — this Decisions entry IS the canonical authority. Decision recorded in chat on 2026-05-23 between user and `role-architect`, immediately after Decision A's gate-1 review surfaced finding R14.
|
||||
|
||||
**Updated consequences for downstream roles** (delta from Decision A):
|
||||
|
||||
- `role-implementer`: command becomes `npm install next@^16.2.6 eslint@^10.4.0 eslint-config-next@^16.2.6`. The `eslint.config.mjs` shape is unchanged. New explicit acceptance check: `npm run lint` running to completion (escalate on `TypeError`, do not patch transitive deps).
|
||||
- `role-reviewer`: lockfile diff will additionally show `@eslint/eslintrc` being removed from the dep tree (v10 dropped it). Lint baseline will shift more than under v9; `|| true` wrapper still tolerates.
|
||||
- `role-design-system-auditor`, `role-a11y-auditor`, `role-doc-writer`: unchanged from Decision A.
|
||||
|
||||
### C — 2026-05-23: Add `typescript` as a devDep (narrow scope expansion in response to implementer escalation)
|
||||
|
||||
**Context.** After Decisions A and B were applied, `role-implementer` ran the migration locally and `npm run lint` immediately crashed with `Cannot find module 'typescript'` during config load — before any rule executed. The implementer escalated. Root-cause diagnosis: `eslint-config-next/core-web-vitals` → `typescript-eslint@^8.46.0` → `@typescript-eslint/typescript-estree@8.59.4` does an unconditional `require('typescript')` at module load (verified after the fact by extracting the published `typescript-estree` tarball — `dist/convert.js:40` and 8 other sites are top-level `require('typescript')` calls, none gated on `try/catch` or `require.resolve`). The `peerDependenciesMeta.typescript.optional: true` annotation in `eslint-config-next@16.2.6`'s `package.json` only suppresses npm's install-time peer-dep warning; it does NOT make `typescript` runtime-optional. **The architect's Boot-the-brief finding #8 misread this annotation** and stated "tcg-vault is JS-only, no `typescript` install needed." That assumption was wrong, and the implementer caught it on first run.
|
||||
|
||||
This is **NOT** a manifestation of Risk R15 (no `TypeError` on a deprecated v9 API; the crash happened before any rule loaded). Reverting to ESLint v9 would not have fixed it — the same `typescript-eslint` chain ships with `eslint-config-next@16` regardless of the ESLint major version.
|
||||
|
||||
**The decision.** User chose **option (a) — add `typescript` as a devDep** at the gate. One-line scope expansion, ~minimal-risk:
|
||||
|
||||
- `package.json` adds `devDependencies.typescript: "^5.9.3"`.
|
||||
- `package-lock.json` regenerates accordingly. The new `typescript` subtree is small (TypeScript itself has no `dependencies` and no `peerDependencies`).
|
||||
- Pin choice: `^5.9.3`. **Note:** `npm view typescript@latest version` returns `6.0.3` (TypeScript 6 is the current `latest` major, contrary to the gate's parenthetical claim that 5 was latest). Latest 5.x is `5.9.3`. Two reasons to pin `^5.9.3` and defer v6: (1) honor the literal gate-1 instruction (`^5`); (2) `typescript-eslint@8.59.4`'s peer range is `>=4.8.4 <6.1.0` — strictly, `typescript@6.0.3` is in range, but `typescript-eslint@8.x` was published before TS 6 GA and has not advertised explicit v6 support, so staying inside the well-trodden 5.x range is safer until a future convoy bumps `typescript-eslint`. `^5.9.3` resolves to the latest 5.x patch.
|
||||
- Pin range scope: full SemVer caret (`^5.9.3`), matching the convention used by `next` (`^15.4.2` → `^16.2.6`) and `react` (`^18.3.1`) elsewhere in `package.json`.
|
||||
- No new files. No `tsconfig.json`. No `.js` → `.ts` migration. The brief's `files:` set is unchanged (`package.json` and `package-lock.json` were already in scope as modifications). The `slice_dependencies` graph is unchanged.
|
||||
- No `eslint.config.mjs` change. The flat-config shape is independent of whether `typescript` is installed.
|
||||
|
||||
**Why option b (replace `eslint-config-next` with a JS-only ESLint preset) was dismissed.** `eslint-config-next@16` does not ship a JS-only entry point. Its `core-web-vitals` export bundles `typescript-eslint` as a regular dependency (not a peer), so consumers cannot opt out without forking the package or reimplementing the rule set. Maintaining a fork is a much larger scope expansion than adding `typescript` as a devDep, and gives up the upstream guarantee that the rule set tracks Next.js best practices.
|
||||
|
||||
**Why option c (keep things broken; CI's `|| true` wrapper tolerates lint failures) was dismissed.** CI's `|| true` wrapper tolerates a non-zero exit code from `eslint`, but it does NOT tolerate a `MODULE_NOT_FOUND` thrown during config load — the crash happens before ESLint emits any structured output, and the wrapper still passes the exit code to the shell, but **lint stops being a useful signal entirely**. Every CI lint run would be a no-op pass. That regresses the lint surface to "always green, regardless of code quality" and silently invalidates the `fix-lint-baseline` convoy's premise (which assumes lint at least executes). Unacceptable.
|
||||
|
||||
**Out-of-scope (still deferred):**
|
||||
|
||||
- All items deferred under Decisions A and B remain deferred.
|
||||
- TypeScript adoption as a project language (no `tsconfig.json`, no `.ts`/`.tsx` source files, no `// @ts-check` directives, no `.d.ts` declaration files). `typescript` is installed purely so `eslint-config-next`'s lint chain can load. If the team later decides to migrate to TypeScript, that's an explicit, separate convoy — not a "while we're here."
|
||||
- Adding `@typescript-eslint/parser` or `@typescript-eslint/eslint-plugin` directly. They're already pulled in transitively by `eslint-config-next@16`; no direct dep needed.
|
||||
- CI workflow changes (still per Decision B's deferral note — pickup point is `adopt-vitest`).
|
||||
|
||||
**Canonical authority.** This Decisions entry IS the canonical authority. Decision recorded in chat on 2026-05-23 between user and `role-architect`, immediately after the implementer's escalation on first lint run. Boot-the-brief #8's misreading of `peerDependenciesMeta.optional` is corrected in place in `brief-1-bump-next-and-migrate-config.md` (finding #8 marked "🔴 SUPERSEDED by Decision C"; new findings #16–#18 added under "Decision C narrow recheck").
|
||||
|
||||
**Updated consequences for downstream roles** (delta from Decision B):
|
||||
|
||||
- `role-implementer`: command becomes `npm install next@^16.2.6 eslint@^10.4.0 eslint-config-next@^16.2.6 typescript@^5.9.3` (or equivalently, run the previous three-package install, then run `npm install --save-dev typescript@^5.9.3` as a follow-up — order doesn't matter; the lockfile is regenerated either way). Re-run `npm run lint` after the install; expectation is now that lint completes with the pre-existing baseline of errors (no `Cannot find module 'typescript'` crash). Failure-mode classification: see Brief 1's "Local verification" section — `Cannot find module 'typescript'` post-install means the install didn't take and is not R15; a `TypeError: context.getCwd is not a function` (or similar v9-deprecated-API error) is R15 and triggers a v9 fallback (keeping the `typescript` install).
|
||||
- `role-reviewer`: lockfile diff will now additionally show the `typescript` package being added. The diff is small (TypeScript has no transitive deps). Verify the brief's no-scope-expansion guardrails were respected — specifically that no `tsconfig.json` was created and no `.js` files were renamed to `.ts`/`.tsx`.
|
||||
- `role-design-system-auditor`, `role-a11y-auditor`, `role-doc-writer`: unchanged from Decisions A and B.
|
||||
|
||||
### D — 2026-05-23: Revert ESLint v10 → v9.39.4 after R15 fired empirically; queue `bump-eslint-10` as upstream-blocked follow-up
|
||||
|
||||
**Context.** After Decisions A, B, and C were applied, `role-implementer` ran the migration locally a second time (pass 2). The Decision-C `typescript` install resolved the original `Cannot find module 'typescript'` crash from pass 1 — but the lint run then surfaced a different `TypeError`:
|
||||
|
||||
- **Crash signature:** `TypeError: scopeManager.addGlobals is not a function`
|
||||
- **Call site:** ESLint v10's `lib/source-code/source-code.js:221` calls `scopeManager.addGlobals(...)` as part of v10's redesigned global-ingestion path.
|
||||
- **Missing-method site:** `@typescript-eslint/scope-manager@8.59.4` (transitive dep of `typescript-eslint@8.59.4`, which `eslint-config-next@16.2.6` bundles as a regular `dependency`) does not implement `addGlobals` on its `ScopeManager` class.
|
||||
- **Why:** `@typescript-eslint/scope-manager@8.x` was published Oct/Nov 2025; ESLint v10 GA was 2026-02-06. The `addGlobals` method is a v10-introduced extension of the `ScopeManager` interface; v9 used a different ingestion path. `typescript-eslint` has not yet shipped a v10-tested release that adds the method.
|
||||
|
||||
This is **the empirical firing of Risk R15**, in a different shape than originally feared. The principal failure mode anticipated at Decision B was a v9-deprecated-API removal (`context.getCwd()`, `SourceCode#getJSDocComment()`, etc.); the actual failure was a v10-introduced-method gap on the typescript-eslint side. Either way, the diagnosis is the same: `eslint-config-next@16.2.6`'s pre-v10-GA bundled plugin set is not runtime-compatible with v10. Reverting `eslint` to v9 is the only working option until upstream catches up.
|
||||
|
||||
**The decision.** User chose **option (a) — execute the brief's documented v9 fallback path AND formally queue a follow-up `bump-eslint-10` convoy** at the gate. This partially reverses Decision B's "avoid back-to-back convoys" rationale, but **Decision B was correct given Boot-the-brief evidence at the time** — the v10 incompat was statically unprovable until a real lint run hit `addGlobals`. Empirical evidence from pass 2 reverses the call.
|
||||
|
||||
Specific changes:
|
||||
|
||||
- `package.json`: re-pin `devDependencies.eslint` from `"^10.4.0"` back to `"^9.39.4"` (npm's `maintenance` dist-tag).
|
||||
- **`devDependencies.typescript: "^5.9.3"` (Decision C) is RETAINED.** Boot-the-brief recheck #17 confirmed at Decision C, and the implementer's pass-2 evidence reconfirmed, that the same `typescript-eslint@8.59.4` chain hard-`require`s `typescript` under v9 too. The typescript install is correct independent of the eslint pin.
|
||||
- **`eslint-config-next@^16.2.6` is unchanged** — its peer-dep `eslint: ">=9.0.0"` accepts v9.39.4 trivially; no `<10` upper bound shift since Decision B's verification.
|
||||
- **`next@^16.2.6` is unchanged.**
|
||||
- **`eslint.config.mjs` shape is unchanged.** `defineConfig` and `globalIgnores` from `eslint/config` were introduced in 9.21.0 and retained in v10.4.0; the same import line resolves correctly on both v9.39.4 and v10.4.0. **This is the load-bearing reason Decision D is a one-line `package.json` re-pin and not a multi-file rollback.** When `bump-eslint-10` lands, this file should not need to change.
|
||||
- Install command for the implementer: `npm install --save-dev eslint@^9.39.4 eslint-config-next@^16.2.6 typescript@^5.9.3` (single atomic command preferred; running them separately is equivalent — the lockfile regenerates either way).
|
||||
|
||||
**Why option b (npm overrides to force a v10-compat `@typescript-eslint/scope-manager`) was dismissed.** The brief explicitly forbids transitive patching ("do NOT patch the plugin or pin transitive deps mid-flight"). There's no guarantee that any released version of `@typescript-eslint/scope-manager` exists with the v10 fix at a version `eslint-config-next@16.2.6` will resolve to under its bundled `typescript-eslint@^8.46.0` constraint. Even if such a version existed, npm overrides bypass the upstream maintainer's compatibility testing entirely — we'd be hand-rolling a custom dep tree that no other project uses, which moves the maintenance burden to us.
|
||||
|
||||
**Why option c (wait for upstream) was dismissed.** Doesn't unblock the Vercel deploys gate that's the convoy's success metric ("npm install next@16.2.6 ships, Vercel deploys complete, no runtime regressions in dev or build"). The convoy's premise is that Vercel was rejecting `next@^15.4.2` as a "Vulnerable version of Next.js"; we have to ship `next@^16.2.6` now. Pinning `eslint` to v9 lets us do that today; v10 can land later when prerequisites are met.
|
||||
|
||||
**Why option d (ship with the lint crash) was dismissed.** Same reasoning as Decision C's option-c dismissal: CI's `|| true` wrapper tolerates a non-zero exit code, but a `TypeError` crash before any rule executes means lint emits no useful signal at all — every CI lint run becomes a no-op pass, and `fix-lint-baseline`'s premise (lint at least executes) collapses. Unacceptable.
|
||||
|
||||
**Reversal of Decision B's "avoid back-to-back `bump-eslint-10` convoy" rationale.** Acknowledged. Decision B's argument was: pivot to v10 now to avoid a follow-up convoy. That argument was correct given the static evidence available at the time of Decision B (peer-dep ranges accepted v10; Boot-the-brief found no obvious incompatibilities). Decision D's empirical evidence — a `TypeError` from a real lint run — supersedes the static evidence. The follow-up `bump-eslint-10` convoy is now formally queued (see "Follow-up convoys queued" section below); it will land as a single-brief mechanical bump once upstream prerequisites are met.
|
||||
|
||||
**Out-of-scope (still deferred):**
|
||||
|
||||
- All items deferred under Decisions A, B, and C remain deferred.
|
||||
- The `bump-eslint-10` follow-up is queued, not authored. The conductor will materialize a convoy file when a human authors one (likely after `typescript-eslint` ships a v10-tested release and `eslint-config-next` bundles it).
|
||||
- npm `overrides` field manipulation, transitive-dep pinning, plugin forking — all forbidden as scope expansion under both this convoy and the future `bump-eslint-10`.
|
||||
|
||||
**Canonical authority.** This Decisions entry IS the canonical authority. Decision recorded in chat on 2026-05-23 between user and `role-architect`, immediately after the pass-2 implementer escalation surfaced R15's `addGlobals` firing.
|
||||
|
||||
**Updated consequences for downstream roles** (delta from Decision C):
|
||||
|
||||
- `role-implementer`: re-run `npm install --save-dev eslint@^9.39.4` (the other three packages — `next@^16.2.6`, `eslint-config-next@^16.2.6`, `typescript@^5.9.3` — are already at correct versions from Decision C's run); re-run `npm run lint`. **Expectation:** exit code 1 or 2 (baseline lint errors present) is fine; exit code 0 is improbable until `fix-lint-baseline`. **NOT expected:** a `TypeError` crash. R15's `scopeManager.addGlobals` signature should not recur on v9 because v9 doesn't call `addGlobals`. If a different `TypeError` fires on v9, escalate via the brief's failure-mode classifier.
|
||||
- `role-reviewer`: lockfile diff under Decision D stays on the v9 dep-tree; `@eslint/eslintrc` (a v9 transitive) remains present. Verify that `package.json` has `eslint: "^9.39.4"` (not `^10.x`), `typescript: "^5.9.3"`, `eslint-config-next: "^16.2.6"`, and `next: "^16.2.6"`.
|
||||
- `role-design-system-auditor`, `role-a11y-auditor`, `role-doc-writer`: unchanged from Decisions A, B, C. Doc-writer's `AGENTS.md` pass should mention the `bump-eslint-10` queued convoy if the doc-writer pass surfaces lint-toolchain documentation.
|
||||
- **Future `bump-eslint-10` implementer:** inherits R15's diagnostic verbatim. The convoy will become a single-brief mechanical bump once `typescript-eslint` ships v10 support and `eslint-config-next` bundles it; until then, the convoy is upstream-blocked and not authored.
|
||||
|
||||
## Follow-up convoys queued
|
||||
|
||||
The following convoys are formally queued by `role-architect` as upstream-blocked follow-ups to this convoy. They are NOT authored as convoy files yet — they exist in this list only. The conductor will materialize a convoy file when a human authors one and the upstream prerequisites are met.
|
||||
|
||||
### `bump-eslint-10` — upstream-blocked
|
||||
|
||||
- **Origin.** Queued under Decision D (2026-05-23) after R15 fired empirically. Decision B's pivot to v10 was reverted; v10 is still the supported `latest` and we want to land it eventually.
|
||||
- **Prerequisites (both must be met before the convoy can run):**
|
||||
1. `typescript-eslint` ships a v10-tested release. Likely shape: `@typescript-eslint/scope-manager` adds the `addGlobals` method (and any other v10-introduced `ScopeManager` interface members) on the v8.x line as a backport, OR the typescript-eslint v9 line ships and adds them. Either way, the release notes will explicitly mention ESLint v10 compatibility.
|
||||
2. `eslint-config-next` bundles a v10-tested `typescript-eslint`. Likely shape: a `16.3+` release that bumps the `typescript-eslint` direct dependency. Confirmed by reading the `eslint-config-next` `package.json` `dependencies.typescript-eslint` range and cross-referencing typescript-eslint's release notes.
|
||||
- **Convoy shape (when materialized):** single-brief mechanical bump matching this convoy's Brief 1 shape. Files: `package.json` (re-pin `eslint` from `^9.39.4` to `^10.x.y`; re-pin `eslint-config-next` if a new minor bundles the v10-tested typescript-eslint), `package-lock.json` (regenerate). No code-shape changes expected — `eslint.config.mjs` is verified compatible on both v9 and v10. No CI workflow changes unless the Node-version pin question (originally raised under R9) bites at v10's stricter floor.
|
||||
- **Risks inherited from this convoy:** R12's "lint baseline shifts more under v10" warning resurfaces; the three new `eslint:recommended` rules, JSX reference tracking, `no-shadow-restricted-names.reportGlobalThis: true` default, and `eslint-env`-comments-as-errors transition all happen at this convoy. CI's `|| true` wrapper still tolerates. R9's CI moving-target concern (`node-version: '20'` resolution) reactivates under v10's `^20.19.0` floor; the `bump-eslint-10` brief should pin CI to `node-version: '20.19'` or `'lts/iron'` if the target convoy permits CI workflow changes.
|
||||
- **Cost of being wrong about prerequisites:** small. If `typescript-eslint` ships a v10-tested release and `eslint-config-next` bundles it but `bump-eslint-10` still surfaces a different incompat at runtime, the convoy itself documents another decision letter and re-pins back to v9 again. The cost is one extra Boot-the-brief recheck and one extra Decisions entry.
|
||||
|
||||
### `bump-typescript-6` — upstream-blocked
|
||||
|
||||
- **Origin.** Queued under Decision D (2026-05-23). Surfaced during Decision C's Boot-the-brief recheck (#16): `npm view typescript@latest` returned `6.0.3` (latest major), but `^5.9.3` was pinned because `typescript-eslint@8.59.4`'s peer range `>=4.8.4 <6.1.0` accepts but doesn't certify v6.
|
||||
- **Prerequisites:**
|
||||
1. `typescript-eslint` advertises explicit v6 support in a release. Currently the peer range accepts `<6.1.0` (so `typescript@6.0.x` is technically in range) but typescript-eslint has not announced v6 testing. Likely shape: a release-notes entry titled "TypeScript 6 support" or a peer-range bump to `<7.0.0` once they're confident.
|
||||
2. (Optional) `eslint-config-next` bundles a `typescript-eslint` version that advertises v6 support. Not strictly required — `typescript@^5.x` in `bump-eslint-10` and `typescript@^6.x` here can be re-pins on different days.
|
||||
- **Convoy shape (when materialized):** single-brief mechanical bump. Files: `package.json` (re-pin `typescript` from `^5.9.3` to `^6.x.y`), `package-lock.json` (regenerate). No code-shape changes — TypeScript is only used by ESLint's lint chain, not by source files (no `tsconfig.json`, no `.ts` files; the `bump-typescript-6` convoy must preserve those guardrails).
|
||||
- **Risks inherited from this convoy:** none specific. The verified runtime require evidence in Brief 1 #17 stays valid (typescript-estree's `require('typescript')` sites are unconditional regardless of TS major).
|
||||
- **Cost of being wrong about prerequisites:** small. Same fallback shape as `bump-eslint-10`.
|
||||
|
||||
### Notes on materialization
|
||||
|
||||
These two convoys can land independently, in either order. Neither blocks the other. The conductor should expect a human to author the convoy file (frontmatter + IA + UX) when they decide to land the upgrade; `role-architect` does not pre-author convoy files for upstream-blocked follow-ups (no Boot-the-brief evidence exists yet to verify against).
|
||||
|
||||
216
.convoys/bump-next-js/brief-1-bump-next-and-migrate-config.md
Normal file
216
.convoys/bump-next-js/brief-1-bump-next-and-migrate-config.md
Normal file
|
|
@ -0,0 +1,216 @@
|
|||
---
|
||||
convoy: bump-next-js
|
||||
brief_number: 1
|
||||
depends_on: []
|
||||
files:
|
||||
- package.json
|
||||
- package-lock.json
|
||||
- next.config.js
|
||||
- eslint.config.mjs
|
||||
deletes:
|
||||
- .eslintrc.json
|
||||
---
|
||||
|
||||
# Brief 1: Bump Next.js to 16.2.6 + migrate `next.config.js`, ESLint flat config, and lint script
|
||||
|
||||
## Goal (1 sentence)
|
||||
|
||||
Replace `next@15.4.3` with `next@16.2.6` so Vercel's platform-level security gate stops blocking every deploy, while paying off the three blockers the upgrade actually introduces in `tcg-vault`: the deprecated `images.domains` config, the **removed** `next lint` command (which forces a `package.json` script change AND an ESLint v8 → **v10** + flat-config migration so `eslint-config-next` can be bumped to `^16` to match `next`), and Turbopack-by-default (no code change required, just informed acceptance).
|
||||
|
||||
## Files in scope (do not edit anything else)
|
||||
|
||||
- `package.json` — modified
|
||||
- `package-lock.json` — modified (regenerated)
|
||||
- `next.config.js` — modified
|
||||
- `eslint.config.mjs` — **new**
|
||||
- `.eslintrc.json` — **deleted**
|
||||
|
||||
## Conventions to follow
|
||||
|
||||
- `AGENTS.md` § "Tech stack quick reference": Next.js 15 (Pages router) → bump the framework version string only as part of the doc-writer pass, not in this brief. **Do not edit `AGENTS.md` here.** That's a separate role-doc-writer PR.
|
||||
- `.cursor/rules/no-go-zones.mdc`: do not touch anything outside `files:` above. In particular: no edits to `pages/`, `components/`, `lib/`, `scripts/`, `styles/`, or any workflow file.
|
||||
- `package.json` formatting: 2-space indent, double-quoted keys/values, trailing newline. Match existing style.
|
||||
- `next.config.js` formatting: ESM (`export default nextConfig`), 2-space indent, JSDoc `@type` comment preserved.
|
||||
- `eslint.config.mjs` formatting: ESM, 2-space indent, no semicolons-only-when-needed convention (match the verbatim shape from the official Next.js docs cited below).
|
||||
- The lock file must be regenerated by `npm install`, not hand-edited.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
### `package.json` changes
|
||||
|
||||
- [ ] `dependencies.next` is `"^16.2.6"` (from `"^15.4.2"`).
|
||||
- [ ] `dependencies.react` and `dependencies.react-dom` stay at `"^18.3.1"`. Next 16 peer-deps accept `react ^18.2.0 || ^19.0.0`; current `18.3.1` is in range. React 19 is its own convoy.
|
||||
- [ ] `devDependencies.eslint` is `"^9.39.4"` (from `"^8"`). **Per gate-1 Decision D (2026-05-23), reverted from `^10.4.0` back to `^9.39.4` after R15 fired empirically on the implementer's pass-2 run** (`TypeError: scopeManager.addGlobals is not a function` from ESLint v10's `source-code.js:221` calling a method that `@typescript-eslint/scope-manager@8.59.4` — bundled by `eslint-config-next@16.2.6`, published before v10 GA — does not implement). v10 will be picked up under the queued follow-up convoy `bump-eslint-10` once `typescript-eslint` ships a v10-tested release and `eslint-config-next` bundles it. v9.39.4 is on npm's `maintenance` dist-tag (current `latest` is 10.4.0). See Boot-the-brief findings #5–#10 below for the v10-pivot verification (now historical) and the Decision D recheck below for v9 sanity.
|
||||
- [ ] `devDependencies.eslint-config-next` is `"^16.2.6"` (from `"15.4.2"`). `eslint-config-next@16.2.6`'s peer dep `eslint: ">=9.0.0"` accepts v9.39.4 (re-verified via `npm view eslint-config-next@16.2.6 peerDependencies`).
|
||||
- [ ] `devDependencies.typescript` is `"^5.9.3"` (newly added). **Per gate-1 Decision C (2026-05-23), `typescript` is a hard runtime requirement** of `eslint-config-next@16.2.6` despite its package.json's `peerDependenciesMeta.typescript.optional: true` annotation. The annotation only suppresses npm's install-time warning; it does NOT mean the runtime path is optional. `eslint-config-next` bundles `typescript-eslint@^8.46.0`, whose `@typescript-eslint/typescript-estree@8.59.4` dependency does an unconditional `require('typescript')` at module load (verified at `dist/convert.js:40` of the published tarball). Without `typescript` installed, `npm run lint` crashes with `Cannot find module 'typescript'` before any rule runs. See Boot-the-brief findings #8 (superseded) and #16–#17 below.
|
||||
- [ ] `scripts.lint` is `"eslint ."` (from `"next lint"`). The `next lint` command was removed in Next.js 16; it would throw at runtime if left in place.
|
||||
- [ ] No new direct dependencies are added beyond the four already listed (`next` bumped, `eslint` bumped, `eslint-config-next` bumped, `typescript` newly added). **In particular, do NOT add `@eslint/eslintrc`** — `eslint-config-next@16.2.6` ships native flat-config exports, so no `FlatCompat` shim is needed (see Boot-the-brief finding #1). Do NOT add `@typescript-eslint/parser`, `@typescript-eslint/eslint-plugin`, `tsx`, `ts-node`, or any other TS toolchain — `typescript` alone resolves the lint crash.
|
||||
- [ ] No new `engines` block is added to `package.json`. Node 20.19+ is required by `eslint@10` (`next@16` requires 20.9+), but the existing `setup-node@v4` step in `.github/workflows/ci.yml` (`node-version: '20'`, which resolves to latest 20.x ≥ 20.19) and Vercel's default Node 22 runtime both satisfy this. Adding an `engines` block is out of scope (see Risk R9 for the residual CI-pin concern).
|
||||
|
||||
### `package-lock.json` changes
|
||||
|
||||
- [ ] Regenerated via `npm install` (no hand edits).
|
||||
- [ ] `npm ls next` reports `next@16.2.6`.
|
||||
- [ ] `npm ls eslint` reports `eslint@9.39.x` (or whatever 9.x patch `^9.39.4` resolves to). **Not `10.x`** — see Decision D below.
|
||||
- [ ] `npm ls eslint-config-next` reports `eslint-config-next@16.2.x`.
|
||||
- [ ] `npm ls typescript` reports `typescript@5.9.x` (latest 5.x; per Decision C — see Boot-the-brief #16 for the v6 deferral rationale).
|
||||
- [ ] `npm install` exits cleanly with no `ERESOLVE` peer-dep failures and no `npm warn deprecated` for any of the four packages above. (Note: under Decision D the lockfile diff stays on the v9 dep-tree — `@eslint/eslintrc` is still present as a v9 transitive dep; the v10 dep-tree changes that would have removed it are deferred to the queued `bump-eslint-10` follow-up convoy. The new `typescript` subtree should still be small — TypeScript itself has no `dependencies`.)
|
||||
|
||||
### `next.config.js` changes
|
||||
|
||||
- [ ] Migrates `images.domains` → `images.remotePatterns`. Verbatim shape (from the [official Next 16 upgrade guide](https://nextjs.org/docs/app/guides/upgrading/version-16) § "`images.domains` Configuration (deprecated)"):
|
||||
|
||||
```js
|
||||
/** @type {import('next').NextConfig} */
|
||||
const nextConfig = {
|
||||
images: {
|
||||
remotePatterns: [
|
||||
{ protocol: 'https', hostname: 'api.scryfall.com' },
|
||||
{ protocol: 'https', hostname: 'images.pokemontcg.io' },
|
||||
{ protocol: 'https', hostname: 'lorcana-api.com' },
|
||||
],
|
||||
},
|
||||
};
|
||||
|
||||
export default nextConfig;
|
||||
```
|
||||
|
||||
- [ ] No other keys are added to `next.config.js`. In particular: do **not** add `cacheComponents`, `reactCompiler`, `turbopack`, or `experimental.*` flags. Those are opt-in features for follow-up convoys. **Do not** add `--webpack` opt-out — see Risk R1.
|
||||
|
||||
### `.eslintrc.json` deletion + `eslint.config.mjs` creation
|
||||
|
||||
- [ ] `.eslintrc.json` is deleted. (It currently contains exactly `{"extends": "next/core-web-vitals"}`. ESLint v9 still tolerates legacy `.eslintrc.*` if `ESLINT_USE_FLAT_CONFIG=false` is set, but the codebase is moving to flat config; leaving both files would be a footgun.)
|
||||
- [ ] `eslint.config.mjs` is created with the verbatim shape below. **This shape comes directly from the [official Next.js docs for `eslint-config-next` v16+](https://nextjs.org/docs/app/api-reference/config/eslint)** — do not improvise, do not add new rules, do not "while we're here" any plugin disables. The only deviation from the docs example is one extra path (`scripts/migrations/**`) added to `globalIgnores` per the user's gate-1 instruction.
|
||||
|
||||
```js
|
||||
import { defineConfig, globalIgnores } from 'eslint/config';
|
||||
import nextVitals from 'eslint-config-next/core-web-vitals';
|
||||
|
||||
const eslintConfig = defineConfig([
|
||||
...nextVitals,
|
||||
globalIgnores([
|
||||
'.next/**',
|
||||
'node_modules/**',
|
||||
'out/**',
|
||||
'build/**',
|
||||
'next-env.d.ts',
|
||||
'scripts/migrations/**',
|
||||
]),
|
||||
]);
|
||||
|
||||
export default eslintConfig;
|
||||
```
|
||||
|
||||
Notes for the implementer (do not include these as comments in the file — they're for the PR description):
|
||||
|
||||
- **The verbatim shape is unchanged across the v8 → v9 (Decision A) → v10 (Decision B) → v9 (Decision D) ping-pong.** `defineConfig` and `globalIgnores` from `eslint/config` exist in both v9.39.4 (added 9.21.0) and v10.4.0 (retained); the import line works identically on both majors. Decision D rolls back only the `package.json` pin — no flat-config edit required. When the queued `bump-eslint-10` follow-up convoy lands, this file should not need to change.
|
||||
- `defineConfig` and `globalIgnores` are built-in helpers exported from `eslint/config` (added in ESLint 9.21.0, retained in v10). `eslint@9.39.4` has them; `eslint@10.4.0` would also have them, but Decision D pins v9.39.4.
|
||||
- `eslint-config-next/core-web-vitals` is a CommonJS array re-exported as the default — spreadable with `...nextVitals` (verified by extracting the `eslint-config-next@16.2.6` tarball; see Boot-the-brief finding #1).
|
||||
- `eslint-config-next` already includes default ignores for `.next/**`, `out/**`, `build/**`, and `next-env.d.ts`. We restate them here to match the docs example exactly and to be explicit about what's ignored.
|
||||
- `node_modules/**` is added explicitly even though ESLint default-ignores it; user's gate-1 instruction lists it as a minimum-cover ignore.
|
||||
- `scripts/migrations/**` is preemptive — the folder doesn't exist yet (`.cursor/rules/no-go-zones.mdc` calls it "TBD"), but we ignore it now so the eventual migration script convoy doesn't have to remember to.
|
||||
- `next-env.d.ts` doesn't exist in `tcg-vault` (JS-only project, no TS). Including it is harmless and matches the docs example verbatim.
|
||||
- **No `parserOptions`, no `rules:` overrides, no `settings:` block.** This brief preserves the exact lint behavior of the previous `.eslintrc.json` extends. Any rule tuning belongs in `fix-lint-baseline`.
|
||||
|
||||
### Local verification (run before pushing)
|
||||
|
||||
- [ ] `npm install` resolves cleanly with no `ERESOLVE` peer-dep failures.
|
||||
- [ ] `npm ls next eslint eslint-config-next` prints the three expected versions (16.2.6, 10.4.x, 16.2.x).
|
||||
- [ ] `npm run dev` boots, prints something like `▲ Next.js 16.2.6 (Turbopack)`, and serves `/` without runtime errors. **No deprecation warning about `images.domains`** is logged at startup.
|
||||
- [ ] `npm run build` exits with code 0. (Turbopack is the default bundler in 16; tcg-vault has no `webpack:` block in `next.config.js`, so no `--webpack` opt-out is needed.)
|
||||
- [ ] Manually smoke the routes `TESTING_GUIDE.md` calls out: `/`, `/login`, `/signup`, `/cards`, `/collections`. They render the same as before — no React hydration errors, no 500s.
|
||||
- [ ] **`npm run lint` runs ESLint v9.39.4 to completion without a `TypeError` crash, emitting the pre-existing baseline of ~100 errors.** This is the integration test for the v8 → v9 + flat-config + `typescript`-devDep migration. Exit code 1 (lint errors present) is **expected**; exit code 0 is improbable until `fix-lint-baseline` runs; CI's `|| true` wrapper tolerates either. **Do not fix lint errors in this brief.** Counting the exact baseline is `fix-lint-baseline`'s job.
|
||||
- [ ] **If `npm run lint` crashes after `typescript@^5.9.3` is installed AND `eslint@^9.39.4` is pinned**, classify the failure mode:
|
||||
- `Cannot find module 'typescript'` or similar module-resolution error → **`typescript` install didn't take.** Re-run `npm install`; verify `node_modules/typescript/package.json` exists; verify `package.json` `devDependencies.typescript` is `"^5.9.3"`. Do not investigate further — this should be deterministic now that Decision C is in place.
|
||||
- `TypeError: scopeManager.addGlobals is not a function` (or any other `scopeManager.*` / `SourceCode.*` `is not a function` error) → **`eslint` pin didn't take, you're still on v10.** Re-run `npm install`; verify `node_modules/eslint/package.json` reports `9.39.x`; verify `package.json` `devDependencies.eslint` is `"^9.39.4"`. This was Risk R15's empirical signature on the pass-2 implementer run; it should NOT recur once v9 is pinned. If it does recur on v9 with a different `is not a function` shape (hypothetically — no evidence this happens), escalate to `role-architect` rather than patching transitive deps.
|
||||
- `TypeError: context.getCwd is not a function` / `SourceCode.prototype.getJSDocComment is not a function` (the v9-deprecated-API signatures originally feared at Decision B) — **not expected on v9** because these APIs are still present in v9 (only removed in v10). If this fires anyway, escalate; do not patch.
|
||||
- Anything else (parse error in a source file, unhandled exception in a rule) → **Lint baseline drift.** This is `fix-lint-baseline`'s problem, not this convoy's. CI's `|| true` wrapper tolerates it.
|
||||
|
||||
### Vercel preview verification (after pushing the PR)
|
||||
|
||||
- [ ] Vercel produces a Preview deployment whose status moves to **Success** (not "Error" / "Vulnerable version of Next.js detected").
|
||||
- [ ] The Preview URL renders `/` end-to-end (not just the build page).
|
||||
- [ ] Capture the Preview URL in the PR description so reviewers (`role-reviewer`, `role-design-system-auditor`, `role-a11y-auditor`) can hit it during the audit fan-out.
|
||||
|
||||
### No-scope-expansion guardrails
|
||||
|
||||
- [ ] No file outside the `files:` / `deletes:` lists is modified.
|
||||
- [ ] No new dependencies beyond the four already specified (two devDep version bumps — `eslint`, `eslint-config-next`; one new devDep — `typescript`; one regular dep bump — `next`). In particular: no `@eslint/eslintrc`, no `@eslint/js`, no `@typescript-eslint/parser`, no `@typescript-eslint/eslint-plugin`, no `tsx`, no `ts-node`, no `babel-plugin-react-compiler`, no `@playwright/test`, no `vitest`. Those belong to other convoys.
|
||||
- [ ] No `eslint.config.mjs` rule tuning beyond the documented `globalIgnores` list. If `eslint-config-next@16` + `eslint@10` surfaces additional warnings/errors, defer to `fix-lint-baseline`.
|
||||
- [ ] **No `tsconfig.json` is created.** Decision C adds `typescript` as a devDep purely so `eslint-config-next@16`'s bundled `typescript-eslint` chain can `require('typescript')` at module load — `tcg-vault` remains a JavaScript project and no source files are migrated to `.ts` / `.tsx`. If a future convoy adopts TypeScript, that's a separate, explicit decision.
|
||||
- [ ] No `.js` / `.jsx` files are renamed to `.ts` / `.tsx`. No `// @ts-check` directives are added. No `.d.ts` declaration files are created.
|
||||
- [ ] No `AGENTS.md` edits. Doc-writer pass happens in a separate PR via `role-doc-writer`.
|
||||
- [ ] No `<Image>` or `<img>` migrations. Audit confirmed `tcg-vault` does not import `next/image` anywhere; pages use plain `<img>`. The `images.remotePatterns` config is being kept (rather than deleted) because it's pre-staged for the eventual `next/image` adoption.
|
||||
- [ ] No `tests added` checkbox: tcg-vault has no test runner installed yet. Adoption is tracked under `adopt-vitest`. Manual smoke per `TESTING_GUIDE.md` is the verification mechanism.
|
||||
- [ ] No `--webpack` flag added to `npm run dev` or `npm run build`. Turbopack-by-default is accepted per gate-1 decision; fallback procedure is documented in Risk R1 (in the convoy file) and in this brief's Rationale.
|
||||
|
||||
## Rationale (≤3 sentences)
|
||||
|
||||
The Next 15 → 16 jump in tcg-vault is unusually narrow at the framework layer (no App Router, no `middleware.js`, no `next/cache`, no `next/image`, no `getServerSideProps`/`getStaticProps`, no `unstable_*`), but the lint toolchain has to move in lockstep: `next lint` was removed, `eslint-config-next@16` requires `eslint >= 9.0.0` (flat config) AND a present `typescript` install (despite its `peerDependenciesMeta.typescript.optional: true` annotation — the bundled `typescript-eslint` chain hard-`require`s `typescript` at module load), and the existing `.eslintrc.json` legacy stub can't extend it — so this single PR bumps `next`, bumps `eslint` to **v9.39.4** (per gate-1 Decision D, after Decision B's earlier v10 pivot was empirically reversed by Risk R15 firing on the implementer's pass-2 run with `TypeError: scopeManager.addGlobals is not a function`; v10 will be picked up under the queued upstream-blocked `bump-eslint-10` follow-up), bumps `eslint-config-next` to `^16` matching `next`, **adds `typescript@^5.9.3` as a devDep** (per gate-1 Decision C), replaces `.eslintrc.json` with `eslint.config.mjs` (same shape works on both v9 and v10, so no further edit needed when `bump-eslint-10` lands), and changes the `package.json` lint script. Both the Next bump and the ESLint migration touch `package.json`, so they cannot run in parallel anyway — keeping them in one brief gives reviewers one PR, one Vercel preview, and one revert boundary if anything regresses. React stays at 18.3.1 (16's peer-deps accept it; React 19 is `bump-react`), Turbopack-by-default is accepted as-is (no `webpack:` config exists; fallback per Risk R1), no `tsconfig.json` is created (tcg-vault remains a JS-only project), and the ~100-error lint baseline stays untouched per `fix-lint-baseline`'s charter. Decision B's pivot to v10 was the right call given Boot-the-brief evidence at the time; Decision D rolls it back specifically because empirical lint runs surfaced the R15 incompatibility with `eslint-config-next@16.2.6`'s pre-v10-GA bundled plugins.
|
||||
|
||||
## Boot-the-brief findings (Architect verified 2026-05-22; re-verified 2026-05-23 after gate-1 scope expansion to ESLint v9; re-verified again 2026-05-23 after gate-1 Decision B pivoted to ESLint v10; re-verified narrowly 2026-05-23 after gate-1 Decision C added `typescript` devDep in response to implementer escalation; re-verified narrowly again 2026-05-23 after gate-1 Decision D reverted the v10 pivot back to v9.39.4 in response to pass-2 implementer escalation showing R15 fired empirically with `TypeError: scopeManager.addGlobals is not a function`)
|
||||
|
||||
**Note on findings #5–#10:** These document the v10-pivot Boot-the-brief from Decision B. They are kept as historical record (the conclusions about v10's engines, peer deps, and `eslint/config` exports are still factually correct) but are **superseded for the active pin** by Decision D. The active `eslint` pin is `^9.39.4` per Decision D's recheck below; v10 is queued under the upstream-blocked `bump-eslint-10` follow-up convoy.
|
||||
|
||||
These were verified before publishing the brief:
|
||||
|
||||
1. **`eslint-config-next@16.2.6` ships native flat-config exports — `FlatCompat` is NOT needed.** Verified two ways: (a) `npm view eslint-config-next@16.2.6 exports` returned `"./core-web-vitals": { "default": "./dist/core-web-vitals.js" }`; (b) extracted the published tarball (`npm pack eslint-config-next@16.2.6`, then `tar -xzf`), opened `package/dist/core-web-vitals.js`, and confirmed it ends with `module.exports = config` where `config` is a flat-config array (line 37: `var config = _to_consumable_array(_index.default).concat([...])`, then `module.exports = config`). Original user instruction at gate 1: "Use `FlatCompat` from `@eslint/eslintrc` if `eslint-config-next@16` doesn't ship a native flat-config export." Result: native is shipped, **FlatCompat dropped**, no `@eslint/eslintrc` dep added. (And ESLint v10 dropped `@eslint/eslintrc` from its own dependency tree entirely, so this is doubly the right call.)
|
||||
2. **Verbatim shape comes directly from the [official Next.js docs](https://nextjs.org/docs/app/api-reference/config/eslint).** That page's "Setup ESLint" section uses exactly the `defineConfig([...nextVitals, globalIgnores([...])])` pattern this brief replicates. The only deviation: this brief adds `'node_modules/**'` and `'scripts/migrations/**'` to the ignores per gate-1 instruction.
|
||||
3. **`next/core-web-vitals` is still a valid extends in `eslint-config-next@16` — but only via the full subpath `eslint-config-next/core-web-vitals` in flat config.** The legacy `extends: 'next/core-web-vitals'` shorthand was an `.eslintrc.json` (legacy-config) sugar; flat config requires the explicit subpath import. Confirmed both via the Next.js docs and the package's `exports` field (`"./core-web-vitals": ...`).
|
||||
4. **`defineConfig` + `globalIgnores` are ESLint built-ins from `eslint/config`.** Introduced in ESLint 9.21.0 (Feb 2025); retained in v10.0.0 (Feb 2026). Confirmed by extracting `eslint@10.4.0`'s tarball: `package/lib/config-api.js` re-exports `{ defineConfig, globalIgnores, includeIgnoreFile }` from `@eslint/config-helpers`. Same shape as v9 — no signature change. v10 also adds `includeIgnoreFile` to that module (not used here).
|
||||
5. **ESLint v10.4.0 is the current `latest` on npm.** `npm view eslint dist-tags` returns `{"latest": "10.4.0", "maintenance": "9.39.4", "next": "10.0.0-rc.2", ...}`. v10.0.0 was released 2026-02-06 per the [release blog post](https://eslint.org/blog/2026/02/eslint-v10.0.0-released/). v9.39.4 is on the `maintenance` tag. Per gate-1 Decision B, this brief pins `^10.4.0`.
|
||||
6. **`eslint@10.4.0` peer deps:** `jiti: *` with `peerDependenciesMeta.jiti.optional: true`. Optional peer; only required if you author your config in TypeScript (`eslint.config.ts`). This brief uses `eslint.config.mjs` (plain JavaScript ESM), so `jiti` is **not** installed. Note: v10 explicitly requires `jiti >= 2.2.0` if used (per migration guide § "Jiti < v2.2.0 are no longer supported"); not a concern for us.
|
||||
7. **`eslint@10.4.0` engines: `node ^20.19.0 || ^22.13.0 || >=24`.** This is a tighter floor than v9's `^18.18.0 || ^20.9.0 || >=21.1.0` (Node 18, 21, and 23 are all dropped; Node 20.x floor raised from 20.9.0 to 20.19.0). **CI satisfies:** `setup-node@v4` with `node-version: '20'` resolves to the latest 20.x at install time; latest 20.x as of 2026-05 is well above 20.19.0 (Node 20.19.0 was released 2025-03; many patches since). **Local satisfies:** `node@22.14.0` is in the `^22.13.0` range. **Vercel satisfies:** default Node 22 runtime (22.x ≥ 22.13). All three environments ✓. See Risk R9 for the residual concern (CI's `node-version: '20'` is a moving target — if it ever resolves to a stale < 20.19.0 patch, ESLint v10 will refuse to start; that's a CI-pin question, out of scope here).
|
||||
8. **`eslint-config-next@16.2.6` peer deps:** `eslint >= 9.0.0` (required), `typescript >= 3.3.1` (declared optional via `peerDependenciesMeta.typescript.optional: true`). **No `<10` upper bound** — re-verified via `npm view eslint-config-next@16.2.6 peerDependencies`. v10 is accepted. **🔴 SUPERSEDED by Decision C (2026-05-23):** the original claim "tcg-vault is JS-only, no `typescript` install needed" was **wrong**. `peerDependenciesMeta.typescript.optional: true` only suppresses npm's install-time peer-dep warning; it does NOT make `typescript` runtime-optional. `eslint-config-next` bundles `typescript-eslint@^8.46.0` as a regular `dependency` (not as a peer), and `@typescript-eslint/typescript-estree@8.59.4` does an unconditional `require('typescript')` at module load (verified via tarball extraction — see finding #17 below). The implementer's first lint run crashed with `Cannot find module 'typescript'` before any rule executed. **Corrected:** `typescript@^5.9.3` is now installed as a devDep. See findings #16 and #17 for the v6-vs-v5 pin choice and the verified runtime-require evidence.
|
||||
9. **`eslint-config-next@16.2.6` was published before ESLint v10 (Oct 2025 vs Feb 2026), so its bundled plugins were not tested against v10.** Bundled plugin set: `@next/eslint-plugin-next@16.2.6`, `eslint-plugin-react@^7.37.0` (latest published `7.37.5`), `eslint-plugin-react-hooks@^7.0.0` (latest `7.1.1`), `eslint-plugin-import@^2.32.0` (latest `2.32.0`, released 2025-06), `eslint-plugin-jsx-a11y@^6.10.0` (latest `6.10.2`), `typescript-eslint@^8.46.0` (latest `8.59.4`). All published before Feb 2026. **The peer-dep range allows v10, but runtime compatibility is not statically provable.** Captured in Risk R15. Mitigation: the "Local verification" section above classifies failure modes — `Cannot find module 'typescript'` was **not** R15 (it was the Decision C `typescript`-missing failure); a `TypeError: context.getCwd is not a function` (or similar v9-deprecated-API error) **would** be R15 and would trigger a v9 fallback.
|
||||
10. **v10 user-impacting breaking changes audited against tcg-vault** ([migration guide](https://eslint.org/docs/latest/use/migrate-to-10.0.0)):
|
||||
- **Node.js floor raised** — covered above (#7).
|
||||
- **`eslint:recommended` updated (3 new rules enabled).** Will shift baseline. Goes to `fix-lint-baseline`.
|
||||
- **Old config format removed.** We're already on flat config in this brief — no impact.
|
||||
- **JSX references now tracked.** Will shift `no-unused-vars` / `no-undef` baseline (likely fewer false positives). Goes to `fix-lint-baseline`.
|
||||
- **`eslint-env` comments are errors.** `rg "eslint-env"` returned zero matches in tcg-vault source. ✓
|
||||
- **`stylish` formatter uses native `styleText` instead of `chalk`.** Cosmetic only. Honors `NO_COLOR` / `NODE_DISABLE_COLORS`. No action.
|
||||
- **`no-shadow-restricted-names` reports `globalThis` by default.** Will potentially add baseline entries. Goes to `fix-lint-baseline`.
|
||||
- **Plugin-developer changes** (deprecated `context` members, deprecated `SourceCode` methods, `Program` AST range, `RuleTester` strictness, `nodeType` on `LintMessage`). Not applicable to tcg-vault — we don't author plugins. **But these are exactly the APIs `eslint-config-next`'s bundled plugins might have used before v10**; that risk is captured in #9 / R15.
|
||||
- **POSIX character classes in glob patterns / `radix` rule deprecated options / `func-names` schema / `no-invalid-regexp.allowConstructorFlags` uniqueness.** None apply (we don't override any of these rules; we don't use POSIX glob syntax).
|
||||
11. **`next@16.2.6` peer deps and engines re-verified.** `react ^18.2.0 || ^19.0.0` ✓ (current `18.3.1`); engines `node >=20.9.0` ✓ (lower than ESLint v10's `^20.19.0` floor — ESLint v10 is now the binding constraint).
|
||||
12. **No `next/image` usage in tcg-vault.** Re-verified via `rg "from ['\"]next/image['\"]"` — zero hits in `pages/`, `components/`, `lib/`. Convoy file's audit list (line 45) was incorrect.
|
||||
13. **No `webpack:` config in `next.config.js`.** Turbopack-by-default in `next dev`/`next build` is safe per gate-1 acceptance. Fallback procedure documented in Risk R1 (convoy file).
|
||||
14. **`.gitignore` already covers `.next/dev/`.** Next 16 splits dev and build outputs; existing `/.next/` rule (line 28) is a directory glob covering both.
|
||||
15. **`scripts/migrations/` doesn't exist yet.** Per `.cursor/rules/no-go-zones.mdc`, "folder TBD." Adding to `globalIgnores` preemptively is harmless.
|
||||
|
||||
### Decision C narrow recheck (added 2026-05-23)
|
||||
|
||||
16. **TypeScript pin: `^5.9.3` (not `^6.0.3`).** `npm view typescript dist-tags` returned `{"latest": "6.0.3", "next": "6.0.0-dev.20260416", "rc": "6.0.1-rc", "beta": "6.0.0-beta", "maintenance": "5.9.3", ...}` — TypeScript 6 is the current `latest`, contrary to the gate-1 instruction's parenthetical claim that "5 is the latest TypeScript major." Latest 5.x is `5.9.3`. Two reasons to pin `^5.9.3` and defer v6:
|
||||
- **Honor the literal gate-1 instruction.** Decision C says "Pin range: `^5`." The parenthetical was a documentation error, not the binding instruction.
|
||||
- **`typescript-eslint@8.59.4`'s peer range is `>=4.8.4 <6.1.0`.** Strictly, `typescript@6.0.3` IS in range (`<6.1.0` ⊃ `6.0.3`), so `^6.0.3` would satisfy it. **But:** typescript-eslint historically pins TS minor versions tightly and ships compatibility releases out-of-band; v8.59.4 was published before TS 6 GA and has not advertised explicit v6 support. Pinning `^5.9.3` keeps us inside the well-trodden range until a future convoy bumps `typescript-eslint` to a v6-tested release. `^5.9.3` resolves to the latest 5.x patch (currently `5.9.3` itself) and is well within the peer range.
|
||||
- **No peer deps on typescript itself.** `npm view typescript@latest peerDependencies` returns empty. `typescript@^5.9.3` adds zero transitive packages — only `typescript`'s own bundle (compiler, language service, declaration files). The lockfile diff is small.
|
||||
- **Engines.** `typescript@5.9.3` and `typescript@6.0.3` both list `engines.node >= 14.17`, well below ESLint v10's `^20.19.0` floor. No new Node constraint introduced.
|
||||
17. **Verified the unconditional `require('typescript')` site.** Extracted `@typescript-eslint/typescript-estree@8.59.4`'s published tarball (`npm pack` then `tar -xzf` in `/tmp/ts-estree-pkg`) and grepped `dist/` for `require('typescript')`:
|
||||
```
|
||||
dist/convert.js:40: const ts = __importStar(require("typescript"));
|
||||
dist/useProgramFromProjectService.js:44:const ts = __importStar(require("typescript"));
|
||||
dist/convert-comments.js:38: const ts = __importStar(require("typescript"));
|
||||
dist/semantic-or-syntactic-errors.js:4: const typescript_1 = require("typescript");
|
||||
dist/getModifiers.js:38: const ts = __importStar(require("typescript"));
|
||||
dist/check-syntax-errors.js:37: const ts = __importStar(require("typescript"));
|
||||
dist/check-modifiers.js:37: const ts = __importStar(require("typescript"));
|
||||
dist/version-check.js:38: const ts = __importStar(require("typescript"));
|
||||
dist/source-files.js:38: const ts = __importStar(require("typescript"));
|
||||
```
|
||||
All 9 sites are top-level `require('typescript')` calls — **no `try { require('typescript') } catch {}` gating, no dynamic-import lazy-loader, no `typeof require !== 'undefined' && require.resolve('typescript')` guard.** The package will throw `MODULE_NOT_FOUND` at import time if `typescript` isn't installed. The package's own `peerDependencies.typescript: ">=4.8.4 <6.1.0"` (in `package.json` at the typescript-estree level — **not flagged optional**) is the accurate signal; `eslint-config-next`'s `peerDependenciesMeta.typescript.optional: true` is a **misleading transitive override** at the wrapper level. Conclusion: any consumer of `eslint-config-next@16` MUST install `typescript` to lint. This is true under both ESLint v9 and v10 (same `typescript-eslint` chain), so reverting to v9 would not have fixed the crash.
|
||||
18. **No `tsconfig.json` in `tcg-vault`.** Verified via `Glob tsconfig*.json` — zero hits. The repo is JS-only (per AGENTS.md §1: "Next.js 15 (Pages router) + React 18, JavaScript (not TypeScript)"). The `typescript` install enables `eslint-config-next`'s lint chain; it does NOT introduce TypeScript as a project language. The "no `tsconfig.json` created" guardrail is enforced explicitly under "No-scope-expansion guardrails."
|
||||
|
||||
### Decision D narrow recheck (added 2026-05-23, after pass-2 implementer escalation reversed Decision B's v10 pivot)
|
||||
|
||||
19. **`eslint@9.39.4` is still on the `maintenance` dist-tag — no superseding 9.x patch since Decision A.** `npm view eslint dist-tags --json` returned `{"latest": "10.4.0", "maintenance": "9.39.4", "next": "10.0.0-rc.2", "es6jsx": "0.11.0-alpha.0"}`. v9.39.4 was the v9 line's last release before v10 GA on 2026-02-06; the v9 line is in maintenance mode but still receives security backports if needed.
|
||||
20. **`eslint@9.39.4` peer deps:** `jiti: *` only, with the same `peerDependenciesMeta.jiti.optional: true` semantics as v10 (only required for `.ts` configs; we use `.mjs`). No surprising new peer added since Decision A. **Engines:** `^18.18.0 || ^20.9.0 || >=21.1.0` — looser than v10's `^20.19.0 || ^22.13.0 || >=24` floor. CI's `setup-node@v4` `node-version: '20'` (latest 20.x), local `node@22.14.0`, and Vercel's default Node 22 runtime all satisfy. The R9 residual concern (CI moving target on `node-version: '20'`) **becomes inert under Decision D** because v9's floor is 20.9.0 instead of 20.19.0; any reasonable 20.x patch will satisfy.
|
||||
21. **`eslint-config-next@16.2.6`'s peer-dep range on `eslint` is unchanged** since Decision B's verification: `>=9.0.0` (no upper bound). v9.39.4 satisfies trivially.
|
||||
22. **`eslint.config.mjs` shape works on v9.39.4 with zero edits.** `defineConfig` and `globalIgnores` from `eslint/config` were introduced in 9.21.0 (per Boot-the-brief #4) and are present in 9.39.4. The same import line — `import { defineConfig, globalIgnores } from 'eslint/config';` — resolves correctly on both v9.39.4 and v10.4.0. **This is the load-bearing reason Decision D is a one-line `package.json` re-pin and not a multi-file rollback.**
|
||||
23. **R15 fired empirically on the pass-2 implementer run with the following diagnostic:**
|
||||
- **Crash signature:** `TypeError: scopeManager.addGlobals is not a function`
|
||||
- **Call site:** ESLint v10's `lib/source-code/source-code.js:221` calls `scopeManager.addGlobals(...)`.
|
||||
- **Missing-method site:** `@typescript-eslint/scope-manager@8.59.4` (a transitive dep of `typescript-eslint@8.59.4`, which `eslint-config-next@16.2.6` bundles as a regular `dependency`) does not implement `addGlobals` on its `ScopeManager` class.
|
||||
- **Why:** `@typescript-eslint/scope-manager@8.x` was published Oct/Nov 2025, before ESLint v10 GA on 2026-02-06. The `addGlobals` method is a v10-introduced extension of the `ScopeManager` interface; v9 used a different ingestion path. `typescript-eslint` has not yet shipped a v10-tested release that adds the v10-required method.
|
||||
- **Resolution path:** revert `eslint` to v9.39.4 (Decision D). The same `typescript-eslint@8.59.4` works correctly on v9 because v9 doesn't call `addGlobals`.
|
||||
- **Pre-emptive note for the queued `bump-eslint-10` convoy:** when `typescript-eslint` ships a v10-tested release (likely `8.6.x`+ or `9.x`) AND `eslint-config-next` bundles it (likely `16.3+`), this incompatibility goes away and `bump-eslint-10` becomes a single-brief mechanical bump matching the shape of this convoy.
|
||||
302
.convoys/fix-auth-bypass.md
Normal file
302
.convoys/fix-auth-bypass.md
Normal file
|
|
@ -0,0 +1,302 @@
|
|||
---
|
||||
name: fix-auth-bypass
|
||||
classification: server-only
|
||||
success_metric: getUserFromRequest returns null for missing tokens; no API route accepts unauthenticated requests; CI green.
|
||||
skip:
|
||||
- ia
|
||||
- ux
|
||||
- visual
|
||||
- a11y
|
||||
- design
|
||||
status: open
|
||||
created: 2026-05-22
|
||||
---
|
||||
|
||||
# Convoy: fix-auth-bypass
|
||||
|
||||
Closes P0 ship-blockers **#1, #2, #4, #5, and #6 (partial)** from `.convoys/ship-readiness.md`. This is the very first real convoy after the bootstrap and gates the rest of the launch sequence — until it lands, every other production-bound PR is paused.
|
||||
|
||||
## Why
|
||||
|
||||
The current `lib/permission-middleware.js::getUserFromRequest` returns a hardcoded admin user (`{ id: 1, role: 'admin', email: 'admin@tcgvault.com' }`) when no `Authorization` header is present. Every API route that calls it (30+ handlers per `user-code-review-graph`) therefore accepts unauthenticated requests **as admin**. Combined with:
|
||||
|
||||
- A weak fallback `JWT_SECRET` (`'your-secret-key-change-in-production'`) duplicated across 7 files,
|
||||
- Four dev-only endpoints (`/api/simple`, `/api/test-auth`, `/api/test-db`, `/api/setup-database`) shipped in `pages/api/`,
|
||||
- `Access-Control-Allow-Origin: *` on auth endpoints,
|
||||
- Zero rate limiting on login,
|
||||
|
||||
…the production URL is effectively wide-open. **No anonymous traffic can touch the live site until this convoy ships.**
|
||||
|
||||
Success looks like:
|
||||
|
||||
1. `getUserFromRequest` returns `null` when there is no Bearer token. Period. No callers receive a synthetic admin.
|
||||
2. There is exactly one source of truth for the JWT secret. If `process.env.JWT_SECRET` is unset, the server fails to boot with a clear error — not a silent fallback.
|
||||
3. The four dev endpoints are gone, and CI fails the build if they reappear.
|
||||
4. The login + register endpoints respond only to the production frontend origin (or no CORS header at all on same-origin Vercel deploy).
|
||||
5. Login + register are rate-limited (the bare minimum of P0 #6; the rest is `add-rate-limiting`).
|
||||
6. CI is green (lint + the new auth tests).
|
||||
|
||||
## Scope
|
||||
|
||||
**In:**
|
||||
|
||||
- `lib/permission-middleware.js` — remove hardcoded admin fallback; return `null` on missing/invalid token.
|
||||
- New `lib/auth-secret.js` (or named equivalent — Architect to confirm) — single export of `JWT_SECRET`, throws at module load if unset.
|
||||
- Refactor `pages/api/auth-utils.js`, `pages/api/auth/login.js`, `pages/api/auth/register.js`, `pages/api/auth/verify.js`, `pages/api/favorites.js`, `pages/api/users/search.js`, and `lib/permission-middleware.js` to import from the new secret helper. Remove all `process.env.JWT_SECRET || '…'` literals.
|
||||
- Reconcile token expiry inconsistency (login = 24h, auth-utils = 7d). Pick one — Architect's call; record in `.cursor/rules/auth-and-permissions.mdc`.
|
||||
- Delete `pages/api/simple.js`, `pages/api/test-auth.js`, `pages/api/test-db.js`, `pages/api/setup-database.js`.
|
||||
- Add a CI grep step to `.github/workflows/ci.yml` that fails the build if `pages/api/test-*`, `pages/api/simple.js`, or `pages/api/setup-database.js` ever re-appear.
|
||||
- Tighten `Access-Control-Allow-Origin` on `pages/api/auth/login.js` and `pages/api/auth/register.js`. Default: drop the header entirely (same-origin on Vercel). Fallback: pin to a `process.env.PUBLIC_FRONTEND_ORIGIN` env var.
|
||||
- Adopt `@upstash/ratelimit` (or equivalent — Architect's pick) and apply to `/api/auth/login` and `/api/auth/register` only. **Other endpoints listed in P0 #6 (search, imports, avatar upload) are deferred to the `add-rate-limiting` convoy.**
|
||||
- Add unit tests for `getUserFromRequest`: missing header → `null`, malformed token → `null`, valid token → user object, expired token → `null`. Architect to decide whether to land this with `vitest` now or defer to the `adopt-vitest` convoy. **Default recommendation: install vitest in this convoy.** The blast radius of an auth refactor justifies bringing the test runner forward by one slot in the launch sequence.
|
||||
|
||||
**Out (deferred to their own convoys):**
|
||||
|
||||
- P0 #3 (default admin creds + README) → `drop-public-setup`.
|
||||
- P0 #7 (Layout default-prop leaks maintainer email) → `fix-layout-default-user`.
|
||||
- P0 #6 (full) — rate limit on search / import / upload routes → `add-rate-limiting`.
|
||||
- Any auth-context client-side cleanup (`lib/auth-context.js` vs `lib/admin-auth.js` vs `lib/use-auth.js`) → `single-auth-provider`.
|
||||
- The `lib/database.js` vs `@vercel/postgres` reconciliation → `single-sql-client`.
|
||||
|
||||
**Hard "do not touch" in this convoy:**
|
||||
|
||||
- No UI files. No `components/`, no `pages/*.js` that aren't under `pages/api/`. If a UI file appears in a brief, kick it back to Architect.
|
||||
- No schema changes. No SQL migrations. (`scripts/setup-neon-db.js` is read-only here.)
|
||||
- No new feature flags. The flag wrapper exists (`lib/flags/index.js`) but this convoy is unflagged — auth fixes don't ship behind a flag.
|
||||
|
||||
## Roles invoked
|
||||
|
||||
Per `server-only` classification (skip: `ia, ux, visual, a11y, design`):
|
||||
|
||||
1. **role-architect** — produces a slice plan with explicit `slice_dependencies:`. Expect 4–6 briefs (auth-secret helper, getUserFromRequest fix + caller audit, dev-endpoint removal + CI guard, CORS tighten, rate-limit pattern + login/register wiring, tests). Architect must declare which briefs are parallel-safe.
|
||||
2. **role-implementer** — runs one brief at a time, except where Architect marks `depends_on: []` and `files:` are disjoint. Then `/multitask` can fan out (see dispatch below).
|
||||
3. **role-reviewer** — single-pass after the PR drafts. **Design-system-auditor and a11y-auditor are skipped** for this convoy — there is no UI surface to audit. Reviewer covers correctness, security regressions, and test coverage.
|
||||
4. **role-doc-writer** — last. Updates `.cursor/rules/auth-and-permissions.mdc` (canonical secret helper, chosen expiry, rate-limit pattern), `AGENTS.md` "Common gotchas" section (remove items that are no longer gotchas), and `docs/SCHEMA_MAP.md` only if any DB read pattern changed (it shouldn't).
|
||||
|
||||
**Multitask dispatch recommendation** (Cursor 3.2 `/multitask`): after Architect publishes briefs with `depends_on: []` and disjoint `files:`, the user may dispatch implementers in parallel. Typical safe fan-out for this convoy:
|
||||
|
||||
- Group `audit-fix-auth-bypass-<pr>`: `role-reviewer` only (no design / a11y).
|
||||
- Implementer fan-out: only if Architect explicitly marks briefs as parallel-safe. The auth-secret helper brief must complete first; everything else depends on it. So realistic fan-out is post-secret-helper: dev-endpoint deletion + CORS tighten + rate-limit wiring in parallel; `getUserFromRequest` fix runs alongside but its tests block on the secret helper landing first.
|
||||
|
||||
## Todos
|
||||
|
||||
High-level checklist for the next role to refine. Each becomes a brief under `.convoys/fix-auth-bypass/brief-N-*.md`.
|
||||
|
||||
- [ ] **Brief 1 — Central JWT secret helper.** Create `lib/auth-secret.js`, fail-loud on missing env. Decide canonical token TTL.
|
||||
- [ ] **Brief 2 — Remove the admin bypass.** Fix `getUserFromRequest`; audit every caller (`user-code-review-graph` query: incoming edges to `lib-admin::getUserFromRequest`). Add unit tests covering missing/invalid/expired/valid token paths.
|
||||
- [ ] **Brief 3 — Delete dev-only endpoints.** Remove four files; add CI guard.
|
||||
- [ ] **Brief 4 — Tighten auth CORS.** Drop `Access-Control-Allow-Origin: *` on login + register. Add same-origin fallback via env var.
|
||||
- [ ] **Brief 5 — Rate-limit login + register.** Install `@upstash/ratelimit` (or Architect-chosen alternative). Wire to login + register only. Defer the full sweep to `add-rate-limiting`.
|
||||
- [ ] **Brief 6 — Test harness (provisional).** Install `vitest`, write the `getUserFromRequest` suite, re-enable the `test:` job in `.github/workflows/ci.yml`. Architect to confirm whether this is in-scope here or split to `adopt-vitest`.
|
||||
- [ ] **Doc-writer pass.** Update auth rules + AGENTS.md gotchas.
|
||||
|
||||
## Hand-off
|
||||
|
||||
**Next role: `role-architect`.**
|
||||
|
||||
To run it in a new chat, paste:
|
||||
|
||||
> *"Run role-architect on convoy `fix-auth-bypass`. Read `.convoys/fix-auth-bypass.md` for scope and todos, then produce a slice plan with explicit `slice_dependencies:`. Output briefs to `.convoys/fix-auth-bypass/brief-N-*.md`. Flag which briefs are parallel-safe so the user can `/multitask` implementers."*
|
||||
|
||||
Conductor exits here. Human-in-the-loop gate: review the convoy file, confirm the scope split, then start the Architect.
|
||||
|
||||
## 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.
|
||||
16
eslint.config.mjs
Normal file
16
eslint.config.mjs
Normal file
|
|
@ -0,0 +1,16 @@
|
|||
import { defineConfig, globalIgnores } from 'eslint/config';
|
||||
import nextVitals from 'eslint-config-next/core-web-vitals';
|
||||
|
||||
const eslintConfig = defineConfig([
|
||||
...nextVitals,
|
||||
globalIgnores([
|
||||
'.next/**',
|
||||
'node_modules/**',
|
||||
'out/**',
|
||||
'build/**',
|
||||
'next-env.d.ts',
|
||||
'scripts/migrations/**',
|
||||
]),
|
||||
]);
|
||||
|
||||
export default eslintConfig;
|
||||
|
|
@ -1,7 +1,11 @@
|
|||
/** @type {import('next').NextConfig} */
|
||||
const nextConfig = {
|
||||
images: {
|
||||
domains: ['api.scryfall.com', 'images.pokemontcg.io', 'lorcana-api.com'],
|
||||
remotePatterns: [
|
||||
{ protocol: 'https', hostname: 'api.scryfall.com' },
|
||||
{ protocol: 'https', hostname: 'images.pokemontcg.io' },
|
||||
{ protocol: 'https', hostname: 'lorcana-api.com' },
|
||||
],
|
||||
},
|
||||
};
|
||||
|
||||
|
|
|
|||
1721
package-lock.json
generated
1721
package-lock.json
generated
File diff suppressed because it is too large
Load diff
11
package.json
11
package.json
|
|
@ -7,7 +7,7 @@
|
|||
"dev": "next dev",
|
||||
"build": "next build",
|
||||
"start": "next start",
|
||||
"lint": "next lint",
|
||||
"lint": "eslint .",
|
||||
"setup-db": "node scripts/setup-neon-db.js",
|
||||
"import-popular": "node scripts/import-popular-sets.js",
|
||||
"import-all": "node scripts/bulk-import-all.js"
|
||||
|
|
@ -19,7 +19,7 @@
|
|||
"bcryptjs": "^3.0.2",
|
||||
"dotenv": "^17.2.1",
|
||||
"jsonwebtoken": "^9.0.2",
|
||||
"next": "^15.4.2",
|
||||
"next": "^16.2.6",
|
||||
"node-fetch": "^3.3.2",
|
||||
"react": "^18.3.1",
|
||||
"react-dom": "^18.3.1",
|
||||
|
|
@ -27,9 +27,10 @@
|
|||
},
|
||||
"devDependencies": {
|
||||
"autoprefixer": "^10.4.21",
|
||||
"eslint": "^8",
|
||||
"eslint-config-next": "15.4.2",
|
||||
"eslint": "^9.39.4",
|
||||
"eslint-config-next": "^16.2.6",
|
||||
"postcss": "^8.5.6",
|
||||
"tailwindcss": "^3.4.17"
|
||||
"tailwindcss": "^3.4.17",
|
||||
"typescript": "^5.9.3"
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue