From 75b0a353279604d09c8eaca885982f6cbd0f21b8 Mon Sep 17 00:00:00 2001 From: developer-agent Date: Thu, 27 Aug 2026 10:32:36 -0400 Subject: [PATCH 1/2] Restore the WebSocket, stop the 5s repaint, share one launch roster (v0.5.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects, grouped because they share two files and one deploy cycle. WebSocket (vikunja#532). v0.4.0 tightened the upgrade guard to reject a *missing* Origin as well as a wrong one, reasoning that only non-browser clients omit it. The one non-browser client here is CloudCLI's own plugin WS proxy, which uses the `ws` library — and that sends no Origin unless explicitly passed. Every handshake was 403'd from 2026-08-02 to 2026-08-27: 2239 error lines, and a tab reading "disconnected" throughout. The guard now gates on the peer (loopback only; anything else refused outright) and applies the Origin allowlist only when an Origin is present. A present-but-wrong Origin is still refused, so this is a narrowing, not a relaxation — the loopback bind was always the real boundary. Extracted to ws-guard.ts as a pure function with tests, since server.ts listens at import time. AGENTS.md carried the old rule as a project invariant. Left there, the next reviewer restores the defect on principle, so that sentence is rewritten rather than merely contradicted by the code. Repaint (vikunja#532). Each failed reconnect emitted _disconnected, which called render() — and render() does root.innerHTML = ''. The whole panel was torn down and rebuilt every 5s while disconnected, losing scroll position and closing any open filter dropdown, even though wsConnected was already false. Connection state no longer reaches render(): the header badge is mutated in place, and only on a genuine transition. Reconnect backoff added at 5s -> 10s -> 30s, capped, reset on open; the first delay is unchanged so a transient blip recovers as fast as before. Launcher (vikunja#523). AGENT_PROJECTS was a second, drifted copy of task-dispatcher.py's roster with no steward entry, so Start refused steward outright. The literal is deleted rather than extended: adding an entry would have made launchSession() spawn `claude` as the plugin's own user, bypassing the launcher whose whole purpose is that agent's isolation — a session appearing as steward in every log while holding none of steward's credentials. Both consumers now read ~/scripts/agent-launch.yml. Validation re-establishes what the literal provided for free: every field checked against a closed set, whole document rejected on any violation, and a missing or malformed file is a named error rather than an empty policy — an empty policy makes run_as_user absent for every agent, which is exactly the impersonation above. An agent with run_as_user is launched via `sudo -n -u --workflow-mode -- `; a missing or non-executable launcher is refused by name, never falling back. Also: - Launch logs move to ~/.claude/comms/artifacts/task-launches/-.log, matching the dispatcher. Two destinations for one concept meant nothing could list "the launches". - Start's `review` maps to the queue's `semi-auto` rather than passing through, which the launcher would refuse by name. For a run-as agent `review` is prompt-enforced only — run-steward.sh sets --dangerously-skip-permissions itself and accepts no permission mode — and the toast now says so instead of implying a tool gate. - lookupAgent() guards the policy lookup: target_agent comes from a queue YAML, and a plain policy['constructor'] is truthy, so a bare !entry check would accept a non-entry. Policy objects are built with a null prototype. - A refused upgrade logs why. v0.4.0's refusals were silent on this side; the only signal was a 403 on the far side of the proxy, naming neither leg nor the cause. Verified against a live instance of the new binary on a scratch port: no Origin -> 101, allowed Origin -> 101, wrong Origin -> 403. Negative test confirmed a broken launcher fails by name with nothing spawned. Tests 12 -> 41; 13 mutations applied across the three modules, all caught. Co-Authored-By: Claude Opus 5 (1M context) agent-id: developer --- AGENTS.md | 13 +- CHANGELOG.md | 68 ++++++++ README.md | 63 ++++++-- manifest.json | 8 +- package.json | 2 +- src/index.ts | 51 ++++-- src/launch-policy.ts | 233 +++++++++++++++++++++++++++ src/panels/ws-client.ts | 24 ++- src/server.ts | 124 ++++++++++---- src/tests/launch-policy.test.ts | 276 ++++++++++++++++++++++++++++++++ src/tests/ws-client.test.ts | 36 +++++ src/tests/ws-guard.test.ts | 85 ++++++++++ src/ws-guard.ts | 55 +++++++ 13 files changed, 971 insertions(+), 67 deletions(-) create mode 100644 src/launch-policy.ts create mode 100644 src/tests/launch-policy.test.ts create mode 100644 src/tests/ws-client.test.ts create mode 100644 src/tests/ws-guard.test.ts create mode 100644 src/ws-guard.ts diff --git a/AGENTS.md b/AGENTS.md index c078c72..33cf4d1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,6 +19,11 @@ src/ control-api.ts The single outbound mutation path to task-queue-mcp. Extracted from server.ts so the auth and transport guards are unit-testable without booting the server. + ws-guard.ts The WebSocket upgrade decision, as a pure function of + (peer address, Origin, allowlist). Extracted for the + same reason: server.ts listens at import time. + launch-policy.ts Reads and validates the shared launch roster, and builds + the spawn argv. Same extraction reason. types.ts Shared types. The Task shape mirrors task-queue-mcp's YAML schema — keep them in step. panels/ @@ -36,8 +41,12 @@ src/ - **The plugin acts as `operator`, never as an agent.** Every proxied mutation sends `actor: 'operator'`. This is what makes the MCP's `amend_task` authorization accept it; the plugin must never assert an agent's identity. - **Mutations fail closed without the secret.** `callControlApi` returns 500 and never attempts the fetch when `TASK_QUEUE_API_SECRET` is empty, and logs why. Do not add a fallback that proceeds without it. - **The version comes from `package.json`.** `server.ts` reads it at startup rather than hardcoding a copy — a hardcoded constant silently drifted and reported a stale version on `/health` for two releases. `package.json` and `manifest.json` must also agree. +- **There is exactly one agent roster, and it is not in this repo.** `~/scripts/agent-launch.yml` (override: `AGENT_LAUNCH_POLICY`) is read by this plugin *and* by `task-dispatcher.py`. A hardcoded `AGENT_PROJECTS` map used to live in `server.ts`; it drifted, lost an agent, and that drift is vikunja#523. Do not add a literal roster back — extend the file. +- **A run-as agent goes through its launcher, always.** An entry with `run_as_user` is spawned as `sudo -n -u …`, never as `claude`. Spawning `claude` directly for such an agent bypasses the launcher's identity guard and yields a session that appears as that agent in every log while holding none of its credentials. A missing or non-executable launcher is refused by name — there is no fallback path, deliberately. +- **The launch policy fails closed, loudly.** A missing or malformed policy file disables Start with a named error. It must never degrade to an empty policy: an empty policy makes `run_as_user` absent for every agent, which is precisely the impersonation above. - **Path guards resolve symlinks.** `previewFile` uses `fs.realpathSync` before the prefix check; `path.resolve` alone normalises `..` but follows nothing, so a symlink inside an allowed prefix would escape it. -- **The WebSocket upgrade rejects a missing Origin**, not just a wrong one. Browsers always send one; only non-browser clients omit it, which is precisely what the check is for. +- **The WebSocket upgrade gates on the peer address first, and on `Origin` only if one is present.** The server binds `127.0.0.1` on an ephemeral port, so a non-loopback peer is refused outright; a loopback peer with a *present but wrong* `Origin` is still refused. A loopback peer with **no** `Origin` is accepted, because that is CloudCLI's own plugin WS proxy — it uses the `ws` client library, which sends no `Origin` unless one is passed, and its browser leg is already authenticated by CloudCLI's `verifyClient` before the proxy is invoked. + v0.4.0 inverted this, rejecting a *missing* `Origin` on the reasoning that only non-browser clients omit it. On this deployment the only non-browser client is that trusted proxy, so every connect was 403'd for three weeks (2239 failures) and the tab read `disconnected` throughout. Do not restore the missing-Origin rejection; the loopback bind is the boundary, and `Origin` alone never did the work this deployment needed. The rule is a pure function in `ws-guard.ts` with tests covering all three cases. ## Testing @@ -47,7 +56,7 @@ npm run build # tsc --noEmit is the typecheck gate; esbuild bundles after it p npm test ``` -Tests cover `control-api.ts`: the secret gate, task-id validation, header and body shape per action, transport-failure mapping, and pass-through of the MCP's authorization rejections. The UI panels are not unit-tested — verify them in CloudCLI after `./deploy.sh && pm2 restart cloudcli`. +Tests cover `control-api.ts` (the secret gate, task-id validation, header and body shape per action, transport-failure mapping, pass-through of the MCP's authorization rejections), `ws-guard.ts` (all three upgrade cases, including the loopback-with-no-Origin one that v0.4.0 broke), `launch-policy.ts` (every closed-set rejection, whole-document rejection, and both argv shapes), and the reconnect schedule. The UI panels are not unit-tested — verify them in CloudCLI after `./deploy.sh && pm2 restart cloudcli`. Two build/test gotchas worth knowing before you touch either script: diff --git a/CHANGELOG.md b/CHANGELOG.md index f1ec73f..68b340d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,74 @@ All notable changes to this project will be documented in this file. +## [0.5.0] - 2026-08-27 + +Requires a launch policy file at `~/scripts/agent-launch.yml` (see README). The Start +button is disabled with a named error if it is missing or malformed — it never falls back +to a built-in roster, because falling back is what this release removes. + +### Fixed +- **The live WebSocket, dead since v0.4.0.** v0.4.0 tightened the upgrade guard to reject a + *missing* `Origin` as well as a wrong one, reasoning that only non-browser clients omit + it. The one non-browser client here is CloudCLI's own plugin WS proxy, which uses the + `ws` library — and that sends no `Origin` unless explicitly passed. Every upstream + handshake was 403'd from 2026-08-02 to 2026-08-27: 2239 `WS proxy error … 403` lines, and + a tab that read `disconnected` the whole time. The guard now gates on the **peer** + (loopback only, refused outright otherwise) and applies the `Origin` allowlist only when + an `Origin` is actually present. A present-but-wrong `Origin` is still refused; the + loopback bind was always the real boundary. `AGENTS.md` carried the old rule as a project + invariant and has been rewritten — leaving it would have invited the same regression from + the next reviewer. New `ws-guard.ts` holds the decision as a pure function, with tests. +- **The five-second full-panel repaint.** Every failed reconnect emitted `_disconnected`, + which called `render()` — and `render()` does `root.innerHTML = ''`. So the entire panel + was torn down and rebuilt every 5s while disconnected, losing scroll position and closing + any open filter dropdown, even though `wsConnected` was already `false`. Connection state + no longer reaches `render()` at all: it updates the header badge in place, and only on a + genuine transition. +- **Start could not launch a run-as agent.** `AGENT_PROJECTS` was a second, drifted copy of + `task-dispatcher.py`'s roster with no `steward` entry, so Start refused it outright. The + literal is deleted rather than extended: simply adding an entry would have made + `launchSession()` spawn `claude` as the plugin's own user, bypassing the launcher whose + entire purpose is that agent's isolation — a session appearing as steward in every log + and holding none of steward's credentials. + +### Added +- **Reconnect backoff** — 5s → 10s → 30s, capped, reset on a successful open. The first + delay is unchanged, so a transient blip still recovers as fast as before; the widening is + for outages. A fixed 5s retry is what turned a three-week outage into 2239 identical log + lines. +- **`launch-policy.ts`** — one roster, read by this plugin and by `task-dispatcher.py`, from + `~/scripts/agent-launch.yml`. `AGENT_LAUNCH_POLICY` overrides the path. Every field is + validated against a closed set (agent name shape, `project_dir` under `~/.claude/projects`, + `run_as_user` matching `agent-*`, `launcher` under `/usr/local/sbin/forge/`) and the whole + document is rejected on any violation — a partially-honoured roster would silently launch + some agent the wrong way. A missing or malformed file is a named error, never an empty + policy. +- An agent carrying `run_as_user` is launched as + `sudo -n -u --workflow-mode -- `, mirroring the + dispatcher. A launcher that is missing or not executable is refused **by name**; it never + degrades to spawning `claude`. +- `env:CLOUDCLI_ORIGIN` in `manifest.json` permissions, so the plugin's allowlist can carry + the same origin the host's proxy sends. +- Tests for the upgrade guard, the reconnect schedule, and the launch policy. The suite grew + from 12 to 39. + +### Changed +- **Launch logs moved** to `~/.claude/comms/artifacts/task-launches/-.log`. + The dispatcher wrote `~/.pm2/logs/agent-launch--.log` and this plugin wrote + `.log` — two destinations for one concept, so nothing could list "the launches". + Both now write the same shape to the same directory. `~/.claude/comms` is the side both + can read; `~/.pm2/logs` is not in `PREVIEW_ALLOWED_PREFIXES` and must not be added, since + that prefix covers every PM2 service log on the host. +- **Start's `review` maps to the queue's `semi-auto`** for run-as agents rather than being + passed through. `review` is not a value `task-queue-mcp` or `run-steward.sh` accepts — + passed through, the launcher refuses it by name. Note that for a run-as agent `review` is + **prompt-enforced only**: `run-steward.sh` sets `--dangerously-skip-permissions` itself and + accepts no permission mode, so `--permission-mode plan` is not reachable. The toast says so + rather than implying a tool gate that is not there. +- A refused WebSocket upgrade now logs why. v0.4.0's refusals were silent on this side; the + only signal was a 403 on the far side of the proxy, naming neither leg nor the cause. + ## [0.4.0] - 2026-08-02 Requires [task-queue-mcp](https://github.com/TadMSTR/task-queue-mcp) **v0.4.0** — the diff --git a/README.md b/README.md index bf9e9b9..f289996 100644 --- a/README.md +++ b/README.md @@ -72,13 +72,14 @@ pm2 restart cloudcli |----------|---------|---------| | `TASK_QUEUE_API` | `http://127.0.0.1:8485` | Base URL of the task-queue-mcp HTTP control API. Configurable — the default assumes the MCP server runs loopback-local to CloudCLI. | | `TASK_QUEUE_API_SECRET` | — | Shared secret sent as `X-Task-Queue-Secret` on every mutation. **Required** — mutations fail closed if unset. Comes from your secret store, never from source. | -| `CLOUDCLI_ORIGIN` | — | Additional allowed WebSocket origin. `http://localhost:3001` and `http://127.0.0.1:3001` are always allowed; set this if CloudCLI is served from another origin. | +| `CLOUDCLI_ORIGIN` | — | Additional allowed WebSocket origin, and the origin the CloudCLI host's plugin proxy sends on its upstream leg. Both sides read the same variable so they cannot disagree. `http://localhost:3001` and `http://127.0.0.1:3001` are always allowed. | +| `AGENT_LAUNCH_POLICY` | `~/scripts/agent-launch.yml` | Path to the launch policy file (see [Session launch behaviour](#session-launch-behaviour)). | ### How the plugin receives its env vars CloudCLI launches the backend as a subprocess and **strips host environment variables from it by default**, including secrets. A host var reaches the plugin only when **both** are true: -1. `manifest.json` declares it — `permissions: ["env:TASK_QUEUE_API", "env:TASK_QUEUE_API_SECRET"]`, and +1. `manifest.json` declares it — `permissions: ["env:TASK_QUEUE_API", "env:TASK_QUEUE_API_SECRET", "env:CLOUDCLI_ORIGIN"]`, and 2. the var is on CloudCLI's host-side plugin env allowlist. This needs a CloudCLI build with permission-gated env passthrough. **Without it the launcher silently strips the secret and every mutation fails closed** — the UI reports an error, but nothing about the failure names the passthrough as the cause. If mutations fail while reads work, check this first. The control-API call path logs missing-secret and unreachable-transport failures to the CloudCLI process's stderr log, and never logs the secret value. @@ -104,7 +105,11 @@ The backend exposes a small HTTP API consumed by the UI via `api.rpc()`. Reads are served directly from the queue YAML. Every proxied mutation carries the `X-Task-Queue-Secret` header and an `actor` of `operator`. -WebSocket upgrade is handled on the same port. Clients receive `{type: "connected", version}` on connect and `{type: "tasks", count, changed}` when task files change. The upgrade handler rejects any request whose `Origin` is missing or not allowed. +WebSocket upgrade is handled on the same port. Clients receive `{type: "connected", version}` on connect and `{type: "tasks", count, changed}` when task files change. + +The upgrade handler gates on the **peer address** first: the server binds `127.0.0.1` on an ephemeral port, so a non-loopback peer is refused outright. An `Origin` is then checked against the allowlist **only if one is present**. A loopback peer that sends no `Origin` is accepted, because that is what CloudCLI's own plugin WS proxy looks like — the `ws` client library sends no `Origin` unless one is passed, and that leg is already authenticated by CloudCLI before the proxy is invoked. A present-but-wrong `Origin` is still refused. + +> Do not "harden" this by rejecting a missing `Origin`. v0.4.0 did exactly that and 403'd every connect for three weeks, because the only client that reaches this port is the trusted proxy. The loopback bind is the boundary. The rule is a pure function in `src/ws-guard.ts` with tests for all three cases. ## Session launch behaviour @@ -113,16 +118,54 @@ WebSocket upgrade is handled on the same port. Clients receive `{type: "connecte | `review` | `plan` | Read the task, present a summary, wait for approval | | `auto` | `default` | Read the task, claim it (`in-progress`), execute | -A task's `target_agent` is mapped to a project directory to launch in. The mapping is **a hardcoded map in `src/server.ts`** — adapting this plugin to a different set of agents means editing that map: +### The launch policy file + +A task's `target_agent` is resolved through a **data file**, not a map in the source: +`~/scripts/agent-launch.yml` by default, overridable with `AGENT_LAUNCH_POLICY`. Adapting +this plugin to a different set of agents means editing that file — no rebuild. + +```yaml +my-agent: + project_dir: ~/.claude/projects/my-agent -```ts -const AGENT_PROJECTS: Record = { - 'my-agent': path.join(HOME, '.claude', 'projects', 'my-agent'), - // ...one entry per agent that can be a target_agent -}; +# An agent that must NOT run as the plugin's own user: +my-isolated-agent: + project_dir: ~/.claude/projects/my-isolated-agent + run_as_user: agent-my-isolated-agent + launcher: /usr/local/sbin/forge/run-my-isolated-agent.sh ``` -A task whose `target_agent` is absent from the map returns a clean `Unknown agent` error rather than launching. +The file is deliberately shared with whatever else launches your agents (on the reference +deployment, a cron dispatcher reads the same file). A second copy of this roster is what +this release removes: the plugin's private map had drifted and was missing an agent +entirely, so Start refused it. + +**`run_as_user` is the part that matters.** An entry carrying it is launched as +`sudo -n -u --workflow-mode -- ` — never as `claude` +directly. That indirection exists because such an agent's credentials are readable only by +that user; spawning `claude` as the plugin's own user instead would produce a session that +appears as the agent in every log while holding none of its credentials. If the launcher is +missing or not executable, Start fails **by name**; it does not fall back. + +Every field is validated against a closed set — agent name shape, `project_dir` under +`~/.claude/projects`, `run_as_user` matching `agent-*`, `launcher` under +`/usr/local/sbin/forge/` — and the whole document is rejected on any violation. A missing or +malformed file disables Start with a named error rather than yielding an empty policy, since +an empty policy makes `run_as_user` absent for *every* agent. + +A task whose `target_agent` is absent from the file returns a clean `Unknown agent` error +rather than launching. + +> **Mode vocabulary.** Start sends `review | auto`; a launcher taking `--workflow-mode` +> receives `semi-auto | auto`, mapped explicitly. For a run-as agent, `review` is +> **prompt-enforced only** — the reference launcher sets `--dangerously-skip-permissions` +> itself and accepts no permission mode, so `--permission-mode plan` is not reachable. The +> UI says so on launch rather than implying a tool gate. + +### Launch logs + +Each launch appends to `~/.claude/comms/artifacts/task-launches/-.log`, the +same shape and directory the reference dispatcher writes, so both are listable together. ## Development diff --git a/manifest.json b/manifest.json index 85e9ce6..ba2a359 100644 --- a/manifest.json +++ b/manifest.json @@ -1,7 +1,7 @@ { "name": "task-queue", "displayName": "Task Queue", - "version": "0.4.0", + "version": "0.5.0", "description": "Task queue dashboard — view, filter, and launch agent tasks.", "author": "TadMSTR", "icon": "icon.svg", @@ -9,5 +9,9 @@ "slot": "tab", "entry": "dist/index.js", "server": "dist/server.js", - "permissions": ["env:TASK_QUEUE_API", "env:TASK_QUEUE_API_SECRET"] + "permissions": [ + "env:TASK_QUEUE_API", + "env:TASK_QUEUE_API_SECRET", + "env:CLOUDCLI_ORIGIN" + ] } diff --git a/package.json b/package.json index 2ac03eb..1e0a5ee 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "cloudcli-plugin-task-queue", - "version": "0.4.0", + "version": "0.5.0", "private": true, "type": "module", "scripts": { diff --git a/src/index.ts b/src/index.ts index 7cff0d3..e395e32 100644 --- a/src/index.ts +++ b/src/index.ts @@ -36,6 +36,19 @@ export function mount(container: HTMLElement, api: PluginAPI): void { let wsClient: WsClient | null = null; let unsubCtx: (() => void) | null = null; + // Header badge nodes, re-seated by renderHeader on every render. Mutated in place by + // updateConnectionBadge so a connection change costs two property writes, not a repaint. + let wsDotEl: HTMLElement | null = null; + let wsLabelEl: HTMLElement | null = null; + + function updateConnectionBadge(): void { + if (!wsDotEl || !wsLabelEl) return; + const c = themeColors(api.context.theme === 'dark'); + wsDotEl.style.background = state.wsConnected ? c.ok : c.muted; + wsDotEl.className = state.wsConnected ? 'tq-live' : ''; + wsLabelEl.textContent = state.wsConnected ? 'live' : 'disconnected'; + } + const root = document.createElement('div'); Object.assign(root.style, { height: '100%', @@ -99,10 +112,13 @@ export function mount(container: HTMLElement, api: PluginAPI): void { async function handleStart(taskId: string, mode: 'review' | 'auto'): Promise { try { - await api.rpc('POST', `tasks/${taskId}/start`, { mode }); + const res = await api.rpc('POST', `tasks/${taskId}/start`, { mode }) as { note?: string }; state.error = null; - // Brief feedback - showToast(`Session launched (${mode} mode)`); + // Brief feedback. `note` carries a caveat the backend could not honour silently — + // e.g. review is prompt-enforced only for a run-as agent, whose launcher accepts + // no permission mode. Showing "launched (review mode)" alone would imply a tool + // gate that is not there. + showToast(res?.note ? `Session launched — ${res.note}` : `Session launched (${mode} mode)`); } catch (err) { state.error = (err as Error).message; render(api.context); @@ -277,20 +293,21 @@ export function mount(container: HTMLElement, api: PluginAPI): void { const header = document.createElement('div'); header.style.cssText = `display:flex;align-items:center;gap:12px;margin-bottom:16px;padding-bottom:12px;border-bottom:1px solid ${c.border};`; - const wsColor = state.wsConnected ? c.ok : c.muted; - const wsDot = state.wsConnected - ? `` - : ``; - header.innerHTML = ` Task Queue ${state.tasks.length} tasks - ${wsDot} - ${state.wsConnected ? 'live' : 'disconnected'} + + `; + // Cache the badge nodes for updateConnectionBadge(). render() rebuilds the header, + // so these are re-seated on every render and must not be captured elsewhere. + wsDotEl = header.querySelector('#tq-ws-dot'); + wsLabelEl = header.querySelector('#tq-ws-label'); + updateConnectionBadge(); + // Refresh button const refreshBtn = document.createElement('button'); refreshBtn.textContent = '\u21BB'; @@ -312,12 +329,14 @@ export function mount(container: HTMLElement, api: PluginAPI): void { wsClient = createWsClient(); wsClient.onEvent((event) => { - if (event.type === '_connected') { - state.wsConnected = true; - render(api.context); - } else if (event.type === '_disconnected') { - state.wsConnected = false; - render(api.context); + if (event.type === '_connected' || event.type === '_disconnected') { + const next = event.type === '_connected'; + // Connection state never reaches render(). render() does root.innerHTML = '', + // so calling it from here tore down and rebuilt the whole panel on every failed + // reconnect — every 5s, forever, losing scroll position and any open dropdown. + if (state.wsConnected === next) return; + state.wsConnected = next; + updateConnectionBadge(); } else if (event.type === 'tasks') { debouncedRefresh(); } diff --git a/src/launch-policy.ts b/src/launch-policy.ts new file mode 100644 index 0000000..2bb92da --- /dev/null +++ b/src/launch-policy.ts @@ -0,0 +1,233 @@ +/** + * Agent launch policy — the roster of how each agent's session is started. + * + * This plugin used to carry its own `AGENT_PROJECTS` literal in server.ts: a second + * copy of a table `task-dispatcher.py` also spelled twice. It never gained a steward + * entry, so the Start button could not launch steward at all — that drift IS + * vikunja#523. The literal is deleted; both consumers now read one file, + * `~/scripts/agent-launch.yml`, which ships in host-forge/scripts beside the dispatcher. + * + * Extracted from server.ts for the same reason control-api.ts and ws-guard.ts were: + * server.ts calls server.listen() at import time, so a test importing it boots a + * listener. Everything here is a pure function of its inputs. + * + * The validation is not decoration. The literal existed so no value outside a closed + * set could reach a subprocess spawn; loading from a file does not preserve that by + * itself, so every field is checked here against the same closed set the dispatcher's + * validate_launch_policy() uses, and the WHOLE document is rejected on any violation. + * A partially-honoured roster would silently launch some agent the wrong way. + */ + +import path from 'node:path'; +import fs from 'node:fs'; +import { load as yamlLoad } from 'js-yaml'; + +// Closed sets. A policy file cannot introduce a user, a launcher directory, or a +// project root outside these — it selects from them. +const AGENT_NAME_RE = /^[a-z][a-z0-9_-]{0,31}$/; +const RUN_AS_USER_RE = /^agent-[a-z0-9-]{1,30}$/; +const LAUNCHER_DIR = '/usr/local/sbin/forge/'; + +export class LaunchPolicyError extends Error { + constructor(message: string) { + super(message); + this.name = 'LaunchPolicyError'; + } +} + +export interface AgentLaunch { + projectDir: string; + /** null for an agent that runs as the plugin's own user. */ + runAsUser: string | null; + /** Non-null if and only if runAsUser is. */ + launcher: string | null; +} + +export type LaunchPolicy = Record; + +/** + * Look an agent up in the policy. `target_agent` comes from a queue YAML file, so a + * plain `policy[agent]` can resolve to `Object.prototype.constructor` and satisfy a + * truthiness check with something that is not a policy entry at all. + */ +export function lookupAgent(policy: LaunchPolicy, agent: string): AgentLaunch | null { + // hasOwnProperty via call, not Object.hasOwn: the project targets ES2020, and + // `policy.hasOwnProperty` is itself absent on a null-prototype object. + if (typeof agent !== 'string' || !Object.prototype.hasOwnProperty.call(policy, agent)) return null; + return policy[agent]; +} + +/** Where the policy file lives. AGENT_LAUNCH_POLICY overrides it for tests. */ +export function policyPath(env: NodeJS.ProcessEnv, home: string): string { + return env.AGENT_LAUNCH_POLICY || path.join(home, 'scripts', 'agent-launch.yml'); +} + +function expandHome(p: string, home: string): string { + if (p === '~') return home; + if (p.startsWith('~/')) return path.join(home, p.slice(2)); + return p; +} + +/** True if `child` is `root` itself or lies beneath it. Segment-wise, not by prefix. */ +function isUnder(root: string, child: string): boolean { + if (child === root) return true; + return child.startsWith(root + path.sep); +} + +export function validateLaunchPolicy(raw: unknown, home: string): LaunchPolicy { + const projectRoot = path.join(home, '.claude', 'projects'); + + if (raw === null || typeof raw !== 'object' || Array.isArray(raw) || Object.keys(raw).length === 0) { + throw new LaunchPolicyError('expected a non-empty mapping of agents'); + } + + // Null prototype, so a lookup by an agent name taken from task content cannot + // resolve to an inherited member. `{}['constructor']` is truthy, and the caller's + // `if (!entry)` guard would let it through as if it were a policy entry. + const policy: LaunchPolicy = Object.create(null) as LaunchPolicy; + for (const [agent, entry] of Object.entries(raw as Record)) { + if (!AGENT_NAME_RE.test(agent)) { + throw new LaunchPolicyError(`invalid agent name ${JSON.stringify(agent)}`); + } + if (entry === null || typeof entry !== 'object' || Array.isArray(entry)) { + throw new LaunchPolicyError(`${agent}: expected a mapping`); + } + const e = entry as Record; + + const unknown = Object.keys(e).filter(k => !['project_dir', 'run_as_user', 'launcher'].includes(k)); + if (unknown.length) { + throw new LaunchPolicyError(`${agent}: unknown key(s) ${unknown.sort().join(', ')}`); + } + + const rawDir = e.project_dir; + if (typeof rawDir !== 'string' || !rawDir) { + throw new LaunchPolicyError(`${agent}: project_dir is required`); + } + const expanded = expandHome(rawDir, home); + if (!path.isAbsolute(expanded)) { + throw new LaunchPolicyError(`${agent}: project_dir must be absolute: ${rawDir}`); + } + // Normalise `..` before the containment check. path.normalize does not follow + // symlinks, which is right here — the directory may legitimately not exist yet, + // and the caller reports that with a better message than this could. + const projectDir = path.normalize(expanded); + if (!isUnder(projectRoot, projectDir)) { + throw new LaunchPolicyError(`${agent}: project_dir must be under ${projectRoot}: ${rawDir}`); + } + + const runAsUser = e.run_as_user ?? null; + const launcher = e.launcher ?? null; + if ((runAsUser === null) !== (launcher === null)) { + throw new LaunchPolicyError(`${agent}: run_as_user and launcher must be given together`); + } + if (runAsUser !== null) { + if (typeof runAsUser !== 'string' || !RUN_AS_USER_RE.test(runAsUser)) { + throw new LaunchPolicyError(`${agent}: invalid run_as_user ${JSON.stringify(runAsUser)}`); + } + if (typeof launcher !== 'string' || !launcher.startsWith(LAUNCHER_DIR)) { + throw new LaunchPolicyError(`${agent}: launcher must be under ${LAUNCHER_DIR}: ${JSON.stringify(launcher)}`); + } + if (path.normalize(launcher) !== launcher) { + throw new LaunchPolicyError(`${agent}: launcher must be a normalised path: ${JSON.stringify(launcher)}`); + } + } + + policy[agent] = { + projectDir, + runAsUser: runAsUser as string | null, + launcher: launcher as string | null, + }; + } + return policy; +} + +/** Read and validate the policy file. Throws LaunchPolicyError; never returns {}. */ +export function loadLaunchPolicy(filePath: string, home: string): LaunchPolicy { + let text: string; + try { + text = fs.readFileSync(filePath, 'utf-8'); + } catch (err) { + throw new LaunchPolicyError(`cannot read launch policy ${filePath}: ${(err as Error).message}`); + } + let raw: unknown; + try { + raw = yamlLoad(text); + } catch (err) { + throw new LaunchPolicyError(`cannot parse launch policy ${filePath}: ${(err as Error).message}`); + } + return validateLaunchPolicy(raw, home); +} + +// ── Launch mode vocabulary ─────────────────────────────────────────────────── + +/** What the plugin's Start button sends. */ +export type StartMode = 'review' | 'auto'; +/** What task-queue-mcp and run-steward.sh accept. */ +export type WorkflowMode = 'semi-auto' | 'auto' | 'manual-then-auto'; + +/** + * The plugin's Start vocabulary is not the queue's. `review` is a UI word for "let me + * see it before it runs"; the queue's nearest value is `semi-auto`. Mapped explicitly + * rather than passed through: `review` is not in task-queue-mcp's VALID_WORKFLOW_MODES, + * and run-steward.sh would reject it by name. See vikunja#533 for the wider unification. + */ +export function toWorkflowMode(mode: StartMode): WorkflowMode { + return mode === 'review' ? 'semi-auto' : 'auto'; +} + +// ── argv construction ──────────────────────────────────────────────────────── + +export interface LaunchArgv { + argv: string[]; + /** Surfaced to the operator when the requested mode could not be honoured literally. */ + note?: string; +} + +/** + * Build the spawn argv for an agent, mirroring task-dispatcher.py's launch path. + * + * A run-as agent goes through its launcher, never through `claude` directly. Spawning + * `claude` as the plugin's own user for such an agent is the failure this replaces: it + * bypasses the launcher's identity guard, producing a session that appears as that + * agent in every log while holding none of its credentials (vikunja#404, #523). + */ +export function buildLaunchArgv( + entry: AgentLaunch, + mode: StartMode, + prompt: string, + claudeBin: string, +): LaunchArgv { + if (entry.runAsUser && entry.launcher) { + // -n: never prompt for a password. The sudoers grant is NOPASSWD, so a prompt + // would mean the grant is gone — fail immediately rather than hang on a tty + // that does not exist. + // --: everything after is the prompt, not a flag. + const argv = [ + 'sudo', '-n', '-u', entry.runAsUser, + entry.launcher, + '--workflow-mode', toWorkflowMode(mode), + '--', prompt, + ]; + // run-steward.sh sets --dangerously-skip-permissions itself and accepts no + // permission-mode, so `review` cannot be enforced by the CLI for a run-as agent + // the way it is for one launched directly. The review prompt still instructs the + // agent to stop and wait, but that is a prompt-level control, not a tool gate. + // Say so rather than implying a guarantee that is not there. + const note = mode === 'review' + ? 'review is prompt-enforced for this agent — its launcher does not accept a permission mode' + : undefined; + return { argv, note }; + } + + const permissionMode = mode === 'review' ? 'plan' : 'default'; + return { argv: [claudeBin, '-p', prompt, '--permission-mode', permissionMode] }; +} + +/** + * Launch-log filename. Shared with task-dispatcher.py, which writes the same shape into + * the same directory — previously it wrote ~/.pm2/logs/agent-launch--.log + * while this plugin wrote .log, so nothing could list "the launches". + */ +export function launchLogName(agent: string, taskId: string): string { + return `${agent}-${taskId.slice(0, 8)}.log`; +} diff --git a/src/panels/ws-client.ts b/src/panels/ws-client.ts index 4a891ed..d6dd7a3 100644 --- a/src/panels/ws-client.ts +++ b/src/panels/ws-client.ts @@ -10,6 +10,24 @@ export type WsEvent = { export type WsListener = (event: WsEvent) => void; +/** + * Reconnect backoff: 5s, 10s, 30s, then 30s forever. Reset to the first step on a + * successful open. + * + * A fixed 5s retry against a persistent outage is what produced 2239 identical + * "WS proxy error" lines in cloudcli-error.log over three weeks — enough noise to + * bury anything else in that log. + * + * Exported for tests: the schedule is the behaviour, and it is not observable + * through the client without waiting real seconds. + */ +export const RECONNECT_DELAYS_MS = [5000, 10000, 30000] as const; + +export function reconnectDelayMs(attempt: number): number { + const i = Math.min(Math.max(attempt, 0), RECONNECT_DELAYS_MS.length - 1); + return RECONNECT_DELAYS_MS[i]; +} + export interface WsClient { readonly connected: boolean; onEvent(listener: WsListener): () => void; @@ -22,6 +40,7 @@ export function createWsClient(): WsClient { let connected = false; let reconnectTimer: ReturnType | null = null; let closed = false; + let reconnectAttempt = 0; function getWsUrl(): string { const proto = location.protocol === 'https:' ? 'wss:' : 'ws:'; @@ -43,6 +62,7 @@ export function createWsClient(): WsClient { ws.onopen = () => { connected = true; + reconnectAttempt = 0; // a real connect resets the backoff emit({ type: '_connected' }); }; @@ -69,10 +89,12 @@ export function createWsClient(): WsClient { function scheduleReconnect(): void { if (closed || reconnectTimer) return; + const delay = reconnectDelayMs(reconnectAttempt); + reconnectAttempt += 1; reconnectTimer = setTimeout(() => { reconnectTimer = null; connect(); - }, 5000); + }, delay); } connect(); diff --git a/src/server.ts b/src/server.ts index fce2265..83d014e 100644 --- a/src/server.ts +++ b/src/server.ts @@ -6,6 +6,17 @@ import { spawn } from 'node:child_process'; import { WebSocketServer, WebSocket } from 'ws'; import { load as yamlLoad } from 'js-yaml'; import { callControlApi, type ControlAction } from './control-api.ts'; +import { evaluateUpgrade, allowedOrigins } from './ws-guard.ts'; +import { + loadLaunchPolicy, + policyPath, + buildLaunchArgv, + launchLogName, + lookupAgent, + LaunchPolicyError, + type LaunchPolicy, + type StartMode, +} from './launch-policy.ts'; // ── Constants ────────────────────────────────────────────────────────── @@ -34,14 +45,29 @@ const VALID_ID = /^[a-zA-Z0-9_-]+$/; const TASK_QUEUE_API = (process.env.TASK_QUEUE_API ?? 'http://127.0.0.1:8485').replace(/\/$/, ''); const TASK_QUEUE_API_SECRET = process.env.TASK_QUEUE_API_SECRET ?? ''; -// Agent-to-project mapping (matches forge task-dispatcher) -const AGENT_PROJECTS: Record = { - sysadmin: path.join(HOME, '.claude', 'projects', 'sysadmin'), - developer: path.join(HOME, '.claude', 'projects', 'developer'), - research: path.join(HOME, '.claude', 'projects', 'research'), - writer: path.join(HOME, '.claude', 'projects', 'writer'), - security: path.join(HOME, '.claude', 'projects', 'security'), -}; +// Agent launch policy — ONE roster, shared with task-dispatcher.py. See +// launch-policy.ts. The hardcoded AGENT_PROJECTS map that used to live here was a +// second copy of the dispatcher's table; it never gained a steward entry, which is +// exactly why Start could not launch steward (vikunja#523). Do not reintroduce it. +// +// Loaded once at startup rather than lazily, so a broken policy is reported when the +// plugin boots instead of on the operator's first Start. It is captured rather than +// thrown, because a throw here would take down the whole plugin — including the +// read-only task list, which needs no policy at all. Every launch then fails with the +// captured error by name; none of them silently falls back to spawning `claude`, +// which for a run-as agent would mean impersonating it. +const LAUNCH_POLICY: { policy: LaunchPolicy | null; error: string | null } = (() => { + const p = policyPath(process.env, HOME); + try { + return { policy: loadLaunchPolicy(p, HOME), error: null }; + } catch (err) { + const message = err instanceof LaunchPolicyError + ? err.message + : `launch policy ${p}: ${(err as Error).message}`; + process.stderr.write(`[task-queue] launch policy unusable — Start is disabled: ${message}\n`); + return { policy: null, error: message }; + } +})(); // ── Task operations (direct YAML file reader) ───────────────────────── @@ -139,6 +165,15 @@ function previewFile(filePath: string, lines = 20): string | null { const LAUNCH_LOG_DIR = path.join(HOME, '.claude', 'comms', 'artifacts', 'task-launches'); +function isExecutable(filePath: string): boolean { + try { + fs.accessSync(filePath, fs.constants.X_OK); + return true; + } catch { + return false; + } +} + // Resolve an executable on PATH (no external deps). Returns absolute path or null. function resolveBin(name: string): string | null { const dirs = (process.env.PATH ?? '').split(path.delimiter).filter(Boolean); @@ -152,23 +187,46 @@ function resolveBin(name: string): string | null { return null; } -function launchSession(taskId: string, targetAgent: string, mode: 'review' | 'auto'): { ok: boolean; error?: string } { +function launchSession(taskId: string, targetAgent: string, mode: StartMode): { ok: boolean; error?: string; note?: string } { if (!VALID_ID.test(taskId)) return { ok: false, error: `Invalid task id: ${taskId}` }; - const projectDir = AGENT_PROJECTS[targetAgent]; - if (!projectDir) return { ok: false, error: `Unknown agent: ${targetAgent}` }; + if (!LAUNCH_POLICY.policy) { + return { ok: false, error: `Launch policy unusable: ${LAUNCH_POLICY.error}` }; + } + + const entry = lookupAgent(LAUNCH_POLICY.policy, targetAgent); + if (!entry) return { ok: false, error: `Unknown agent: ${targetAgent}` }; - if (!fs.existsSync(projectDir)) return { ok: false, error: `Project dir missing: ${projectDir}` }; + if (!fs.existsSync(entry.projectDir)) { + return { ok: false, error: `Project dir missing: ${entry.projectDir}` }; + } + + // A run-as agent must go through its launcher; an absent or non-executable launcher + // is refused by name here rather than surfacing as an opaque sudo failure in the log. + // It must never degrade to spawning `claude` — for a run-as agent that would be + // impersonation, which is the whole of vikunja#523. + if (entry.launcher && !isExecutable(entry.launcher)) { + return { + ok: false, + error: `Launcher missing or not executable for run-as agent '${targetAgent}': ` + + `${entry.launcher} — deploy it with forge-scripts-deploy.sh`, + }; + } // Resolve the binary up front so a missing CLI is a clean 400, not a silent exit. - const claudeBin = resolveBin('claude'); - if (!claudeBin) return { ok: false, error: 'claude CLI not found on PATH' }; + // Only a directly-launched agent needs it; a run-as agent's launcher resolves its own. + let claudeBin = ''; + if (!entry.runAsUser) { + const resolved = resolveBin('claude'); + if (!resolved) return { ok: false, error: 'claude CLI not found on PATH' }; + claudeBin = resolved; + } const prompt = mode === 'review' ? `You have a pending task (id=${taskId}). Read it from task-queue-mcp via get_task. Present a summary of the work entailed. Do NOT begin execution — wait for operator approval.` : `You have a pending task (id=${taskId}). Read it from task-queue-mcp via get_task. Claim it (update status to in-progress), then execute the task.`; - const permissionMode = mode === 'review' ? 'plan' : 'default'; + const { argv, note } = buildLaunchArgv(entry, mode, prompt, claudeBin); // Per-launch log replaces stdio:'ignore' so a failed launch is diagnosable. // cwd:projectDir is how Claude Code resolves project config — `--project` is not a valid flag. @@ -176,18 +234,15 @@ function launchSession(taskId: string, targetAgent: string, mode: 'review' | 'au let logPath: string; try { fs.mkdirSync(LAUNCH_LOG_DIR, { recursive: true }); - logPath = path.join(LAUNCH_LOG_DIR, `${taskId}.log`); + logPath = path.join(LAUNCH_LOG_DIR, launchLogName(targetAgent, taskId)); logFd = fs.openSync(logPath, 'a'); } catch (err) { return { ok: false, error: `Cannot open launch log: ${(err as Error).message}` }; } try { - const child = spawn(claudeBin, [ - '-p', prompt, - '--permission-mode', permissionMode, - ], { - cwd: projectDir, + const child = spawn(argv[0], argv.slice(1), { + cwd: entry.projectDir, stdio: ['ignore', logFd, logFd], detached: true, }); @@ -197,7 +252,7 @@ function launchSession(taskId: string, targetAgent: string, mode: 'review' | 'au }); child.unref(); - return { ok: true }; + return note ? { ok: true, note } : { ok: true }; } catch (err) { return { ok: false, error: `spawn failed: ${(err as Error).message}` }; } finally { @@ -302,7 +357,7 @@ const server = http.createServer(async (req, res) => { const startMatch = pathname.match(/^\/tasks\/([a-zA-Z0-9_-]+)\/start$/); if (startMatch && req.method === 'POST') { const body = await readBody(req); - const { mode } = JSON.parse(body) as { mode: 'review' | 'auto' }; + const { mode } = JSON.parse(body) as { mode: StartMode }; const taskData = getTask(startMatch[1]); if (!taskData) { res.statusCode = 404; res.end(JSON.stringify({ error: 'task not found' })); return; } const result = launchSession(startMatch[1], taskData.target_agent ?? '', mode ?? 'review'); @@ -380,19 +435,18 @@ function readBody(req: http.IncomingMessage): Promise { }); } -// WebSocket upgrade +// WebSocket upgrade — see ws-guard.ts for the rule and why it is shaped this way. server.on('upgrade', (req, socket, head) => { - const origin = req.headers.origin ?? ''; - const allowed = [ - process.env.CLOUDCLI_ORIGIN ?? '', - 'http://localhost:3001', - 'http://127.0.0.1:3001', - ].filter(Boolean); - - // Reject a *missing* Origin as well as a wrong one. Browsers always send one, so the - // only clients that omit it are non-browser — exactly what this check exists to stop. - // Previously `origin && !allowed.includes(origin)` let them skip the check entirely. - if (!allowed.includes(origin)) { + const decision = evaluateUpgrade( + req.socket.remoteAddress, + req.headers.origin, + allowedOrigins(), + ); + + if (!decision.allow) { + // Log the reason. v0.4.0 refused every connect for three weeks and the only signal + // was a 403 on the far side of the proxy, naming neither leg nor the cause. + process.stderr.write(`[task-queue] ws upgrade refused — ${decision.reason}\n`); socket.write('HTTP/1.1 403 Forbidden\r\n\r\n'); socket.destroy(); return; diff --git a/src/tests/launch-policy.test.ts b/src/tests/launch-policy.test.ts new file mode 100644 index 0000000..e9bb606 --- /dev/null +++ b/src/tests/launch-policy.test.ts @@ -0,0 +1,276 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +import { + validateLaunchPolicy, + loadLaunchPolicy, + policyPath, + buildLaunchArgv, + launchLogName, + toWorkflowMode, + LaunchPolicyError, + lookupAgent, + type AgentLaunch, +} from '../launch-policy.ts'; + +// A synthetic HOME. Nothing here touches the real one, and no directory needs to +// exist — validation is about shape, not about the filesystem. +const HOME = '/home/testuser'; +const PROJECTS = `${HOME}/.claude/projects`; + +function doc(extra: Record = {}) { + return { developer: { project_dir: '~/.claude/projects/developer' }, ...extra }; +} + +const STEWARD = { + project_dir: '~/.claude/projects/steward', + run_as_user: 'agent-steward', + launcher: '/usr/local/sbin/forge/run-steward.sh', +}; + +function rejects(raw: unknown, label: string) { + assert.throws( + () => validateLaunchPolicy(raw, HOME), + LaunchPolicyError, + `${label} — must be rejected as a LaunchPolicyError`, + ); +} + +// ── The failure this whole module exists to prevent ───────────────────────── + +test('a malformed document throws — it never degrades to an empty policy', () => { + // An empty policy leaves runAsUser absent for every agent, and an absent runAsUser + // is how steward gets spawned as `claude` under the plugin's own user: a session + // that appears as steward in every log and holds none of its credentials. + for (const [raw, label] of [ + [null, 'null'], + [undefined, 'undefined'], + [{}, 'empty mapping'], + [[], 'a list'], + ['developer', 'a bare string'], + [42, 'a number'], + ] as Array<[unknown, string]>) { + rejects(raw, label); + } +}); + +test('one bad entry rejects the whole document, even alongside a valid steward', () => { + // A loader that skipped bad entries would pass every other test in this file: + // steward would launch correctly and sysadmin would silently vanish. + rejects({ steward: STEWARD, sysadmin: { project_dir: '/etc/passwd' } }, + 'valid steward + one bad entry'); +}); + +// ── project_dir ───────────────────────────────────────────────────────────── + +test('project_dir is constrained to ~/.claude/projects', () => { + rejects(doc({ x: { project_dir: '/tmp/evil' } }), 'outside the projects root'); + rejects(doc({ x: { project_dir: '~/.claude/projects/../../evil' } }), 'escaping via ..'); + rejects(doc({ x: { project_dir: 'relative/path' } }), 'relative'); + rejects(doc({ x: { project_dir: '' } }), 'empty'); + rejects(doc({ x: { project_dir: 42 } }), 'non-string'); + rejects({ developer: {} }, 'missing'); + // Must not pass a naive startsWith check. + rejects(doc({ x: { project_dir: '~/.claude/projectsX/evil' } }), 'sibling sharing the prefix'); +}); + +test('a valid project_dir expands ~ and comes back absolute', () => { + const p = validateLaunchPolicy(doc(), HOME); + assert.equal(p.developer.projectDir, `${PROJECTS}/developer`); + assert.equal(p.developer.runAsUser, null); + assert.equal(p.developer.launcher, null); +}); + +test('the projects root itself is accepted, not treated as an escape', () => { + const p = validateLaunchPolicy({ x: { project_dir: '~/.claude/projects' } }, HOME); + assert.equal(p.x.projectDir, PROJECTS); +}); + +// ── run_as_user / launcher ────────────────────────────────────────────────── + +test('run_as_user and launcher must be given together', () => { + rejects(doc({ x: { project_dir: '~/.claude/projects/steward', run_as_user: 'agent-steward' } }), + 'run_as_user alone'); + rejects(doc({ x: { project_dir: '~/.claude/projects/steward', launcher: '/usr/local/sbin/forge/run-steward.sh' } }), + 'launcher alone'); +}); + +test('run_as_user is constrained to agent-* — root and ted are refused', () => { + for (const user of ['root', 'ted', 'agent-steward; rm -rf /', 'AGENT-STEWARD', 'agent_steward', '', 'agent-']) { + rejects(doc({ x: { ...STEWARD, run_as_user: user } }), `run_as_user ${JSON.stringify(user)}`); + } +}); + +test('launcher is constrained to /usr/local/sbin/forge/', () => { + for (const l of [ + '/usr/bin/env', + '/usr/local/sbin/forge/../../../bin/sh', + 'run-steward.sh', + '/usr/local/sbin/forgery/run-steward.sh', + '', + ]) { + rejects(doc({ x: { ...STEWARD, launcher: l } }), `launcher ${JSON.stringify(l)}`); + } +}); + +test('a nonexistent but well-shaped launcher validates — existence is a launch-time check', () => { + // Validating existence here would make an undeployed launcher break the whole + // policy load, disabling Start for every other agent too. server.ts checks X_OK + // per launch and reports it by name. + const p = validateLaunchPolicy( + doc({ x: { ...STEWARD, launcher: '/usr/local/sbin/forge/does-not-exist.sh' } }), HOME); + assert.equal(p.x.launcher, '/usr/local/sbin/forge/does-not-exist.sh'); +}); + +// ── names and keys ────────────────────────────────────────────────────────── + +test('agent names and keys are constrained', () => { + for (const name of ['Developer', '9developer', 'dev eloper', '../developer', '', 'dev/eloper']) { + rejects({ [name]: { project_dir: '~/.claude/projects/developer' } }, `agent name ${JSON.stringify(name)}`); + } + rejects(doc({ x: { project_dir: '~/.claude/projects/developer', extra: 'x' } }), 'unknown key'); + rejects({ developer: 'not-a-mapping' }, 'entry is not a mapping'); + rejects({ developer: ['a'] }, 'entry is an array'); +}); + +// ── argv ──────────────────────────────────────────────────────────────────── + +test('a run-as agent produces a sudo argv through its launcher, never a claude argv', () => { + const steward: AgentLaunch = { + projectDir: `${PROJECTS}/steward`, + runAsUser: 'agent-steward', + launcher: '/usr/local/sbin/forge/run-steward.sh', + }; + const { argv } = buildLaunchArgv(steward, 'auto', 'do the thing', '/usr/bin/claude'); + assert.deepEqual(argv, [ + 'sudo', '-n', '-u', 'agent-steward', + '/usr/local/sbin/forge/run-steward.sh', + '--workflow-mode', 'auto', + '--', 'do the thing', + ]); + // The regression guard: the claude binary must not appear anywhere in a run-as argv. + assert.ok(!argv.includes('/usr/bin/claude'), 'claude must not be spawned for a run-as agent'); + assert.equal(argv[0], 'sudo'); +}); + +test('the prompt sits after -- so a prompt cannot be read as a launcher flag', () => { + const steward: AgentLaunch = { + projectDir: `${PROJECTS}/steward`, + runAsUser: 'agent-steward', + launcher: '/usr/local/sbin/forge/run-steward.sh', + }; + const { argv } = buildLaunchArgv(steward, 'auto', '--workflow-mode auto', '/usr/bin/claude'); + const sep = argv.indexOf('--'); + assert.ok(sep > 0, 'a -- separator must be present'); + assert.deepEqual(argv.slice(sep + 1), ['--workflow-mode auto']); +}); + +test('a normal agent produces a claude argv with the permission mode', () => { + const dev: AgentLaunch = { projectDir: `${PROJECTS}/developer`, runAsUser: null, launcher: null }; + assert.deepEqual( + buildLaunchArgv(dev, 'auto', 'go', '/usr/bin/claude').argv, + ['/usr/bin/claude', '-p', 'go', '--permission-mode', 'default'], + ); + assert.deepEqual( + buildLaunchArgv(dev, 'review', 'go', '/usr/bin/claude').argv, + ['/usr/bin/claude', '-p', 'go', '--permission-mode', 'plan'], + ); + assert.equal(buildLaunchArgv(dev, 'review', 'go', '/usr/bin/claude').note, undefined); +}); + +test('review on a run-as agent maps to semi-auto and says it is prompt-enforced', () => { + const steward: AgentLaunch = { + projectDir: `${PROJECTS}/steward`, + runAsUser: 'agent-steward', + launcher: '/usr/local/sbin/forge/run-steward.sh', + }; + const { argv, note } = buildLaunchArgv(steward, 'review', 'go', '/usr/bin/claude'); + assert.deepEqual(argv.slice(-4), ['--workflow-mode', 'semi-auto', '--', 'go']); + // run-steward.sh sets --dangerously-skip-permissions itself, so `plan` is not + // reachable for this agent. The operator must be told, not reassured. + assert.ok(note && /prompt-enforced/.test(note), 'a caveat must be returned for review'); + assert.ok(!argv.includes('--permission-mode'), 'the launcher accepts no permission mode'); +}); + +// ── mode vocabulary ───────────────────────────────────────────────────────── + +test('the Start vocabulary maps onto the queue vocabulary, never passing review through', () => { + assert.equal(toWorkflowMode('review'), 'semi-auto'); + assert.equal(toWorkflowMode('auto'), 'auto'); + // `review` is not a value task-queue-mcp or run-steward.sh accepts. Passing it + // through would be rejected by name at the launcher. + const VALID = new Set(['semi-auto', 'auto', 'manual-then-auto']); + for (const m of ['review', 'auto'] as const) { + assert.ok(VALID.has(toWorkflowMode(m)), `${m} must map into the queue vocabulary`); + } +}); + +// ── log naming ────────────────────────────────────────────────────────────── + +test('launch log names match the dispatcher: -.log', () => { + assert.equal( + launchLogName('steward', 'e17c99ee-d727-442a-80d3-2ca1a1c751d0'), + 'steward-e17c99ee.log', + ); + // Short ids must not throw or pad. + assert.equal(launchLogName('developer', 'abc'), 'developer-abc.log'); +}); + +// ── path resolution ───────────────────────────────────────────────────────── + +test('policyPath defaults beside the dispatcher and honours the env override', () => { + assert.equal(policyPath({} as NodeJS.ProcessEnv, HOME), `${HOME}/scripts/agent-launch.yml`); + assert.equal( + policyPath({ AGENT_LAUNCH_POLICY: '/tmp/other.yml' } as NodeJS.ProcessEnv, HOME), + '/tmp/other.yml', + ); +}); + +// ── file loading ──────────────────────────────────────────────────────────── + +test('loadLaunchPolicy throws by name on a missing or unparseable file', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'tq-policy-')); + try { + assert.throws( + () => loadLaunchPolicy(path.join(dir, 'nope.yml'), HOME), + (e: Error) => e instanceof LaunchPolicyError && /cannot read launch policy/.test(e.message), + ); + + const bad = path.join(dir, 'bad.yml'); + fs.writeFileSync(bad, 'developer: {project_dir: [unclosed\n'); + assert.throws( + () => loadLaunchPolicy(bad, HOME), + (e: Error) => e instanceof LaunchPolicyError && /cannot parse launch policy/.test(e.message), + ); + + const ok = path.join(dir, 'ok.yml'); + fs.writeFileSync(ok, 'steward:\n project_dir: ~/.claude/projects/steward\n' + + ' run_as_user: agent-steward\n launcher: /usr/local/sbin/forge/run-steward.sh\n'); + const policy = loadLaunchPolicy(ok, HOME); + assert.equal(policy.steward.runAsUser, 'agent-steward'); + assert.equal(policy.steward.projectDir, `${PROJECTS}/steward`); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +// ── IV-01: target_agent comes from a queue YAML, not from the policy ──────── + +test('a prototype-chain name does not resolve as a policy entry', () => { + // `policy['constructor']` on a plain object is Object itself — truthy, so a bare + // `if (!entry)` guard would accept it and carry a non-entry into the launch path. + const policy = validateLaunchPolicy(doc({ steward: STEWARD }), HOME); + for (const name of ['constructor', '__proto__', 'toString', 'valueOf', 'hasOwnProperty']) { + assert.equal(lookupAgent(policy, name), null, `${name} must not resolve`); + } + assert.notEqual(lookupAgent(policy, 'developer'), null, 'a real agent still resolves'); + assert.equal(lookupAgent(policy, 'steward')?.runAsUser, 'agent-steward'); +}); + +test('the policy object itself carries no prototype', () => { + const policy = validateLaunchPolicy(doc(), HOME); + assert.equal(Object.getPrototypeOf(policy), null); +}); diff --git a/src/tests/ws-client.test.ts b/src/tests/ws-client.test.ts new file mode 100644 index 0000000..8671a91 --- /dev/null +++ b/src/tests/ws-client.test.ts @@ -0,0 +1,36 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { reconnectDelayMs, RECONNECT_DELAYS_MS } from '../panels/ws-client.ts'; + +test('reconnect backoff widens 5s -> 10s -> 30s and caps there', () => { + assert.equal(reconnectDelayMs(0), 5000); + assert.equal(reconnectDelayMs(1), 10000); + assert.equal(reconnectDelayMs(2), 30000); + // Caps rather than growing or wrapping. A fixed 5s retry against a three-week + // outage produced 2239 identical error lines; an unbounded one would eventually + // stop retrying at all in practice. + for (const attempt of [3, 10, 1000]) { + assert.equal(reconnectDelayMs(attempt), 30000, `attempt ${attempt}`); + } +}); + +test('the schedule is monotonically non-decreasing and never zero', () => { + let prev = 0; + for (let i = 0; i < 10; i++) { + const d = reconnectDelayMs(i); + assert.ok(d > 0, `delay at ${i} must be positive`); + assert.ok(d >= prev, `delay must not shrink at attempt ${i}`); + prev = d; + } +}); + +test('a negative attempt clamps to the first step rather than indexing out', () => { + assert.equal(reconnectDelayMs(-1), 5000); +}); + +test('the first delay matches the documented 5s so behaviour is unchanged for a blip', () => { + // A transient disconnect must still recover as fast as it did before the backoff + // was added — the backoff is for persistent outages only. + assert.equal(RECONNECT_DELAYS_MS[0], 5000); +}); diff --git a/src/tests/ws-guard.test.ts b/src/tests/ws-guard.test.ts new file mode 100644 index 0000000..0047517 --- /dev/null +++ b/src/tests/ws-guard.test.ts @@ -0,0 +1,85 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { evaluateUpgrade, allowedOrigins } from '../ws-guard.ts'; + +const ALLOWED = ['http://cloudcli.example:3001', 'http://localhost:3001', 'http://127.0.0.1:3001']; + +// ── The regression this suite exists for ───────────────────────────────── +// v0.4.0 rejected a loopback peer that sent no Origin. That is exactly what +// CloudCLI's own plugin WS proxy sends (the `ws` client omits Origin by +// default), so every connect was 403'd for three weeks. If this test ever +// goes red, the tab has gone dead again. + +test('loopback peer with no Origin is accepted — this is CloudCLI\'s own proxy', () => { + for (const peer of ['127.0.0.1', '::1', '::ffff:127.0.0.1']) { + const d = evaluateUpgrade(peer, undefined, ALLOWED); + assert.equal(d.allow, true, `peer ${peer} must be accepted with no Origin`); + } +}); + +test('loopback peer with an empty-string Origin is accepted', () => { + // Node gives undefined for an absent header, but a header present-and-empty + // must not be treated as a mismatch against a non-empty allowlist. + assert.equal(evaluateUpgrade('127.0.0.1', '', ALLOWED).allow, true); +}); + +test('loopback peer with an allowed Origin is accepted', () => { + for (const origin of ALLOWED) { + assert.equal(evaluateUpgrade('127.0.0.1', origin, ALLOWED).allow, true, origin); + } +}); + +test('loopback peer with a present-but-wrong Origin is refused', () => { + const d = evaluateUpgrade('127.0.0.1', 'http://evil.example', ALLOWED); + assert.equal(d.allow, false); + assert.match(d.reason ?? '', /origin not allowed/); +}); + +test('a wrong Origin is refused even when it is a prefix of an allowed one', () => { + // Guards against a startsWith/includes-style relaxation. + const d = evaluateUpgrade('127.0.0.1', 'http://localhost:3001.evil.example', ALLOWED); + assert.equal(d.allow, false); +}); + +test('non-loopback peer is refused outright, even with a good Origin', () => { + for (const peer of ['192.168.1.50', '10.0.0.1', '::ffff:192.168.1.50', '2001:db8::1']) { + const d = evaluateUpgrade(peer, 'http://localhost:3001', ALLOWED); + assert.equal(d.allow, false, `peer ${peer} must be refused`); + assert.match(d.reason ?? '', /non-loopback peer/); + } +}); + +test('non-loopback peer with no Origin is refused', () => { + // The peer check must run first — otherwise the missing-Origin acceptance + // above would hand a remote client an open socket. + const d = evaluateUpgrade('192.168.1.50', undefined, ALLOWED); + assert.equal(d.allow, false); + assert.match(d.reason ?? '', /non-loopback peer/); +}); + +test('an unknown peer address is refused', () => { + // req.socket.remoteAddress is optional in Node's types; a destroyed socket + // reports undefined. Fail closed. + const d = evaluateUpgrade(undefined, undefined, ALLOWED); + assert.equal(d.allow, false); + assert.match(d.reason ?? '', /non-loopback peer/); +}); + +test('allowedOrigins includes CLOUDCLI_ORIGIN when set and drops it when not', () => { + assert.deepEqual( + allowedOrigins({ CLOUDCLI_ORIGIN: 'http://forge.example:3001' } as NodeJS.ProcessEnv), + ['http://forge.example:3001', 'http://localhost:3001', 'http://127.0.0.1:3001'], + ); + assert.deepEqual( + allowedOrigins({} as NodeJS.ProcessEnv), + ['http://localhost:3001', 'http://127.0.0.1:3001'], + ); + // An empty value must not enter the list — '' would then match a header + // that is present and empty on a *non*-loopback... and more practically it + // is a meaningless allowlist entry. + assert.deepEqual( + allowedOrigins({ CLOUDCLI_ORIGIN: '' } as NodeJS.ProcessEnv), + ['http://localhost:3001', 'http://127.0.0.1:3001'], + ); +}); diff --git a/src/ws-guard.ts b/src/ws-guard.ts new file mode 100644 index 0000000..e4984c1 --- /dev/null +++ b/src/ws-guard.ts @@ -0,0 +1,55 @@ +/** + * WebSocket upgrade authorization. + * + * Extracted from server.ts for the same reason control-api.ts was: server.ts calls + * server.listen() at import time, so a test that imports it boots a real listener. + * The decision is a pure function of (peer address, Origin header, allowlist). + */ + +// req.socket.remoteAddress spellings for a loopback peer. Node reports the +// IPv4-mapped form on a dual-stack socket, so all three are the same peer. +const LOOPBACK_PEERS = new Set(['127.0.0.1', '::1', '::ffff:127.0.0.1']); + +export interface UpgradeDecision { + allow: boolean; + /** Populated only when allow is false — logged, never sent to the client. */ + reason?: string; +} + +/** + * The Origin values a browser leg may present. CLOUDCLI_ORIGIN is how the operator + * actually reaches CloudCLI; the two literals are the local-dev defaults. + */ +export function allowedOrigins(env: NodeJS.ProcessEnv = process.env): string[] { + return [ + env.CLOUDCLI_ORIGIN ?? '', + 'http://localhost:3001', + 'http://127.0.0.1:3001', + ].filter(Boolean); +} + +/** + * Gate on the peer, then on the Origin if one is present. + * + * The peer check is the real boundary: the server binds 127.0.0.1 on an ephemeral + * port, so a non-loopback peer cannot legitimately occur and is refused outright. + * + * A loopback peer with NO Origin is CloudCLI's own plugin WS proxy — it uses the + * `ws` client library, which sends no Origin header unless one is passed explicitly, + * and that leg is already authenticated by CloudCLI's verifyClient before the proxy + * is invoked. A loopback peer WITH an Origin is a browser reaching the port directly + * and must still match the allowlist. + */ +export function evaluateUpgrade( + peer: string | undefined, + origin: string | undefined, + allowed: string[], +): UpgradeDecision { + if (!peer || !LOOPBACK_PEERS.has(peer)) { + return { allow: false, reason: `non-loopback peer: ${peer ?? ''}` }; + } + if (origin && !allowed.includes(origin)) { + return { allow: false, reason: `origin not allowed: ${origin}` }; + } + return { allow: true }; +} From fef1de4fd8145cfd74c0c66e44a00b120f877a6e Mon Sep 17 00:00:00 2001 From: developer-agent Date: Thu, 27 Aug 2026 10:49:54 -0400 Subject: [PATCH 2/2] security: document that neither validator resolves symlinks (audit Low) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit of task-queue-plugin-repair-2026-08 found the two hand-written launch-policy validators computed their containment root differently: Python called .resolve() on it, this side uses a plain path join. Neither resolves the CANDIDATE project_dir, so resolving only the root compares a canonical path against an uncanonical one. The fix is on the Python side (the .resolve() is removed there). What changes here is the comment: the plain join is now stated as a rule the other implementation must match, rather than being an unexplained coincidence that the next edit could break. Reproduced before fixing rather than taken on trust — with ~/.claude/projects made a symlink, the same document was REJECTED by Python and ACCEPTED here. After the fix both return the identical unresolved path. Co-Authored-By: Claude Opus 5 (1M context) agent-id: developer --- CHANGELOG.md | 6 ++++++ src/launch-policy.ts | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68b340d..35acc91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,12 @@ to a built-in roster, because falling back is what this release removes. **prompt-enforced only**: `run-steward.sh` sets `--dangerously-skip-permissions` itself and accepts no permission mode, so `--permission-mode plan` is not reachable. The toast says so rather than implying a tool gate that is not there. +- **Symlink handling in the launch-policy validator is now identical to the dispatcher's** + (security audit, Low). Neither side resolves symlinks — not in the containment root and + not in the candidate `project_dir`. The Python side used to `.resolve()` its root while + this side used a plain join, so with a symlink anywhere on the path the two disagreed + about the same input. Documented in both files as a rule rather than left as an accident + of two independent implementations. - A refused WebSocket upgrade now logs why. v0.4.0's refusals were silent on this side; the only signal was a 403 on the far side of the proxy, naming neither leg nor the cause. diff --git a/src/launch-policy.ts b/src/launch-policy.ts index 2bb92da..5dfd59a 100644 --- a/src/launch-policy.ts +++ b/src/launch-policy.ts @@ -75,6 +75,13 @@ function isUnder(root: string, child: string): boolean { } export function validateLaunchPolicy(raw: unknown, home: string): LaunchPolicy { + // Plain join, no symlink resolution — and the Python side's validate_launch_policy() + // must keep computing this the same way. It used to call .resolve() here while this + // side did not, so with a symlink anywhere on the path the two disagreed: Python + // rejected entries this accepted. Neither side resolves the CANDIDATE project_dir + // either (it may not exist yet), so resolving only the root compares a canonical path + // against an uncanonical one, which is not a comparison at all. + // Found by the security audit of task-queue-plugin-repair-2026-08. const projectRoot = path.join(home, '.claude', 'projects'); if (raw === null || typeof raw !== 'object' || Array.isArray(raw) || Object.keys(raw).length === 0) {