From ccd119c47afc07e633341c15bbca3c47e2882bec Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 24 Oct 2026 04:15:13 -0700 Subject: [PATCH] Limit shell inheritance of pi provider keys (TASK-139) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Load a shell environment extension on every pi turn. Scrub the configured provider key and default LiteLLM key from model bash and manual shell commands while keeping pi's per-call authentication environment intact. This reduces inheritance only; pi remains unconfined. Written by Sprint Impl, a Commonly agent — https://commonly.me/pods/6a692a1be833c668acdb84cf Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium Security scope: a model shell can still read the key from the pi process environment via parent-process inspection. This limits ordinary inheritance and does not complete TASK-139. Written by Sprint Impl, a Commonly agent — https://commonly.me/pods/6a692a1be833c668acdb84cf Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- cli/__tests__/adapters.pi.shell-env.test.mjs | 91 ++++++++++++++++++++ cli/__tests__/adapters.pi.test.mjs | 9 +- cli/src/lib/adapters/pi-shell-env.mjs | 41 +++++++++ cli/src/lib/adapters/pi.js | 6 ++ docs/cli/README.md | 15 ++++ 5 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 cli/__tests__/adapters.pi.shell-env.test.mjs create mode 100644 cli/src/lib/adapters/pi-shell-env.mjs diff --git a/cli/__tests__/adapters.pi.shell-env.test.mjs b/cli/__tests__/adapters.pi.shell-env.test.mjs new file mode 100644 index 0000000000..21f4c0cb0f --- /dev/null +++ b/cli/__tests__/adapters.pi.shell-env.test.mjs @@ -0,0 +1,91 @@ +import { jest } from '@jest/globals'; +import { readFileSync } from 'node:fs'; +import { createContext, SourceTextModule, SyntheticModule } from 'node:vm'; + +const execute = jest.fn(); +const localExec = jest.fn(); +const createBashTool = jest.fn(() => ({ name: 'bash', execute })); +// pi resolves its own bundled SDK inside its extension loader. It is not a CLI +// dependency, so link that import explicitly while evaluating the shipped file. +const context = createContext({ process }); +const sdk = new SyntheticModule(['createBashTool', 'createLocalBashOperations'], function linkSdk() { + this.setExport('createBashTool', createBashTool); + this.setExport('createLocalBashOperations', () => ({ exec: localExec })); +}, { context }); +const extension = new SourceTextModule(readFileSync(new URL('../src/lib/adapters/pi-shell-env.mjs', import.meta.url), 'utf8'), { context }); +await extension.link((name) => { + expect(name).toBe('@earendil-works/pi-coding-agent'); + return sdk; +}); +await extension.evaluate(); +const install = extension.namespace.default; + +let tool; +let manual; +beforeEach(() => { + jest.clearAllMocks(); + process.env.COMMONLY_PI_PROVIDER_KEY_ENV = 'CUSTOM_PROVIDER_KEY'; + process.env.CUSTOM_PROVIDER_KEY = 'fake-provider-key'; + process.env.COMMONLY_LITELLM_KEY = 'fake-default-key'; + install({ + registerTool(value) { tool = value; }, + on(name, handler) { expect(name).toBe('user_bash'); manual = handler; }, + }); +}); +afterEach(() => { + delete process.env.COMMONLY_PI_PROVIDER_KEY_ENV; + delete process.env.CUSTOM_PROVIDER_KEY; + delete process.env.COMMONLY_LITELLM_KEY; +}); + +test('model bash strips the configured and default keys without mutating provider auth', async () => { + const hook = createBashTool.mock.calls[0][1].spawnHook; + const env = { ...process.env, SAFE_VALUE: 'keep' }; + const result = hook({ command: 'env', cwd: '/seat', env }); + expect(result.command).toBe('env'); + expect(result.cwd).toBe('/seat'); + expect(result.env.CUSTOM_PROVIDER_KEY).toBeUndefined(); + expect(result.env.COMMONLY_LITELLM_KEY).toBeUndefined(); + expect(result.env.COMMONLY_PI_PROVIDER_KEY_ENV).toBeUndefined(); + expect(result.env.SAFE_VALUE).toBe('keep'); + expect(env.CUSTOM_PROVIDER_KEY).toBe('fake-provider-key'); + expect(process.env.CUSTOM_PROVIDER_KEY).toBe('fake-provider-key'); + + execute.mockResolvedValue('result'); + await expect(tool.execute('id', { command: 'pwd' }, 'signal', 'update', { cwd: '/current' })) + .resolves.toBe('result'); + expect(createBashTool.mock.calls[1]).toEqual(['/current', { spawnHook: hook }]); + expect(execute).toHaveBeenCalledWith('id', { command: 'pwd' }, 'signal', 'update', { cwd: '/current' }); +}); + +test('mixed-case excluded names cannot survive as plain-object keys', () => { + const env = { + Custom_Provider_Key: 'fake-custom', commonly_litellm_key: 'fake-default', + Commonly_PI_Provider_Key_Env: 'CUSTOM_PROVIDER_KEY', Path: '/safe', + }; + const hook = createBashTool.mock.calls[0][1].spawnHook; + expect(hook({ env }).env).toEqual({ Path: '/safe' }); + manual().operations.exec('env', '/seat', { env }); + expect(localExec.mock.calls[0][2].env).toEqual({ Path: '/safe' }); + expect(env.Custom_Provider_Key).toBe('fake-custom'); +}); + +test.each([true, false])('manual shell strips credentials with explicit env = %s', async (explicit) => { + const env = { ...process.env, SAFE_VALUE: 'keep' }; + const options = { signal: 'signal', timeout: 10, ...(explicit ? { env } : {}) }; + localExec.mockResolvedValue({ exitCode: 0 }); + await expect(manual().operations.exec('env', '/seat', options)).resolves.toEqual({ exitCode: 0 }); + const [command, cwd, passed] = localExec.mock.calls[0]; + expect([command, cwd, passed.signal, passed.timeout]).toEqual(['env', '/seat', 'signal', 10]); + expect(passed.env.CUSTOM_PROVIDER_KEY).toBeUndefined(); + expect(passed.env.COMMONLY_LITELLM_KEY).toBeUndefined(); + expect(passed.env.COMMONLY_PI_PROVIDER_KEY_ENV).toBeUndefined(); + if (explicit) expect(passed.env.SAFE_VALUE).toBe('keep'); + expect(env.CUSTOM_PROVIDER_KEY).toBe('fake-provider-key'); + expect(process.env.CUSTOM_PROVIDER_KEY).toBe('fake-provider-key'); +}); + +test('missing provider-key metadata refuses extension setup', () => { + delete process.env.COMMONLY_PI_PROVIDER_KEY_ENV; + expect(() => install({})).toThrow('missing provider key environment name'); +}); diff --git a/cli/__tests__/adapters.pi.test.mjs b/cli/__tests__/adapters.pi.test.mjs index b6b640f343..2febc06ea2 100644 --- a/cli/__tests__/adapters.pi.test.mjs +++ b/cli/__tests__/adapters.pi.test.mjs @@ -84,7 +84,9 @@ describe('spawn', () => { expect(args[args.indexOf('--session-id') + 1]).toBe(res.newSessionId); expect(args).not.toContain('--session'); expect(args[args.length - 1]).toContain('hello'); - expect(args).not.toContain('-e'); // no MCP declared → no bridge + expect(args.filter((arg) => arg === '-e')).toHaveLength(1); + expect(args[args.indexOf('-e') + 1]).toMatch(/pi-shell-env\.mjs$/); + expect(opts.env.COMMONLY_PI_PROVIDER_KEY_ENV).toBe('COMMONLY_LITELLM_KEY'); expect(opts.cwd).toBe('/tmp/seat'); expect(opts.env.PI_CODING_AGENT_DIR).toBe(join(home, 'agent')); // Provider by env reference: the key never lands in models.json. @@ -247,7 +249,8 @@ describe('spawn', () => { // No list means no fd 3 either: the pipe exists only when there is a secret. expect(calls[0].opts.stdio).toEqual(['ignore', 'pipe', 'pipe']); expect(calls[0].proc.fd3Payload).toBe(''); - expect(calls[0].args).not.toContain('-e'); + expect(calls[0].args).not.toContain('/x/bridge.mjs'); + expect(calls[0].args[calls[0].args.indexOf('-e') + 1]).toMatch(/pi-shell-env\.mjs$/); expect(JSON.stringify(calls[0].opts.env)).not.toContain('cm_agent_secret'); expect(warn.mock.calls.filter(([line]) => line.includes(GRANT_BROKER_REFUSAL))).toHaveLength(1); } finally { @@ -258,6 +261,8 @@ describe('spawn', () => { test('the environment can name its own provider and model; effort maps onto pi\'s ladder', async () => { const { impl, calls } = makeSpawnImpl({ stdout: assistant('x') }); await pi.spawn('p', baseCtx({ _spawnImpl: impl, env: { DS_KEY: 'k' }, environment: { model: 'deepseek-flash', effort: 'none', provider: { name: 'deepseek', baseUrl: 'https://api.deepseek.com/v1', apiKeyEnv: 'DS_KEY' } } })); + expect(calls[0].opts.env.COMMONLY_PI_PROVIDER_KEY_ENV).toBe('DS_KEY'); + expect(calls[0].opts.env.DS_KEY).toBe('k'); const models = JSON.parse(await readFile(join(home, 'agent', 'models.json'), 'utf8')); expect(models.providers.deepseek.apiKey).toBe('$DS_KEY'); expect(calls[0].args).toEqual(expect.arrayContaining(['--provider', 'deepseek', '--model', 'deepseek-flash', '--thinking', 'off'])); diff --git a/cli/src/lib/adapters/pi-shell-env.mjs b/cli/src/lib/adapters/pi-shell-env.mjs new file mode 100644 index 0000000000..e44f5828f0 --- /dev/null +++ b/cli/src/lib/adapters/pi-shell-env.mjs @@ -0,0 +1,41 @@ +/** + * Keep the provider key available to pi's per-call auth resolver, but out of + * shells started by its model bash tool and by manual ! / !! commands. + * This limits environment inheritance; pi itself is not an OS sandbox. + */ +import { createBashTool, createLocalBashOperations } from '@earendil-works/pi-coding-agent'; + +export default function commonlyShellEnvironment(pi) { + const providerKey = process.env.COMMONLY_PI_PROVIDER_KEY_ENV; + if (!providerKey) throw new Error('pi adapter: missing provider key environment name'); + const excluded = new Set([ + providerKey, 'COMMONLY_LITELLM_KEY', 'COMMONLY_PI_PROVIDER_KEY_ENV', + ].map((key) => key.toUpperCase())); + const cleanEnv = (env) => { + const clean = { ...env }; + // Windows environment lookup ignores case; spreading it loses that behavior. + for (const key of Object.keys(clean)) { + if (excluded.has(key.toUpperCase())) delete clean[key]; + } + return clean; + }; + const spawnHook = (context) => ({ ...context, env: cleanEnv(context.env) }); + const bash = createBashTool(process.cwd(), { spawnHook }); + pi.registerTool({ + ...bash, + execute(id, params, signal, onUpdate, ctx) { + return createBashTool(ctx.cwd, { spawnHook }).execute(id, params, signal, onUpdate, ctx); + }, + }); + const local = createLocalBashOperations(); + pi.on('user_bash', () => ({ + operations: { + exec(command, cwd, options) { + return local.exec(command, cwd, { + ...options, + env: cleanEnv(options.env ?? process.env), + }); + }, + }, + })); +} diff --git a/cli/src/lib/adapters/pi.js b/cli/src/lib/adapters/pi.js index 638853846d..9a409bde64 100644 --- a/cli/src/lib/adapters/pi.js +++ b/cli/src/lib/adapters/pi.js @@ -72,6 +72,7 @@ const THINKING = { none: 'off', off: 'off', minimal: 'minimal', low: 'low', medi export const thinkingFor = (effort) => (effort ? THINKING[String(effort).toLowerCase()] || null : null); const BRIDGE_PATH = join(dirname(fileURLToPath(import.meta.url)), 'pi-commonly-mcp.mjs'); +const SHELL_ENV_PATH = join(dirname(fileURLToPath(import.meta.url)), 'pi-shell-env.mjs'); // Same substitution contract as claude.js / codex.js: ${COMMONLY_*} // placeholders in the declared MCP env are the wrapper's per-(agent, pod) @@ -457,6 +458,9 @@ export default { ...baseEnv, PI_CODING_AGENT_DIR: agentDir, PI_SKIP_VERSION_CHECK: '1', + // A variable NAME, never its value. The always-loaded extension uses + // this to scrub custom provider keys as well as the default LiteLLM key. + COMMONLY_PI_PROVIDER_KEY_ENV: provider.apiKeyEnv, }; // `baseEnv` is normally the process environment, which carries the // bootstrap export. Nothing below this process needs the value: the bridge @@ -472,6 +476,8 @@ export default { prompt: fullPrompt, provider: provider.name, model, thinking, sessionId, isResume, sessionDir, bridge: servers.length ? (ctx._bridgePath || BRIDGE_PATH) : null, }); + // Shell isolation must not depend on whether this turn declares MCP. + args.splice(args.length - 1, 0, '-e', SHELL_ENV_PATH); const reply = await runPi({ args, diff --git a/docs/cli/README.md b/docs/cli/README.md index f088fd751f..3a2b30d9be 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -86,6 +86,21 @@ commonly Requires Node 20+. No compiled build step — source is ESM. +### pi provider credentials + +The pi adapter keeps its configured provider key in pi's environment because +pi resolves authentication on each model call. The always-loaded +`pi-shell-env.mjs` extension removes that key and `COMMONLY_LITELLM_KEY` from +the environments of the model's `bash` tool and manual `!` / `!!` commands. +It loads even when no MCP server is declared. MCP subprocesses already use a +separate environment allowlist in `pi-mcp-client.mjs`. + +pi's `user_bash` event covers manual commands only; a handler for that event +does not protect model tool calls. Both paths are exercised with pi 0.84.1. +This prevents ordinary shell inheritance, not access through pi's own process +memory, parent-process inspection, filesystem reads, or another extension. +The pi adapter still provides no OS sandbox and refuses declared sandboxes. + --- ## Command reference