officerdev/platform
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

make both env guards able to fire, and follow the symlink

Browse Source 
host was right twice, including about his own advice. Two dead guards had
shipped here, both for the same reason: 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 either one firing.

THE SUBSET CHECK WAS ALSO TAUTOLOGICAL. `permitted` came from the same constants
memberEnv builds childEnv from, so it was empty under every edit where that
holds — the exact criticism that retired NEVER_ENV. Worse, it lost the one live
trigger the denylist had: a credential added to ALLOWED_ENV used to throw, and
under the subset check widened the permitted set in the same motion and passed
silently. That is the realistic future edit and it was the one left unguarded.

Now both, and the denylist tests the LIST rather than the instance, so it fires
on exactly that edit. Extracted as `assertEnvSafe` so a test can pass a poisoned
allowlist — the guards being untestable in place is why they were decorative
twice.

THE BINARY CHECK WOULD HAVE THROWN ON EVERY TURN. Anthropic's installer puts a
symlink at ~/.local/bin/claude into a versioned directory; resolve() does not
follow symlinks, so the string compare matched only while `command` arrived as
the symlink spelling, and would have failed the moment anything upstream
normalised it — at exactly the point the hook gets wired. Compared through
realpathSync on both sides now, per turn and never cached, since `claude update`
moves the target.

Nine tests pin all of it: a poisoned allowlist, a stray key, each NEVER_ENV name,
and the symlink/target/missing-path cases.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
pastilhas
2026-08-11 22:10:34 +00:00
co-authored by Claude Opus 5
parent 06bfcf9556
commit 6b7aad91db
2 changed files with 163 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/servers/sidecar/claude/spawn-as-member.test.ts
+81
View File
@@ -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);
});
});
src/servers/sidecar/claude/spawn-as-member.ts
+82 -24
View File
@@ -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<string> = new Set<string>([...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<string>, childEnv: Record<string, string>, 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<string, string | undefined>): Record<string, string> {
const env: Record<string, string> = {};
@@ -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<string>([...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