fix(acp): scrub simple env for spawned children (#5395)

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<typeof import('node:child_process')>();
+  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<string>(['QWEN_SERVER_TOKEN']);
+const SCRUBBED = new Set<string>(['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<string>([
       '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');

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<string> = new Set([
   'QWEN_SERVER_TOKEN',
+  'QWEN_CODE_SIMPLE',
 ]);
 
 /**
@@ -259,9 +264,8 @@ const SCRUBBED_CHILD_ENV_KEYS: ReadonlySet<string> = 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