diff --git a/.convoys/drop-public-setup.md b/.convoys/drop-public-setup.md new file mode 100644 index 0000000..c869ba4 --- /dev/null +++ b/.convoys/drop-public-setup.md @@ -0,0 +1,182 @@ +--- +name: drop-public-setup +classification: convoy +success_metric: | + No hardcoded admin credentials in repo (source or README). setup-neon-db.js + requires ADMIN_INITIAL_PASSWORD env var. New env-var setup documented in + README. Existing deployed admin user (if hash matches the known-weak default) + is either rotated or flagged for manual rotation. +skip: + - role-design-system-auditor + - role-a11y-auditor + - role-ux-reviewer + - role-ia-architect + - browser-smoke +status: open +created: 2026-05-23 +parent: ship-readiness +addresses: P0 #3 +depends_on: + - bump-next-js (shipped) + - fix-auth-bypass (shipped — already removed pages/api/setup-database.js) +--- + +# Drop public setup + +Close P0 #3 from `.convoys/ship-readiness.md`: remove the hardcoded admin +credentials (`admin@tcgvault.com` / `admin123`) from the seed script and +the README. + +## Scope (verbatim from ship-readiness P0 #3) + +- **`scripts/setup-neon-db.js` lines 130-138** — currently creates the admin + user with a hardcoded `admin123` bcrypt hash. Change to require an + `ADMIN_INITIAL_PASSWORD` env var with no default. Fail loudly (exit + non-zero with a clear message) if the env var is unset. +- **`README.md`** — strip the "Default Admin Account" section. Replace + with "run `npm run setup-db` and follow the prompt" (or however the + architect decides to phrase it; the spec is "no credentials in README"). +- **`pages/api/setup-database.js`** — already deleted by fix-auth-bypass + Brief 3 (commit fc0dd73). Not in scope here; just noting for completeness. + +## Out of scope + +- Migration tooling (`migration-tool` convoy, P1 #11) +- Renaming the admin email from `admin@tcgvault.com` to anything else + (branding decision belongs to `pick-a-name` convoy, P1 #12) +- Any other `scripts/seed-*.js` or `scripts/add-*.js` files (those are + one-off historical jobs per no-go-zones) +- Any change to `pages/api/auth/*.js` (fix-auth-bypass shipped; further + hardening lives in `cors-tighten` / `add-rate-limiting`) + +## Architect's questions + +1. **Existing-admin story.** If a deployed environment already has the + admin row with `bcrypt.hash('admin123', ...)`, this convoy alone + does NOT rotate that password — it only prevents the weak default + on *new* envs. Options: + a. Going-forward only. Flag for manual rotation; assume any + environment with the default already needs operator attention. + b. Add a one-time idempotent script under `scripts/migrations/` + that checks the admin row's password_hash against the known + weak default and forces a reset / requires `ADMIN_RESET_PASSWORD` + env var. + c. Add a startup check to the deploy pipeline that fails the + deploy if the admin row matches the weak hash. + + Recommend the architect pick (a) for simplicity and queue (b) as + a follow-up if needed. (a) is consistent with how Brief 1 of + fix-auth-bypass handled the JWT_SECRET — going-forward fix-loud, + not a sweep of existing data. + +2. **`setup-neon-db.js` no-go-zones rule.** The rule says "do NOT + edit `scripts/setup-neon-db.js` in place — it's idempotent and + meant for first-time setup only." That rule is about *schema* + changes (adding columns, etc.). This convoy edits the script's + admin-creation logic to add an env-var gate — operational, not + schema. Architect should confirm this reading and document the + distinction in the brief. + +3. **Test coverage.** Brief 5 of fix-auth-bypass introduced vitest. + Does this convoy add tests for the env-var-required path? Probably + not (setup scripts are typically tested via manual smoke, not unit + tests). Architect decides. + +## Expected size + +1-2 briefs, ~3 files total, no UI, no API surface, no migration. Should +ship as a single PR (no fan-out). + +## Architecture + +Architect: `role-architect`. Date: 2026-05-23. Convoy decomposed into **1 brief** — the surface is two files (one script, one doc) and the fix is a single coherent change (env-var gate + README rewrite); splitting would force the README update to land first or last on its own, which adds review overhead without any parallelization win. + +### File plan + +| File | Action | Brief | Purpose | +| --- | --- | --- | --- | +| `scripts/setup-neon-db.js` | modified | 1 | Read `ADMIN_INITIAL_PASSWORD` env var at the top of `setupNeonDatabase()`. If unset or empty string, log a clear actionable error (names the env var, points at `.env.local`, suggests `openssl rand -base64 24`) and `process.exit(1)` **before** any DDL or DB connection. Replace `bcrypt.hash('admin123', 12)` with `bcrypt.hash(adminPassword, 12)`. Remove the trailing `console.log(' Admin User: admin@tcgvault.com')` and `console.log(' Admin Password: admin123')` lines so neither the literal nor the env-var value lands in stdout. Keep `ON CONFLICT (email) DO NOTHING` — see R4. Match the existing CJS `require()` style (the file's top is `#!/usr/bin/env node` + `require('dotenv').config(...)`; do **not** convert to ESM in this brief — that's a separate concern flagged in **Out-of-scope** below). | +| `README.md` | modified | 1 | Strip the "🔐 Default Admin Account" section (lines 115-119) entirely. Add `ADMIN_INITIAL_PASSWORD` to the install-step env-example block alongside `POSTGRES_URL` / `JWT_SECRET`, with a one-line note that it is required for `npm run setup-db` and can be set as a CI secret instead of `.env.local` if setup runs from CI. Add a short "First-time admin setup" paragraph under or near "Set up the database" that documents the env-var requirement, the `openssl rand -base64 24` tip, and the fact that operators of envs predating this convoy must rotate the admin password manually (Decision A). | + +### API surface + +**No new routes, no modified routes.** This convoy is purely operational — the seed script is a CLI tool (`npm run setup-db`) and `README.md` is documentation. The `/api/setup-database.js` route referenced in the original P0 #3 spec was already deleted by `fix-auth-bypass` Brief 3 (commit `fc0dd73`); CI's `forbidden-endpoints` job blocks reintroduction. + +### Schema diff + +**No schema changes.** No `CREATE TABLE`, no `ALTER`, no `INSERT INTO users` semantic change beyond *which password gets hashed and stored on first run*. Same column shape, same `ON CONFLICT` clause, same `bcrypt` cost factor (12). This is what Decision B turns on — see below. + +### Test plan + +**No vitest coverage added** (Decision C — C1). The setup script runs once per environment; the fail-loud env-var path is verified by **manual smoke** in the brief's acceptance criteria: + +1. **Happy path:** With `ADMIN_INITIAL_PASSWORD=` set in `.env.local`, run `npm run setup-db`. Expect the admin row to be created (or skipped if it already exists) and the script to print neither the literal `'admin123'` nor the chosen password value to stdout. Then `curl -X POST http://localhost:3000/api/auth/login -H 'content-type: application/json' -d '{"email":"admin@tcgvault.com","password":""}'` returns 200 with a JWT. +2. **Fail-loud path:** Unset `ADMIN_INITIAL_PASSWORD` (`unset ADMIN_INITIAL_PASSWORD` or comment it out in `.env.local`) and run `npm run setup-db`. Expect the script to print the error message and exit with code 1 **before** opening a DB connection. +3. **Idempotency:** With the admin row already present (weak `admin123` hash or otherwise), re-run setup-db with `ADMIN_INITIAL_PASSWORD=`. Expect the admin row to be **unchanged** (`ON CONFLICT (email) DO NOTHING` short-circuits the INSERT). This is the documented behavior — rotation is out of scope (Decision A). +4. **README accuracy:** Read README top-to-bottom; confirm no occurrence of `admin123` remains, the new env-example block lists `ADMIN_INITIAL_PASSWORD`, and the "First-time admin setup" copy matches the script's actual behavior. + +Implementer pastes the stdout from steps 1 + 2 into the PR description for the reviewer. + +**Why not unit tests** (Decision C rationale): the script is run a handful of times per environment lifetime, the failure modes are loud (`process.exit(1)` + clear stderr), and unit-testing the env-var-required path would require either (a) extracting the admin-creation logic into a new `lib/seed-admin.js` module (scope expansion — Decision C option C3) or (b) spawning `node scripts/setup-neon-db.js` from a `vitest` test and asserting on stdout / exit code (slow, brittle, requires mocking `@neondatabase/serverless`). Manual smoke catches the same regressions at a fraction of the LOC cost, and the brief's acceptance criteria forces the implementer to actually run it. + +### Risk list + +- **R1 — Existing deployed admin rows are unchanged.** Any environment where `scripts/setup-neon-db.js` has already run with the old `bcrypt.hash('admin123', ...)` keeps the weak hash after this convoy merges, because `ON CONFLICT (email) DO NOTHING` skips the INSERT on re-run. **Mitigation:** Decision A (going-forward only). The brief's "Pre-merge operator checklist" and the README's new copy explicitly state that operators of pre-convoy envs must rotate the admin password manually (via the app's password-change UI or a future `rotate-default-admin` follow-up convoy). The PR description must include this callout for reviewers. +- **R2 — Operator runs `npm run setup-db` without setting the env var and the error is unhelpful.** A bare `throw new Error('missing')` would leave a new operator confused. **Mitigation:** the brief mandates a verbatim-near actionable message that names `ADMIN_INITIAL_PASSWORD`, points at `.env.local`, suggests `openssl rand -base64 24`, and notes the CI-secret alternative. Matches the wording shape Brief 1 of `fix-auth-bypass` used for `JWT_SECRET` in `lib/auth-secret.js`. +- **R3 — Setup-script stdout leaks the new admin password into CI logs.** The current script prints `Admin User: admin@tcgvault.com` and `Admin Password: admin123` after success. If we leave the second line and interpolate the env-var value into it, the chosen password lands in plaintext stdout — visible to anyone with CI log access, including the Vercel deploy log if setup ever runs there. **Mitigation:** the brief deletes both `console.log` lines outright. The success summary becomes `'✅ Admin user ready (email: admin@tcgvault.com)'` with no password echo. The chosen password is set only in the operator's env-var source (`.env.local` or CI secret), where access control already lives. +- **R4 — Changing `ON CONFLICT (email) DO NOTHING` to `DO UPDATE SET password = ...` would silently rotate every existing dev's admin password to whatever they put in `ADMIN_INITIAL_PASSWORD`.** This is **not** what we want; rotation is a separate concern (Decision A). **Mitigation:** the brief explicitly keeps `ON CONFLICT (email) DO NOTHING` unchanged and adds a verbatim acceptance-criterion line forbidding the change. A future `rotate-default-admin` follow-up convoy may add a separate idempotent migration script under `scripts/migrations/YYYY-MM-DD-*.js` if a real audit finds a deploy still carrying the weak hash. +- **R5 — README install-step env block omits the new variable.** A first-time operator following the README copies the env example, runs `npm run setup-db`, and hits the fail-loud error — confusing if the env example didn't mention `ADMIN_INITIAL_PASSWORD`. **Mitigation:** the brief explicitly updates the env block in step 3 of the README's installation section, not just the "Default Admin Account" section. +- **R6 — Three sibling files still hardcode `admin@tcgvault.com` / `admin123`.** Out of scope for this convoy (per the convoy spec's "Out of scope" list — these are historical scripts and a manual-QA doc): `scripts/reset-db.js` (lines ~141-156), `scripts/create-test-users.js` (line ~34), `TESTING_GUIDE.md` (line ~7). **Mitigation:** flag as a follow-up under § Anything flagged but not acted on below. The argument for *not* including them here: `reset-db.js` is a no-go-zone (`scripts/fix-*.js` family — historical / already-run), `create-test-users.js` is a dev seed for non-admin alice / bob / carol accounts where the weak admin reference is informational only, and `TESTING_GUIDE.md` is the test-data table for `create-test-users.js`. Sweeping them together would either widen the convoy's scope to "credential hygiene full sweep" or violate no-go-zones. + +### Decomposition + +| Brief # | Title | Files | Depends on | Estimated PR size | +| --- | --- | --- | --- | --- | +| 1 | Require `ADMIN_INITIAL_PASSWORD` env var; strip credentials from README | `scripts/setup-neon-db.js`, `README.md` | — | ~25 LOC net (10 added, 15 removed) | +| 2 | Convert `scripts/setup-neon-db.js` from CJS to ESM | `scripts/setup-neon-db.js` | brief 1 | ~6 LOC net (3 added, 3 removed) | + +### Slice dependencies (multitask-ready) + +```yaml +slice_dependencies: + - brief: 1 + depends_on: [] + files: + - scripts/setup-neon-db.js + - README.md + - brief: 2 + depends_on: + - brief: 1 + files: + - scripts/setup-neon-db.js +``` + +No fan-out — both briefs touch the same script and ship in the same PR on `convoy/drop-public-setup`. `/multitask` is not applicable. Brief 2 must land *after* brief 1 because brief 2's "do not touch the env-var check block" acceptance criterion references brief 1's code shape verbatim. + +## Decisions + +### Decision A: Existing-admin rotation story → **A1 (going-forward only)** + +We ship the env-var gate alone; rotation of any already-deployed weak `admin123` hash is left to manual operator action, documented in the README and the PR description. **Rationale (3 sentences):** this matches the pattern fix-auth-bypass Brief 1 used for `JWT_SECRET` — fix-loud the input path, do not sweep existing data, and queue any data-sweep work as a separate convoy when an audit demands it. The alternative (a one-time idempotent rotation script under `scripts/migrations/`) is sensible but doubles the brief count and introduces a new env var (`ADMIN_RESET_PASSWORD`) for a problem we cannot confirm exists in any specific deploy. If the queued `pick-a-name` / launch-audit convoys surface a real deploy still using the weak hash, the follow-up convoy `rotate-default-admin` is queued in § Anything flagged but not acted on for that purpose. + +### Decision B: `no-go-zones` reading → **operational change is allowed** + +The no-go-zones rule prohibits editing `scripts/setup-neon-db.js` for **schema changes** (`CREATE TABLE`, `ALTER`, new columns, idempotency-of-DDL concerns). **Rationale (3 sentences):** this convoy's edit is purely operational — it gates one bcrypt input on an env var and adjusts two `console.log` lines, with zero changes to DDL, table definitions, or the `ON CONFLICT` clause. That's structurally equivalent to Brief 1 of fix-auth-bypass, which added env-var fail-loud to `lib/auth-secret.js` without anyone treating it as a schema-rule violation. The brief documents the distinction explicitly in its "Conventions to follow" section so future agents reading either file see the reasoning. + +### Decision C: vitest coverage → **C1 (no tests)** + +The setup script runs a handful of times per environment lifetime; its failure modes are loud (`process.exit(1)` + clear stderr) and immediate; and the manual-smoke acceptance criteria in the brief cover both the happy and fail-loud paths. **Rationale (3 sentences):** unit-testing the env-var path would require either extracting a `lib/seed-admin.js` module (Decision C3 — scope expansion, ~2× brief LOC, plus a new file to maintain) or spawning `node scripts/setup-neon-db.js` from vitest and mocking `@neondatabase/serverless` (slow, fragile, low signal). Following the same heuristic the existing vitest suite uses — "tests cover the runtime auth surface that 30+ handlers depend on, not the operational scripts that run once per env" — leaves the test inventory focused on the highest-blast-radius surfaces. If a future convoy extracts seed logic for any other reason (e.g. multi-env seed templates), it can add tests at that point for free. + +### Decision D: scope expansion to include CJS → ESM conversion → **Option B (expand this convoy)** + +**Discovered mid-convoy:** the implementer for brief 1 confirmed that `scripts/setup-neon-db.js` does not actually run via `npm run setup-db` on Node 22.x. The `bump-next-js` convoy added `"type": "module"` to `package.json` (required for ESLint v9 flat config); the seed script still uses CJS `require()` calls and throws `ReferenceError: require is not defined in ES module scope` on first invocation. The architect's note in § "Anything flagged but not acted on" #1 — "it runs successfully today under Node 22" — was incorrect for Node 22.14.0. + +**Decision:** expand this convoy to include brief 2 (`convert-setup-db-to-esm`) rather than queue a separate `convert-setup-db-to-esm` follow-up convoy. **Rationale (3 sentences):** brief 1's env-var gate is theatrical security on a script no operator can actually execute on Node 22.x, so the two fixes are logically coupled and shipping them in one PR creates a single coherent "setup-db is now both safe and functional" change. The CJS→ESM conversion is mechanical (~6 LOC, no functional changes) and touches the same file as brief 1, so review and audit overhead is near-zero. Splitting into two convoys would mean operators on Node 22.x cannot bootstrap a database between PRs — an unnecessary regression window for a fix that fits cleanly in the same surface area. User ratified the expansion (Option B) on 2026-05-23 after the implementer's gate-1 report surfaced the breakage. + +## Anything flagged but not acted on + +These are real findings surfaced during architecture but **deliberately out of scope** for this convoy. Each should be tracked separately so the audit trail survives. + +1. ~~**CommonJS `require()` in an ESM package (`scripts/setup-neon-db.js`).**~~ **Resolved by brief 2 (added 2026-05-23 mid-convoy per Decision D).** Original architect's claim that "it runs successfully today under Node 22" was incorrect — the implementer for brief 1 confirmed the script throws `ReferenceError: require is not defined in ES module scope` on Node 22.14.0. Scope was expanded to include the CJS→ESM conversion in this convoy rather than queue it as a separate follow-up. See § Decisions D for the ratification. +2. **Sibling weak-credential references in `scripts/reset-db.js`, `scripts/create-test-users.js`, and `TESTING_GUIDE.md`.** `reset-db.js` mirrors `setup-neon-db.js`'s admin-INSERT and password echo (lines ~141-156); `create-test-users.js` prints `admin@tcgvault.com / admin123 (Admin)` as a usage hint (line ~34); `TESTING_GUIDE.md` has the same row in its test-account table (line ~7). **Why not fixed here:** the convoy spec's "Out of scope" list explicitly excludes "any other `scripts/seed-*.js` or `scripts/add-*.js` files (those are one-off historical jobs per no-go-zones)", and `reset-db.js` is in the `scripts/fix-*` / historical family. `TESTING_GUIDE.md` is the manual-QA doc that doc-writer is expected to rename to `docs/MANUAL_QA.md` in the launch-polish convoy (P3, ship-readiness § Role-doc-writer findings) — folding it in here adds review overhead. **Follow-up suggestion:** queue a small `purge-weak-creds-from-helpers` convoy as part of the launch-polish phase, or roll it into `pick-a-name` (since the email itself is also changing). +3. **The success-summary `console.log(' Admin User: admin@tcgvault.com')` line.** This brief deletes it along with the password line (R3), but the admin email itself is still hardcoded in the SQL `INSERT` (line 135) and is going to be renamed under the queued `pick-a-name` convoy (P1 #12). Not a credentials issue, but worth noting that the email is still a known constant. **Why not fixed here:** branding decision belongs to `pick-a-name`; this convoy is credentials-only. diff --git a/.convoys/drop-public-setup/brief-1-env-var-admin-password.md b/.convoys/drop-public-setup/brief-1-env-var-admin-password.md new file mode 100644 index 0000000..4e80764 --- /dev/null +++ b/.convoys/drop-public-setup/brief-1-env-var-admin-password.md @@ -0,0 +1,167 @@ +--- +convoy: drop-public-setup +brief_number: 1 +depends_on: [] +files: + - scripts/setup-neon-db.js + - README.md +--- + +# Brief 1: Require `ADMIN_INITIAL_PASSWORD`; strip credentials from README + +## Goal (1 sentence) + +Gate admin-user creation in `scripts/setup-neon-db.js` behind a required `ADMIN_INITIAL_PASSWORD` env var (fail-loud with an actionable message and exit code 1 if unset), remove the hardcoded `'admin123'` literal and the `console.log` lines that echo credentials to stdout, and replace the README's "Default Admin Account" section with a "First-time admin setup" paragraph that documents the new env var. + +## Files in scope (do not edit anything else) + +- `scripts/setup-neon-db.js` — modified +- `README.md` — modified + +## Conventions to follow + +- **No-go-zones reading.** `.cursor/rules/no-go-zones.mdc` prohibits editing `scripts/setup-neon-db.js` for **schema** changes (DDL, columns, idempotency-of-DDL). This brief's change is **operational** — env-var gate plus two `console.log` removals, with zero changes to `CREATE TABLE` blocks, table shape, or the `ON CONFLICT` clause. The Architect explicitly recorded this distinction in `.convoys/drop-public-setup.md` § Decisions B. **Do not** treat the no-go-zones rule as banning operational hardening of this file. +- **Match existing file style.** `scripts/setup-neon-db.js` uses `#!/usr/bin/env node` shebang and CommonJS `require('dotenv')` / `require('@neondatabase/serverless')` / `require('bcryptjs')`. The repo's `package.json` is `"type": "module"`, but converting CJS → ESM is **out of scope** for this brief (flagged for a separate follow-up convoy in `.convoys/drop-public-setup.md` § Anything flagged but not acted on). Keep `require()` everywhere; do not introduce `import` statements. +- **Fail-loud-message shape, modeled on Brief 1 of `fix-auth-bypass`.** That brief's `lib/auth-secret.js::JWT_SECRET` throw is the template for the actionable-message shape (names the env var, points at `.env.local`, suggests a generation tip, mentions CI as an alternative). See `.convoys/fix-auth-bypass/brief-1-central-jwt-secret-helper.md` § "Acceptance criteria → `lib/auth-secret.js` (new)" for the verbatim message form. **Difference:** that brief throws at module load; this brief calls `console.error(...)` + `process.exit(1)` at function entry, because the script is a CLI tool, not an imported module — exiting with a non-zero code is the canonical CLI "fail loudly" signal. +- **No new dependencies.** `bcryptjs` and `dotenv` are already in `dependencies`. No `package.json` change. +- **No edits to** any file outside `files:` above. In particular: no `lib/`, no `pages/`, no `components/`, no `scripts/reset-db.js`, no `scripts/create-test-users.js`, no `TESTING_GUIDE.md`. Those three sibling files still reference `admin@tcgvault.com` / `admin123` but are explicitly out of scope per the convoy spec and the architect's flagged follow-ups. If you find yourself wanting to touch them, **stop and flag in PR description**; do not expand the diff. + +## Acceptance criteria + +### `scripts/setup-neon-db.js` + +- [ ] **Add the env-var check at the top of `setupNeonDatabase()`**, **before** the `const sql = neon(process.env.POSTGRES_URL);` line. The check must: + - Read `process.env.ADMIN_INITIAL_PASSWORD` into a local `const adminPassword`. + - If `adminPassword` is `undefined`, `null`, empty string, or only whitespace, write a clear error message to `console.error` and call `process.exit(1)`. **Do not** call `process.exit(1)` directly without first logging. + - The error message body MUST name the env var, point at `.env.local`, give a generation suggestion, and mention the CI-secret alternative. Verbatim shape (the message body MAY be reworded for tone, but every claim MUST be present): + +```js +const adminPassword = process.env.ADMIN_INITIAL_PASSWORD; +if (!adminPassword || !adminPassword.trim()) { + console.error( + '❌ ADMIN_INITIAL_PASSWORD environment variable is not set.\n' + + '\n' + + ' Set it in .env.local for local dev, or as a CI secret if you run setup from CI.\n' + + ' Generate a strong password with: openssl rand -base64 24\n' + + ' See README.md → "First-time admin setup" for the full flow.\n' + ); + process.exit(1); +} +``` + +- [ ] **The check sits BEFORE the `neon(...)` call** so the script does not open a DB connection (and does not run any DDL) when the env var is missing. This is the documented contract — operators see the error and fix their env BEFORE touching the database. +- [ ] **Replace `bcrypt.hash('admin123', 12)`** (currently line 131) with `bcrypt.hash(adminPassword, 12)`. The `12` cost factor is unchanged. +- [ ] **Do not change `ON CONFLICT (email) DO NOTHING`** on the `INSERT INTO users` statement (line 136). This is intentional — re-running `setup-db` on an env that already has the admin row is a no-op for the password. Rotation is out of scope (Decision A — see convoy file). +- [ ] **Delete the two credential-echo `console.log` lines** in the success summary (currently lines 144-145): + +```js +// before: +console.log(' Admin User: admin@tcgvault.com'); +console.log(' Admin Password: admin123'); + +// after: replace BOTH lines with a single line that does not echo credentials: +console.log(' Admin user ready (email: admin@tcgvault.com)'); +``` + + Do **not** interpolate `adminPassword` into the log — that would put the chosen password into stdout (and into CI logs if setup ever runs there). The chosen password lives in the operator's env-var source only. +- [ ] The script's existing CommonJS `require()` calls, transaction structure (none — each `sql\`\`` is its own connection per Neon HTTP semantics), and error handling (`try/catch` with `process.exit(1)` on failure) are **unchanged**. +- [ ] No new `require()` imports. `bcryptjs` is already required inline at line 130 (move it to the top of the file ONLY if the linter complains; otherwise leave inline to keep the diff minimal). + +### `README.md` + +- [ ] **Update the env-example block in step 3** (currently lines 44-52) of the Installation section. Add `ADMIN_INITIAL_PASSWORD` after `JWT_SECRET` with a comment. Final shape of that block: + +```env +POSTGRES_URL="postgresql://your-username:your-password@your-host/your-database" +JWT_SECRET="" +# Required for `npm run setup-db` — used once to hash the initial admin password. +# Set in .env.local for local dev, or as a CI secret if you run setup from CI. +ADMIN_INITIAL_PASSWORD="" +# Optional — exercise the rate limiter locally. Without them, `lib/rate-limit.js` +# warn-and-no-ops in dev. In production these are auto-provisioned by the +# Vercel Upstash Marketplace integration. +KV_REST_API_URL="https://.upstash.io" +KV_REST_API_TOKEN="" +``` + + Keep the existing trailing line `` `JWT_SECRET` is **required** — `lib/auth-secret.js` throws at import time if it's unset. ``. Add a second sentence immediately after it: `` `ADMIN_INITIAL_PASSWORD` is **required** for `npm run setup-db` — the script exits with code 1 if it's unset. `` +- [ ] **Replace the "🔐 Default Admin Account" section** (currently lines 115-119) with a "🔐 First-time admin setup" section. New copy (markdown body is suggestive, not verbatim — adjust prose to match repo voice, but every claim below MUST be present): + +```markdown +## 🔐 First-time admin setup + +`npm run setup-db` creates a single admin user the first time it runs. The +password is read from the `ADMIN_INITIAL_PASSWORD` environment variable; the +script exits with code 1 (and does not open a database connection) if the +variable is unset or empty. + +- **Local dev:** set `ADMIN_INITIAL_PASSWORD` in `.env.local` before running + `npm run setup-db`. Use `openssl rand -base64 24` (or any other strong + source) to generate the value. +- **CI / Vercel:** set `ADMIN_INITIAL_PASSWORD` as a project secret if setup + ever runs from CI. The env var is **only** read by the seed script; runtime + auth uses the per-user password stored in the database. +- **Admin email:** the seed creates `admin@tcgvault.com`. Change the password + immediately after first login via the app's profile settings. + +> **Operators of envs that pre-date this change:** `npm run setup-db` is +> idempotent (`ON CONFLICT (email) DO NOTHING`) — re-running it with +> `ADMIN_INITIAL_PASSWORD` set will **not** rotate an existing admin row's +> password. If your environment was set up before this change and still has +> the weak default (`admin123`), rotate the password manually via the app +> after logging in, or wait for the queued `rotate-default-admin` follow-up +> convoy. +``` + +- [ ] **Remove every other occurrence of `admin123` in `README.md`.** Grep verification: `rg 'admin123' README.md` returns **zero** hits after this brief. +- [ ] **Do NOT remove the line `**Admin Panel**: Manage cards and users`** under "## 🚀 Features" — that's a feature description, not a credential. +- [ ] **Do NOT touch the "Database Schema" / "API Endpoints" / "Deployment" sections.** Out of scope. + +### Repo-wide grep verification (run before opening PR) + +- [ ] `rg "'admin123'" --type js scripts/setup-neon-db.js` returns **zero** hits. +- [ ] `rg 'admin123' README.md` returns **zero** hits. +- [ ] `rg 'process\.env\.ADMIN_INITIAL_PASSWORD' --type js` returns exactly one hit — in `scripts/setup-neon-db.js`. +- [ ] `rg 'admin123' --type js scripts/` returns hits in `scripts/reset-db.js` and `scripts/create-test-users.js`. **Leave those alone** — they are explicitly out of scope per the convoy spec (see § "Out of scope" in `.convoys/drop-public-setup.md` and § "Anything flagged but not acted on" in the architecture section). Document them in the PR description as known-but-deferred references. +- [ ] `rg 'admin123' TESTING_GUIDE.md` still has one hit (line 7). Leave it; same follow-up bucket. + +### Smoke (manual — no test runner for this brief; Decision C — C1) + +Run these in order and paste the relevant terminal output into the PR description: + +- [ ] **Fail-loud path.** In your shell, `unset ADMIN_INITIAL_PASSWORD` (or comment it out in `.env.local`), then run `npm run setup-db`. Expect: + - Exit code is 1 (`echo $?` immediately after returns `1`). + - Stderr contains the error message body (env var name, `.env.local` reference, `openssl rand -base64 24` suggestion, README pointer). + - **Stdout does NOT contain `✅ Connecting to Neon database...`** — the check fires before the `neon(...)` call. +- [ ] **Happy path.** Set `ADMIN_INITIAL_PASSWORD=temporary-strong-pw-for-smoke` in `.env.local` (or `export` it in the shell), then run `npm run setup-db`. Expect: + - Exit code is 0. + - Stdout includes `✅ Created admin user` (or the existing equivalent line if the row already exists — both are acceptable; `ON CONFLICT DO NOTHING` keeps the script idempotent). + - Stdout does **NOT** contain the literal string `admin123` anywhere. + - Stdout does **NOT** contain `temporary-strong-pw-for-smoke` (the chosen password) anywhere. +- [ ] **End-to-end login.** With the env var still set, run `npm run dev`. From a second terminal: `curl -sX POST http://localhost:3000/api/auth/login -H 'content-type: application/json' -d '{"email":"admin@tcgvault.com","password":"temporary-strong-pw-for-smoke"}'`. Expect HTTP 200 + a JWT in the body **only if** the admin row was created by this run (i.e. the DB was empty for `admin@tcgvault.com` before step 2). If the row pre-existed with a different password (e.g. the weak `admin123` from before this convoy), expect 401 — that's the documented R1 / Decision A behavior (re-running setup-db does **not** rotate; that's manual rotation territory). Paste whichever outcome you got and note which case applies. +- [ ] **Lint baseline.** `npm run lint` exits 0 (or matches the existing pre-PR baseline — pre-existing lint errors are fine; do not introduce new ones). +- [ ] **Tests.** `npm run test:run` is green (no new tests added per Decision C; existing 16 auth tests should still pass — this brief does not touch any file they cover). + +### Pre-merge operator checklist (paste into PR description) + +This is the human-side handoff. Reviewer confirms each item is acknowledged before merging: + +- [ ] **R1 callout:** "Existing deploys with the weak `admin123` hash are NOT rotated by this PR. If any deployed environment (production, staging, dev branches) currently has `admin@tcgvault.com / admin123` in its database, the operator must rotate the password manually after this merges — log in with the weak password, change it via profile settings, then verify the new hash. The queued `rotate-default-admin` follow-up convoy will land an idempotent rotation script if any real deploy still has the weak hash after manual triage." +- [ ] **CI / Vercel env var:** "Before merging, confirm `ADMIN_INITIAL_PASSWORD` is set as a Vercel project secret on any branch that runs `npm run setup-db` from CI (today: none — this is a `package.json` script run manually). If/when a setup CI step is added, the secret MUST be in place first or the CI job will exit 1." +- [ ] **Reviewer ran the fail-loud and happy-path smoke locally** OR has confirmed the PR description includes terminal output proving both paths. + +### Out of scope (do not do these) + +- [ ] No edit to `scripts/reset-db.js`, `scripts/create-test-users.js`, `TESTING_GUIDE.md`. Sibling weak-credential references — convoy out-of-scope, architect-flagged follow-up. +- [ ] No edit to `lib/auth-secret.js`. JWT secret model is `fix-auth-bypass` Brief 1's surface, already shipped. +- [ ] No edit to `pages/api/auth/login.js` or `pages/api/auth/register.js`. CORS / rate-limit are `fix-auth-bypass` Brief 4's surface, already shipped (partial — `verify.js` deferred to `cors-tighten`). +- [ ] No CommonJS → ESM conversion of `scripts/setup-neon-db.js`. Flagged for a separate convoy. +- [ ] No idempotent rotation script under `scripts/migrations/`. Decision A defers this to the queued `rotate-default-admin` follow-up convoy. +- [ ] No new `vitest` tests for the env-var path. Decision C — C1. +- [ ] No `package.json` change. `bcryptjs` and `dotenv` are already installed. +- [ ] No `AGENTS.md` or `.cursor/rules/*.mdc` updates. Doc-writer pass updates these AFTER the convoy lands. (Specifically: Gotcha #4 in `AGENTS.md` is the next doc-writer change; do not touch it in this PR.) +- [ ] No `.github/workflows/*.yml` change. CI gates for forbidden endpoints are already in place from `fix-auth-bypass` Brief 3. + +## Rationale (≤3 sentences) + +Gating the seed script behind a required env var converts the one remaining hardcoded credential in the source tree (the `'admin123'` bcrypt input) into operator-supplied input, while the README rewrite removes the same credential from the documentation surface — together they close P0 #3 from `.convoys/ship-readiness.md` and resolve `AGENTS.md` Gotcha #4. Keeping `ON CONFLICT (email) DO NOTHING` and deleting (rather than reformatting) the credential-echo `console.log` lines ensures the brief does not introduce a silent-rotation surprise (R4) or a stdout-leak surprise (R3) on top of the intended fix. Holding the no-go-zones reading at "operational change is allowed" — and documenting it explicitly in this brief — gives the next agent (likely doc-writer or whoever opens `rotate-default-admin`) a clear precedent for distinguishing operational hardening of `setup-neon-db.js` from the prohibited schema-edit case. diff --git a/.convoys/drop-public-setup/brief-2-convert-setup-db-to-esm.md b/.convoys/drop-public-setup/brief-2-convert-setup-db-to-esm.md new file mode 100644 index 0000000..dad5bdb --- /dev/null +++ b/.convoys/drop-public-setup/brief-2-convert-setup-db-to-esm.md @@ -0,0 +1,179 @@ +--- +convoy: drop-public-setup +brief_number: 2 +depends_on: + - brief: 1 +files: + - scripts/setup-neon-db.js +--- + +# Brief 2: Convert `scripts/setup-neon-db.js` from CJS to ESM + +## Goal (1 sentence) + +Convert `scripts/setup-neon-db.js` from CommonJS (`require()`) to native ES modules (`import`) so it runs on Node 22.x where `package.json` has `"type": "module"`; no functional changes — pure module-system conversion. + +## Why this brief exists (context) + +`bump-next-js` added `"type": "module"` to `package.json` so ESLint v9's flat config (`eslint.config.mjs`) could be picked up under the default loader semantics. As a side effect, every untyped `.js` file in the repo is now treated as ESM by Node. `scripts/setup-neon-db.js` still uses `require()` and immediately throws on Node 22.x: + +``` +ReferenceError: require is not defined in ES module scope +This file is being treated as an ES module because it has a '.js' file extension +and '/.../package.json' contains "type": "module". +``` + +Brief 1 added an `ADMIN_INITIAL_PASSWORD` env-var gate to this script, but the gate is theatrical until the script actually executes. This brief makes the script executable so brief 1's hardening takes effect. + +The CJS→ESM conversion was originally flagged in `.convoys/drop-public-setup.md` § "Anything flagged but not acted on" #1 as a separate follow-up. After mid-convoy discovery (the implementer for brief 1 confirmed the script is non-functional today on Node 22.14.0), the scope was expanded to land both fixes together. See `.convoys/drop-public-setup.md` § Decisions D for the ratification. + +## Files in scope (do not edit anything else) + +- `scripts/setup-neon-db.js` — modified (module-system conversion only) + +**Do NOT** edit `README.md`, `package.json`, `package-lock.json`, any other `scripts/*.js`, or any file under `lib/`, `pages/`, `components/`, `.github/`, or `.cursor/`. Brief 1's README changes already shipped on the convoy branch; do not touch them. + +## Conventions to follow + +- **No functional changes.** This brief is a 1:1 module-system conversion. Same logic, same DDL, same `process.exit(1)`, same env-var gate, same `console.log` lines. The diff should be 4-6 lines of `require` → `import` plus the `dotenv` invocation change. +- **No new dependencies.** `bcryptjs`, `dotenv`, and `@neondatabase/serverless` are already installed (verified by `npm run setup-db` failing on `require` rather than on missing modules). +- **Match brief 1's verbatim env-var-check block.** Do not touch the `if (!adminPassword || !adminPassword.trim())` block from brief 1 — only the `require()` calls around it change. +- **No top-level `await`.** ESM supports it, but the existing `setupNeonDatabase()` bottom-of-file invocation is intentionally fire-and-forget (the function does its own `try/catch` + `process.exit(1)`). Keep that pattern; do not introduce `await setupNeonDatabase()`. +- **Move `bcrypt` import to the top of the file.** The CJS version uses inline `const bcrypt = require('bcryptjs')` mid-function. ESM has no equivalent of late `require()` — all `import` statements must be at the top. This is a structural requirement of ESM, not a stylistic preference. +- **No `__dirname` or `__filename`.** The script does not currently use either, so no ESM equivalents (`import.meta.url`, `fileURLToPath`) are needed. +- **No `.cjs` rename.** The right answer is real ESM, not bypassing `"type": "module"` with a `.cjs` extension. A `.cjs` rename would also break the `setup-db` npm script (`node scripts/setup-neon-db.js`) without a corresponding `package.json` edit, which is out of scope here. + +## Acceptance criteria + +### `scripts/setup-neon-db.js` + +- [ ] **Replace the `dotenv` invocation** (currently line 11): + +```js +// before: +require('dotenv').config({ path: '.env.local' }); + +// after: +import dotenv from 'dotenv'; +dotenv.config({ path: '.env.local' }); +``` + +- [ ] **Replace the `neon` import** (currently line 13): + +```js +// before: +const { neon } = require('@neondatabase/serverless'); + +// after: +import { neon } from '@neondatabase/serverless'; +``` + +- [ ] **Hoist the `bcryptjs` import to the top of the file**, alongside the other imports. Remove the inline `const bcrypt = require('bcryptjs');` from inside `setupNeonDatabase()` (currently line 142): + +```js +// at the top of the file, after the dotenv block: +import bcrypt from 'bcryptjs'; + +// inside setupNeonDatabase(), delete this line: +const bcrypt = require('bcryptjs'); +``` + +- [ ] **Final import-block order at the top of the file** (after the shebang + JSDoc header): + +```js +#!/usr/bin/env node + +/** + * Neon Database Setup Script + * ... (existing JSDoc) ... + */ + +import dotenv from 'dotenv'; +dotenv.config({ path: '.env.local' }); + +import { neon } from '@neondatabase/serverless'; +import bcrypt from 'bcryptjs'; + +async function setupNeonDatabase() { + // ... env-var check from brief 1, unchanged ... + // ... rest of function, with the inline require deleted ... +} + +setupNeonDatabase(); +``` + + **Note:** placing `dotenv.config(...)` *between* the import statements is intentional — `dotenv` must run before any other module reads `process.env`. ESM hoists `import` declarations but executes them in source order, so the `import dotenv from 'dotenv'` declaration hoists to the top, the `dotenv.config({...})` call executes after the import resolves, and the subsequent `import { neon }` / `import bcrypt` resolve after that. If the linter complains about "imports not grouped together," resolve by moving the `dotenv.config({...})` call into a separate side-effect import (`import 'dotenv/config'` will not work here because we need the custom `.env.local` path). + +- [ ] **The env-var check from brief 1 is unchanged.** Lines 15-26 (the `const adminPassword = ...` block, the `if (!adminPassword || !adminPassword.trim())` block, and the `process.exit(1)` call) stay byte-identical. This brief only touches the import block at the top of the file and the inline `require('bcryptjs')` deletion. + +- [ ] **The DDL, `INSERT`, `ON CONFLICT` clause, success summary `console.log` lines, and `try/catch/process.exit(1)` are all unchanged.** Pure module-system conversion. + +- [ ] **`setupNeonDatabase();` at the bottom of the file stays as-is** (bare call, no `await`). + +### Verification + +- [ ] **Smoke: fail-loud path.** With `ADMIN_INITIAL_PASSWORD` unset: + +```bash +unset ADMIN_INITIAL_PASSWORD +npm run setup-db +echo "exit=$?" +``` + + Expected: + - Exit code: `1` + - Stderr contains the brief-1 actionable error message (names `ADMIN_INITIAL_PASSWORD`, points at `.env.local`, suggests `openssl rand -base64 24`) + - **Stdout does NOT contain `✅ Connecting to Neon database...`** — the env-var check fires before `neon(...)`, just like in the brief-1 smoke + - **No `ReferenceError: require is not defined`** — this is the bug brief 2 fixes; its absence is the primary signal + - **No `SyntaxError: Cannot use import statement outside a module`** — would indicate the conversion picked the wrong direction + +- [ ] **Smoke: happy path.** With the env var set: + +```bash +export ADMIN_INITIAL_PASSWORD=brief-2-smoke-temporary-pw +npm run setup-db +echo "exit=$?" +``` + + Expected: + - Exit code: `0` + - Stdout contains `✅ Connecting to Neon database...` and `✅ Created admin user` (or the equivalent for an already-existing admin row — `ON CONFLICT DO NOTHING` keeps the script idempotent) + - Stdout does **NOT** contain `admin123` or `brief-2-smoke-temporary-pw` (R3 from brief 1 — credentials never echoed to stdout) + - No `ReferenceError`, no `SyntaxError`, no `import`/`require` complaints + +- [ ] **Lint.** `npm run lint` exits 0 (or matches the existing baseline — pre-existing lint errors are fine; do not introduce new ones). + +- [ ] **Tests.** `npm run test:run` is green. All 16 existing vitest tests should still pass — this brief does not touch any file they cover. + +- [ ] **Grep sanity.** `rg 'require\(' scripts/setup-neon-db.js` returns **zero** hits after the conversion. + +- [ ] **Brief-1 smoke still passes.** Re-run the brief-1 end-to-end login smoke against `npm run dev`: + +```bash +curl -sS -X POST http://localhost:3000/api/auth/login \ + -H 'content-type: application/json' \ + -d '{"email":"admin@tcgvault.com","password":"brief-2-smoke-temporary-pw"}' +``` + + Expected: same R1 case-A behavior as brief 1 (if the local admin row predates `ADMIN_INITIAL_PASSWORD`, the new password 401s and the old `admin123` still 200s; if the admin row was newly created, the new password 200s with a JWT). Paste the result into the PR description. + +### Pre-merge checklist (paste into PR description, alongside brief 1's checklist) + +- [ ] **R1 callout (carried over from brief 1):** existing deployed admin rows with the weak `admin123` hash are NOT rotated by this PR. Operators rotate manually after merge. +- [ ] **Brief 2 makes `npm run setup-db` executable again on Node 22.x.** Before this brief, the script threw `ReferenceError: require is not defined in ES module scope` immediately on invocation. After this brief, the env-var gate from brief 1 actually fires. +- [ ] **Reviewer ran the brief-2 fail-loud and happy-path smoke locally** OR confirmed the PR description includes terminal output proving both paths. + +### Out of scope (do not do these) + +- [ ] No `package.json` change. `"type": "module"` stays; this brief makes the script align with that setting, not the other way around. +- [ ] No edit to `scripts/reset-db.js`, `scripts/create-test-users.js`, or any other `scripts/*.js`. They have the same CJS-in-ESM bug, but converting all of them widens the diff and bleeds into the no-go-zones rule for historical scripts. Queue `convert-helper-scripts-to-esm` as a separate convoy if/when those scripts need to run. +- [ ] No edit to `README.md`. Brief 1 owns the README changes; this brief is code-only. +- [ ] No `.cjs` rename. Real ESM only. +- [ ] No top-level `await` introduction at the bottom of the file. +- [ ] No new dependencies. +- [ ] No edit to `lib/`, `pages/`, `components/`, `.github/`, or `.cursor/`. +- [ ] No vitest tests. The smoke commands above are the verification; setup-db is not a runtime-auth surface (Decision C1 from brief 1 still applies — operational scripts use manual smoke, not unit tests). + +## Rationale (≤3 sentences) + +`bump-next-js`'s `"type": "module"` flag silently broke `npm run setup-db` on Node 22.x because the script still uses CJS `require()` calls. Brief 1's `ADMIN_INITIAL_PASSWORD` env-var gate is the right hardening but lands as theatrical security on a script no operator can actually execute — converting to native ESM imports here ensures the gate fires as intended and unblocks first-time DB setup on any Node 22+ environment (including Vercel's default). The diff is 4-6 lines of mechanical conversion with no functional changes, so review risk is minimal and the convoy stays a single coherent PR. diff --git a/README.md b/README.md index dfb3937..8e0ebdc 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,9 @@ A modern trading card game collection manager built with Next.js and Neon Databa ```env POSTGRES_URL="postgresql://your-username:your-password@your-host/your-database" JWT_SECRET="" + # Required for `npm run setup-db` — used once to hash the initial admin password. + # Set in .env.local for local dev, or as a CI secret if you run setup from CI. + ADMIN_INITIAL_PASSWORD="" # Optional — exercise the rate limiter locally. Without them, `lib/rate-limit.js` # warn-and-no-ops in dev. In production these are auto-provisioned by the # Vercel Upstash Marketplace integration. @@ -51,6 +54,7 @@ A modern trading card game collection manager built with Next.js and Neon Databa KV_REST_API_TOKEN="" ``` `JWT_SECRET` is **required** — `lib/auth-secret.js` throws at import time if it's unset. + `ADMIN_INITIAL_PASSWORD` is **required** for `npm run setup-db` — the script exits with code 1 if it's unset. 4. **Set up the database** ```bash @@ -112,11 +116,29 @@ tcg-vault/ └── .env.local # Environment variables ``` -## 🔐 Default Admin Account +## 🔐 First-time admin setup -After running the database setup: -- **Email**: admin@tcgvault.com -- **Password**: admin123 +`npm run setup-db` creates a single admin user the first time it runs. The +password is read from the `ADMIN_INITIAL_PASSWORD` environment variable; the +script exits with code 1 (and does not open a database connection) if the +variable is unset or empty. + +- **Local dev:** set `ADMIN_INITIAL_PASSWORD` in `.env.local` before running + `npm run setup-db`. Use `openssl rand -base64 24` (or any other strong + source) to generate the value. +- **CI / Vercel:** set `ADMIN_INITIAL_PASSWORD` as a project secret if setup + ever runs from CI. The env var is **only** read by the seed script; runtime + auth uses the per-user password stored in the database. +- **Admin email:** the seed creates `admin@tcgvault.com`. Change the password + immediately after first login via the app's profile settings. + +> **Operators of envs that pre-date this change:** `npm run setup-db` is +> idempotent (`ON CONFLICT (email) DO NOTHING`) — re-running it with +> `ADMIN_INITIAL_PASSWORD` set will **not** rotate an existing admin row's +> password. If your environment was set up before this change and still has +> the prior weak default, rotate the password manually via the app +> after logging in, or wait for the queued `rotate-default-admin` follow-up +> convoy. ## 🤝 Contributing diff --git a/scripts/setup-neon-db.js b/scripts/setup-neon-db.js index f619b96..08166ae 100644 --- a/scripts/setup-neon-db.js +++ b/scripts/setup-neon-db.js @@ -8,11 +8,25 @@ */ // Load environment variables from .env.local -require('dotenv').config({ path: '.env.local' }); +import dotenv from 'dotenv'; +dotenv.config({ path: '.env.local' }); -const { neon } = require('@neondatabase/serverless'); +import { neon } from '@neondatabase/serverless'; +import bcrypt from 'bcryptjs'; async function setupNeonDatabase() { + const adminPassword = process.env.ADMIN_INITIAL_PASSWORD; + if (!adminPassword || !adminPassword.trim()) { + console.error( + '❌ ADMIN_INITIAL_PASSWORD environment variable is not set.\n' + + '\n' + + ' Set it in .env.local for local dev, or as a CI secret if you run setup from CI.\n' + + ' Generate a strong password with: openssl rand -base64 24\n' + + ' See README.md → "First-time admin setup" for the full flow.\n' + ); + process.exit(1); + } + const sql = neon(process.env.POSTGRES_URL); try { @@ -127,8 +141,7 @@ async function setupNeonDatabase() { console.log('✅ Created deck_cards table'); // Create admin user - const bcrypt = require('bcryptjs'); - const hashedPassword = await bcrypt.hash('admin123', 12); + const hashedPassword = await bcrypt.hash(adminPassword, 12); await sql` INSERT INTO users (email, password, role) @@ -141,8 +154,7 @@ async function setupNeonDatabase() { console.log(''); console.log('📋 Database Details:'); console.log(' Database: Neon PostgreSQL'); - console.log(' Admin User: admin@tcgvault.com'); - console.log(' Admin Password: admin123'); + console.log(' Admin user ready (email: admin@tcgvault.com)'); console.log(''); console.log('🔧 Next Steps:'); console.log(' 1. Test the API endpoints');