diff --git a/docs/superpowers/plans/2026-06-09-visual-companion-issues.md b/docs/superpowers/plans/2026-06-09-visual-companion-issues.md index 82802336..f3ca90b9 100644 --- a/docs/superpowers/plans/2026-06-09-visual-companion-issues.md +++ b/docs/superpowers/plans/2026-06-09-visual-companion-issues.md @@ -21,7 +21,7 @@ grounded against the current code, not the PR author's description. - **Remote serving is a first-class scenario.** Superpowers is general-purpose; users connect from remote (SSH tunnel, Tailscale, `--host 0.0.0.0`). The security fix MUST protect those users, not just loopback. **Decision: a - per-session secret key**, not a Host/Origin allowlist. The allowlist only + per-session secret key**, not a Host allowlist. A Host allowlist only defends the loopback browser-confused-deputy; a direct remote client just sends the expected `Host`, so the allowlist is theater for remote exposure. A secret key is the only thing that authenticates a client uniformly across @@ -43,8 +43,8 @@ grounded against the current code, not the PR author's description. | ID | Item | Source | Disposition | |----|------|--------|-------------| -| A1 | Per-session secret key on `/`, `/files/*`, and WS (supersedes Host/Origin) | issues #1014, PRs #1110/#1553 | **Do** — chosen approach | -| A2 | ~~Host/Origin allowlist~~ | PRs #1110/#1553 | Dropped — subsumed by A1 | +| A1 | Per-session secret key on `/`, `/files/*`, and WS (supersedes Host allowlist) | issues #1014, PRs #1110/#1553 | **Do** — chosen approach | +| A2 | Host allowlist; browser WS Origin check | PRs #1110/#1553 | Host allowlist dropped; WS Origin check retained after auth for browser confused-deputy defense | | A3 | Crash on `null` / non-object WS payload | PR #1504 | Do | | A4 | Frame-length bound in `decodeFrame` | issue #1446 | Already fixed — verify/close | | B1 | Dotfile screens served as content (`._*.html`) | PR #950 | Do | @@ -52,10 +52,10 @@ grounded against the current code, not the PR author's description. | B3 | WS client reconnect backoff + status indicator | PR #856 | Do | | C1 | Idle timeout too short / not configurable; WS not closed on shutdown | issue #1237 (PR #1689) | Do | | C2 | Server death is invisible to user/agent | issue #1237 (residual) | Do | -| D1 | Permanent opt-out of the companion | issue #892 | Do — design choice open | -| D2 | Free-text feedback from the browser | issue #957 | Do — needs server change too | -| D3 | Auto-open the companion URL | PR #759 (#755) | Do | -| D4 | Light/dark contrast helpers in the frame | PR #1683 | Do | +| D1 | Permanent opt-out of the companion | issue #892 | Deferred - not in PR #1720 | +| D2 | Free-text feedback from the browser | issue #957 | Deferred - not in PR #1720 | +| D3 | Auto-open the companion URL | PR #759 (#755) | Done in PR #1720 via `--open` | +| D4 | Light/dark contrast helpers in the frame | PR #1683 | Deferred - not in PR #1720 | | E1 | Hard-gate terminal-vs-HTML per question | PR #1037 | **Workshop** | | E2 | Move session state out of the working tree | issue #975 (PR #977) | **Deferred** | | E3 | Vendor Alpine.js for interactive mockups | PR #1639 | **Dropped** | @@ -78,13 +78,13 @@ route to the port, directly, with no same-origin policy in the way. Today `handleUpgrade` (`server.cjs:176`) checks only `Sec-WebSocket-Key`, and `handleRequest` (`server.cjs:138`) checks nothing — both are wide open. -**Why a key, not an allowlist.** A Host/Origin allowlist only defends the +**Why a key, not a Host allowlist.** A Host allowlist only defends the loopback browser-deputy. A direct remote client just sends the expected `Host` and forges/omits `Origin`, so the allowlist is theater for exactly the remote case we must protect. A per-session secret authenticates the client uniformly across loopback, SSH tunnel, and direct-remote, and it also kills DNS rebinding (the rebound page neither knows the key nor receives the host-scoped cookie). -So the key **supersedes** A1/A2's allowlist entirely — no `BRAINSTORM_ALLOWED_HOSTS`. +So the key **supersedes** A1/A2's Host allowlist entirely — no `BRAINSTORM_ALLOWED_HOSTS`. **Design.** Random token (`crypto.randomBytes(32)` hex), generated in `server.cjs` at startup (overridable via `BRAINSTORM_TOKEN` for deterministic @@ -117,12 +117,14 @@ bears initial-auth load. fallback). `start-server.sh` none. `visual-companion.md` doc note (URL now has `?key=`; don't strip it). Tests updated to pass the token. -### A2 — Host/Origin allowlist — DROPPED +### A2 — Host allowlist dropped; browser WS Origin retained Subsumed by A1. The secret key closes the WS-injection vector (#1014), the HTTP/WS DNS-rebinding read vector (PR #1553), and the cross-origin WS vector (PR #1110) in one mechanism, and unlike an allowlist it actually protects the -remote-bind case. No `Host`/`Origin` validation, no `BRAINSTORM_ALLOWED_HOSTS`. +remote-bind case. No `BRAINSTORM_ALLOWED_HOSTS` and no Host allowlist. The final +implementation still checks browser WebSocket `Origin` after session auth so a +cross-origin localhost tab cannot ride the companion cookie. ### A3 — Server crashes on `null` / primitive WS payload @@ -208,7 +210,7 @@ server dies mid-session. Separately, `shutdown()` (`server.cjs:310-321`) calls alive past shutdown. **Change.** -- Raise the default (≈2h) and make it configurable: +- Raise the default to 4 hours and make it configurable: `--idle-timeout-minutes` in `start-server.sh` → an env var → `IDLE_TIMEOUT_MS`, with validation against Node timer overflow. - Expose the effective timeout in the startup JSON / `state/server-info`. @@ -219,7 +221,7 @@ alive past shutdown. **Problem.** When the server exits it writes `state/server-stopped` and removes `state/server-info` (`server.cjs:312-317`), and the skill is *told* to check -those files (`visual-companion.md:98`) — but it's soft guidance the model skips, +those files (`visual-companion.md:108`) — but it's soft guidance the model skips, and the browser just shows a generic "can't be reached." The user diagnoses it manually; the agent keeps referring to a dead URL. @@ -315,7 +317,7 @@ this is behavior-shaping skill content and not specced here. Today `--project-dir` writes session state to `/.superpowers/brainstorm/` (`start-server.sh:80-84`) and the skill tells the user to gitignore it -(`visual-companion.md:48`). The ask is a `--state-dir` / `SUPERPOWERS_STATE_DIR` +(`visual-companion.md:58`). The ask is a `--state-dir` / `SUPERPOWERS_STATE_DIR` default outside the repo (XDG), keeping `--project-dir` as an alias. **Deferred by Jesse for now.** Captured so it isn't lost. @@ -344,7 +346,7 @@ These cluster into a few coherent passes (each independently testable against 2. **Lifecycle pass** — C1 + C2 together (both touch `shutdown()` and the server-death story). 3. **Robustness pass** — B1, B2, B3 (independent, small). -4. **Feature pass** — D1, D2, D3, D4 (D2 needs the server persistence tweak; - D1's config mechanism is shared with D3's opt-out). +4. **Deferred feature pass** - D1, D2, D4 are not part of PR #1720. D3 is + shipped through the `--open` flow. E1 is a separate workshop session. E2/E3 are out of scope for this round. diff --git a/skills/brainstorming/visual-companion.md b/skills/brainstorming/visual-companion.md index 7072b728..e9502293 100644 --- a/skills/brainstorming/visual-companion.md +++ b/skills/brainstorming/visual-companion.md @@ -62,7 +62,7 @@ without repeating it. **Claude Code:** ```bash # Default mode works — the script backgrounds the server itself. -scripts/start-server.sh --project-dir /path/to/project +scripts/start-server.sh --project-dir /path/to/project --open ``` On Windows, the script auto-detects and switches to foreground mode (which blocks the tool call). Use `run_in_background: true` on the Bash tool call so the server survives across conversation turns, then read `$STATE_DIR/server-info` on the next turn to get the URL and port. @@ -71,14 +71,14 @@ On Windows, the script auto-detects and switches to foreground mode (which block ```bash # Codex reaps background processes. The script auto-detects CODEX_CI and # switches to foreground mode. Run it normally — no extra flags needed. -scripts/start-server.sh --project-dir /path/to/project +scripts/start-server.sh --project-dir /path/to/project --open ``` **Gemini CLI:** ```bash # Use --foreground and set is_background: true on your shell tool call # so the process survives across turns -scripts/start-server.sh --project-dir /path/to/project --foreground +scripts/start-server.sh --project-dir /path/to/project --open --foreground ``` **Copilot CLI:** @@ -86,7 +86,7 @@ scripts/start-server.sh --project-dir /path/to/project --foreground # Use --foreground and start the server via the bash tool with mode: "async" # so the process survives across turns. Capture the returned shellId for # read_bash / stop_bash if you need to interact with it later. -scripts/start-server.sh --project-dir /path/to/project --foreground +scripts/start-server.sh --project-dir /path/to/project --open --foreground ``` **Other environments:** The server must keep running in the background across conversation turns. If your environment reaps detached processes, use `--foreground` and launch the command with your platform's background execution mechanism.