diff --git a/.planning/phases/09-botfights-platform-upgrade/09-02-SUMMARY.md b/.planning/phases/09-botfights-platform-upgrade/09-02-SUMMARY.md new file mode 100644 index 00000000..7d2a80ee --- /dev/null +++ b/.planning/phases/09-botfights-platform-upgrade/09-02-SUMMARY.md @@ -0,0 +1,189 @@ +--- +phase: 09-botfights-platform-upgrade +plan: 02 +subsystem: auth +tags: [nostr, nip-98, nip-07, jwt, hono, vitest, drizzle, sqlite] + +# Dependency graph +requires: + - phase: 09-botfights-platform-upgrade (plan 01 — arena-proxy tracer) + provides: identical pre-existing migrate.ts/schema.ts drift finding (documented in deferred-items.md), confirmed unrelated to arena-proxy and now fixed here +provides: + - "GET /api/auth/me — JWT-only session restore route in botfight's server/src/routes/auth.ts" + - "POST /api/auth/login reduced to a documented-deprecated, read-only lookup (no DB writes)" + - "Client (useNostr.ts) auto-restore now calls GET /api/auth/me instead of POSTing a bare pubkey" + - "server/src/db/migrate.ts brought back in sync with schema.ts (7 missing tables, 9 missing columns) — fixes the pre-existing auth/tournament test failures documented in 09-01's deferred-items.md" +affects: [09-03 (unified prompt/docs), 09-04 (arena deploy/manifest), any future botfight auth work] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "GET /me pattern: extractPubkeyFromAuth(Authorization header) -> verifyJwt -> 401 or read-only db.select — never trust a client-claimed pubkey for identity" + - "Deprecated-but-kept read-only endpoint: strip mutation side effects, keep the read shape, document with an in-code comment pointing at the sanctioned replacement" + +key-files: + created: + - /home/archipelago/Projects/botfight/server/src/routes/auth-me.test.ts + modified: + - /home/archipelago/Projects/botfight/server/src/routes/auth.ts + - /home/archipelago/Projects/botfight/server/src/routes/auth.test.ts + - /home/archipelago/Projects/botfight/frontend/src/composables/useNostr.ts + - /home/archipelago/Projects/botfight/e2e/helpers/auth.ts + - /home/archipelago/Projects/botfight/server/src/db/migrate.ts + +key-decisions: + - "Fixed server/src/db/migrate.ts as an in-scope deviation (Rule 1) rather than leaving it deferred a second time — it directly blocked meaningful verification of this plan's own auth test changes, and the fix was cheap (copy the already-correct DDL from server/src/db/startup.ts, which is the actual startup migration path and was never stale)." + - "Did not touch frontend/src/composables/useWallet.ts, BetPanel.vue, or JoinBoutPage.vue's other pubkey-in-body calls (register/update/wallet/bet) — those are authenticated resource-ownership actions, not identity-claiming session establishment, and are out of this plan's scope per its own read_first/action framing." + - "Ran vitest/tsc/vue-tsc via their compiled binaries directly (./node_modules/.bin/vitest, etc.) instead of through `pnpm vitest`/`pnpm exec` — this environment's pnpm repeatedly demanded an interactive `pnpm approve-builds` step (native module build approval for better-sqlite3 et al.) that is unrelated to this task and, when approved, produced a large unrelated pnpm-lock.yaml/pnpm-workspace.yaml diff (825 lockfile deletions) that was reverted every time rather than committed, to avoid touching shared files outside this task's blast radius in the concurrently-worked-on repo." + +requirements-completed: [BOT-01] + +coverage: + - id: D1 + description: "GET /api/auth/me exists, is JWT-only (no bare pubkey accepted), read-only, and covers missing/malformed/forged/blacklisted/unregistered/valid JWT cases" + requirement: "BOT-01" + verification: + - kind: unit + ref: "server/src/routes/auth-me.test.ts (7 tests)" + status: pass + human_judgment: false + - id: D2 + description: "Client (useNostr.ts) restores sessions via GET /api/auth/me with its stored JWT and never POSTs a bare pubkey to establish/restore a session" + requirement: "BOT-01" + verification: + - kind: unit + ref: "grep -q \"authFetch('/api/auth/me')\" frontend/src/composables/useNostr.ts && test -z \"$(grep -rl 'auth/login' frontend/src)\"" + status: pass + human_judgment: false + - id: D3 + description: "POST /api/auth/login performs no database writes and is documented as a deprecated read-only lookup; an unregistered creator pubkey no longer auto-creates a row" + requirement: "BOT-01" + verification: + - kind: unit + ref: "server/src/routes/auth.test.ts#login: an unregistered creator pubkey returns exists=false and creates no row (auto-create removed — D-01)" + status: pass + - kind: unit + ref: "awk-scoped grep over the /login handler body confirms zero db.insert/db.update calls" + status: pass + human_judgment: false + - id: D4 + description: "Full existing server auth/tournament suite passes against a correctly migrated dev DB" + requirement: "BOT-01" + verification: + - kind: unit + ref: "auth.test.ts + auth-edge.test.ts + auth-audit.test.ts + auth-me.test.ts = 56/56 pass; full server suite 810/817 pass (7 pre-existing timing/perf flakes, none touching auth)" + status: pass + human_judgment: false + - id: D5 + description: "Real-signer verification (NIP-07 browser extension, NIP-55 Amber) against the live JWT-gated flow" + verification: [] + human_judgment: true + rationale: "Explicitly deferred to plan 09-07's human checkpoint per 09-RESEARCH.md Pitfall 5 — no automated harness exists for driving a real window.nostr provider from Playwright, and this plan's own section states this is deliberately NOT claimed here." + +# Metrics +duration: 55min +completed: 2026-07-31 +status: complete +--- + +# Phase 9 Plan 02: Close the bare-pubkey auth gap (BOT-01) Summary + +**JWT-gated `GET /api/auth/me` replaces the client's bare-pubkey auto-restore call, and `POST /api/auth/login`'s creator auto-create/auto-upgrade side effects are removed so an unauthenticated request can no longer mutate the database.** + +## Performance + +- **Duration:** 55 min +- **Started:** 2026-07-31T01:38:00Z (approx.) +- **Completed:** 2026-07-31T02:33:48Z +- **Tasks:** 2 (both `type="auto"`, no checkpoints) +- **Files modified:** 6 (1 new test file, 5 modified) + +## Accomplishments +- `GET /api/auth/me` derives the caller's identity from a verified, non-expired, non-blacklisted JWT only (`extractPubkeyFromAuth` → `verifyJwt`), returns the same `{ exists, bot }` shape `POST /login` used to, and performs no writes. +- `useNostr.ts`'s session auto-restore now calls `authFetch('/api/auth/me')` (a bare GET, no body) instead of `POST /api/auth/login` with `{ pubkey: pubkey.value }` — the client never sends a bare pubkey to claim an identity. +- `POST /api/auth/login`'s creator auto-create branch and creator auto-upgrade `db.update` block were removed; the endpoint is now a pure, documented-deprecated read. The identical creator logic still runs, correctly gated behind NIP-98 verification, inside `POST /nostr/session`. +- `e2e/helpers/auth.ts`'s doc comments now describe `loginWithPubkey` as a read-only test lookup helper, not a login — its request/signature is unchanged so existing e2e specs are unaffected. +- Fixed a pre-existing, previously-deferred bug (Rule 1 deviation): `server/src/db/migrate.ts` had drifted from `schema.ts` (missing 7 tables, 9 columns) and was the reason `auth.test.ts`/`auth-audit.test.ts`/`auth-edge.test.ts`/`tournaments.test.ts` failed against a freshly migrated dev DB. Brought it back in sync with the already-correct DDL in `server/src/db/startup.ts`. + +## Auth surface: before → after + +| Endpoint | Before | After | +|---|---|---| +| `POST /api/auth/login` | Accepted a bare `{pubkey}`; if unregistered AND a creator pubkey, auto-created a bot row and returned it — an unauthenticated request could mutate the DB. If registered, auto-upgraded the creator's `archetype`/`webhookUrl` on every call. | Pure read: looks up the bot by pubkey, returns `{exists, bot}` or `{exists:false, pubkey}`. Zero `db.insert`/`db.update` calls. Doc comment marks it deprecated, session-establishing/token-issuing capability removed. Kept only for backward-compatible lookups (leaderboard-style) and the e2e test helper. | +| `POST /api/auth/nostr/session` | NIP-98 verified, issues JWT, creator auto-create/upgrade gated behind verification. Unchanged by this plan. | Unchanged — remains the only way to establish a session. | +| `GET /api/auth/me` | Did not exist. | New. `Authorization: Bearer ` required; 401 on missing/malformed/forged/expired/blacklisted token (all via `extractPubkeyFromAuth`/`verifyJwt`); 200 `{exists, bot}` for a valid token, read-only. | +| Client session restore (`useNostr.ts`) | `authFetch('/api/auth/login', {method:'POST', body: JSON.stringify({pubkey})})` | `authFetch('/api/auth/me')` — GET, no body, no bare pubkey. | + +## Task Commits + +Each task was committed atomically (TDD RED/GREEN split for Task 1): + +1. **Task 1 RED — failing test for GET /api/auth/me** — `e824f4c` (test) +2. **Task 1 GREEN — GET /api/auth/me implementation** — `635ee39` (feat) +3. **Deviation — sync migrate.ts with schema.ts** — `bf240ce` (fix, committed separately per plan's explicit allowance) +4. **Task 2 — retire the bare-pubkey session path (client + server)** — `2a343ac` (feat) + +All four commits pushed to `origin main` (`https://source.archipelago-foundation.org/lfg2025/botfights.git`). + +## Files Created/Modified +- `server/src/routes/auth-me.test.ts` — new: 7 tests covering missing/malformed/forged/blacklisted/unregistered/valid-token cases for `GET /me`, plus a no-writes assertion. +- `server/src/routes/auth.ts` — added `GET /me`; reduced `POST /login` to a read-only deprecated lookup (removed creator auto-create + auto-upgrade); deduped the now-single `extractPubkeyFromAuth` import. +- `server/src/routes/auth.test.ts` — added a case asserting an unregistered creator pubkey via `/login` returns `exists:false` and leaves the `bots` row count unchanged. +- `frontend/src/composables/useNostr.ts` — auto-restore now calls `GET /api/auth/me` instead of `POST /api/auth/login`. +- `e2e/helpers/auth.ts` — doc comments updated to describe `loginWithPubkey` as a read-only test helper, not a login. +- `server/src/db/migrate.ts` — (deviation) brought the standalone `pnpm migrate` CLI script's DDL back in sync with `schema.ts`, matching `startup.ts`'s already-correct migrations. + +## Decisions Made +- Fixed `migrate.ts` as an in-scope deviation rather than deferring it again — the failures it caused were directly in this plan's own auth test blast radius, and the correct DDL already existed verbatim in `startup.ts`, making the fix low-risk (copy, don't invent). +- Left every other `pubkey`-in-body client call site (register, update, wallet ops, betting, tournament join) untouched — those are authenticated actions using pubkey as an established resource key, not "claim an identity to get a session," and are out of this plan's stated scope. +- Used direct binary invocation (`./node_modules/.bin/vitest`, `./node_modules/.bin/tsc`, `./frontend/node_modules/.bin/vue-tsc`) for verification runs instead of `pnpm vitest`/`pnpm exec tsc`, because this environment's `pnpm` repeatedly required an interactive `pnpm approve-builds` step unrelated to this task; each time it was run to unblock a test pass it produced a large, unrelated `pnpm-lock.yaml`/`pnpm-workspace.yaml` diff (~825 lockfile line deletions) that was reverted (`git checkout --`) rather than committed, since committing it was outside this plan's scope and risked contaminating the concurrently-worked-on shared tree. + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 1 - Bug] `server/src/db/migrate.ts` DDL had drifted from `schema.ts`, causing 15 pre-existing auth/tournament test failures** +- **Found during:** Baseline test run before Task 1 (per the executor prompt's explicit instruction to check whether this was cheap to fix within the auth blast radius). +- **Issue:** `migrate.ts` (the standalone `pnpm --filter server migrate` CLI script) was missing 7 tables (`payments`, `wallet_connections`, `bets`, `tournaments`, `tournament_entries`, `analytics`, `tournament_matches`) and 9 columns on `bots`/`fights` (`sats_won`, `sats_wagered`, `has_wallet`, `zaps_received`, `bot_type`, `mode`, `pot_sats`, `payout_status`, `current_season`) present in `schema.ts`. The route-level tests (`auth.test.ts`, `auth-audit.test.ts`, `auth-edge.test.ts`, `tournaments.test.ts`) hit the real `db/index.ts` singleton against the on-disk, gitignored `server/data/botfights.db`, which only `startup.ts`'s `runMigrations()` (called from `index.ts` at server boot, never from `app.ts` which tests import directly) or this `migrate.ts` script populate. Vitest itself never runs a migration, so a freshly migrated or freshly deleted dev DB reproduced the failures described in plan 09-01's `deferred-items.md`. +- **Fix:** Rewrote `migrate.ts`'s `CREATE TABLE`/`ALTER TABLE` statements to exactly match `schema.ts`, column-for-column and table-for-table — copying the already-correct DDL that exists in `server/src/db/startup.ts` (the actual runtime migration path, which was never stale). +- **Files modified:** `server/src/db/migrate.ts`. +- **Verification:** `pnpm --filter server migrate` against a fresh DB, then full server suite went from 15 failed / 789 passed (baseline) to 6 failed / 798 passed on the first re-run, and 810 passed / 7 failed after this plan's own auth changes and the concurrent agent's unrelated commits landed — the remaining failures are all pre-existing timing/perf flakes (`answers.test.ts`, `lifecycle.test.ts` x2-3, `bot-auth.test.ts` constant-time, `docs.test.ts`, `speed-meta.test.ts`, `tier-balance.test.ts`) documented in 09-01's `deferred-items.md` as CPU-contention-sensitive, none touching auth. +- **Committed in:** `bf240ce` (separate commit, per the executor prompt's explicit instruction). + +--- + +**Total deviations:** 1 auto-fixed (Rule 1 — bug fix, out-of-scope-but-cheap per explicit plan/prompt allowance). +**Impact on plan:** Necessary for this plan's own auth test verification to be meaningful (running against tables that didn't exist would have masked real regressions). No scope creep — the fix only touches DDL already correctly defined elsewhere in the same repo; no new architecture, no new endpoints. + +## Test Counts + +- **Baseline before any change (per executor prompt instruction):** `pnpm vitest run --project server` → 15 failed / 789 passed (804 total). Matches the ~14 pre-existing failures the prompt described. +- **After the migrate.ts fix alone:** 6 failed / 798 passed (804 total) — all 15 previously-failing auth/tournament tests now pass; the 6 remaining are pre-existing timing/perf flakes. +- **After both auth tasks (final):** + - Targeted: `auth.test.ts` + `auth-edge.test.ts` + `auth-audit.test.ts` + `auth-me.test.ts` = **56/56 pass** (7 new in `auth-me.test.ts`, 1 new in `auth.test.ts`, 48 pre-existing). + - Full server suite: **810 passed / 7 failed** (817 total; the +8 vs. the 804/809 baseline count is the new `auth-me.test.ts` tests plus the new `auth.test.ts` case). The 7 remaining failures are the same pre-existing timing/perf-under-CPU-load class documented in `deferred-items.md` (`lifecycle.test.ts` x2-3, `speed-meta.test.ts`, `tier-balance.test.ts`, `bot-auth.test.ts` constant-time) — **zero of them touch auth**, and this run happened after the concurrent 09-03 agent's commits (`a080956`, `2dd9947`) had already landed in the same shared working tree, confirming no cross-plan regression. + - `tsc --noEmit -p server/tsconfig.json` — exit 0. + - `vue-tsc --noEmit -p frontend/tsconfig.json` — exit 0. + +## Issues Encountered +- This environment's `pnpm` required an interactive `pnpm approve-builds` step (native module build approval for `better-sqlite3`/`esbuild`/`onnxruntime-node`/`protobufjs`/`sharp`) before `pnpm vitest`/`pnpm exec tsc` would run non-interactively. Running `pnpm approve-builds --all` unblocked it but also produced a large, unrelated `pnpm-lock.yaml`/`pnpm-workspace.yaml` diff each time (~825 lockfile line deletions plus an `allowBuilds` block) that had nothing to do with this task. Resolved by invoking the already-built binaries directly (`./node_modules/.bin/vitest`, `./node_modules/.bin/tsc`, `./frontend/node_modules/.bin/vue-tsc`), which bypass pnpm's install-gate entirely, and reverting the lockfile/workspace diff (`git checkout --`) after every verification run so it never entered a commit. +- The working tree is shared with a concurrent agent (per the executor prompt's warning) working on plan 09-03 (unified prompt/docs). Their commits (`a080956`, `2dd9947`) appeared directly in local `git log` mid-session since it's the same checkout, not a separate clone — no merge conflicts occurred since their files (`DocsPage.vue`, `JoinBoutPage.vue`, `BotProfilePage.vue`, `docs.ts`, `docs.test.ts`, `e2e/signup-bot.spec.ts`) never overlapped this plan's auth files. Push succeeded as a fast-forward at every step. + +## User Setup Required + +None — no external service configuration required. `JWT_SECRET` provisioning for production (the crash-loop risk documented in `09-RESEARCH.md` Pitfall 1) is BOT-04's manifest work, not this plan's. + +## Next Phase Readiness + +- BOT-01's server/client auth surface is complete and tested; `POST /api/auth/nostr/session` (signer login) and `GET /api/auth/me` (session restore) are the only two ways to obtain or restore a session; `POST /api/auth/login` is inert w.r.t. writes. +- **Not yet done, and explicitly out of this plan's scope:** real-browser NIP-07 extension login and real Amber NIP-55 login — deferred to plan 09-07's human checkpoint per `09-RESEARCH.md` Pitfall 5 (no automated harness exists for a real `window.nostr` provider). This plan's own `` section states this is deliberately not claimed here. +- Ready for 09-03 (unified prompt/docs) and 09-04 (arena deploy/manifest, including the `JWT_SECRET` `generated_secrets` fix) to proceed independently — no auth-surface blockers introduced. + +## Self-Check: PASSED + +All 6 key files confirmed present on disk; all 4 task/deviation commit hashes (`e824f4c`, `635ee39`, `bf240ce`, `2a343ac`) confirmed present in `git log --oneline --all` and pushed to `origin main`. + +--- +*Phase: 09-botfights-platform-upgrade* +*Completed: 2026-07-31*