diff --git a/CLAUDE.md b/CLAUDE.md index aea64149..a347d65b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -280,7 +280,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Cross-session search**: `GET /api/search` federates an in-memory search over session metadata, run-summary events, and attachment-history entries. The pure core `searchSources()` does substring matching with hard per-type caps: **no regex (so no ReDoS) and no filesystem reads (so no traversal)**. The server-private `externalPath` is never read. PAST sessions (#261) come from `session-history-index.ts`, a capped snapshot of the unified list filled **outside** the request path (`/api/sessions/unified` publishes it; a stale one is rebuilt fire-and-forget), that indirection is what keeps the no-fs property. ⚠️ The snapshot is stored UNSCOPED with a per-row owner and MUST be re-filtered through `canAccessOwned()` on read; history rows carry `jumpTo.kind:'resume-session'`, since a closed session has no tab to select. → [architecture-invariants#cross-session-search](docs/architecture-invariants.md#cross-session-search) -**Web tabs** (dashboard URLs as tabs): a saved URL renders as a tab beside agent sessions. **NOT a `SessionMode` of its own** (no PTY, no tmux, no respawn), same reasoning that keeps Docker/remote-SSH as case overlays. Dashboards are **proxied through Codeman's own origin** by default, because a direct iframe fails three ways at once: prod is HTTPS so `http://` targets are blocked as mixed content, many dashboards send `X-Frame-Options: DENY`, and our own `default-src 'self'` CSP blocks cross-origin frames. Proxying leaves the prod CSP unchanged (`/webview/...` is `'self'`). ⚠️ The proxy is **NOT an API surface**: it authenticates on an in-memory capability in the path and is correspondingly exempt from the cookie + Origin checks; that exemption is fenced to safe methods and non-`/api` paths and is pinned by `test/webview-auth-exemption.test.ts`. ⚠️ Iframes omit `allow-same-origin` unless a dashboard is explicitly marked `trusted`, and `Authorization`/`codeman_session` are stripped upstream in **both** modes so `CODEMAN_PASSWORD` cannot leak. ⚠️ A sandboxed frame is **opaque-origin**, which breaks two things `curl` can never reproduce: its runtime-built root-absolute URLs escape `` (fixed by an injected `runtimeUrlShim()`), and its same-host `fetch`/XHR are CORS-checked with `Origin: null` (fixed by `buildProxyCorsHeaders()` plus exempting the proxy from the global `OPTIONS`-204 short-circuit in `registerSecurityHeaders`). Both present as the dashboard's own "Failed to fetch" while the page renders fine. ⚠️ **Egress guard**: link-local and cloud-metadata targets (`169.254.0.0/16`, `fe80::/10`, `fd00:ec2::254`, the Azure/Alibaba fixed addresses, `metadata.google.internal`) are refused at save time AND on the RESOLVED address at connect time (`webview-egress-policy.ts`, pure, plus `webview-egress.ts`: a `lookup` hook on the undici Agent behind `webviewFetch()` and on the `ws` client). An IP literal never reaches a lookup hook (`net.connect` skips DNS for it), so the synchronous hostname check at each connect site is NOT redundant. Loopback and RFC1918 stay allowed on purpose: a `localhost` Grafana is the feature. The proxy uses the `undici` PACKAGE's own `fetch` + `Agent`, never Node's global fetch with a foreign dispatcher (Node bundles its own copy; a protocol mismatch fails silently). Capabilities are revoked on logout / admin logout / user deletion (`revokeOwner`, which had NO caller for two releases while its docstring said otherwise), and proxied responses carry `Referrer-Policy: same-origin` so a dashboard cannot hand the capability-bearing URL to a third party. → [architecture-invariants#web-tabs](docs/architecture-invariants.md#web-tabs), `docs/web-tabs.md` +**Web tabs** (dashboard URLs as tabs): a saved URL renders as a tab beside agent sessions. **NOT a `SessionMode` of its own** (no PTY, no tmux, no respawn), same reasoning that keeps Docker/remote-SSH as case overlays. Dashboards are **proxied through Codeman's own origin** by default, because a direct iframe fails three ways at once: prod is HTTPS so `http://` targets are blocked as mixed content, many dashboards send `X-Frame-Options: DENY`, and our own `default-src 'self'` CSP blocks cross-origin frames. Proxying leaves the prod CSP unchanged (`/webview/...` is `'self'`). ⚠️ The proxy is **NOT an API surface**: it authenticates on an in-memory capability in the path and is correspondingly exempt from the cookie + Origin checks; that exemption is fenced to safe methods and non-`/api` paths and is pinned by `test/webview-auth-exemption.test.ts`. ⚠️ Iframes omit `allow-same-origin` unless a dashboard is explicitly marked `trusted`, and `Authorization`/`codeman_session` are stripped upstream in **both** modes so `CODEMAN_PASSWORD` cannot leak. ⚠️ A sandboxed frame is **opaque-origin**, which breaks two things `curl` can never reproduce: its runtime-built root-absolute URLs escape `` (fixed by an injected `runtimeUrlShim()`), and its same-host `fetch`/XHR are CORS-checked with `Origin: null` (fixed by `buildProxyCorsHeaders()` plus exempting the proxy from the global `OPTIONS`-204 short-circuit in `registerSecurityHeaders`). Both present as the dashboard's own "Failed to fetch" while the page renders fine. ⚠️ **Egress guard**: link-local and cloud-metadata targets (`169.254.0.0/16`, `fe80::/10`, `fd00:ec2::254`, the Azure/Alibaba fixed addresses, `metadata.google.internal`) are refused at save time AND on the RESOLVED address at connect time (`webview-egress-policy.ts`, pure, plus `webview-egress.ts`: a `lookup` hook on the undici Agent behind `webviewFetch()` and on the `ws` client). An IP literal never reaches a lookup hook (`net.connect` skips DNS for it), so the synchronous hostname check at each connect site is NOT redundant. Loopback and RFC1918 stay allowed on purpose: a `localhost` Grafana is the feature. The proxy uses the `undici` PACKAGE's own `fetch` + `Agent`, never Node's global fetch with a foreign dispatcher (Node bundles its own copy; a protocol mismatch fails silently). ⚠️ **A loopback link in agent output opens as a proxied web tab** (`openLinkThroughWebTabIfLoopback` in webview-tabs.js, called from the terminal link provider and the response viewer): a phone cannot reach the box's `localhost:5173`, so the tap routes through Codeman's origin instead, reusing a saved same-origin dashboard or saving one under `host:port`. Two rules that look cosmetic and are not. **`*.localhost` is deliberately NOT in the auto-route set** even though it IS loopback to a browser: the link source is agent-written terminal output and prompt-injectable, every other member of that set is an address literal that can only mean this box, and a `*.localhost` DNS name is not one (a resolver with a search domain retries `evil.localhost` as `evil.localhost.`, which an attacker can control, turning agent output plus one tap into a persisted server-side fetch of an agent-chosen origin). A user who really runs `api.localhost` saves it by hand, which is an explicit action. The PAGE-side test is deliberately broader (`isOnBoxHostname`), since a false positive there only declines to proxy. And **`this.webviews` being set does NOT mean it is loaded**: `initWebviews()` assigns a truthy EMPTY map synchronously and only then awaits the list, so a tap during page load must join the in-flight refresh (`_webviewsRefresh`/`_webviewsLoaded`) or it finds nothing to reuse and POSTs a duplicate record for an origin that already exists. Capabilities are revoked on logout / admin logout / user deletion (`revokeOwner`, which had NO caller for two releases while its docstring said otherwise), and proxied responses carry `Referrer-Policy: same-origin` so a dashboard cannot hand the capability-bearing URL to a third party. → [architecture-invariants#web-tabs](docs/architecture-invariants.md#web-tabs), `docs/web-tabs.md` **Multi-user mode** (opt-in `--multiuser` / `CODEMAN_MULTIUSER=1`, OFF by default): named users with scrypt-hashed passwords in `~/.codeman/users.json`. Gated everywhere by `isMultiUserMode()`; when OFF, behavior is byte-identical to single-user because every scoping helper short-circuits. ⚠️ **Not a security boundary at the agent layer**: every session still runs as the SAME OS account. This separates WORKSPACES; it does not sandbox users (Docker cases are the isolation story). Ownership threads through `Session.owner` and is enforced in `findSessionOrFail`, list endpoints, SSE routing (fail-closed), WS, search, and file-preview. → [architecture-invariants#multi-user-mode](docs/architecture-invariants.md#multi-user-mode), `docs/multi-user-plan.md` diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index ba84342d..238ccb0a 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -52,7 +52,7 @@ Model is NOT a session field: it is a composition entry in the profile's config ### Remote SSH cases -**Remote SSH cases** (COD-94/#145): cases can point at a **remote host** (`~/.codeman/remote-hosts.json` + `remote-cases.json` via `src/remote-hosts.ts`; CRUD under `/api/cases` — cases route file). A remote session launches a LOCAL tmux pane running `ssh ` that creates a durable REMOTE tmux session on a **dedicated socket** `-L codeman-remote` with name `codeman-ssh-` — deliberately failing the remote Codeman's `SAFE_MUX_NAME_PATTERN` so a Codeman instance on the target host never adopts it; no `-g` global tmux options are set remotely. `remotePath`/`identityFile` are schema-guarded against shell injection (backticks/`$` rejected — same approach as `extraSshOptions`); remote tmux availability is probed via `checkRemoteTmuxAvailable()` in quick-start (ssh args carry `-o ConnectTimeout=10`). Remote claude defaults to `exec claude --dangerously-skip-permissions`; per-host `commands.*` override. Session kill best-effort kills the remote tmux too. `SessionState.remote`/`MuxSession.remote` round-trip through recovery (`restoreMuxSessions` passes `remote` back into the Session constructor). ⚠️ Run flows must route remote cases through `POST /api/quick-start` (which resolves the remote case and skips LOCAL CLI availability gates) — `POST /api/sessions` stat-validates `workingDir` locally and has no `caseName`. `envOverrides`/`effort`/`modelOverride`/`codexConfig`/`geminiConfig` are rejected for remote quick-starts (not silently dropped). UI: Create Case modal → Remote tab. Tests: `test/remote-hosts.test.ts`, `test/remote-ssh-options.test.ts`. +**Remote SSH cases** (COD-94/#145): cases can point at a **remote host** (`~/.codeman/remote-hosts.json` + `remote-cases.json` via `src/remote-hosts.ts`; CRUD under `/api/cases` — cases route file). A remote session launches a LOCAL tmux pane running `ssh ` that creates a durable REMOTE tmux session on a **dedicated socket** `-L codeman-remote` with name `codeman-ssh-` — deliberately failing the remote Codeman's `SAFE_MUX_NAME_PATTERN` so a Codeman instance on the target host never adopts it; no `-g` global tmux options are set remotely. `remotePath`/`identityFile` are schema-guarded against shell injection (backticks/`$` rejected — same approach as `extraSshOptions`); remote tmux availability is probed via `checkRemoteTmuxAvailable()` in quick-start (ssh args carry `-o ConnectTimeout=10`). Remote claude defaults to an idempotent `claude --session-id || claude --resume ` pair under a login shell, so a respawn or reattach continues the SAME conversation rather than starting a fresh one (remote omp gets the same treatment via `--continue`; ⚠️ because the claude arm is an `a || b` pair under `-c`, that pane's PID is the login shell, not the agent); per-host `commands.*` override. Session kill best-effort kills the remote tmux too. `SessionState.remote`/`MuxSession.remote` round-trip through recovery (`restoreMuxSessions` passes `remote` back into the Session constructor). ⚠️ Run flows must route remote cases through `POST /api/quick-start` (which resolves the remote case and skips LOCAL CLI availability gates) — `POST /api/sessions` stat-validates `workingDir` locally and has no `caseName`. `envOverrides`/`effort`/`modelOverride`/`codexConfig`/`geminiConfig` are rejected for remote quick-starts (not silently dropped). UI: Create Case modal → Remote tab. Tests: `test/remote-hosts.test.ts`, `test/remote-ssh-options.test.ts`. ### Docker cases diff --git a/docs/omp-integration.md b/docs/omp-integration.md index d4e59dc9..5cf5cf4c 100644 --- a/docs/omp-integration.md +++ b/docs/omp-integration.md @@ -153,6 +153,14 @@ shared nor seeded. include `~/.local/bin`. Per-session config and `envOverrides` do not cross ssh and are rejected rather than silently ignored; use the per-host command override instead. +⚠️ A **respawn or reattach** of a remote omp session runs `omp --continue`, not a +bare `omp`, so it lands back in the same conversation. It is deliberately +`--continue` rather than the exact `--resume ` the local and docker paths +pin: `omp-session-resolver.ts` only ever reads THIS host's `~/.omp/agent/sessions/`, +and a remote conversation's session file lives on the remote host under the +remote user's home, so resolving locally would pin a stranger's id. See +[Respawn / reattach continuation](remote-sessions.md#respawn--reattach-continuation). + ## Known gaps - **No idle/completion hook.** Idle detection falls back to output-stabilization diff --git a/docs/remote-sessions.md b/docs/remote-sessions.md index 32b3b36b..e9714cae 100644 --- a/docs/remote-sessions.md +++ b/docs/remote-sessions.md @@ -24,14 +24,14 @@ custom port, identity file, `-J` jump host, `-o ProxyCommand`). Types live in `src/types/session.ts`; persistence in `src/remote-hosts.ts`. -| Type | Role | -| -------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `RemoteSshOptions` | The **HOW-to-reach** fields, shared by host + session: `identityFile`, `socksProxy` (`host:port`), `jumpHost` (`[user@]host[:port]`), `extraSshOptions` (`KEY=VALUE[]`). Every field optional — all-absent reproduces port-22, default-identity, directly-SSH-able behavior. | -| `RemoteHost` (extends `RemoteSshOptions`) | A saved host: `id`, `label`, `host`, `username`, `port?`, `commands?` (per-mode launch command override). | -| `RemoteCase` | A working directory on a host: `name`, `type: 'remote'`, `hostId`, `remotePath`. | +| Type | Role | +|------|------| +| `RemoteSshOptions` | The **HOW-to-reach** fields, shared by host + session: `identityFile`, `socksProxy` (`host:port`), `jumpHost` (`[user@]host[:port]`), `extraSshOptions` (`KEY=VALUE[]`). Every field optional — all-absent reproduces port-22, default-identity, directly-SSH-able behavior. | +| `RemoteHost` (extends `RemoteSshOptions`) | A saved host: `id`, `label`, `host`, `username`, `port?`, `commands?` (per-mode launch command override). | +| `RemoteCase` | A working directory on a host: `name`, `type: 'remote'`, `hostId`, `remotePath`. | | `SessionRemote` (extends `RemoteSshOptions`) | The resolved bundle stamped onto a live session: host coordinates + `remotePath` + `commands`, plus **`owned?`** and **`remoteSessionName?`** (COD-105 — see [Ownership](#ownership-launched-vs-discovered-and-attached-cod-105)). Built by `toSessionRemote(host, case)` (sets `owned: true`) for the launch path, or `toAttachedSessionRemote(host, name, path)` (sets `owned: false`) for the attach path. Both copy the advanced SSH options through so every connection is identical. | -| `RemoteCommandMode` | `Extract` — the modes that can run remotely. | -| `RemoteSessionInfo` (COD-105) | One discovered remote tmux session: `name` (always `codeman-*`), `attached` (a client is connected), `created` (epoch s), `windows`. Returned by `listRemoteCodemanSessions()`. | +| `RemoteCommandMode` | `Extract` — the modes that can run remotely. | +| `RemoteSessionInfo` (COD-105) | One discovered remote tmux session: `name` (always `codeman-*`), `attached` (a client is connected), `created` (epoch s), `windows`. Returned by `listRemoteCodemanSessions()`. | Persistence is two flat JSON arrays in the instance data dir: @@ -73,7 +73,7 @@ Rules that keep this safe — **do not bypass them by hand-building an ssh line single-quote `shellescape`d (`'…'` with embedded `'\''`). The helper mirrors the one in `tmux-manager.ts`. - **`~`/`$HOME` in `identityFile` is expanded at build time** (`expandIdentityPath`), - _before_ escaping — ssh does not expand `~` inside `-i`, and the escaped value + *before* escaping — ssh does not expand `~` inside `-i`, and the escaped value never reaches a shell that would. - **The ProxyCommand is one shellescaped `-o KEY=VALUE` token**, so its spaces and the `%h`/`%p` placeholders reach ssh as a single argument. `%h %p` survive @@ -112,10 +112,15 @@ Key points: asymmetry: **discovery/attach (COD-105) target the canonical `-L codeman` socket** — they join sessions the remote's own Codeman manages, while owned durable launches live on `-L codeman-remote`. -- **`exec `** replaces the pane shell with the agent, so the pane PID _is_ +- **`exec `** replaces the pane shell with the agent, so the pane PID *is* the agent. The per-mode command comes from `remote.commands?.[mode]` or `defaultRemoteCommandForMode(mode)` (`exec claude` / `exec opencode` / `exec codex` / `exec gemini` / `exec agy` / `exec bash -l`). + ⚠️ **claude and omp no longer take that path**: both have their own arm in + `buildRemoteLaunchCommand` so a respawn can continue the same conversation + (see [Respawn / reattach continuation](#respawn--reattach-continuation)), and + because the claude arm is an `a || b` pair under `-c`, its pane PID is the + **login shell**, not the agent. - The **whole tmux invocation is a single shell-quoted ssh argument**, and the pane command is independently quoted, so a `remotePath` with spaces is safe. - Connection options come from the **same `buildSshConnectionArgs(remote)`** as @@ -128,9 +133,9 @@ Because durable remote sessions require tmux on the remote host, `checkRemoteTmuxAvailable(host)` runs `command -v tmux` over SSH **before** creating a remote case/session and returns a structured, never-throwing result: -- empty stdout / non-zero exit → _"remote host `` needs tmux installed for - durable remote sessions"_ -- stderr present → _"could not verify tmux on remote host ``: ``"_ +- empty stdout / non-zero exit → *"remote host `` needs tmux installed for + durable remote sessions"* +- stderr present → *"could not verify tmux on remote host ``: ``"* (a real connection failure, surfaced to the operator) - success → `{ ok: true, tmuxPath }` @@ -147,7 +152,7 @@ skipped; command construction is still asserted by unit tests. ## Ownership: launched vs. discovered-and-attached (COD-105) -COD-104 (above) was Phase 1 — Codeman _launches_ a remote session and owns it. +COD-104 (above) was Phase 1 — Codeman *launches* a remote session and owns it. COD-105 is Phase 2 — Codeman can also **discover** `codeman-*` tmux sessions already running on a remote host (created by the remote's own Codeman or another instance) and **attach** to one it didn't launch. Ownership decides what happens @@ -188,7 +193,7 @@ remote command line by ownership: - **`owned === false`** → `buildRemoteAttachCommand(remote, name)` — emits `ssh … -t … 'tmux -L codeman attach -t '`. It uses **`attach`, - NOT `new-session -A`**, so it only _joins_ an existing session and never creates + NOT `new-session -A`**, so it only *joins* an existing session and never creates one. - **owned (default)** → `buildRemoteLaunchCommand` (the COD-104 path above). @@ -196,7 +201,7 @@ remote command line by ownership: `TmuxManager.killSession()` has an **early return for non-owned remote sessions**: it tears down **only the LOCAL pane** holding the ssh client (`tmux -L codeman -kill-session` on _this_ host's socket). Killing the local ssh sends SIGHUP to the +kill-session` on *this* host's socket). Killing the local ssh sends SIGHUP to the remote `tmux attach`, which **detaches** — the durable remote session survives. The early return is a structural guarantee that **no code path can ever issue a remote `kill-session` for a session we don't own** — the only `kill-session` run is @@ -250,14 +255,14 @@ stale `true` from one transport drop can never revive the NEXT clean exit. Routes are registered in `src/web/routes/case-routes.ts`: -| Method | Path | Purpose | -| -------- | ------------------------------------ | ---------------------------------------------------------------------------------------------- | -| `GET` | `/api/remote-hosts` | List saved hosts | -| `POST` | `/api/remote-hosts` | Create a host | -| `PUT` | `/api/remote-hosts/:id` | Update a host | -| `DELETE` | `/api/remote-hosts/:id` | Delete a host | -| `GET` | `/api/remote-hosts/:hostId/sessions` | Discover `codeman-*` sessions on the host (COD-105; `listRemoteCodemanSessions`, never errors) | -| `POST` | `/api/cases/remote-link` | Link a case to a remote host (creates the `RemoteCase`) | +| Method | Path | Purpose | +|--------|------|---------| +| `GET` | `/api/remote-hosts` | List saved hosts | +| `POST` | `/api/remote-hosts` | Create a host | +| `PUT` | `/api/remote-hosts/:id` | Update a host | +| `DELETE` | `/api/remote-hosts/:id` | Delete a host | +| `GET` | `/api/remote-hosts/:hostId/sessions` | Discover `codeman-*` sessions on the host (COD-105; `listRemoteCodemanSessions`, never errors) | +| `POST` | `/api/cases/remote-link` | Link a case to a remote host (creates the `RemoteCase`) | Attaching to a discovered session is a **session-create** path, not a host route: `POST /api/sessions` accepts `attachRemoteSession: { hostId, remoteSessionName }` diff --git a/src/web/public/app.js b/src/web/public/app.js index b6c2ed01..643fa1a3 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -2398,7 +2398,12 @@ class CodemanApp { viewer.classList.add('visible'); backdrop.classList.add('visible'); - body.scrollTop = 0; + // A multi-row turn opens at its NEWEST text, matching loadFullContext's + // "scroll to bottom (latest message)". `scrollTop = 0` was right when the + // brief view was a single card holding the last row; with the whole turn + // rendered, the top is the turn's first narration line and the answer the + // eye button exists to show can be several screens down. + body.scrollTop = turnMessages.length > 1 ? body.scrollHeight : 0; } catch (err) { console.error('Failed to load response:', err); } diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 2ff05f1f..7d82004c 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -1400,8 +1400,21 @@ Object.assign(CodemanApp.prototype, { this.terminal.onData((data) => { // Canonical xterm data. Telling the controller is what lets it know a // keystroke was already delivered and needs no recovery. + // + // ⚠️ onData ALSO fires for output xterm produces on its own initiative: + // the DA/DSR/CPR/OSC replies it answers during Ink redraws, and the SGR + // mouse and focus reports (see the two predicates above, used for exactly + // this question at the send sites). Any one of those landing between the + // keydown and the candidate's zero-delay resolution would be read as + // "xterm spoke for this keystroke", standing the recovery down and + // leaving the character dropped, worst on a busy agent pane, which is + // the case this exists for. Narrowing the counter cannot cause a + // duplicate: it only ever makes the controller less sure it can stand down. try { - this._keyCode229Recovery?.notifyCanonicalData?.(); + const input = window.CodemanTerminalInput; + if (!input?.shouldSuppressTerminalQueryResponse(data) && !input?.isTerminalFocusOrMouseReport(data)) { + this._keyCode229Recovery?.notifyCanonicalData?.(); + } } catch { // Bookkeeping must never block real input. } diff --git a/src/web/public/webview-tabs.js b/src/web/public/webview-tabs.js index d17ae582..93800039 100644 --- a/src/web/public/webview-tabs.js +++ b/src/web/public/webview-tabs.js @@ -30,18 +30,53 @@ const LOOPBACK_HOSTNAMES = new Set(['localhost', '0.0.0.0', '::1', '[::1]', '::', '[::]']); -/** `localhost`, `*.localhost`, 127.0.0.0/8, 0.0.0.0 and the IPv6 loopback forms. */ -function isLoopbackHostname(hostname) { - const host = String(hostname || '') +function normalizeHostname(hostname) { + return String(hostname || '') .trim() .toLowerCase() .replace(/\.$/, ''); +} + +/** + * `localhost`, 127.0.0.0/8, 0.0.0.0 and the IPv6 loopback forms: names that can + * only ever mean this box. + * + * ⚠️ `*.localhost` is deliberately NOT here. The link source is agent-written + * terminal output and response-viewer markdown, i.e. prompt-injectable, and + * this set is the whole confinement on a tap that makes Codeman fetch a URL + * server-side and persist it. Every other member is an address literal; a + * `*.localhost` DNS name is not one: on a resolver that does not synthesise it + * locally and has a search domain configured, `evil.localhost` NXDOMAINs as + * absolute and is retried as `evil.localhost.`, which an + * attacker can control. A user who really runs `api.localhost` dev hosts can + * still save that dashboard by hand, which is an explicit action. + */ +function isLoopbackHostname(hostname) { + const host = normalizeHostname(hostname); if (!host) return false; - if (LOOPBACK_HOSTNAMES.has(host) || host.endsWith('.localhost')) return true; + if (LOOPBACK_HOSTNAMES.has(host)) return true; const ipv4 = /^(\d{1,3})\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.exec(host); return !!ipv4 && Number(ipv4[1]) === 127; } +/** + * Whether the PAGE is being viewed on the box itself. Broader than the + * auto-route set on purpose, and safe in the opposite direction: a false + * positive here only ever DECLINES to proxy, leaving the caller's direct open. + */ +function isOnBoxHostname(hostname) { + const host = normalizeHostname(hostname); + return isLoopbackHostname(host) || host.endsWith('.localhost'); +} + +/** + * One key per dev server, so `localhost:5173` and `127.0.0.1:5173` reuse a + * single saved dashboard and a single tab instead of one per host spelling. + */ +function webTabOriginKey(url) { + return isLoopbackHostname(url.hostname) ? `${url.protocol}//loopback:${url.port}` : url.origin; +} + /** * Whether a link should open through a proxied web tab rather than directly: * an http(s) URL on a loopback host, viewed from a page that is NOT itself on @@ -57,11 +92,11 @@ function linkNeedsWebTabProxy(rawUrl, pageHostname) { } if (url.protocol !== 'http:' && url.protocol !== 'https:') return false; if (!isLoopbackHostname(url.hostname)) return false; - return !isLoopbackHostname(pageHostname); + return !isOnBoxHostname(pageHostname); } if (typeof window !== 'undefined') { - window.CodemanWebviewLinks = { isLoopbackHostname, linkNeedsWebTabProxy }; + window.CodemanWebviewLinks = { isLoopbackHostname, isOnBoxHostname, webTabOriginKey, linkNeedsWebTabProxy }; } Object.assign(CodemanApp.prototype, { @@ -91,17 +126,24 @@ Object.assign(CodemanApp.prototype, { } catch { return; } - if (!this.webviews) await this.refreshWebviews(); + // ⚠️ "is `this.webviews` set" does NOT answer "is it loaded": initWebviews() + // assigns a truthy EMPTY map synchronously and only then awaits the list, so + // a tap during page load used to find nothing to reuse and POST a duplicate + // record for an origin that already exists server-side. Join an in-flight + // load; start one only when none has ever run. + if (this._webviewsRefresh) await this._webviewsRefresh; + else if (!this._webviewsLoaded) await this.refreshWebviews(); if (!this.webviews) { this.showToast?.('Could not open URL', 'error'); return; } + const wantedKey = webTabOriginKey(url); let existing = null; for (const webview of this.webviews.values()) { if (webview.managed || (webview.embedMode ?? 'proxy') !== 'proxy') continue; try { - if (new URL(webview.url).origin === url.origin) { + if (webTabOriginKey(new URL(webview.url)) === wantedKey) { existing = webview; break; } @@ -122,9 +164,17 @@ Object.assign(CodemanApp.prototype, { } await this.refreshWebviews(); id = created.id; + // The create is a persisted record: it writes webviews.json, broadcasts + // over SSE, adds a Run-dropdown row on every device this owner is signed + // in on and counts toward MAX_WEBVIEWS. Adding one by hand goes through a + // modal; a tap should not do all that with a new tab as its only signal. + this.showToast?.(`Saved ${url.host} as a web tab`, 'success'); } + // `/` is passed through rather than flattened to '': openWebview reads an + // empty path as "no deep link" and leaves an already-open frame on whatever + // page it was showing, so a link to the origin root did nothing visible. const path = `${url.pathname}${url.search}${url.hash}`; - await this.openWebview(id, { path: path === '/' ? '' : path }); + await this.openWebview(id, { path: path || '/' }); }, // ── State ───────────────────────────────────────────────────────────────── @@ -152,11 +202,19 @@ Object.assign(CodemanApp.prototype, { }, async refreshWebviews() { - const data = await this._apiJson('/api/webviews'); - if (!data) return; - this.webviews = new Map((data.webviews || []).map((w) => [w.id, w])); - if (typeof data.maxLiveFrames === 'number') this._webviewMaxFrames = data.maxLiveFrames; - this.renderWebviewMenuItems(); + const inFlight = this._apiJson('/api/webviews').then((data) => { + if (!data) return; + this.webviews = new Map((data.webviews || []).map((w) => [w.id, w])); + if (typeof data.maxLiveFrames === 'number') this._webviewMaxFrames = data.maxLiveFrames; + this._webviewsLoaded = true; + this.renderWebviewMenuItems(); + }); + this._webviewsRefresh = inFlight; + try { + await inFlight; + } finally { + if (this._webviewsRefresh === inFlight) this._webviewsRefresh = null; + } }, /** SSE: the saved list changed (possibly on another device). */ diff --git a/test/omp-fresh-run-no-resume.test.ts b/test/omp-fresh-run-no-resume.test.ts index fd1302ec..5c61a23f 100644 --- a/test/omp-fresh-run-no-resume.test.ts +++ b/test/omp-fresh-run-no-resume.test.ts @@ -170,4 +170,47 @@ describe('OMP: fresh session vs. reattach must not share resumeSessionId resolut expect(state.ompConfig?.continueSession).toBe(true); expect(session.claudeSessionId).toBe(session.id); }); + + it('_maybeCaptureOmpSessionId() is subject to the same remote guard, so a first idle turn cannot alias a remote session onto a local conversation', () => { + // The sibling guard in _pinOmpRespawnId has the test above; this one runs on + // the FIRST turn going idle, before any respawn, and reads the same local + // ~/.omp tree. Without the `this._remote` early return it would claim this + // unrelated local conversation's uuid as the remote session's identity, and + // every later respawn would then inherit the wrong pin. + seedOmpSessionFile('wrong-local-conversation-id'); + + const remote: SessionRemote = { + hostId: 'remote-box', + label: 'remote-box', + host: 'remote-box', + username: 'someone', + remotePath: workingDir, + owned: true, + }; + + const muxSession: MuxSession = { + sessionId: 'placeholder', + muxName: 'codeman-deadbeef', + pid: 1, + createdAt: Date.now(), + workingDir, + mode: 'omp', + attached: false, + }; + + const session = new Session({ + workingDir, + mode: 'omp', + mux: new TmuxManager(), + useMux: true, + muxSession, + remote, + }); + sessions.push(session); + + (session as unknown as { _maybeCaptureOmpSessionId(): void })._maybeCaptureOmpSessionId(); + + expect(session.claudeSessionId).toBe(session.id); + expect(session.toState().ompConfig?.resumeSessionId).toBeUndefined(); + }); }); diff --git a/test/response-viewer-last-turn.test.ts b/test/response-viewer-last-turn.test.ts index b940553c..8b65e9c4 100644 --- a/test/response-viewer-last-turn.test.ts +++ b/test/response-viewer-last-turn.test.ts @@ -145,6 +145,47 @@ describe('response viewer brief view (last answered turn)', () => { expect(viewer.classList.contains('visible')).toBe(true); }); + it('opens a multi-row turn at its NEWEST text, and a single card at the top', async () => { + // `body.scrollTop = 0` was right when the brief view was one card holding + // the last row. With the whole turn rendered, the top of the scroller is + // the turn's FIRST narration line and the answer the eye button exists to + // show can be several screens below it; loadFullContext already scrolls to + // the bottom for the same turn, so the two views disagreed. + // jsdom does no layout, so scrollHeight is stubbed and the write recorded. + const spyScroll = (body: HTMLElement) => { + const writes: number[] = []; + Object.defineProperty(body, 'scrollHeight', { configurable: true, get: () => 4200 }); + Object.defineProperty(body, 'scrollTop', { + configurable: true, + get: () => writes[writes.length - 1] ?? 0, + set: (v: number) => void writes.push(v), + }); + return writes; + }; + + const many = mountViewer(); + const manyWrites = spyScroll(many.body); + await makeApp({ + text: 'Done.', + timestamp: 't', + messages: [ + { role: 'assistant', text: 'Looking at the file.', turn: 2 }, + { role: 'assistant', text: 'Done.', turn: 2 }, + ], + }).app.toggleResponseViewer(); + expect(manyWrites.at(-1)).toBe(4200); + + document.body.innerHTML = ''; + const one = mountViewer(); + const oneWrites = spyScroll(one.body); + await makeApp({ + text: 'Done.', + timestamp: 't', + messages: [{ role: 'assistant', text: 'Done.', turn: 2 }], + }).app.toggleResponseViewer(); + expect(oneWrites.at(-1)).toBe(0); + }); + it('falls back to text when the server sends no messages, keeping one badged card', async () => { const { body } = mountViewer(); const { app } = makeApp({ text: 'Only the last row.', timestamp: 't' }); diff --git a/test/terminal-keycode229-recovery.test.ts b/test/terminal-keycode229-recovery.test.ts index 25cf87dd..6080224b 100644 --- a/test/terminal-keycode229-recovery.test.ts +++ b/test/terminal-keycode229-recovery.test.ts @@ -89,6 +89,27 @@ function harness({ screenReader = false } = {}) { }; } +/** terminal-ui.js's exported predicates, loaded the same way as in test/mobile-shell-keyboard.test.ts. */ +function loadTerminalInput() { + const source = readFileSync(new URL('../src/web/public/terminal-ui.js', import.meta.url), 'utf8'); + const win: Record = { + addEventListener() {}, + matchMedia: () => ({ matches: false, addEventListener() {} }), + }; + const sandbox: Record = { + window: win, + globalThis: win, + document: { addEventListener() {} }, + CodemanApp: class {}, + }; + win.CodemanApp = sandbox.CodemanApp; + vm.runInNewContext(source, sandbox, { filename: 'terminal-ui.js' }); + return win.CodemanTerminalInput as { + shouldSuppressTerminalQueryResponse(data: string): boolean; + isTerminalFocusOrMouseReport(data: string): boolean; + }; +} + describe('orphaned terminal input recovery', () => { it('forwards the committed text when xterm stayed silent', () => { const h = harness(); @@ -276,3 +297,60 @@ describe('orphaned terminal input recovery', () => { expect(h.emitted).toEqual([]); }); }); + +describe('terminal-ui wiring: what counts as "xterm spoke for this keystroke"', () => { + const terminalSource = readFileSync(new URL('../src/web/public/terminal-ui.js', import.meta.url), 'utf8'); + + it('gates notifyCanonicalData on the two predicates this file already owns', () => { + // onData does NOT only carry keystrokes: xterm answers DA/DSR/CPR/OSC + // queries through it during Ink redraws, and emits SGR mouse and focus + // reports on its own initiative. Counting one of those as canonical data + // for the pending keystroke stands the recovery down and leaves the + // character dropped, worst on a busy agent pane, which is the case this + // exists for. Same gate, same two predicates, as the one-shot Ctrl + // modifier uses for the same question (test/mobile-shell-keyboard.test.ts). + const notify = terminalSource.indexOf('_keyCode229Recovery?.notifyCanonicalData?.()'); + expect(notify).toBeGreaterThan(0); + + const gate = terminalSource.lastIndexOf( + '!input?.shouldSuppressTerminalQueryResponse(data) && !input?.isTerminalFocusOrMouseReport(data)', + notify + ); + expect(gate).toBeGreaterThan(0); + expect(gate).toBeLessThan(notify); + + // ⚠️ The predicates live inside the module IIFE that ends long before this + // call site, so they are reachable ONLY through the global. Bare references + // would throw a ReferenceError straight into the surrounding try/catch, + // which swallows it, and notifyCanonicalData would then NEVER run: the + // recovery would re-emit a character xterm already delivered. + expect(terminalSource.slice(gate - 120, notify)).toContain('window.CodemanTerminalInput'); + }); + + it('stands down for a real keystroke, but not for a mouse report or a query reply', () => { + // The gate as terminal-ui.js writes it. The wiring test above pins the real + // source; this proves the behaviour it buys. + const input = loadTerminalInput(); + const onData = (h: ReturnType, data: string) => { + if (!input.shouldSuppressTerminalQueryResponse(data) && !input.isTerminalFocusOrMouseReport(data)) { + h.controller.notifyCanonicalData(); + } + }; + + for (const noise of ['\x1b[<0;10;5M', '\x1b[I', '\x1b[?1;2c']) { + const h = harness(); + h.keydown(); + h.input('x'); + onData(h, noise); + h.flushTimers(); + expect(h.emitted, `${JSON.stringify(noise)} must not stand the recovery down`).toEqual(['x']); + } + + const typed = harness(); + typed.keydown(); + typed.input('x'); + onData(typed, 'x'); + typed.flushTimers(); + expect(typed.emitted, 'xterm really did deliver this one').toEqual([]); + }); +}); diff --git a/test/webview-loopback-links.test.ts b/test/webview-loopback-links.test.ts index 6c1376e4..2f9efe61 100644 --- a/test/webview-loopback-links.test.ts +++ b/test/webview-loopback-links.test.ts @@ -95,6 +95,8 @@ function boot(pageUrl = 'http://192.168.1.135:8095/') { interface WebviewLinks { isLoopbackHostname(host: string): boolean; + isOnBoxHostname(host: string): boolean; + webTabOriginKey(url: URL): string; linkNeedsWebTabProxy(url: string, pageHostname: string): boolean; } @@ -108,16 +110,7 @@ describe('loopback link decision', () => { const links = win.CodemanWebviewLinks; it('recognises every loopback spelling and nothing else', () => { - for (const host of [ - 'localhost', - 'LOCALHOST', - 'app.localhost', - '127.0.0.1', - '127.1.2.3', - '0.0.0.0', - '[::1]', - '::1', - ]) { + for (const host of ['localhost', 'LOCALHOST', '127.0.0.1', '127.1.2.3', '0.0.0.0', '[::1]', '::1']) { expect(links.isLoopbackHostname(host), host).toBe(true); } for (const host of [ @@ -133,6 +126,31 @@ describe('loopback link decision', () => { } }); + it('keeps *.localhost OUT of the auto-route set, because it is a DNS name an attacker can steer', () => { + // Every other member of the set is an address literal that can only mean + // this box. `evil.localhost` is not: on a resolver that does not synthesise + // *.localhost locally and has a search domain configured, it NXDOMAINs as + // absolute and is retried as `evil.localhost.`. The link + // source is agent-written terminal output, so this set is the whole + // confinement on a tap that makes Codeman fetch a URL and persist it. + expect(links.isLoopbackHostname('app.localhost')).toBe(false); + expect(links.isLoopbackHostname('evil.localhost')).toBe(false); + expect(links.linkNeedsWebTabProxy('http://evil.localhost/', '192.168.1.135')).toBe(false); + + // The PAGE-side test stays broader: a false positive there only ever + // DECLINES to proxy, leaving the caller's own direct open untouched. + expect(links.isOnBoxHostname('app.localhost')).toBe(true); + expect(links.linkNeedsWebTabProxy('http://localhost:5173/', 'app.localhost')).toBe(false); + }); + + it('keys a dashboard per dev server, not per host spelling', () => { + const key = (u: string) => links.webTabOriginKey(new URL(u)); + expect(key('http://localhost:5173/')).toBe(key('http://127.0.0.1:5173/')); + expect(key('http://localhost:5173/')).not.toBe(key('http://localhost:5174/')); + expect(key('http://localhost:5173/')).not.toBe(key('https://localhost:5173/')); + expect(key('http://box.ts.net:3000/')).toBe('http://box.ts.net:3000'); + }); + it('proxies a loopback http(s) link only when the page is not on that box', () => { expect(links.linkNeedsWebTabProxy('http://localhost:5173/', '192.168.1.135')).toBe(true); expect(links.linkNeedsWebTabProxy('https://127.0.0.1:8443/x?y=1', 'box.ts.net')).toBe(true); @@ -171,6 +189,54 @@ describe('openLinkThroughWebTabIfLoopback', () => { expect(win.document.querySelectorAll('.webview-frame').length).toBe(1); }); + it('navigates an already-mounted frame back to the origin ROOT, which used to do nothing', async () => { + // openUrlInWebTab used to flatten '/' to '', and openWebview reads an empty + // path as "no deep link", so it mounted with navigate:false and an open + // frame stayed on whatever page it was showing. Deep links navigated; a tap + // on the bare origin silently did not. + const { win, app } = boot(); + await app.openUrlInWebTab('http://localhost:5173/deep/page?a=1'); + expect(frameSrc(win, 'dev')).toBe('/webview/cap-dev/deep/page?a=1'); + + await app.openUrlInWebTab('http://localhost:5173/'); + expect(frameSrc(win, 'dev')).toBe('/webview/cap-dev/'); + expect(win.document.querySelectorAll('.webview-frame').length).toBe(1); + }); + + it('reuses one dashboard across host spellings of the same dev server', async () => { + const { win, app, calls } = boot(); + // The saved dashboard is http://localhost:5173/; a 127.0.0.1 link to the + // same port is the same server and must not mint a second tab. + await app.openUrlInWebTab('http://127.0.0.1:5173/status'); + expect(frameSrc(win, 'dev')).toBe('/webview/cap-dev/status'); + expect(calls.find((c) => c.method === 'POST' && c.path === '/api/webviews')).toBeUndefined(); + expect(win.document.querySelectorAll('.webview-frame').length).toBe(1); + }); + + it('waits for an in-flight webview load instead of POSTing a duplicate record', async () => { + // initWebviews() assigns a truthy EMPTY map synchronously and only then + // awaits GET /api/webviews, so "is this.webviews set" answered "is it + // loaded" wrongly: a tap inside that round trip found nothing to reuse and + // saved a second dashboard for an origin that already existed server-side. + const { win, app, calls } = boot(); + const loaded = app.webviews; + app.webviews = new Map(); + let release: () => void = () => {}; + const gate = new Promise((resolve) => { + release = resolve; + }); + (app as unknown as { _webviewsRefresh: Promise })._webviewsRefresh = gate.then(() => { + app.webviews = loaded; + }); + + const tap = app.openUrlInWebTab('http://localhost:5173/late'); + release(); + await tap; + + expect(calls.find((c) => c.method === 'POST' && c.path === '/api/webviews')).toBeUndefined(); + expect(frameSrc(win, 'dev')).toBe('/webview/cap-dev/late'); + }); + it('saves an unknown origin under its host:port, then opens it', async () => { const { win, app, calls } = boot(); expect(app.openLinkThroughWebTabIfLoopback('http://127.0.0.1:3000/')).toBe(true);