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); +});