diff --git a/cli/__tests__/adapters.claude.environment.test.mjs b/cli/__tests__/adapters.claude.environment.test.mjs index 56b39928..7e3c444f 100644 --- a/cli/__tests__/adapters.claude.environment.test.mjs +++ b/cli/__tests__/adapters.claude.environment.test.mjs @@ -265,12 +265,11 @@ describe('claude adapter — ctx.environment', () => { expect(calls).toHaveLength(0); }); - // ── ${COMMONLY_*} placeholder substitution ──────────────────────────────── - // Lets users keep their checked-in env files free of secrets — the - // wrapper substitutes the runtime token + instance URL at spawn time - // from values it already has on hand (the saved token record). + // ── ${COMMONLY_*} native MCP environment expansion ──────────────────────── + // Values exist only in Claude's per-spawn environment. The JSON retains + // placeholders so a cm_agent_* bearer token never exists on disk. - test('${COMMONLY_AGENT_TOKEN} in MCP env values is substituted with ctx.runtimeToken', async () => { + test('${COMMONLY_AGENT_TOKEN} stays literal on disk and is supplied only in child env', async () => { const { impl, calls } = makeSpawnImpl(); const environment = { mcp: [ @@ -295,15 +294,18 @@ describe('claude adapter — ctx.environment', () => { _spawnImpl: impl, }); const cfg = calls[0].config; - expect(cfg.mcpServers.commonly.env.COMMONLY_AGENT_TOKEN).toBe('cm_agent_real_token_12345'); - expect(cfg.mcpServers.commonly.env.COMMONLY_API_URL).toBe('https://api-dev.commonly.me'); - // Substitution is literal — interpolation works inside larger strings. + expect(cfg.mcpServers.commonly.env.COMMONLY_AGENT_TOKEN).toBe('${COMMONLY_AGENT_TOKEN}'); + expect(cfg.mcpServers.commonly.env.COMMONLY_API_URL).toBe('${COMMONLY_API_URL}'); expect(cfg.mcpServers.commonly.env.CUSTOM).toBe( - 'literal-value-cm_agent_real_token_12345-suffix', + 'literal-value-${COMMONLY_AGENT_TOKEN}-suffix', ); + expect(JSON.stringify(cfg)).not.toContain('cm_agent_real_token_12345'); + expect(JSON.stringify(calls[0].args)).not.toContain('cm_agent_real_token_12345'); + expect(calls[0].opts.env.COMMONLY_AGENT_TOKEN).toBe('cm_agent_real_token_12345'); + expect(calls[0].opts.env.COMMONLY_API_URL).toBe('https://api-dev.commonly.me'); }); - test('${COMMONLY_INSTANCE_URL} alias substitutes to the same value as ${COMMONLY_API_URL}', async () => { + test('${COMMONLY_INSTANCE_URL} alias is supplied to native expansion', async () => { const { impl, calls } = makeSpawnImpl(); await claude.spawn('hi', { sessionId: null, @@ -314,10 +316,11 @@ describe('claude adapter — ctx.environment', () => { _spawnImpl: impl, }); const cfg = calls[0].config; - expect(cfg.mcpServers.x.env.U).toBe('http://localhost:5000'); + expect(cfg.mcpServers.x.env.U).toBe('${COMMONLY_INSTANCE_URL}'); + expect(calls[0].opts.env.COMMONLY_INSTANCE_URL).toBe('http://localhost:5000'); }); - test('placeholders in command args + url are also substituted', async () => { + test('placeholders in command args + url stay token-free and receive expansion env', async () => { const { impl, calls } = makeSpawnImpl(); await claude.spawn('hi', { sessionId: null, @@ -341,8 +344,10 @@ describe('claude adapter — ctx.environment', () => { _spawnImpl: impl, }); const cfg = calls[0].config; - expect(cfg.mcpServers['sse-server'].url).toBe('https://api-dev.commonly.me/mcp/sse'); - expect(cfg.mcpServers['arg-server'].args).toEqual(['--token', 'cm_agent_x']); + expect(cfg.mcpServers['sse-server'].url).toBe('${COMMONLY_API_URL}/mcp/sse'); + expect(cfg.mcpServers['arg-server'].args).toEqual(['--token', '${COMMONLY_AGENT_TOKEN}']); + expect(calls[0].opts.env.COMMONLY_API_URL).toBe('https://api-dev.commonly.me'); + expect(calls[0].opts.env.COMMONLY_AGENT_TOKEN).toBe('cm_agent_x'); }); test('unknown ${COMMONLY_*} placeholders are left intact (so misspellings surface as MCP errors, not silent empties)', async () => { @@ -364,13 +369,15 @@ describe('claude adapter — ctx.environment', () => { }); const cfg = calls[0].config; expect(cfg.mcpServers.x.env.TYPO).toBe('${COMMONLY_AGNT_TOKEN}'); + expect(calls[0].opts.env.COMMONLY_AGNT_TOKEN).toBeUndefined(); }); - test('substitution is a no-op when ctx.runtimeToken / instanceUrl are absent (literal env values pass through)', async () => { + test('missing runtime context never creates a token environment entry', async () => { const { impl, calls } = makeSpawnImpl(); await claude.spawn('hi', { sessionId: null, cwd, + env: { PATH: process.env.PATH }, environment: { mcp: [{ name: 'x', @@ -384,7 +391,7 @@ describe('claude adapter — ctx.environment', () => { }); const cfg = calls[0].config; expect(cfg.mcpServers.x.env.LITERAL).toBe('plain-string'); - // Empty token → placeholder left intact (not substituted with empty string). expect(cfg.mcpServers.x.env.PLACEHOLDER).toBe('${COMMONLY_AGENT_TOKEN}'); + expect(calls[0].opts.env.COMMONLY_AGENT_TOKEN).toBeUndefined(); }); }); diff --git a/cli/scripts/security/mock-claude-sandbox-mcp.mjs b/cli/scripts/security/mock-claude-sandbox-mcp.mjs index c0b46376..5f9a421a 100644 --- a/cli/scripts/security/mock-claude-sandbox-mcp.mjs +++ b/cli/scripts/security/mock-claude-sandbox-mcp.mjs @@ -148,9 +148,14 @@ input.on('line', async (line) => { } else if (message.params?.name === 'attempt_shell_read') { output = await exec('/bin/sh', ['-c', `cat -- "$1"`, 'sandbox-attack', args.path]); } else if (message.params?.name === 'inspect_environment') { - output = JSON.stringify(Object.keys(process.env) + const credentialKeys = Object.keys(process.env) .filter((key) => /^(?:COMMONLY_|cm_agent_)/i.test(key)) - .sort()); + .sort(); + output = JSON.stringify({ + keys: credentialKeys, + runtimeTokenResolved: /^cm_agent_[A-Za-z0-9]+$/ + .test(process.env.COMMONLY_AGENT_TOKEN || ''), + }); } else if (message.params?.name === 'record_observation') { output = 'OBSERVATION_RECORDED'; } else { @@ -172,4 +177,3 @@ input.on('line', async (line) => { ); reply(message.id, toolResult(output)); }); - diff --git a/cli/scripts/security/verify-public-claude-seatbelt.mjs b/cli/scripts/security/verify-public-claude-seatbelt.mjs index bc0d83d7..8f983c2f 100644 --- a/cli/scripts/security/verify-public-claude-seatbelt.mjs +++ b/cli/scripts/security/verify-public-claude-seatbelt.mjs @@ -12,12 +12,13 @@ * It never reads or prints real credentials. * * Scope note: the declared MCP server is a trusted capability and receives - * its configured environment by design. Moving the Commonly token out of the - * transient MCP config entirely is phase IV; this phase proves that Claude - * and declared MCP children cannot read unrelated host/workspace secrets. + * its configured environment by design. The Commonly token exists only in + * Claude's one-run environment and is expanded natively; the probe fails if + * it appears in the transient MCP JSON or argv. */ import { randomUUID } from 'crypto'; +import { spawn } from 'child_process'; import { access, appendFile, @@ -29,7 +30,7 @@ import { unlink, writeFile, } from 'fs/promises'; -import { constants } from 'fs'; +import { constants, readFileSync } from 'fs'; import { homedir, tmpdir } from 'os'; import { dirname, join } from 'path'; import { fileURLToPath } from 'url'; @@ -82,6 +83,16 @@ const everyCanary = [ workspaceTokenCanary, inheritedEnvCanary, ]; +let materializedMcpConfig = ''; +let spawnedArgv = ''; +const spawnImpl = (cmd, args, opts) => { + spawnedArgv = JSON.stringify([cmd, args]); + const configIndex = args.indexOf('--mcp-config'); + if (configIndex !== -1) { + materializedMcpConfig = readFileSync(args[configIndex + 1], 'utf8'); + } + return spawn(cmd, args, opts); +}; const parseCapture = (text) => text .split('\n') @@ -143,6 +154,7 @@ try { }, runtimeToken, timeoutMs: 5 * 60 * 1000, + _spawnImpl: spawnImpl, }); const capture = await readFile(captureFile, 'utf8'); @@ -190,6 +202,12 @@ try { fail('SANDBOX FAILURE: a protected canary crossed into model/MCP output'); } } + if (!materializedMcpConfig.includes('${COMMONLY_AGENT_TOKEN}')) { + fail('MCP config did not preserve the native environment placeholder'); + } + if (materializedMcpConfig.includes(runtimeToken) || spawnedArgv.includes(runtimeToken)) { + fail('TOKEN MATERIALIZATION FAILURE: runtime token entered MCP config or argv'); + } if ((await readFile(allowedWrite, 'utf8')) !== 'ALLOWED') { fail('SANDBOX FAILURE: workspace write did not succeed'); @@ -218,6 +236,15 @@ try { .test(environmentAttempt?.output || '')) { fail('SANDBOX FAILURE: host credential-shaped environment reached the MCP child'); } + let inspectedEnvironment; + try { + inspectedEnvironment = JSON.parse(environmentAttempt?.output || '{}'); + } catch { + fail('Attack was incomplete: MCP child environment result was not JSON'); + } + if (inspectedEnvironment.runtimeTokenResolved !== true) { + fail('MCP env expansion failed: server did not receive a resolved runtime token'); + } const redactedReport = everyCanary.reduce( (text, canary) => text.replaceAll(canary, '[REDACTED_CANARY]'), diff --git a/cli/src/commands/agent.js b/cli/src/commands/agent.js index 6f801ac2..a2b8c2f5 100644 --- a/cli/src/commands/agent.js +++ b/cli/src/commands/agent.js @@ -527,9 +527,10 @@ export const performRun = ({ env: process.env, memoryLongTerm, environment, - // Runtime context the adapter uses to substitute ${COMMONLY_AGENT_TOKEN} - // / ${COMMONLY_API_URL} placeholders in MCP env values, command args, - // and URLs. Lets users keep tokens out of their checked-in env files. + // Runtime context the Claude/Codex adapters expose only to their + // per-spawn MCP environment. Claude keeps ${COMMONLY_*} placeholders + // literal on disk and lets its native MCP parser expand them, so the + // bearer token never enters the generated config file. runtimeToken: token, instanceUrl, agentName, diff --git a/cli/src/lib/adapters/claude.js b/cli/src/lib/adapters/claude.js index 9551668f..968cd3c3 100644 --- a/cli/src/lib/adapters/claude.js +++ b/cli/src/lib/adapters/claude.js @@ -173,16 +173,20 @@ const preparePublicClaudeState = async (ctx) => { return { statePath, tmpPath, configPath }; }; -const buildPublicClaudeEnv = (input, state) => { - const output = {}; - for (const [key, value] of Object.entries(input || {})) { - if (PUBLIC_SAFE_ENV.test(key)) output[key] = value; +const buildClaudeEnv = (input, state, expansionEnv = {}) => { + const source = input || process.env; + const output = state ? {} : { ...source }; + if (state) { + for (const [key, value] of Object.entries(source)) { + if (PUBLIC_SAFE_ENV.test(key)) output[key] = value; + } + output.HOME = state.statePath; + output.TMPDIR = state.tmpPath; + output.XDG_CACHE_HOME = join(state.statePath, '.cache'); + output.XDG_CONFIG_HOME = join(state.statePath, '.config'); + output.XDG_DATA_HOME = join(state.statePath, '.local', 'share'); } - output.HOME = state.statePath; - output.TMPDIR = state.tmpPath; - output.XDG_CACHE_HOME = join(state.statePath, '.cache'); - output.XDG_CONFIG_HOME = join(state.statePath, '.config'); - output.XDG_DATA_HOME = join(state.statePath, '.local', 'share'); + Object.assign(output, expansionEnv); return output; }; @@ -247,80 +251,76 @@ const runClaude = ({ cmd, args, cwd, env, timeoutMs, spawnImpl = childSpawn }) = // ── MCP config write — claude consumes this via --mcp-config ───────── -// Substitute Commonly-supplied placeholders in MCP env values, command args, -// and URLs so users don't have to hand-paste secrets into their env file -// every time they re-attach. Surfaced during the 2026-04-17 cross-agent demo: -// every spec referencing commonly-mcp had to be rewritten with the agent's -// runtime token after attach, because the token is minted at attach time and -// only known to the wrapper. +// Keep Commonly placeholders in the MCP JSON and expose their values only in +// Claude's per-spawn environment. Claude Code natively expands ${VAR} in MCP +// command/args/env/url fields. Substituting here used to materialize the raw +// cm_agent_* bearer token in a transient JSON file, which made the token +// readable to any co-confined child allowed to read that config directory. // -// Recognised placeholders (substituted everywhere a string appears in the -// MCP config): +// Recognised placeholders: // ${COMMONLY_AGENT_TOKEN} — the per-(agent, pod) cm_agent_* runtime token // ${COMMONLY_API_URL} — the instance URL the agent is attached to // ${COMMONLY_INSTANCE_URL} — alias for COMMONLY_API_URL (clearer in context) // -// Substitution is one-pass + literal — no nested expansion, no shell quoting. -// Unknown placeholders are left intact so the user sees a clear runtime error -// from the MCP server rather than a silent empty string. -const SUBSTITUTION_KEYS = ['COMMONLY_AGENT_TOKEN', 'COMMONLY_API_URL', 'COMMONLY_INSTANCE_URL']; -const PLACEHOLDER_RE = /\$\{(COMMONLY_[A-Z_]+)\}/g; - -const substitutePlaceholders = (value, ctx) => { - if (typeof value !== 'string') return value; - if (!value.includes('${COMMONLY_')) return value; - const subs = { +// Only values actually referenced by this MCP declaration are added to the +// child environment. Unknown placeholders remain in JSON so Claude's parser +// fails clearly instead of receiving a silent empty string. +const buildMcpExpansionEnv = (mcpConfig, ctx) => { + const serialized = JSON.stringify(mcpConfig); + const values = { COMMONLY_AGENT_TOKEN: ctx.runtimeToken || '', COMMONLY_API_URL: ctx.instanceUrl || '', COMMONLY_INSTANCE_URL: ctx.instanceUrl || '', }; - return value.replace(PLACEHOLDER_RE, (whole, key) => ( - SUBSTITUTION_KEYS.includes(key) && subs[key] ? subs[key] : whole - )); + const output = {}; + for (const [key, value] of Object.entries(values)) { + if (value && serialized.includes(`\${${key}}`)) output[key] = value; + } + return output; }; -const buildMcpConfig = (mcpServers, ctx = {}) => { +const buildMcpConfig = (mcpServers) => { // Shape: `{ mcpServers: { : { ... } } }` — the standard MCP client // config, which claude's `--mcp-config` reads directly. const mcpServersMap = {}; for (const server of mcpServers) { const entry = { type: server.transport || 'stdio' }; - if (server.url) entry.url = substitutePlaceholders(server.url, ctx); + if (server.url) entry.url = server.url; if (server.command) { const [command, ...args] = server.command; entry.command = command; - if (args.length) entry.args = args.map((a) => substitutePlaceholders(a, ctx)); - } - if (server.env) { - entry.env = {}; - for (const [k, v] of Object.entries(server.env)) { - entry.env[k] = substitutePlaceholders(v, ctx); - } + if (args.length) entry.args = args; } + if (server.env) entry.env = { ...server.env }; mcpServersMap[server.name] = entry; } return { mcpServers: mcpServersMap }; }; // Materialized once per spawn outside the workspace, then removed in the -// spawn's finally block. Keeping this file under made the literal -// cm_agent_* bearer token readable to any public agent allowed Read(./**). -// The directory and file modes are explicit because this file briefly carries -// live credentials; relying on the caller's umask is not a security boundary. +// spawn's finally block. The JSON contains placeholders, never resolved +// Commonly credentials; Claude expands them from its one-run environment. +// Directory/file modes stay strict because this config still declares the +// agent's trusted MCP capabilities and endpoints. const createMcpConfig = async (mcpServers, ctx = {}) => { const dir = await mkdtemp(join(tmpdir(), 'commonly-claude-mcp-')); const file = join(dir, 'mcp-config.json'); + const config = buildMcpConfig(mcpServers); try { await chmod(dir, 0o700); await writeFile( file, - JSON.stringify(buildMcpConfig(mcpServers, ctx), null, 2), + JSON.stringify(config, null, 2), { encoding: 'utf8', mode: 0o600 }, ); // writeFile's mode only applies when creating. Pin the final mode too so // this stays correct if the implementation ever starts reusing the path. await chmod(file, 0o600); - return { dir, file }; + return { + dir, + file, + expansionEnv: buildMcpExpansionEnv(config, ctx), + }; } catch (err) { try { await rm(dir, { recursive: true, force: true }); } catch { /* ignore */ } throw err; @@ -355,7 +355,7 @@ const resolveClaudePath = () => { const prepareArgv = async (innerArgv, ctx) => { const env = ctx.environment; - if (!env) return { cmd: 'claude', args: innerArgv, env: ctx.env }; + if (!env) return { cmd: 'claude', args: innerArgv, env: ctx.claudeEnv }; let allowedPatterns = []; if (Array.isArray(env.mcp) && env.mcp.length > 0) { @@ -414,7 +414,7 @@ const prepareArgv = async (innerArgv, ctx) => { return { cmd: wrapped[0], args: wrapped.slice(1), - env: ctx.publicClaudeEnv, + env: ctx.claudeEnv, }; } @@ -427,10 +427,10 @@ const prepareArgv = async (innerArgv, ctx) => { workspacePath: ctx.cwd, readOnlyPaths: ctx.mcpConfigDir ? [ctx.mcpConfigDir] : [], }); - return { cmd: wrapped[0], args: wrapped.slice(1), env: ctx.env }; + return { cmd: wrapped[0], args: wrapped.slice(1), env: ctx.claudeEnv }; } - return { cmd: 'claude', args: innerArgv, env: ctx.env }; + return { cmd: 'claude', args: innerArgv, env: ctx.claudeEnv }; }; export default { @@ -497,9 +497,11 @@ export default { mcpConfigPath: mcpConfig?.file || null, mcpConfigDir: mcpConfig?.dir || null, publicClaudeState, - publicClaudeEnv: publicClaudeState - ? buildPublicClaudeEnv(ctx.env, publicClaudeState) - : ctx.env, + claudeEnv: buildClaudeEnv( + ctx.env, + publicClaudeState, + mcpConfig?.expansionEnv, + ), }; const { cmd, args, env } = await prepareArgv(baseArgs, spawnCtx); const stdout = await runClaude({ @@ -524,9 +526,11 @@ export default { mcpConfigPath: mcpConfig?.file || null, mcpConfigDir: mcpConfig?.dir || null, publicClaudeState, - publicClaudeEnv: publicClaudeState - ? buildPublicClaudeEnv(ctx.env, publicClaudeState) - : ctx.env, + claudeEnv: buildClaudeEnv( + ctx.env, + publicClaudeState, + mcpConfig?.expansionEnv, + ), }); const stdout = await runClaude({ cmd: retry.cmd,