Handoff: record P0/P1 fixes as done, move eval debt to ROADMAP

Refresh AGENTS.md conventions (client IP via req.ip; safeFilename exported), rewrite Current state to a lean snapshot, move the P2 known-debt detail to ROADMAP.md.
2026-06-15 13:48:20 -05:00
parent d0e98424c1
commit aca2ba9e2e
2 changed files with 21 additions and 32 deletions
@@ -79,7 +79,8 @@ vendor/keysat-licensing-client/   local-link Keysat SDK
 - **Sanitize operator-internal language at error boundaries.** Strings like "Spark Control", "parakeet", "vLLM", LAN IPs, `*.local` URLs come from the sibling relay and must not reach cloud users.
 - **Multi-mode credit gates fire BEFORE the pipeline.** See `/api/process` for the order — admin → license → free tenant → trial → anonymous-mint. Don't reorder without reading the comment block.
 - **Trial IP cap is per-IP for IPv4, per-/64 prefix for IPv6.** Dual-stack home networks would otherwise bypass it via privacy-extension address rotation.
- **`safeFilename()` already exists** for any user-content → on-disk path. Use it; don't roll your own.
+- **Client IP comes from `req.ip`, never a raw `X-Forwarded-For` entry.** Express `trust proxy` is set in `index.js` from `RECAP_TRUSTED_PROXY_HOPS` (default 1); `getClientIp` (`anon-trial.js`) returns `req.ip`. Trusting raw `XFF[0]` let clients spoof the trial-cap IP — don't reintroduce it.
 - **`safeFilename()` is exported from `history.js`** — import and use it for any user-content → on-disk path; don't roll your own. It validates against `/^[A-Za-z0-9_-]+$/` and throws on traversal/separators (the library-import file-write hole was a missing call).
 - **The relay owns cloud Pro/Max tier + expiry** (core-decoupling; `docs/core-decoupling-plan.md`). In multi-mode, paid status is `users.tier` — cached from the relay, keyed by the Recaps user-id — NOT a per-user Keysat license. Don't gate cloud paid features by `keysat_license`; the license only matters for self-hosted "take it home" portability. Cloud requests carry `X-Recap-User-Id` + the operator key; server-to-server tier reads/writes go through `providers/relay.js`.
 - **Self-serve purchase has two rails, both prepaid (no auto-renew yet).** Bitcoin = a BTCPay invoice rendered as an INLINE Lightning QR on-screen — the relay returns the BOLT11 server-to-server, so the buyer never loads BTCPay (replicate the buy-credits inline flow; do NOT redirect to a hosted checkout). Card = a Zaprite one-time hosted order (Zaprite's API has no recurring — see ROADMAP). Both settle webhooks land on the relay (it owns subscription expiry); the frontend just polls `/api/billing/status`. Expiry reminders go out via the existing System-SMTP transport (`smtp.js`): the relay enumerates who's expiring (`GET /relay/expiring-subscriptions`), Recaps maps user-id → email and sends.
 - **Tier credit allotments are operator-config-driven, never hardcoded.** The cards' "N relay credits each period" comes from the relay's tier-quota config (`credits_per_period` on `/relay/tier-plans`); `null` → "Unlimited". Don't bake a number or "Unlimited" into the UI.
@@ -130,36 +131,11 @@ unsure whether a change is contract-affecting, assume it is and check.
 - **Core-decoupling live** (relay owns cloud tier; `docs/core-decoupling-plan.md`) and **per-tenant subscriptions live** (`docs/per-tenant-subscriptions-plan.md`).
 - The Bitcoin pill matches the standard purple; the relay-side **internal-meeting re-polish fix** (re-attributes topic summaries to the operator's corrected speaker names) shipped this session.
-**Pending real-world tests (operator):**
+**This session (2026-06-15):** ran the 2026-06-14 full-eval (`EVALUATION.md`) and cleared its 3 P0 + 4 P1 findings. Five code fixes shipped with tests + a reviewer pass — library-import file-write, podcast SSRF, ESM `require`, multi-mode concurrency lock, and the `X-Forwarded-For` trial-cap bypass — committed + pushed (`d0e9842`), 119 tests pass. The leaked Gemini key was purged from all git history and force-pushed; **the rewrite re-hashed every commit** (solo private repo, no other clones — harmless). Registry-submission blockers deferred.
 1. First on-device Bitcoin purchase — sign in as a Core tenant → Upgrade → Pay with Bitcoin → pay the inline invoice → badge flips.
 2. Enable cards — run the relay's "Set Zaprite Connection" action (API key) + register the Zaprite webhook at `https://<relay-host>/relay/zaprite/webhook`.
 3. Eyeball a reminder email via the test trigger above.
-### Evaluation work queue (P0/P1) — from the 2026-06-14 full-eval (`EVALUATION.md`)
+**Pending operator actions:**
 1. (optional) Rotate the Gemini key in AI Studio — the purge removed it from the repo, but the key itself is still live. Then delete the pre-purge backup: `rm /Users/macpro/Projects/recap-keyleak-purge-backup.bundle` (it contains the old key).
 2. Real-world cloud tests: first on-device Bitcoin purchase (Core tenant → Upgrade → Pay with Bitcoin → badge flips); enable cards (relay "Set Zaprite Connection" + webhook `https://<relay-host>/relay/zaprite/webhook`); eyeball a reminder email (`POST /api/admin/reminders/run` `{test_email}`).
 3. If recaps.cc ever gains a CDN/LB hop in front of the app, set `RECAP_TRUSTED_PROXY_HOPS` accordingly or the trial-cap bypass reopens.
-Five of seven items FIXED + tested (2026-06-15, reviewer-checked); the leaked-key purge awaits operator confirmation and the registry blockers are deferred.
+**Next dev steps + open decisions** live in `ROADMAP.md`: the eval's **P2 known-debt** list (SSE error-string scrub, credit-debit TOCTOU, `GET /api/history` perf, dependency CVEs, integration tests for the untested hot paths, doc drift) and **P3** hardening/cleanup, plus the standing decisions (Zaprite recurring, "take Recaps home" broken for relay-tier users, cloud paid-only, no CI lint/type-check). Tests: `cd server && npm test` → **119 pass**.
 - **[P0] ✅ FIXED — arbitrary file write in `POST /api/library/import`.** `safeFilename()` is now exported from `history.js` and validates each import key (`server/library.js`); a `../../` key is skipped, not written outside the scope dir. Tests in `test/history.test.js`. *(Adjacent P2 still open: array-form import + `PUT /api/history/move` write unvalidated IDs into `_meta.json` — no file-path escape, and read-time `safeFilename` guards the load. See Known debt.)*
 - **[P0] ✅ FIXED — SSRF in podcast download.** `downloadPodcastAudio` (`server/audio.js`) rejects non-HTTP(S) schemes and blocks IP-literal AND DNS-resolved private/link-local/loopback/reserved/multicast/translation-prefix targets (closing the DNS-rebinding window), caps + resolves redirects. Tests in `test/audio.test.js`. *(Response size/time cap still deferred — ROADMAP P3.)*
 - **[P0] ⏳ Leaked Gemini key in git history** (`git show d5046a0:.env`). Operator to rotate in Google AI Studio (recommended, not strictly required since the repo was never shared); git-history purge via `git filter-repo --path .env --invert-paths` + force-push to Gitea is queued and runs on the operator's go-ahead.
 - **[P1] ✅ FIXED — ESM `require("crypto")`.** Replaced with a top-level `import { randomBytes }` in `server/license-purchase.js`.
 - **[P1] ✅ FIXED — global `currentFreeJob` lock serialized the whole cloud.** Now skipped in multi-mode (`server/index.js`: `const isFree = req.recapMode !== "multi" && isFreeUser()`); per-tenant credit metering is the control there.
 - **[P1] ✅ FIXED — `X-Forwarded-For` trial-cap bypass.** `app.set("trust proxy", …)` from `RECAP_TRUSTED_PROXY_HOPS` (default `1`) + `getClientIp` now returns `req.ip` (`server/index.js`, `server/anon-trial.js`). Tests in `test/anon-trial.test.js`. **Watch:** if the cloud ever gains a CDN/LB hop, bump `RECAP_TRUSTED_PROXY_HOPS` or the bypass reopens.
 - *(StartOS registry-submission blockers — deferred by decision 2026-06-15; moved to `ROADMAP.md`. They never affected `make install`.)*
 ### Known debt (P2) — track; not release-blocking for self-host
 - **Operator-internal strings leak to cloud users at the SSE error boundary** (Parakeet/Gemma/CUDA/LAN IPs) — no scrub exists, violating the scrub contract above. `server/index.js:3432,3003,4246` + `providers/relay.js:135-144`. (Sharp edge: `index.js:3419` *detects* these strings, then forwards them anyway.)
 - **Credit over-spend TOCTOU on licensed installs** — N parallel requests pass the `total>0` check before any blind `debitOne` lands. Make check+debit atomic (reserve up front, refund on failure). `index.js:2497-2550` vs `:3158,:4197`.
 - **Multi-mode tenant can spend the operator's server Gemini key** via `transcriptionProvider:"gemini"` + empty key (bypasses relay metering) — `providers/index.js:104-114`. Refuse the operator-key fallback for non-admin tenants.
 - **`GET /api/history` parses every full session file** (transcript+summary, MB each) just to list ~8 metadata fields — cache them into `_meta.json` on save. `history.js:418-437`.
 - **Dependency CVEs** — nodemailer 6.10.1 (high; low practical reach here), ws/qs/express/protobufjs (moderate). `npm audit fix` (nodemailer is a major bump).
 - **No tests on the riskiest files** (`/api/process` gating, `relay.js`, `tenant-auth.js`, billing) — every code P0/P1 above lives here, and no agent could run the real summarize→save→debit path end-to-end (no key/credits). Add an integration test as the regression net before/with the fixes.
 - **Smaller hardening:** unsanitized IDs persisted to `_meta.json` (array-form import + `history/move`); `PUT /api/history/meta` accepts arbitrary JSON shapes with no schema; `index.js` is 4351 lines mixing routing/pipeline/yt-dlp/SSE.
 - **Doc drift (high-value):** AGENTS.md credit-gate order omits the "paid cloud user" bypass state (`:77` vs `index.js:2464-2472`); operator-facing `startos-registry/.../INSTRUCTIONS.md` + `assets/ABOUT.md` are stale Gemini-first (relay is the default provider). Lower-severity doc nits are deferred in `ROADMAP.md`.
 **Known issues / open decisions** (details + next actions in `ROADMAP.md`):
 - **Zaprite recurring isn't built** — Zaprite's API only does one-time orders. Card = prepaid until that's confirmed with Zaprite.
 - **"Take Recaps home" is likely broken** for relay-tier users (no `keysat_license` after decoupling).
 - **Cloud still has a free signed-in tier** (the simplification plan's "cloud paid-only" is unbuilt).
 - No CI lint / type-check (unchanged).
@@ -12,6 +12,19 @@ Longer-term backlog for Recaps. Near-term in-flight work and known issues live i
 - **Close the architecture-simplification gaps** (`docs/architecture-simplification-plan.md`). After core-decoupling + self-serve, these steps remain OPEN: **(8) "Take Recaps home"** — mint a fresh Keysat token on demand at click time; likely BROKEN today because relay-tier cloud users have no `keysat_license` for `/api/account/license-key` to return. **(10) cloud paid-only** — the free signed-in tier + signup-grant credits are still live; the plan wanted cloud to be paid-only with self-hosted as the free path (product call — confirm intent before building). **(5, partial) anon signup→Pro** still routes through `/api/license/purchase` + `pending_signups` (Keysat license) instead of the relay tier like the signed-in flow does. **(6, partial) tokenized renew** — the reminder email's renew link is `?renew=1` (requires sign-in); the plan wanted a one-time-token `/renew?token=…` for friction-free renewal. NOTE: the doc's Zaprite-*recurring* / cancel-button / Recaps-DB-owns-expiry parts were intentionally SUPERSEDED by the prepaid + relay-owns-tier model — don't build those.
 - **Decide the Max tier-quota default.** The relay code default is `max.monthly: null` (unlimited) → cards render "Unlimited" on a fresh install. The operator set `max.monthly: 120` on their box via the Adjust-Tier-Quotas action (so cards show 120 there). Decide whether a metered number (e.g. 120) should be the shipped default in `recap-relay/server/config.js` — note it also enforces the ceiling, not just the card label.
 ## Known debt (P2, from the 2026-06-14 full-eval — `EVALUATION.md`)
 Real but not release-blocking for self-host. The P0/P1 findings from the same eval were fixed 2026-06-15 (see git log + `EVALUATION.md`).
 - **Operator-internal strings leak to cloud users at the SSE error boundary** (Parakeet/Gemma/CUDA/LAN IPs) — no scrub exists, violating the scrub convention in `AGENTS.md`. `server/index.js:3432,3003,4246` + `providers/relay.js:135-144`. (Sharp edge: `index.js:3419` *detects* these strings, then forwards them anyway.)
 - **Credit over-spend TOCTOU on licensed installs** — N parallel requests pass the `total>0` check before any blind `debitOne` lands. Make check+debit atomic (reserve up front, refund on failure). `index.js:2497-2550` vs `:3158,:4197`.
 - **Multi-mode tenant can spend the operator's server Gemini key** via `transcriptionProvider:"gemini"` + empty key (bypasses relay metering) — `providers/index.js:104-114`. Refuse the operator-key fallback for non-admin tenants.
 - **`GET /api/history` parses every full session file** (transcript+summary, MB each) just to list ~8 metadata fields — cache them into `_meta.json` on save. `history.js:418-437`.
 - **Dependency CVEs** — nodemailer 6.10.1 (high; low practical reach here), ws/qs/express/protobufjs (moderate). `npm audit fix` (nodemailer is a major bump).
 - **No tests on the riskiest files** (`/api/process` gating, `relay.js`, `tenant-auth.js`, billing) — the real summarize→save→debit path can't run end-to-end without a key/credits. Add an integration test as the regression net.
 - **Smaller hardening:** unsanitized IDs persisted to `_meta.json` (array-form library import + `PUT /api/history/move`) — no file-path escape (read-time `safeFilename` guards the load), but sanitize at write too; `PUT /api/history/meta` accepts arbitrary JSON shapes with no schema; `index.js` is 4351 lines mixing routing/pipeline/yt-dlp/SSE.
 - **Doc drift (high-value):** AGENTS.md credit-gate order omits the "paid cloud user" bypass state (`:77` vs `index.js:2464-2472`); operator-facing `startos-registry/.../INSTRUCTIONS.md` + `assets/ABOUT.md` are stale Gemini-first (relay is the default provider).
 ## Deferred hardening & cleanup (P3, from the 2026-06-14 full-eval — `EVALUATION.md`)
 Low-severity; batch when convenient. None block release. (P0/P1 work queue and P2 known debt live in `AGENTS.md` → Current state.)