deckhearth/.convoys/drop-public-setup.md
varutasu ad0e254324
chore(convoys): mark shipped convoys and refresh ship-readiness (#66)
Close stale convoy frontmatter for merged scanner, lint, and hygiene work;
record P1 #11.5 and queued follow-ups as RESOLVED with PR references.

Co-authored-by: Cursor <cursoragent@cursor.com>
2026-06-02 11:01:27 -05:00

184 lines
20 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

---
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: shipped
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
**As-shipped:** Brief 1 `ff80753` + Brief 2 `b63b509` (2026-05-23). Closes P0 #3.
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=<value>` 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":"<value>"}'` 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=<different-value>`. 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.