From 3db3cd5d17661d57eaab0feec272a50f90c9d03a Mon Sep 17 00:00:00 2001 From: tt-a1i <53142663+tt-a1i@users.noreply.github.com> Date: Fri, 19 Jun 2026 15:16:19 +0800 Subject: [PATCH] fix(acp): scrub simple env for spawned children (#5395) --- packages/acp-bridge/src/spawnChannel.test.ts | 100 ++++++++++++++++++- packages/acp-bridge/src/spawnChannel.ts | 20 ++-- 2 files changed, 108 insertions(+), 12 deletions(-) diff --git a/packages/acp-bridge/src/spawnChannel.test.ts b/packages/acp-bridge/src/spawnChannel.test.ts index a0d058299b..666c00f9d0 100644 --- a/packages/acp-bridge/src/spawnChannel.test.ts +++ b/packages/acp-bridge/src/spawnChannel.test.ts @@ -20,6 +20,8 @@ * - `QWEN_SERVER_TOKEN` (the daemon's own bearer token) leaking into * the spawned agent's environment, where prompt-injection could * turn the agent into an authenticated client of its own daemon. + * - `QWEN_CODE_SIMPLE` leaking from the daemon/IDE process into + * per-session ACP children, where it would silently suppress skills. * - An `overrides` map smuggling a scrubbed key BACK into the child * env (defense-in-depth — operators / embedders can pass overrides, * but the denylist still wins). @@ -30,13 +32,85 @@ * Each branch listed below is now regression-guarded by an assertion. */ -import { describe, expect, it, vi } from 'vitest'; +import { EventEmitter } from 'node:events'; +import type { ChildProcess } from 'node:child_process'; +import { PassThrough } from 'node:stream'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const mockSpawn = vi.hoisted(() => vi.fn()); +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawn: mockSpawn, + }; +}); + import { + createSpawnChannelFactory, createStderrForwarder, getAcpMemoryArgs, scrubChildEnv, } from './spawnChannel.js'; +function createFakeChildProcess(): ChildProcess { + return Object.assign(new EventEmitter(), { + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + pid: 12345, + exitCode: null, + signalCode: null, + kill: vi.fn(() => true), + }) as unknown as ChildProcess; +} + +describe('createSpawnChannelFactory env policy', () => { + const originalArgv1 = process.argv[1]; + let originalSimple: string | undefined; + let originalServerToken: string | undefined; + + beforeEach(() => { + mockSpawn.mockReset(); + originalSimple = process.env['QWEN_CODE_SIMPLE']; + originalServerToken = process.env['QWEN_SERVER_TOKEN']; + process.argv[1] = '/tmp/qwen.js'; + process.env['QWEN_CODE_SIMPLE'] = '1'; + process.env['QWEN_SERVER_TOKEN'] = 'secret'; + }); + + afterEach(() => { + process.argv[1] = originalArgv1; + if (originalSimple === undefined) { + delete process.env['QWEN_CODE_SIMPLE']; + } else { + process.env['QWEN_CODE_SIMPLE'] = originalSimple; + } + if (originalServerToken === undefined) { + delete process.env['QWEN_SERVER_TOKEN']; + } else { + process.env['QWEN_SERVER_TOKEN'] = originalServerToken; + } + }); + + it('scrubs daemon-only env vars from the spawned ACP child', async () => { + mockSpawn.mockReturnValue(createFakeChildProcess()); + + const factory = createSpawnChannelFactory(); + await factory('/tmp/project', { + QWEN_CODE_SIMPLE: '1', + QWEN_SERVER_TOKEN: 'override-secret', + }); + + const spawnOptions = mockSpawn.mock.calls[0]?.[2] as + | { env?: NodeJS.ProcessEnv } + | undefined; + expect(spawnOptions?.env).not.toHaveProperty('QWEN_CODE_SIMPLE'); + expect(spawnOptions?.env).not.toHaveProperty('QWEN_SERVER_TOKEN'); + expect(spawnOptions?.env?.['QWEN_CODE_NO_RELAUNCH']).toBe('true'); + }); +}); + describe('createStderrForwarder', () => { it('calls onDiagnosticLine for each complete line', () => { const captured: Array<{ line: string; level?: string }> = []; @@ -133,7 +207,7 @@ describe('createStderrForwarder', () => { // of any current production denylist. The multi-key test below // forward-guards expansion when a future sandboxed-agent mode grows // the production set per the WARNING on `SCRUBBED_CHILD_ENV_KEYS`. -const SCRUBBED = new Set(['QWEN_SERVER_TOKEN']); +const SCRUBBED = new Set(['QWEN_SERVER_TOKEN', 'QWEN_CODE_SIMPLE']); describe('scrubChildEnv (defaultSpawnChannelFactory env policy)', () => { it('shallow-clones source — never aliases into the live process.env', () => { @@ -150,6 +224,13 @@ describe('scrubChildEnv (defaultSpawnChannelFactory env policy)', () => { expect(result['PATH']).toBe('/usr/bin'); }); + it('strips QWEN_CODE_SIMPLE from the child env', () => { + const source = { QWEN_CODE_SIMPLE: '1', PATH: '/usr/bin' }; + const result = scrubChildEnv(source, SCRUBBED); + expect(result).not.toHaveProperty('QWEN_CODE_SIMPLE'); + expect(result['PATH']).toBe('/usr/bin'); + }); + it('passes through non-scrubbed env vars unchanged', () => { const source = { OPENAI_API_KEY: 'sk-test', @@ -187,6 +268,14 @@ describe('scrubChildEnv (defaultSpawnChannelFactory env policy)', () => { expect(result).not.toHaveProperty('QWEN_SERVER_TOKEN'); }); + it('overrides CANNOT re-introduce QWEN_CODE_SIMPLE', () => { + const source = { PATH: '/usr/bin' }; + const result = scrubChildEnv(source, SCRUBBED, { + QWEN_CODE_SIMPLE: '1', + }); + expect(result).not.toHaveProperty('QWEN_CODE_SIMPLE'); + }); + it('overrides CANNOT undo the scrub by setting undefined for a scrubbed key', () => { // Edge case: `undefined` value would normally delete; but for a // scrubbed key, the `continue` in the loop short-circuits BEFORE @@ -229,17 +318,20 @@ describe('scrubChildEnv (defaultSpawnChannelFactory env policy)', () => { // anticipates), this verifies the loop handles multiple keys. const sandboxScrub = new Set([ 'QWEN_SERVER_TOKEN', + 'QWEN_CODE_SIMPLE', 'AWS_SECRET_ACCESS_KEY', 'OPENAI_API_KEY', ]); const source = { QWEN_SERVER_TOKEN: 't1', - AWS_SECRET_ACCESS_KEY: 't2', - OPENAI_API_KEY: 't3', + QWEN_CODE_SIMPLE: 't2', + AWS_SECRET_ACCESS_KEY: 't3', + OPENAI_API_KEY: 't4', PATH: '/usr/bin', }; const result = scrubChildEnv(source, sandboxScrub); expect(result).not.toHaveProperty('QWEN_SERVER_TOKEN'); + expect(result).not.toHaveProperty('QWEN_CODE_SIMPLE'); expect(result).not.toHaveProperty('AWS_SECRET_ACCESS_KEY'); expect(result).not.toHaveProperty('OPENAI_API_KEY'); expect(result['PATH']).toBe('/usr/bin'); diff --git a/packages/acp-bridge/src/spawnChannel.ts b/packages/acp-bridge/src/spawnChannel.ts index 2d77b98e6e..7887d5b683 100644 --- a/packages/acp-bridge/src/spawnChannel.ts +++ b/packages/acp-bridge/src/spawnChannel.ts @@ -230,11 +230,15 @@ const KILL_HARD_DEADLINE_MS = 10_000; * environment. Everything else is passed through — see the * threat-model rationale at the call site in `defaultSpawnChannelFactory`. * - * Currently just `QWEN_SERVER_TOKEN`: the daemon's own bearer token, - * which the agent doesn't need (it speaks to the daemon over stdio, - * not HTTP). Leaving it in the child's env would let prompt injection - * turn the agent into an authenticated client of its own daemon — an - * escalation the agent doesn't otherwise have. + * `QWEN_SERVER_TOKEN`: the daemon's own bearer token, which the agent + * doesn't need (it speaks to the daemon over stdio, not HTTP). Leaving + * it in the child's env would let prompt injection turn the agent into + * an authenticated client of its own daemon — an escalation the agent + * doesn't otherwise have. + * + * `QWEN_CODE_SIMPLE`: an invocation-level bare-mode override. Letting a + * daemon or IDE environment leak it into per-session `qwen --acp` + * children silently disables skills in those children. * * **WARNING**: this denylist is correct *only because the agent * already has unrestricted shell-tool access* — anything in the env @@ -250,6 +254,7 @@ const KILL_HARD_DEADLINE_MS = 10_000; */ const SCRUBBED_CHILD_ENV_KEYS: ReadonlySet = new Set([ 'QWEN_SERVER_TOKEN', + 'QWEN_CODE_SIMPLE', ]); /** @@ -259,9 +264,8 @@ const SCRUBBED_CHILD_ENV_KEYS: ReadonlySet = new Set([ * * 1. Start from a shallow clone of `source` (no aliasing into the * daemon's `process.env`). - * 2. Delete every key listed in `scrubbed` (the daemon-internal secret - * denylist — currently just `QWEN_SERVER_TOKEN`, see security - * rationale on the constant). + * 2. Delete every key listed in `scrubbed` (the daemon-internal + * child-env denylist; see the rationale on the constant). * 3. Apply `overrides` per-handle. `undefined` value deletes the key * (lets an embedded caller scrub a stale inherited var without * mutating the daemon's global `process.env`). Anything else