docs(09-02): complete BOT-01 bare-pubkey auth gap closure plan
GET /api/auth/me (JWT-only session restore) shipped, POST /login reduced to a read-only deprecated lookup, client auto-restore moved off the bare pubkey path. Includes a fixed pre-existing migrate.ts/schema.ts drift (15 pre-existing auth/tournament test failures resolved as a deviation). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
archipelago
parent
1fc006a05b
commit
903481afb3
189
.planning/phases/09-botfights-platform-upgrade/09-02-SUMMARY.md
Normal file
189
.planning/phases/09-botfights-platform-upgrade/09-02-SUMMARY.md
Normal file
@ -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 <verification> 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 <jwt>` 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 `<verification>` 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*
|
||||
Loading…
x
Reference in New Issue
Block a user