diff --git a/src/servers/sidecar/claude/spawn-as-member.test.ts b/src/servers/sidecar/claude/spawn-as-member.test.ts new file mode 100644 index 00000000..43e6e988 --- /dev/null +++ b/src/servers/sidecar/claude/spawn-as-member.test.ts @@ -0,0 +1,81 @@ +import { afterAll, describe, expect, test } from 'bun:test'; +import { mkdtempSync, mkdirSync, symlinkSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { PERMITTED_ENV, assertEnvSafe, sameFile } from './spawn-as-member'; + +// Pins the two guards standing between a member's agent turn and the owner's credential. +// +// Both of them shipped dead before this test existed, and in both cases the reason was the same: they were +// written inside the spawn closure, where the only way to reach them is to spawn, and the passing path spawns +// `sudo`. So nothing ever demonstrated them firing, and "it looks right" carried the weight. That is what +// these tests are for — not the happy path, which is obvious, but the two edits a future reader will actually +// make. + +const OWNER_CRED = 'ANTHROPIC_API_KEY'; + +describe('assertEnvSafe', () => { + test('the shipping lists are clean', () => { + // The regression this exists to catch: somebody adds a credential to ALLOWED_ENV. + expect(() => assertEnvSafe(PERMITTED_ENV, { HOME: '/home/x' }, 'x')).not.toThrow(); + }); + + test('throws when a credential is in the allowlist — guards the constant', () => { + // The realistic dangerous edit. Note the credential is NOT in childEnv: the point is that the *list* + // permits it, so the next call that inherits one would pass it through silently. + const poisoned = new Set([...PERMITTED_ENV, OWNER_CRED]); + expect(() => assertEnvSafe(poisoned, { HOME: '/home/x' }, 'green')).toThrow(/is in the allowlist/); + }); + + test('throws when the built env carries an unvetted key — guards the construction', () => { + expect(() => assertEnvSafe(PERMITTED_ENV, { HOME: '/home/x', POSTGRES_URL: 'postgres://…' }, 'green')).toThrow( + /unvetted env/, + ); + }); + + test('names the offending variable, so the error is actionable', () => { + expect(() => assertEnvSafe(PERMITTED_ENV, { JWT_SECRET: 'x' }, 'green')).toThrow(/JWT_SECRET/); + }); + + test('every NEVER_ENV name would be caught if it were permitted', () => { + // Guards against the list being quietly narrowed as well as the allowlist being widened. + for (const name of ['ANTHROPIC_BASE_URL', 'CLAUDE_CODE_OAUTH_TOKEN', 'POSTGRES_URL', 'JWT_SECRET']) { + expect(() => assertEnvSafe(new Set([...PERMITTED_ENV, name]), {}, 'green')).toThrow(name); + } + }); +}); + +// Anthropic's installer puts a symlink at ~/.local/bin/claude pointing into a versioned directory, and +// `claude update` moves that target. A string compare on the path matches only while the caller happens to +// pass the symlink spelling — which is why the check follows symlinks on both sides. +describe('sameFile', () => { + const dir = mkdtempSync(join(tmpdir(), 'spawn-as-member-')); + const versioned = join(dir, 'versions', '2.1.227'); + const link = join(dir, 'bin', 'claude'); + + mkdirSync(join(dir, 'versions'), { recursive: true }); + mkdirSync(join(dir, 'bin'), { recursive: true }); + writeFileSync(versioned, '#!/bin/sh\n', { mode: 0o755 }); + symlinkSync(versioned, link); + + afterAll(() => rmSync(dir, { recursive: true, force: true })); + + test('a symlink and its target are the same file', () => { + expect(sameFile(link, versioned)).toBe(true); + }); + + test('the symlink matches itself — the spelling the caller passes today', () => { + expect(sameFile(link, link)).toBe(true); + }); + + test('a different file does not match', () => { + const other = join(dir, 'bin', 'not-claude'); + writeFileSync(other, '#!/bin/sh\n', { mode: 0o755 }); + expect(sameFile(other, link)).toBe(false); + }); + + test('a missing path refuses rather than throwing', () => { + // Inside a spawn hook an ENOENT would surface as an unexplained crash; false reads as "not their binary". + expect(sameFile(join(dir, 'bin', 'absent'), link)).toBe(false); + }); +}); diff --git a/src/servers/sidecar/claude/spawn-as-member.ts b/src/servers/sidecar/claude/spawn-as-member.ts index c0c02fb0..13c0f8ad 100644 --- a/src/servers/sidecar/claude/spawn-as-member.ts +++ b/src/servers/sidecar/claude/spawn-as-member.ts @@ -1,6 +1,7 @@ import type { SpawnOptions, SpawnedProcess } from '@anthropic-ai/claude-agent-sdk'; import { spawn } from 'node:child_process'; -import { join, resolve } from 'node:path'; +import { realpathSync } from 'node:fs'; +import { join } from 'node:path'; import { runAsArgv } from '@@/os-user'; import { claudeBinIn } from '@@/os-user-claude'; @@ -54,19 +55,37 @@ export type MemberRun = { */ const ALLOWED_ENV = ['LANG', 'LC_ALL', 'TERM', 'TZ', 'NO_COLOR', 'CLAUDE_CODE_ENTRYPOINT'] as const; -/** - * Everything the child is allowed to have, beyond the allowlist above. - * - * The first version of this was a denylist of names that must never cross — the owner's proxy variables, - * `POSTGRES_URL`, the JWT secret. The live-server review pointed out it could never fire: `memberEnv` builds - * the environment *from* `ALLOWED_ENV`, so a denied name was already impossible, and the list was - * simultaneously incomplete (the installed SDK also reads `CLAUDE_CODE_OAUTH_TOKEN`, - * `CLAUDE_CODE_OAUTH_REFRESH_TOKEN`, `CLAUDE_API_KEY`, `CLAUDE_CODE_SESSION_ACCESS_TOKEN`, - * `CLAUDE_CODE_CLIENT_KEY`, `ANTHROPIC_FOUNDRY_API_KEY`). A denylist has to be right about every variable - * anyone will ever add; the subset check below is complete by construction and cannot rot. - */ +/** Set by `memberEnv` itself rather than inherited, so vetted the same way. */ const ALSO_ALLOWED = ['HOME', 'CLAUDE_CONFIG_DIR'] as const; +/** + * Credential names that must never appear in the allowlist. **This guards the constant, not the instance.** + * + * Two dead checks were written here before this one worked, and the distinction is the whole lesson. A + * denylist tested against `childEnv` cannot fire, because `memberEnv` builds that object *from* `ALLOWED_ENV`. + * Inverting it to a subset test cannot fire either, for the same reason — and it is strictly worse, because + * widening `ALLOWED_ENV` widens the permitted set in the same motion, so the one realistic dangerous edit + * (somebody adds a credential to the allowlist) stops throwing and starts passing silently. + * + * Tested against the *list*, it fires on exactly that edit. Being incomplete is then survivable: a name this + * misses degrades to the status quo rather than to false confidence. The Anthropic and Claude entries are what + * the installed SDK reads; the last two are what make this process more privileged than a member's shell. + */ +const NEVER_ENV = [ + 'ANTHROPIC_BASE_URL', + 'ANTHROPIC_API_KEY', + 'ANTHROPIC_AUTH_TOKEN', + 'ANTHROPIC_FOUNDRY_API_KEY', + '_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL', + 'CLAUDE_CODE_OAUTH_TOKEN', + 'CLAUDE_CODE_OAUTH_REFRESH_TOKEN', + 'CLAUDE_CODE_SESSION_ACCESS_TOKEN', + 'CLAUDE_CODE_CLIENT_KEY', + 'CLAUDE_API_KEY', + 'POSTGRES_URL', + 'JWT_SECRET', +]; + /** * The `claude` config directory for a member. * @@ -77,6 +96,43 @@ const ALSO_ALLOWED = ['HOME', 'CLAUDE_CONFIG_DIR'] as const; */ export const memberClaudeConfigDir = (home: string): string => join(home, '.claude'); +/** Everything a member's turn may carry. Exported so a test can assert the shipping lists are clean. */ +export const PERMITTED_ENV: ReadonlySet = new Set([...ALLOWED_ENV, ...ALSO_ALLOWED]); + +/** + * The two env guards, as one pure function so they can be tested. + * + * They were unreachable from a test while they lived inside the spawn closure — the only way to exercise them + * was to spawn, and the passing path spawns `sudo`. Which is how both earlier versions of this guard shipped + * dead: nothing could demonstrate them firing. + * + * @param permitted the names allowed for this turn — parameterised so a test can pass a poisoned list + * @param childEnv what `memberEnv` actually produced + */ +export function assertEnvSafe(permitted: ReadonlySet, childEnv: Record, who: string): void { + // Guards the constant: a credential added to the allowlist throws rather than reaching a member. + const leaked = NEVER_ENV.filter((name) => permitted.has(name)); + if (leaked.length) throw new Error(`refusing to run ${who}'s agent: ${leaked.join(', ')} is in the allowlist`); + + // Guards the construction: a key `memberEnv` invents that nobody vetted. + const unexpected = Object.keys(childEnv).filter((name) => !permitted.has(name)); + if (unexpected.length) throw new Error(`refusing to run ${who}'s agent with unvetted env: ${unexpected.join(', ')}`); +} + +/** + * Whether two paths are the same file, following symlinks on both sides. + * + * False rather than throwing when either path does not exist, so a missing install reads as "not their + * binary" and refuses, instead of surfacing an ENOENT from inside a spawn hook. + */ +export function sameFile(a: string, b: string): boolean { + try { + return realpathSync(a) === realpathSync(b); + } catch { + return false; + } +} + /** Build the child environment for a member's turn: allowlist in, everything else absent. */ function memberEnv(run: MemberRun, inherited: Record): Record { const env: Record = {}; @@ -106,20 +162,22 @@ export function spawnClaudeAsMember(run: MemberRun): (options: SpawnOptions) => return ({ command, args, cwd, env, signal }: SpawnOptions): SpawnedProcess => { const childEnv = memberEnv(run, env); - // Complete by construction: anything not named in one of the two lists is a leak, whatever it is called. - const permitted = new Set([...ALLOWED_ENV, ...ALSO_ALLOWED]); - const unexpected = Object.keys(childEnv).filter((name) => !permitted.has(name)); - if (unexpected.length) { - throw new Error(`refusing to run ${run.osUser}'s agent with unvetted env: ${unexpected.join(', ')}`); - } + assertEnvSafe(PERMITTED_ENV, childEnv, run.osUser); - // The binary must be theirs. Without this the check is a comment: `command` arrives from the SDK, and - // `claude-manager.ts` currently resolves it to the OWNER'S `CLAUDE_BIN` at module load — so the wired - // version would run the owner's install as the member, which is exactly the confusion this file exists - // to prevent. Their own binary is the one their own `claude update` maintains. + // The binary must be theirs. Without this the claim is only a comment: `command` arrives from the SDK, and + // `claude-manager.ts` resolves it to the OWNER'S `CLAUDE_BIN` at module load — so the wired version would + // run the owner's install as the member, the exact confusion this file exists to prevent. + // + // Compared through `realpathSync` because Anthropic's installer puts a SYMLINK at `~/.local/bin/claude` + // pointing into a versioned directory. `resolve()` does not follow symlinks, so a plain string compare + // matches only while `command` happens to arrive as the symlink path — and throws on every turn the moment + // anything upstream normalises it. Accepting either form is what makes the check mean "their install" + // rather than "this spelling of their install". + // + // Resolved per turn, never cached: `claude update` moves the symlink's target. const expectedBin = claudeBinIn(run.home); - if (resolve(command) !== expectedBin) { - throw new Error(`refusing to run ${resolve(command)} as ${run.osUser}; expected their own ${expectedBin}`); + if (!sameFile(command, expectedBin)) { + throw new Error(`refusing to run ${command} as ${run.osUser}; expected their own ${expectedBin}`); } // `env K=V …` inside the argv, because `--reset-env` clears anything handed to `setpriv` itself. This is