the binary check that was a comment, and a guard that could fire
Four fixes from the live server's review. One was a real defect.
THE BINARY WAS NEVER CHECKED. spawn-as-member passed `command` from the SDK
through untouched while a comment claimed the member's own install was what ran.
Since claude-manager resolves the OWNER'S CLAUDE_BIN at module load, wiring the
hook would have exec'd the owner's binary as the member — the precise confusion
this file exists to prevent, asserted in prose and enforced nowhere. Now throws
unless the command resolves to claudeBinIn(run.home).
NEVER_ENV COULD NOT FIRE. It tested an environment that memberEnv builds from
ALLOWED_ENV, so a denied name was already impossible; it was also missing six
credential variables the installed SDK reads. Replaced with the subset check the
reviewer proposed: anything not in ALLOWED_ENV or {HOME, CLAUDE_CONFIG_DIR} is a
leak whatever it is called. Complete by construction, and it cannot rot as the
SDK grows variables — which the denylist provably had already.
Also: one derivation of the binary path instead of two (install resolved from
the email, exec from the home — fine until they disagree), and the constraint
that ALLOWED_ENV may never hold a secret written at the list itself, since
`env K=V` in the argv is visible in /proc/<pid>/cmdline to every account.
Not acted on, and said so in COMMS: their finding that the 711 in 401dcb7 is
inert, and that a retrofit needs a mode pass. Both are theirs. Nor pulled chat
from DEFAULT_ROLE_CAPABILITIES despite agreeing a member currently sees a tile
that 403s — that is the owner's call, not a defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -32,8 +32,18 @@ import { osUserHome, runAs } from './os-user';
|
||||
/** Anthropic's own installer — the same one `scripts/setup.sh` uses for the owner, chosen for auto-update. */
|
||||
const CLAUDE_INSTALL_URL = 'https://claude.ai/install.sh';
|
||||
|
||||
/** Where the installer puts it. Also the first path `claude-manager.ts` probes after `$CLAUDE_BIN`. */
|
||||
export const claudeBinPath = (email: string): string => join(osUserHome(email), '.local', 'bin', 'claude');
|
||||
/**
|
||||
* Where the installer puts it, given a home. Also the first path `claude-manager.ts` probes after
|
||||
* `$CLAUDE_BIN`.
|
||||
*
|
||||
* Takes a home rather than an email because the spawn side only ever has the home — it comes from
|
||||
* `resolveHomeDir`, not from a lookup. One derivation for both sides: installing to one path and exec'ing
|
||||
* another is the kind of divergence that surfaces as "the agent works for some members".
|
||||
*/
|
||||
export const claudeBinIn = (home: string): string => join(home, '.local', 'bin', 'claude');
|
||||
|
||||
/** The same path, for callers that hold an email. */
|
||||
export const claudeBinPath = (email: string): string => claudeBinIn(osUserHome(email));
|
||||
|
||||
/**
|
||||
* The file whose existence means "this account has logged in".
|
||||
|
||||
@@ -1,7 +1,8 @@
|
||||
import type { SpawnOptions, SpawnedProcess } from '@anthropic-ai/claude-agent-sdk';
|
||||
import { spawn } from 'node:child_process';
|
||||
import { join } from 'node:path';
|
||||
import { join, resolve } from 'node:path';
|
||||
import { runAsArgv } from '@@/os-user';
|
||||
import { claudeBinIn } from '@@/os-user-claude';
|
||||
|
||||
// Running a member's agent turn as the member, without a second sidecar.
|
||||
//
|
||||
@@ -45,28 +46,26 @@ export type MemberRun = {
|
||||
*
|
||||
* Deliberately short. `setpriv --init-groups --reset-env` already supplies HOME, USER, LOGNAME, SHELL and
|
||||
* PATH from their passwd entry, so this list is only what the harness itself needs on top of that.
|
||||
*
|
||||
* **Nothing here may ever be a secret.** These are passed as `env K=V …` inside the argv, which means every
|
||||
* one of them is visible in `/proc/<pid>/cmdline` to every account on the box. That is fine for locale and
|
||||
* terminal settings and is not fine for a token — a credential belongs in the member's own `~/.claude`, which
|
||||
* is theirs and mode 600, not on a command line.
|
||||
*/
|
||||
const ALLOWED_ENV = ['LANG', 'LC_ALL', 'TERM', 'TZ', 'NO_COLOR', 'CLAUDE_CODE_ENTRYPOINT'] as const;
|
||||
|
||||
/**
|
||||
* Names that must never reach a member's process, asserted rather than assumed.
|
||||
* Everything the child is allowed to have, beyond the allowlist above.
|
||||
*
|
||||
* Redundant with the allowlist by construction — which is the point. If someone later widens the allowlist,
|
||||
* or the SDK starts merging its own environment into `SpawnOptions.env`, this is what turns a silent
|
||||
* credential leak into a thrown error at the spawn site. The Anthropic three are the owner's proxy
|
||||
* (`user-instance.ts:148-171`); the other two are what make this process more privileged than a shell.
|
||||
* 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.
|
||||
*/
|
||||
const NEVER_ENV = [
|
||||
'ANTHROPIC_BASE_URL',
|
||||
'ANTHROPIC_API_KEY',
|
||||
'ANTHROPIC_AUTH_TOKEN',
|
||||
'_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL',
|
||||
'POSTGRES_URL',
|
||||
'JWT_SECRET',
|
||||
];
|
||||
|
||||
/** Their own install, in their own home. Not this process's `CLAUDE_BIN`, which is the owner's. */
|
||||
export const memberClaudeBin = (home: string): string => join(home, '.local', 'bin', 'claude');
|
||||
const ALSO_ALLOWED = ['HOME', 'CLAUDE_CONFIG_DIR'] as const;
|
||||
|
||||
/**
|
||||
* The `claude` config directory for a member.
|
||||
@@ -107,9 +106,20 @@ export function spawnClaudeAsMember(run: MemberRun): (options: SpawnOptions) =>
|
||||
return ({ command, args, cwd, env, signal }: SpawnOptions): SpawnedProcess => {
|
||||
const childEnv = memberEnv(run, env);
|
||||
|
||||
const leaked = NEVER_ENV.filter((name) => name in childEnv);
|
||||
if (leaked.length) {
|
||||
throw new Error(`refusing to run ${run.osUser}'s agent with owner credentials in env: ${leaked.join(', ')}`);
|
||||
// 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(', ')}`);
|
||||
}
|
||||
|
||||
// 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.
|
||||
const expectedBin = claudeBinIn(run.home);
|
||||
if (resolve(command) !== expectedBin) {
|
||||
throw new Error(`refusing to run ${resolve(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
|
||||
|
||||
Reference in New Issue
Block a user