From 51d0f04ad9b362c84eb06be350c3f1ca657bac17 Mon Sep 17 00:00:00 2001 From: developer-agent Date: Sat, 29 Aug 2026 16:11:39 -0400 Subject: [PATCH] Gate the launch policy against the dispatcher's; guard a Start's credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Build plan agent-workflow-interop-2026-08, Phase 5.5 and 5.6. 5.5 — npm run gate:corpus, its own CI step. task-dispatcher owns a 27-case accept/reject corpus for the launch policy; this side fetches it from that repo's main and asserts validateLaunchPolicy() agrees on every case. launch-policy.ts carried a comment saying the Python side "must keep computing this the same way" — this is that comment as a test. It compares RESOLVED VALUES, not just verdicts, and that immediately paid for itself: Node's path.normalize keeps a trailing separator where Python's os.path.normpath strips it, so a project_dir written with a trailing slash was ACCEPTED by both sides and resolved to two different strings — one of which becomes a spawned session's cwd. No verdict ever disagreed. That is the .resolve() divergence in a second costume, and the fix is stripTrailingSep(). Separate from gate:vocabulary because it reads a different upstream: one red means "edit vocabulary.ts", the other means "edit launch-policy.ts". Sharing a step would let either hide the other. 5.6 — src/launch-guards.ts refuses a Start that cannot authenticate, by name, instead of spawning a session that 401s from every scoped-mcp tool or short-circuits to "Not logged in" before reading its prompt. Both present to the operator as an agent that started and did nothing. The port is not just the two checks. The dispatcher layers /opt/appdata/agents//.env into the child env and this plugin did not; measured on forge, the CloudCLI process env has CLAUDE_CODE_OAUTH_TOKEN but no SCOPED_MCP_BEARER_TOKEN. A guard reading only process.env would have refused every non-run-as Start — correctly, in that those sessions really were starting without scoped-mcp tools, but that is the bug and not the fix. spawn() now uses the environment that was checked; checking one object and spawning with another would make the guard decoration. BOTH CHECKS ARE SKIPPED FOR A RUN-AS AGENT, DELIBERATELY. Those credentials are absent from this process by design — the launcher sources them as the target user and makes the equivalent checks itself. Running them here would fail every launch for the one agent whose isolation is working correctly. Two tests pin that asymmetry so a later "fix" cannot quietly remove it. 151 -> 175 tests. Ten mutants covering every new branch each confirmed to turn the suite or gate:corpus red. One survived the first run: the agent-name traversal test pointed at a path that did not exist, so readFileSync threw and the function returned {} whether or not the name check ran — the right outcome for the wrong reason. The target file is now planted, so deleting the guard changes the result rather than the route to it. Co-Authored-By: Claude Opus 5 (1M context) agent-id: developer --- .github/workflows/ci.yml | 11 ++ CHANGELOG.md | 62 ++++++++ README.md | 51 ++++++- manifest.json | 4 +- package.json | 5 +- src/gates/launch-policy-corpus.ts | 236 +++++++++++++++++++++++++++++ src/launch-guards.ts | 183 +++++++++++++++++++++++ src/launch-policy.ts | 20 ++- src/server.ts | 22 ++- src/tests/launch-guards.test.ts | 241 ++++++++++++++++++++++++++++++ 10 files changed, 826 insertions(+), 9 deletions(-) create mode 100644 src/gates/launch-policy-corpus.ts create mode 100644 src/launch-guards.ts create mode 100644 src/tests/launch-guards.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1d48a6a..4599d60 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,6 +45,17 @@ jobs: - name: Vocabulary parity with task-queue-mcp main run: npm run gate:vocabulary + # Its own step for the same reason, and separate from the vocabulary gate because + # it reads a DIFFERENT upstream: this one goes red when task-dispatcher's launch + # policy corpus changes, which is a different instruction again ("go make + # launch-policy.ts match"). One step per upstream, so which one moved is legible + # from the job list rather than from a log dive. + # + # Do NOT set TASK_DISPATCHER_REF here — it defaults to `main`, which is what the + # dispatcher is actually running. + - name: Launch policy parity with task-dispatcher main + run: npm run gate:corpus + # Production dependencies only. The dev tree carries advisories that do not # apply to a plugin bundled at build time and never served by a dev server. - name: Audit production dependencies diff --git a/CHANGELOG.md b/CHANGELOG.md index 427b711..0747138 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,68 @@ All notable changes to this project will be documented in this file. +## [0.10.0] - 2026-08-29 + +Tracker: vikunja#560. Build plan: agent-workflow-interop-2026-08, Phase 5.5 and 5.6. + +### Fixed + +- **A trailing slash in `project_dir` resolved differently here than in the dispatcher.** + Node's `path.normalize` keeps a trailing separator (`/a/b/` → `/a/b/`) where Python's + `os.path.normpath` strips it (`/a/b/` → `/a/b`). Both sides ACCEPTED the entry, so no + verdict ever disagreed — only the resolved value did, and that value becomes a spawned + session's working directory. Found by the new corpus gate on its first run, not by + review; it is the `.resolve()` divergence in a second costume. + +### Added + +- **`npm run gate:corpus`** — a second parity gate, in its own CI step. task-dispatcher + owns `tests/fixtures/launch-policy-corpus.json` (27 accept/reject cases) and this + plugin fetches it from that repo's `main`, asserting `validateLaunchPolicy()` agrees on + every case. `launch-policy.ts` carried a comment saying the Python side "must keep + computing this the same way"; this is that comment as a test. + + **Resolved values are compared, not just verdicts.** A verdict-only comparison would + have reported the two implementations in agreement throughout both divergences found so + far. Like `gate:vocabulary` it has no skip-on-no-network path, and it refuses an empty + corpus rather than reporting a vacuous pass. + + It is a separate step from `gate:vocabulary` because it reads a different upstream — + one red means "edit `src/vocabulary.ts`", the other means "edit `src/launch-policy.ts`". + +- **Pre-launch credential guards (`src/launch-guards.ts`).** A Start of a + directly-launched agent is now refused, by name, when `SCOPED_MCP_BEARER_TOKEN` is + unresolved or no usable Anthropic credential is available — the two checks + task-dispatcher has always made. Previously such a Start spawned a session doomed to + 401 from every scoped-mcp tool, or to short-circuit to "Not logged in" before reading + the prompt; both present to the operator as an agent that started and did nothing. + + The port includes the dispatcher's `load_agent_env`: `/opt/appdata/agents//.env` + is layered into the child environment, which this plugin never did. That is the + substance of the fix. Measured on forge: the CloudCLI process env carries + `CLAUDE_CODE_OAUTH_TOKEN` but no `SCOPED_MCP_BEARER_TOKEN`, so a guard checking only + `process.env` would have refused every non-run-as Start — correctly, in that those + sessions really were starting without scoped-mcp tools, but that is the bug rather than + the fix. The environment that is checked is the one the child is spawned with; passing + `process.env` on to `spawn` after checking a different object would make the guard + decoration. + + **Both checks are skipped for a `run_as_user` agent, deliberately.** Those credentials + are not in this process's environment by design — the launcher sources them as the + target user and makes the equivalent checks itself. Running them here would fail every + launch for the one agent whose isolation is working correctly. + +### Tests + +151 → 175. Ten mutants covering every new branch were each confirmed to turn the suite (or +`gate:corpus`) red, including "the run-as short-circuit is removed" and "the run-as path +reads the agent env file it must not touch". + +One of them survived the first run: the agent-name traversal test pointed at a path that +did not exist, so `readFileSync` threw and the function returned `{}` whether or not the +name check ran. It asserted the right outcome for the wrong reason. The target file is now +planted, so removing the guard changes the result rather than the route to it. + ## [0.9.0] - 2026-08-29 Reads the run records both launchers now write, and makes a Start leave a mark on the task diff --git a/README.md b/README.md index 2aa83ff..0c1b8ad 100644 --- a/README.md +++ b/README.md @@ -254,6 +254,44 @@ deployment, a cron dispatcher reads the same file). A second copy of this roster this release removes: the plugin's private map had drifted and was missing an agent entirely, so Start refused it. +#### Two validators, one corpus + +`~/scripts/agent-launch.yml` is validated independently by this plugin and by the cron +dispatcher, in two languages, with no shared code. They have already disagreed: one +resolved symlinks on the project root and the other did not, so an entry accepted here was +rejected there — and on the reference deployment that did not merely reject an entry, it +made the dispatcher fail to import on every tick. + +`npm run gate:corpus` closes that. task-dispatcher owns +`tests/fixtures/launch-policy-corpus.json`, a set of accept/reject cases; this plugin +fetches it from that repo's `main` and asserts its own validator agrees on every one. + +It compares **resolved values**, not just verdicts, and that is not belt-and-braces. Its +first run found a second live divergence: Node's `path.normalize` keeps a trailing +separator where Python's `os.path.normpath` strips it, so a `project_dir` written with a +trailing slash was accepted by both sides and resolved to two different strings — one of +which becomes a spawned session's working directory. No verdict ever disagreed. + +#### Pre-launch credential guards + +A Start of a directly-launched agent is refused, by name, if `SCOPED_MCP_BEARER_TOKEN` is +unresolved or no usable Anthropic credential is available. Without this, such a session +spawns and then fails deep inside — a 401 from every scoped-mcp tool, or a `claude -p` +that short-circuits to "Not logged in" before it reads the prompt. From the operator's +side both look like an agent that started and did nothing. + +The plugin also layers `/opt/appdata/agents//.env` into the child environment, +which the dispatcher has always done and this plugin did not. That is the substance of the +fix rather than a side effect: on the reference deployment the plugin's own process +carries no `SCOPED_MCP_BEARER_TOKEN`, so directly-launched sessions were genuinely +starting without one. + +**Neither guard runs for a `run_as_user` agent, and that asymmetry is deliberate.** Such +an agent's credentials are not in this process's environment by design — they are in a +file only the target user can read, sourced by the launcher as that user, which performs +the equivalent checks itself. Running these checks on that path would fail every launch +for the one agent whose isolation is working correctly. + **`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 @@ -310,11 +348,18 @@ then, and reporting the launch as failed would be the bigger lie. npm install npm run build # tsc --noEmit (typecheck) + esbuild bundle to dist/ npm test # node --test — requires Node 22.18+ -npm run gate:vocabulary # asserts the queue vocabulary matches task-queue-mcp's main. - # Reaches the network and fails if it cannot — deliberately; - # a parity check that skips offline has verified nothing. +npm run gate:vocabulary # asserts the queue vocabulary matches task-queue-mcp's main +npm run gate:corpus # asserts the launch-policy validator agrees with + # task-dispatcher's, over a corpus that repo owns ``` +Both gates reach the network and fail if they cannot — deliberately; a parity check that +skips offline has verified nothing. They are **separate** npm scripts and separate CI +steps because they read different upstreams: a red from one means "edit +`src/vocabulary.ts`" and a red from the other means "edit `src/launch-policy.ts`". Folding +them together would let either hide the other, and would make "which upstream moved" a log +dive. + `npm run build` is the typecheck gate — `tsc --noEmit` runs first and the bundle only happens if it passes. The test runner executes the `.ts` files directly using Node's built-in type stripping, so **`npm test` needs Node 22.18+** even though the plugin itself runs on Node 20+ (`dist/` is bundled plain JS). diff --git a/manifest.json b/manifest.json index 390af0d..48d55df 100644 --- a/manifest.json +++ b/manifest.json @@ -1,8 +1,8 @@ { "name": "task-queue", "displayName": "Task Queue", - "version": "0.9.0", - "description": "Task queue dashboard — view, filter, and launch agent tasks.", + "version": "0.10.0", + "description": "Task queue dashboard \u2014 view, filter, and launch agent tasks.", "author": "TadMSTR", "icon": "icon.svg", "type": "module", diff --git a/package.json b/package.json index 4ad159c..6f94df9 100644 --- a/package.json +++ b/package.json @@ -1,12 +1,13 @@ { "name": "cloudcli-plugin-task-queue", - "version": "0.9.0", + "version": "0.10.0", "private": true, "type": "module", "scripts": { "build": "tsc --noEmit && esbuild src/index.ts --bundle --format=esm --outfile=dist/index.js --sourcemap && esbuild src/server.ts --bundle --format=esm --platform=node --outfile=dist/server.js --sourcemap --external:ws", "test": "node --test src/tests/*.test.ts", - "gate:vocabulary": "node src/gates/vocabulary-parity.ts" + "gate:vocabulary": "node src/gates/vocabulary-parity.ts", + "gate:corpus": "node src/gates/launch-policy-corpus.ts" }, "devDependencies": { "@types/js-yaml": "^4.0.9", diff --git a/src/gates/launch-policy-corpus.ts b/src/gates/launch-policy-corpus.ts new file mode 100644 index 0000000..c6c92d4 --- /dev/null +++ b/src/gates/launch-policy-corpus.ts @@ -0,0 +1,236 @@ +/** + * Assert this plugin's validateLaunchPolicy() agrees with task-dispatcher's, case for case. + * + * WHY THIS EXISTS (plan agent-workflow-interop-2026-08, Phase 5.5) + * + * `~/scripts/agent-launch.yml` has two independent validators — the dispatcher's Python + * `validate_launch_policy()` and this repo's `validateLaunchPolicy()`. Both feed values into + * a subprocess spawn, and neither shares a line of code with the other. `launch-policy.ts` + * carried a comment stating that the Python side "must keep computing this the same way", + * which is a comment where a test should be. + * + * They have already disagreed. Python called `.resolve()` on the project root while this + * side did a plain join, so with a symlink anywhere on the path the two diverged — and + * because `LAUNCH_POLICY = load_launch_policy()` runs at import, the effect on forge was not + * a rejected entry but the dispatcher module failing to import on every tick. + * + * WHY IT COMPARES RESOLVED VALUES, NOT JUST VERDICTS + * + * A verdict-only comparison would have reported these two implementations in perfect + * agreement throughout that bug, and through a second one this gate found on its first run: + * Node's `path.normalize` keeps a trailing separator while Python's `os.path.normpath` + * strips it, so `~/.claude/projects/writer/` was ACCEPTED by both sides and resolved to two + * different strings — one of which becomes a spawn's `cwd`. Verdicts are the cheap half of + * the contract. + * + * DIRECTION AND MECHANISM + * + * task-dispatcher owns the corpus; this side fetches it from that repo's `main` over HTTPS. + * Same direction and mechanism as `vocabulary-parity.ts`, so this fleet has one pattern for + * cross-repo contracts rather than two. Nothing fetched is executed — it is parsed as JSON + * and its `policy` values are fed to a pure function. + * + * DELIBERATELY NOT SKIPPABLE + * + * There is no "no network, skip" path and no vendored copy. A parity check that quietly + * passes when it could not read the upstream is indistinguishable from one that verified + * something. Nor does it tolerate an empty case list: zero cases is the fetch or the parse + * being broken, not the two validators agreeing, and reporting a vacuous pass over an empty + * comparison is the same failure wearing a green tick. + * + * CONSEQUENCE WORTH KNOWING: this tracks task-dispatcher's `main`. A case added there turns + * this repo's CI red on the next push if the two sides genuinely differ. That is the alarm. + * Fix `launch-policy.ts` — do not edit this gate, and do not delete a case upstream. + * + * Run: `npm run gate:corpus`. Its OWN CI step, not part of `npm test`: a red here means "go + * make this validator match the dispatcher's", which is a different instruction from any + * unit failure, and folding the two together lets either hide the other. + */ + +import { validateLaunchPolicy, LaunchPolicyError } from '../launch-policy.ts'; + +// `main` is the contract — it is what the dispatcher is running. The override exists for +// the one situation that recurs whenever the corpus changes: a paired change lands in two +// repos and this one is legitimately ahead of task-dispatcher's `main` until its PR merges. +// +// CI MUST NOT SET THIS. It defaults to `main` precisely so an unset environment gets the +// strict check. +const UPSTREAM_REF = process.env.TASK_DISPATCHER_REF ?? 'main'; + +// The ref is interpolated into a URL path, so constrain it. `..` would traverse out of this +// repo's path segment on raw.githubusercontent.com and fetch somebody else's corpus — which +// this gate would then treat as authoritative. +if (!/^[A-Za-z0-9._][A-Za-z0-9._/-]*$/.test(UPSTREAM_REF) || UPSTREAM_REF.includes('..')) { + console.error(`FATAL: refusing TASK_DISPATCHER_REF=${JSON.stringify(UPSTREAM_REF)} — not a plain git ref`); + process.exit(2); +} + +const UPSTREAM_URL = + `https://raw.githubusercontent.com/TadMSTR/task-dispatcher/${UPSTREAM_REF}` + + '/tests/fixtures/launch-policy-corpus.json'; + +// A synthetic home. Nothing here touches the real one and no directory needs to exist — +// validation is about shape, not about the filesystem. The corpus writes `{HOME}` rather +// than a literal path because each side substitutes its OWN home: Python expands `~` +// against the ambient $HOME while this side expands it against this argument, so a +// hardcoded path would make the two incomparable for exactly the `~` cases. +const HOME = '/home/testuser'; + +interface CorpusCase { + name: string; + why?: string; + policy: unknown; + expect: 'accept' | 'reject'; + resolved?: Record; +} + +const failures: string[] = []; + +function check(cond: boolean, label: string): void { + console.log(cond ? ` ok ${label}` : ` FAIL ${label}`); + if (!cond) failures.push(label); +} + +/** Substitute {HOME} through a nested structure, leaving non-strings alone. */ +function sub(value: unknown): unknown { + if (typeof value === 'string') return value.split('{HOME}').join(HOME); + if (Array.isArray(value)) return value.map(sub); + if (value !== null && typeof value === 'object') { + // Object.entries, then a fresh object: a corpus case deliberately contains + // `__proto__` as an agent name, and a spread or an index assignment onto `{}` would + // either drop it or mutate the prototype instead of setting a key. Object.create(null) + // has no prototype to pollute and keeps the key where the validator can reject it. + const out = Object.create(null) as Record; + for (const [k, v] of Object.entries(value as Record)) { + Object.defineProperty(out, k, { value: sub(v), enumerable: true, writable: true, configurable: true }); + } + return out; + } + return value; +} + +async function fetchCorpus(): Promise { + let text: string; + try { + // SECURITY[accepted]: no `redirect: "manual"`, deviating from baseline pattern SSRF-02 — + // the same accepted deviation as vocabulary-parity.ts, for the same reasons. Host and + // path are literals, only the ref segment varies and it is charset-checked and + // `..`-rejected above, and the response is parsed as JSON and never executed. Worst case + // from a hostile redirect is a false red (loud, blocks CI) or a false green requiring the + // attacker to serve a corpus this validator already satisfies, which achieves nothing. + const resp = await fetch(UPSTREAM_URL, { signal: AbortSignal.timeout(30_000) }); + if (!resp.ok) throw new Error(`HTTP ${resp.status}`); + text = await resp.text(); + } catch (err) { + console.error(`FATAL: could not read the launch-policy corpus at ${UPSTREAM_URL}`); + console.error(` ${(err as Error).name}: ${(err as Error).message}`); + console.error( + ' This is a hard failure on purpose — see the header. A parity check that skips ' + + 'when it cannot read the upstream reports the same result whether or not the two ' + + 'validators agree.', + ); + process.exit(2); + } + + let parsed: { cases?: CorpusCase[] }; + try { + parsed = JSON.parse(text) as { cases?: CorpusCase[] }; + } catch (err) { + console.error(`FATAL: the corpus at ${UPSTREAM_URL} is not valid JSON: ${(err as Error).message}`); + process.exit(2); + } + + const cases = parsed.cases ?? []; + if (cases.length === 0) { + console.error( + `FATAL: no cases parsed from ${UPSTREAM_URL} — the fetch or the shape is broken, not ` + + 'the validators. Failing rather than reporting a vacuous pass over an empty corpus.', + ); + process.exit(2); + } + return cases; +} + +async function main(): Promise { + console.log('launch policy parity (plugin ↔ task-dispatcher, shared corpus)'); + console.log(` upstream: ${UPSTREAM_URL}`); + if (UPSTREAM_REF !== 'main') { + console.log( + ` NOTE: comparing against ref ${JSON.stringify(UPSTREAM_REF)}, NOT main. This is a ` + + 'pre-merge check and does not prove parity with what is deployed.', + ); + } + + const cases = await fetchCorpus(); + console.log(` ${cases.length} case(s)`); + + for (const c of cases) { + let got: Record | null = null; + let verdict: 'accept' | 'reject'; + let error: string | null = null; + try { + got = validateLaunchPolicy(sub(c.policy), HOME); + verdict = 'accept'; + } catch (err) { + if (!(err instanceof LaunchPolicyError)) { + // A TypeError here means the validator crashed rather than refusing, which is a + // different and worse failure than a wrong verdict — say so by name. + check(false, `corpus[${c.name}]: threw ${(err as Error).name}, not LaunchPolicyError: ${(err as Error).message}`); + continue; + } + verdict = 'reject'; + error = err.message; + } + + if (verdict !== c.expect) { + check(false, `corpus[${c.name}]: expected ${c.expect}, got ${verdict}${error ? ` (${error})` : ''}`); + continue; + } + check(true, `corpus[${c.name}]: ${verdict}`); + + if (c.expect !== 'accept' || !c.resolved) continue; + + // The resolved values, in the corpus's field names. This is the half a verdict-only + // comparison misses — see the header. + const want = sub(c.resolved) as Record>; + const flat: Record> = {}; + for (const [agent, entry] of Object.entries(got!)) { + flat[agent] = { + project_dir: entry.projectDir, + run_as_user: entry.runAsUser, + launcher: entry.launcher, + }; + } + const wantKeys = Object.keys(want).sort(); + const gotKeys = Object.keys(flat).sort(); + const same = + JSON.stringify(wantKeys) === JSON.stringify(gotKeys) + && wantKeys.every(a => + (['project_dir', 'run_as_user', 'launcher'] as const).every(f => flat[a][f] === want[a][f])); + + check( + same, + same + ? `corpus[${c.name}]: resolved values match` + : `corpus[${c.name}]: resolved ${JSON.stringify(flat)} != expected ${JSON.stringify(want)}`, + ); + } + + console.log(''); + if (failures.length > 0) { + console.error(`LAUNCH POLICY PARITY DRIFT (${failures.length}):`); + for (const f of failures) console.error(` - ${f}`); + console.error(''); + console.error( + 'This plugin and task-dispatcher disagree about a roster both of them feed into a ' + + 'subprocess spawn. Fix src/launch-policy.ts to match — or, if the DISPATCHER is the ' + + 'one that is wrong, fix it there and the corpus with it. Do not edit this gate, and ' + + 'do not delete the case.', + ); + return 1; + } + console.log('both validators agree on every case'); + return 0; +} + +process.exit(await main()); diff --git a/src/launch-guards.ts b/src/launch-guards.ts new file mode 100644 index 0000000..c780589 --- /dev/null +++ b/src/launch-guards.ts @@ -0,0 +1,183 @@ +/** + * Pre-launch credential guards, ported from task-dispatcher's launch path. + * + * WHY (plan agent-workflow-interop-2026-08, Phase 5.6) + * + * The dispatcher refuses to spawn a directly-launched agent without a resolved + * `SCOPED_MCP_BEARER_TOKEN` and without a usable Anthropic credential, and says which one + * is missing by name. This plugin had neither check, so a Start could spawn a session + * doomed to fail deep inside — a 401 from every scoped-mcp tool, or a `claude -p` that + * short-circuits to "Not logged in" and never reads the task prompt. Both look, from the + * operator's side, like an agent that started and did nothing. + * + * THE PORT IS NOT JUST THE TWO CHECKS. The dispatcher layers + * `/opt/appdata/agents//.env` into the child environment before checking it + * (`load_agent_env`, mirroring what `run-scoped-mcp-http.sh` sources server-side); this + * plugin spawned with a bare `{...process.env}`. Measured on forge: the CloudCLI process + * env carries `CLAUDE_CODE_OAUTH_TOKEN` but NO `SCOPED_MCP_BEARER_TOKEN`. So a guard that + * checked only `process.env` would have refused every non-run-as Start — correctly, in the + * sense that those sessions really were starting without scoped-mcp tools, but that is the + * bug rather than the fix. Layering the agent's own env is what makes the guard a guard + * instead of a blanket refusal, and it is what the dispatcher has always done. + * + * THE RUN-AS ASYMMETRY IS DELIBERATE — DO NOT "FIX" IT + * + * Neither check runs for an agent with `runAsUser` set, exactly as in the dispatcher. For + * such an agent the credentials are not in this process's environment BY DESIGN: they live + * in a file owned by and readable only by the target user, and the launcher sources them as + * that user. Running these checks on that path would fail every launch for the one agent + * whose isolation is working correctly. + * + * The launcher performs the equivalent fail-loud checks itself, by name and as the right + * user (`: "${VAR:?}"` on SCOPED_MCP_BEARER_TOKEN, TASK_QUEUE_TOKEN, GITHOST_MCP_AUTH_TOKEN, + * CLAUDE_CODE_OAUTH_TOKEN). What it cannot cover is the launcher itself being absent, and + * the caller already checks that separately. + */ + +import fs from 'node:fs'; +import path from 'node:path'; + +/** Where each agent's server-side environment file lives. */ +const AGENT_ENV_ROOT = '/opt/appdata/agents'; + +/** + * The same shape the roster's agent names are validated against. Re-checked here rather + * than trusted, because this value becomes a path segment: callers reach this function + * with a name that came out of a queue YAML, and a validated-elsewhere invariant is one + * refactor away from not holding. + */ +const AGENT_NAME_RE = /^[a-z][a-z0-9_-]{0,31}$/; + +/** + * Parse a KEY=VALUE file. A missing or unreadable file is an empty object, not an error — + * an agent may legitimately have none, and the caller's check is on the RESULT. + * + * Mirrors task-dispatcher's `read_env_file` deliberately, including its documented + * deviations from `source`: no `$VAR` expansion, no `export ` prefix, no line + * continuations. These files are written by this fleet and every one is flat KEY=VALUE. + * The quote handling repeats Python's `.strip().strip('"').strip("'")` in the same order, + * because "one layer of quoting" and "all leading/trailing quote characters" differ on + * inputs like `""x""` and the two sides must agree on which they implement. + */ +export function parseEnvFile(text: string): Record { + const env: Record = Object.create(null) as Record; + for (const rawLine of text.split('\n')) { + const line = rawLine.trim(); + if (!line || line.startsWith('#') || !line.includes('=')) continue; + const eq = line.indexOf('='); + const key = line.slice(0, eq).trim(); + if (!key) continue; + let value = line.slice(eq + 1).trim(); + value = value.replace(/^"+/, '').replace(/"+$/, ''); + value = value.replace(/^'+/, '').replace(/'+$/, ''); + env[key] = value; + } + return env; +} + +/** + * Read `/opt/appdata/agents//.env`, or `{}` if it is absent or unreadable. + * + * `envRoot` is injectable so tests need not own a path under /opt. + */ +export function loadAgentEnv(agent: string, envRoot: string = AGENT_ENV_ROOT): Record { + if (!AGENT_NAME_RE.test(agent)) return {}; + try { + return parseEnvFile(fs.readFileSync(path.join(envRoot, agent, '.env'), 'utf-8')); + } catch { + return {}; + } +} + +/** + * Whether a headless `claude -p` launched with this environment can authenticate. + * + * Ported from the dispatcher's `anthropic_creds_usable` (SMCP-29/SMCP-32). Any one of the + * three token variables is sufficient — they short-circuit the same way — and the OAuth + * file is the last resort. + * + * `expiresAt > now` is a correct usability test here rather than an over-strict one: + * headless mode does NOT interactively refresh an expired OAuth token, it prints the login + * prompt instead. So an expired token is genuinely unusable, not merely stale. + */ +export function anthropicCredsUsable( + env: Record, + oauthPath: string, + now: number = Date.now(), +): boolean { + if (env.ANTHROPIC_API_KEY || env.ANTHROPIC_AUTH_TOKEN || env.CLAUDE_CODE_OAUTH_TOKEN) return true; + try { + const oauth = (JSON.parse(fs.readFileSync(oauthPath, 'utf-8')) as { + claudeAiOauth?: { accessToken?: string; expiresAt?: number }; + }).claudeAiOauth; + if (!oauth?.accessToken) return false; + return (oauth.expiresAt ?? 0) > now; + } catch { + return false; + } +} + +export interface GuardOk { + ok: true; + /** The environment the child should be spawned with. */ + env: Record; +} +export interface GuardFail { + ok: false; + error: string; +} + +export interface GuardOptions { + /** Injectable for tests; defaults to the real locations. */ + envRoot?: string; + oauthPath: string; + now?: number; +} + +/** + * Build the child environment and refuse the launch if it cannot possibly authenticate. + * + * Returns the environment to spawn with on success — the caller must use it rather than + * rebuilding one, or the thing that was checked and the thing that is used come apart, + * which is how a guard becomes decoration. + * + * `runAsUser` short-circuits BOTH checks and returns the parent environment untouched. See + * the module header: that asymmetry is the correct behaviour, not an oversight. + */ +export function preLaunchEnv( + agent: string, + runAsUser: string | null, + parentEnv: Record, + opts: GuardOptions, +): GuardOk | GuardFail { + if (runAsUser) { + // sudo scrubs the environment anyway, and the launcher sources the agent's + // credentials as the target user. Checking this process's env would be asking the + // wrong question of the wrong user. + return { ok: true, env: parentEnv }; + } + + const env = { ...parentEnv, ...loadAgentEnv(agent, opts.envRoot) }; + + if (!env.SCOPED_MCP_BEARER_TOKEN) { + return { + ok: false, + error: + `SCOPED_MCP_BEARER_TOKEN unresolved for agent '${agent}' — refusing to launch. ` + + `.mcp.json interpolates bare $VAR only, with no :?/:- operators, so an unresolved ` + + `token fails as a 401 deep inside the session instead of here.`, + }; + } + + if (!anthropicCredsUsable(env, opts.oauthPath, opts.now)) { + return { + ok: false, + error: + `No usable Anthropic credential (OAuth expired, or no ANTHROPIC_API_KEY / ` + + `ANTHROPIC_AUTH_TOKEN / CLAUDE_CODE_OAUTH_TOKEN) — refusing to launch '${agent}'. ` + + `Run \`claude /login\` to restore headless launches.`, + }; + } + + return { ok: true, env }; +} diff --git a/src/launch-policy.ts b/src/launch-policy.ts index c203738..8ac4f5c 100644 --- a/src/launch-policy.ts +++ b/src/launch-policy.ts @@ -69,6 +69,14 @@ function expandHome(p: string, home: string): string { return p; } +/** + * Drop a trailing path separator, so this side agrees with Python's os.path.normpath. + * The filesystem root is left alone — '/' is not a trailing separator on a name. + */ +function stripTrailingSep(p: string): string { + return p.length > 1 && p.endsWith(path.sep) ? p.slice(0, -1) : 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; @@ -118,7 +126,17 @@ export function validateLaunchPolicy(raw: unknown, home: string): LaunchPolicy { // 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); + // + // The trailing separator is then stripped, and that is NOT cosmetic. Node's + // path.normalize KEEPS a trailing slash ('/a/b/' -> '/a/b/') while Python's + // os.path.normpath removes it ('/a/b/' -> '/a/b'), so a roster entry written with + // one produced two different strings on the two sides for the same input — and + // that string becomes a spawn's cwd. Both sides accepted the entry, so no verdict + // ever disagreed; only the resolved value did. This is the .resolve() divergence + // in a second costume, and it is why the shared corpus compares resolved values + // rather than just accept/reject. Found by that corpus + // (tests/fixtures/launch-policy-corpus.json in task-dispatcher), not by review. + const projectDir = stripTrailingSep(path.normalize(expanded)); if (!isUnder(projectRoot, projectDir)) { throw new LaunchPolicyError(`${agent}: project_dir must be under ${projectRoot}: ${rawDir}`); } diff --git a/src/server.ts b/src/server.ts index 2d49dc7..f53d7a3 100644 --- a/src/server.ts +++ b/src/server.ts @@ -12,6 +12,7 @@ import { resolveAllowedPath } from './path-guard.ts'; import type { DeadLetter, HeadlessRun, HeadlessRunDetail } from './types.ts'; import { toDeadLetter } from './dead-letters.ts'; import { isTerminal } from './vocabulary.ts'; +import { preLaunchEnv } from './launch-guards.ts'; import { loadRunRecord, outcomeLabel, @@ -394,6 +395,23 @@ function launchSession( const { argv, note } = buildLaunchArgv(entry, mode, prompt, claudeBin, queuedMode); + // Refuse a session that cannot possibly authenticate, and say which credential is + // missing — the same two guards task-dispatcher applies, ported in Phase 5.6. Without + // them a Start spawns a process that 401s from every scoped-mcp tool, or that + // short-circuits to "Not logged in" before reading the prompt; from the operator's side + // both look like an agent that started and did nothing. + // + // This ALSO layers /opt/appdata/agents//.env into the child env, which the + // dispatcher has always done and this plugin did not. That is the substance of the fix, + // not a side effect: the CloudCLI process env has no SCOPED_MCP_BEARER_TOKEN, so + // directly-launched sessions were genuinely starting without one. + // + // Both checks are skipped for a run-as agent, deliberately — see launch-guards.ts. + const guard = preLaunchEnv(targetAgent, entry.runAsUser, process.env, { + oauthPath: path.join(os.homedir(), '.claude', '.credentials.json'), + }); + if (!guard.ok) return { ok: false, error: guard.error }; + // 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. let logFd: number; @@ -415,7 +433,9 @@ function launchSession( // The run identity, so a Langfuse trace can be joined back to the task that paid // for it. Same two names the dispatcher uses; a run-as agent gets them from its // launcher's flags instead, because sudo scrubs the environment. - env: { ...process.env, FORGE_RUN_ID: runId, FORGE_TASK_ID: taskId }, + // guard.env, not process.env: the environment that was CHECKED must be the one + // that is used, or the guard is checking a different process than it protects. + env: { ...guard.env, FORGE_RUN_ID: runId, FORGE_TASK_ID: taskId }, }); child.on('error', (err) => { diff --git a/src/tests/launch-guards.test.ts b/src/tests/launch-guards.test.ts new file mode 100644 index 0000000..dbe2f94 --- /dev/null +++ b/src/tests/launch-guards.test.ts @@ -0,0 +1,241 @@ +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 { + parseEnvFile, + loadAgentEnv, + anthropicCredsUsable, + preLaunchEnv, +} from '../launch-guards.ts'; + +/** + * The property under test is not "these functions parse files". It is that a Start which + * cannot authenticate is refused HERE, by name, instead of becoming a session that 401s + * deep inside — and that the one agent whose isolation works correctly is not refused + * along with it. + */ + +function tmpdir(): string { + return fs.mkdtempSync(path.join(os.tmpdir(), 'launch-guards-')); +} + +function withAgentEnv(agent: string, contents: string): string { + const root = tmpdir(); + fs.mkdirSync(path.join(root, agent), { recursive: true }); + fs.writeFileSync(path.join(root, agent, '.env'), contents); + return root; +} + +function oauthFile(body: unknown): string { + const f = path.join(tmpdir(), '.credentials.json'); + fs.writeFileSync(f, JSON.stringify(body)); + return f; +} + +const FUTURE = Date.now() + 3_600_000; +const PAST = Date.now() - 3_600_000; + +// ── parseEnvFile ───────────────────────────────────────────────────────────── + +test('parses flat KEY=VALUE lines', () => { + const env = parseEnvFile('A=1\nB=two\n'); + assert.equal(env.A, '1'); + assert.equal(env.B, 'two'); +}); + +test('skips comments, blank lines, and lines with no =', () => { + const env = parseEnvFile('# comment\n\nnot-an-assignment\nA=1\n'); + assert.deepEqual(Object.keys(env), ['A']); +}); + +test('a value containing = keeps everything after the first one', () => { + // Base64 and JWT-shaped secrets end in `=` padding; splitting on every `=` would + // truncate exactly the values this guard exists to check. + assert.equal(parseEnvFile('TOKEN=abc=def==\n').TOKEN, 'abc=def=='); +}); + +test('surrounding quotes are stripped', () => { + assert.equal(parseEnvFile('A="quoted"\n').A, 'quoted'); + assert.equal(parseEnvFile("A='quoted'\n").A, 'quoted'); +}); + +test('$VAR is NOT expanded', () => { + // Deliberate deviation from `source` semantics, matching the dispatcher's parser. A + // side that expanded would resolve a token the other side left literal. + assert.equal(parseEnvFile('A=$HOME\n').A, '$HOME'); +}); + +// ── loadAgentEnv ───────────────────────────────────────────────────────────── + +test('reads the agent env file', () => { + const root = withAgentEnv('developer', 'SCOPED_MCP_BEARER_TOKEN=tok\n'); + assert.equal(loadAgentEnv('developer', root).SCOPED_MCP_BEARER_TOKEN, 'tok'); +}); + +test('a missing env file is empty, not an error', () => { + assert.deepEqual(loadAgentEnv('nobody', tmpdir()), {}); +}); + +test('an agent name outside the roster shape reads nothing', () => { + // The name becomes a path segment and reaches here from a queue YAML. Traversal is + // refused rather than sanitised: there is no legitimate agent named `../../etc`. + // + // THE TARGET FILE IS PLANTED ON PURPOSE. An earlier version of this test pointed the + // traversal at a path that did not exist, so `readFileSync` threw and the function + // returned {} whether or not the name check ran — it asserted the right outcome for + // the wrong reason, and deleting the guard left it green. Removing AGENT_NAME_RE must + // now change the RESULT, not just the route to it. + const base = tmpdir(); + const root = path.join(base, 'agents'); + fs.mkdirSync(path.join(root, 'developer'), { recursive: true }); + fs.writeFileSync(path.join(root, 'developer', '.env'), 'SCOPED_MCP_BEARER_TOKEN=tok\n'); + + // `/../reachable/.env` exists, so an unguarded join would read it. + fs.mkdirSync(path.join(base, 'reachable'), { recursive: true }); + fs.writeFileSync(path.join(base, 'reachable', '.env'), 'SCOPED_MCP_BEARER_TOKEN=stolen\n'); + + assert.deepEqual(loadAgentEnv('../reachable', root), {}); + assert.deepEqual(loadAgentEnv('/etc/shadow', root), {}); + assert.deepEqual(loadAgentEnv('Developer', root), {}); + // The sanity half: a name that IS in shape still reads its file, so the assertions + // above are about the name check rather than about the fixture being unreadable. + assert.equal(loadAgentEnv('developer', root).SCOPED_MCP_BEARER_TOKEN, 'tok'); +}); + +// ── anthropicCredsUsable ───────────────────────────────────────────────────── + +for (const key of ['ANTHROPIC_API_KEY', 'ANTHROPIC_AUTH_TOKEN', 'CLAUDE_CODE_OAUTH_TOKEN']) { + test(`${key} alone is sufficient`, () => { + // SMCP-32: all three short-circuit the same way, so any one of them is enough. + assert.equal(anthropicCredsUsable({ [key]: 'x' }, '/nonexistent'), true); + }); +} + +test('an unexpired OAuth file is sufficient', () => { + const f = oauthFile({ claudeAiOauth: { accessToken: 'a', expiresAt: FUTURE } }); + assert.equal(anthropicCredsUsable({}, f), true); +}); + +test('an EXPIRED OAuth token is not usable', () => { + // Headless mode does not interactively refresh — it prints the login prompt and the + // session never reads the task. So expiry is a usability answer, not a staleness one. + const f = oauthFile({ claudeAiOauth: { accessToken: 'a', expiresAt: PAST } }); + assert.equal(anthropicCredsUsable({}, f), false); +}); + +test('an OAuth file with no accessToken is not usable', () => { + assert.equal(anthropicCredsUsable({}, oauthFile({ claudeAiOauth: { expiresAt: FUTURE } })), false); +}); + +test('a missing or malformed OAuth file is not usable, and does not throw', () => { + assert.equal(anthropicCredsUsable({}, '/nonexistent/creds.json'), false); + const bad = path.join(tmpdir(), 'bad.json'); + fs.writeFileSync(bad, 'not json'); + assert.equal(anthropicCredsUsable({}, bad), false); +}); + +// ── preLaunchEnv: the guard itself ─────────────────────────────────────────── + +test('a directly-launched agent with no bearer token is refused BY NAME', () => { + const root = withAgentEnv('developer', '# no token here\n'); + const r = preLaunchEnv('developer', null, {}, { envRoot: root, oauthPath: '/nonexistent' }); + + assert.equal(r.ok, false); + assert.match((r as { error: string }).error, /SCOPED_MCP_BEARER_TOKEN/); + assert.match((r as { error: string }).error, /developer/); +}); + +test('a directly-launched agent with no usable Anthropic credential is refused BY NAME', () => { + const root = withAgentEnv('developer', 'SCOPED_MCP_BEARER_TOKEN=tok\n'); + const r = preLaunchEnv('developer', null, {}, { envRoot: root, oauthPath: '/nonexistent' }); + + assert.equal(r.ok, false); + assert.match((r as { error: string }).error, /Anthropic credential/); +}); + +test('the agent env file is layered in — this is what makes the guard passable at all', () => { + // Measured on forge: the CloudCLI process env has CLAUDE_CODE_OAUTH_TOKEN but NO + // SCOPED_MCP_BEARER_TOKEN. Without this layering the guard would refuse every + // non-run-as Start, and the sessions it did allow would 401 anyway. + const root = withAgentEnv('developer', 'SCOPED_MCP_BEARER_TOKEN=from-agent-env\n'); + const r = preLaunchEnv('developer', null, { CLAUDE_CODE_OAUTH_TOKEN: 'x' }, { + envRoot: root, + oauthPath: '/nonexistent', + }); + + assert.equal(r.ok, true); + assert.equal((r as { env: Record }).env.SCOPED_MCP_BEARER_TOKEN, 'from-agent-env'); +}); + +test('the agent env overrides the parent env, as the dispatcher does', () => { + const root = withAgentEnv('developer', 'SCOPED_MCP_BEARER_TOKEN=agents-own\n'); + const r = preLaunchEnv('developer', null, { SCOPED_MCP_BEARER_TOKEN: 'cloudclis', CLAUDE_CODE_OAUTH_TOKEN: 'x' }, { + envRoot: root, + oauthPath: '/nonexistent', + }); + + // Layering order matters: a session must run with the identity of the agent it is, + // not with whatever token the launching process happened to be holding. + assert.equal((r as { env: Record }).env.SCOPED_MCP_BEARER_TOKEN, 'agents-own'); +}); + +test('the parent env is otherwise preserved', () => { + const root = withAgentEnv('developer', 'SCOPED_MCP_BEARER_TOKEN=tok\n'); + const r = preLaunchEnv('developer', null, { PATH: '/usr/bin', CLAUDE_CODE_OAUTH_TOKEN: 'x' }, { + envRoot: root, + oauthPath: '/nonexistent', + }); + + assert.equal((r as { env: Record }).env.PATH, '/usr/bin'); +}); + +// ── the asymmetry that must be preserved ───────────────────────────────────── + +test('a RUN-AS agent is not subjected to either check', () => { + // THE REGRESSION THIS GUARDS. For a run-as agent the credentials are not in this + // process's env by design — they are in a file only the target user can read, sourced + // by the launcher as that user. Running these checks here would fail every launch for + // the one agent whose isolation is working correctly. An empty env and no OAuth file + // is exactly the state a correctly-isolated steward launch presents. + const r = preLaunchEnv('steward', 'agent-steward', {}, { + envRoot: tmpdir(), + oauthPath: '/nonexistent', + }); + + assert.equal(r.ok, true); +}); + +test('a run-as agent gets the parent env untouched, with no agent env layered in', () => { + // sudo scrubs the environment anyway, and reading the agent's file from here would be + // this process reading a credential it has no business holding. + const root = withAgentEnv('steward', 'SCOPED_MCP_BEARER_TOKEN=should-not-be-read\n'); + const r = preLaunchEnv('steward', 'agent-steward', { PATH: '/usr/bin' }, { + envRoot: root, + oauthPath: '/nonexistent', + }); + + assert.equal(r.ok, true); + const env = (r as { env: Record }).env; + // Named explicitly and BEFORE the deepEqual: assert.deepEqual narrows `env` to the + // expected object's type, so a subsequent lookup of an absent key is a compile error + // rather than the assertion it is meant to be. + assert.equal(env.SCOPED_MCP_BEARER_TOKEN, undefined, 'the agent env must not be read here'); + assert.deepEqual(env, { PATH: '/usr/bin' }); +}); + +test('a fully-credentialled direct launch is allowed', () => { + const root = withAgentEnv('developer', 'SCOPED_MCP_BEARER_TOKEN=tok\n'); + const f = oauthFile({ claudeAiOauth: { accessToken: 'a', expiresAt: FUTURE } }); + + assert.equal(preLaunchEnv('developer', null, {}, { envRoot: root, oauthPath: f }).ok, true); +}); + +test('an expired OAuth blocks a direct launch even with a bearer token', () => { + const root = withAgentEnv('developer', 'SCOPED_MCP_BEARER_TOKEN=tok\n'); + const f = oauthFile({ claudeAiOauth: { accessToken: 'a', expiresAt: PAST } }); + + assert.equal(preLaunchEnv('developer', null, {}, { envRoot: root, oauthPath: f }).ok, false); +});