`admin/SELF-CRITIQUE.md`

34 lines
# sf-admin — Self-Critique
Date: 2026-04-30

## Security gaps

| # | Severity | Gap |
|---|----------|-----|
| S1 | high | **No CSRF protection on PATCH/POST.** Any authenticated browser session can be tricked by a third-party page into POSTing to `/api/sites/:domain/palette` or triggering `/api/actions`. `sameSite=lax` cookies blunt this for top-level navigations only — fetch from another origin still rides the cookie. Mitigation: csurf middleware + double-submit token on every state-changing call. |
| S2 | high | **`SF_ADMIN_DEV=1` is shipped enabled in `ecosystem.config.js`.** Anyone reaching :9883 (incl. via SSH tunnel, LAN, or accidental Cloudflare exposure) is auto-admin. Mitigation: bind to `127.0.0.1` only (current default for local PG/Express, but not enforced here — Express binds 0.0.0.0); flip the ecosystem default to `0` once Google OAuth creds exist. |
| S3 | high | **`SESSION_SECRET` defaults to a hard-coded literal.** A copy of the source = ability to forge sessions. Mitigation: refuse to boot in `NODE_ENV=production` unless `SESSION_SECRET` is set and ≥ 32 chars. |
| S4 | medium | **No rate limiting on `/auth/google/callback` or PATCH endpoints.** Brute-force/CSRF amplification is unbounded. Mitigation: express-rate-limit, 60 req/min per IP for write paths. |
| S5 | medium | **`MemoryStore` for sessions** (express-session default) — restarts log everyone out, and there's no clustering safety. Mitigation: `connect-pg-simple` against the existing `site_factory` Postgres. |
| S6 | medium | **Iframe loads `http://localhost:<port>` cross-port** — same-origin policy still blocks DOM access from the parent (good) but the iframe is fully unsandboxed. A compromised dev server could framebust or pop dialogs. Mitigation: `<iframe sandbox="allow-scripts allow-same-origin">` once we know dev servers tolerate it. |
| S7 | low | **Orchestrator at :9880 is unauthenticated.** sf-admin proxies, but anyone on the host can curl `http://127.0.0.1:9880/sites/.../palette` directly and bypass auth entirely. Mitigation: shared-secret bearer token between sf-admin and sf-orchestrator. |
| S8 | low | No `helmet()` — missing X-Content-Type-Options, X-Frame-Options, basic CSP. |

## UX gaps

| # | Severity | Gap |
|---|----------|-----|
| U1 | high | **Iframe assumes the local Next.js dev server is running.** If `localhost:3001` is down, user sees a blank/error frame with no clear explanation. Should detect failed load and show a "Start `wholivedthere` dev server" hint with the exact `pnpm dev --port 3001` command. |
| U2 | high | **Theme/copy edits are saved to `theme_overrides`/`copy_overrides`, but nothing pushes them to the running site yet.** The iframe won't reflect changes — the user will think the editor is broken. Need either (a) a "Publish" step that writes to each site's `theme.json`, or (b) the Next.js apps must read from the orchestrator at runtime. |
| U3 | medium | **No "reset to palette default" button.** Once a color is overridden it sticks; only way back is to hand-type the original hex. |
| U4 | medium | **Skills tab gives no feedback beyond a toast** — no spinner, no "running"/"finished"/"failed" states, no link to logs. The "last triggered" timestamp is wiped on page reload because it's not persisted; should fetch last action per skill from `actions_log`. |
| U5 | medium | **Findings modal has no resolve action.** The whole point of the badge is to drive remediation; you can read but not check off. |
| U6 | medium | **Color picker has no contrast warning.** Two of the five pairs (ink-on-surface, primary-on-surface) need WCAG AA — should compute and display the ratio inline. |
| U7 | low | **No keyboard shortcuts** (Cmd-S to save copy, j/k to move site list). |
| U8 | low | **Mobile/narrow-window unusable** — three fixed columns cram below ~900px width. |
| U9 | low | **No unsaved-changes indicator** on the Copy textareas — easy to navigate away and lose work. |

## Verdict

The skeleton works end-to-end (auth → list → edit → PATCH → broadcast), but for any non-dev exposure it needs items **S1, S2, S3, U1, U2** fixed before being safe and useful. Recommend disabling `SF_ADMIN_DEV` and adding csurf + a publish step before the next stage.