Skip to content

fix(core): strip daemon secrets from hook and tool-discovery child env #7527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 3 commits into from
Jul 23, 2026
Merged

fix(core): strip daemon secrets from hook and tool-discovery child env #7527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3dee6b5
fix(core): strip daemon secrets from hook and tool-discovery child env
chinesepowered Jul 22, 2026
01b5f3a
fix(core): normalize Windows PATH on the tool-registry child env
chinesepowered Jul 22, 2026
53f891b
Merge branch 'main' into fix/sanitize-child-env-hooks-registry
wenshao Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions packages/core/src/hooks/hookRunner.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,49 @@ describe('HookRunner', () => {
expect(mockSpawn).toHaveBeenCalled();
});

it('strips Qwen-internal daemon secrets from the hook child env (#6601)', async () => {
const originalServerToken = process.env['QWEN_SERVER_TOKEN'];
const originalDaemonToken = process.env['QWEN_DAEMON_TOKEN'];
process.env['QWEN_SERVER_TOKEN'] = 'serve-secret';
process.env['QWEN_DAEMON_TOKEN'] = 'daemon-secret';
try {
const mockProcess = createMockProcess(0, 'hello');
mockSpawn.mockImplementation(() => mockProcess);

const hookConfig: HookConfig = {
type: HookType.Command,
command: 'echo hello',
source: HooksConfigSource.Project,
};

await hookRunner.executeHook(
hookConfig,
HookEventName.PreToolUse,
createMockInput(),
);

const spawnOptions = mockSpawn.mock.calls[0][2];
// A user-authored hook command is a child process launched on the
// agent's behalf; internal daemon secrets must not leak into it.
expect(spawnOptions.env['QWEN_SERVER_TOKEN']).toBeUndefined();
expect(spawnOptions.env['QWEN_DAEMON_TOKEN']).toBeUndefined();
// Benign inherited env and the hook's own vars are still present.
expect(spawnOptions.env['PATH']).toBeDefined();
expect(spawnOptions.env['QWEN_PROJECT_DIR']).toBe('/test');
} finally {
if (originalServerToken === undefined) {
delete process.env['QWEN_SERVER_TOKEN'];
} else {
process.env['QWEN_SERVER_TOKEN'] = originalServerToken;
}
if (originalDaemonToken === undefined) {
delete process.env['QWEN_DAEMON_TOKEN'];
} else {
process.env['QWEN_DAEMON_TOKEN'] = originalDaemonToken;
}
}
});

it('should return failure for non-zero exit code', async () => {
const mockProcess = createMockProcess(1, '', 'error');
mockSpawn.mockImplementation(() => mockProcess);
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/hooks/hookRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { PromptHookRunner } from './promptHookRunner.js';
import { AsyncHookRegistry, generateHookId } from './asyncHookRegistry.js';
import type { Config } from '../config/config.js';
import { getShellContextEnvVars } from '../utils/shellContextEnv.js';
import { sanitizeChildEnv } from '../utils/sanitize-child-env.js';

const debugLogger = createDebugLogger('TRUSTED_HOOKS');

Expand Down Expand Up @@ -584,7 +585,9 @@ export class HookRunner {
);

const env = {
...process.env,
// Hook commands are child processes launched on the agent's behalf,
// so they must not inherit Qwen-internal daemon secrets.
...sanitizeChildEnv(process.env),

@qwen-code-ci-bot qwen-code-ci-bot Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Missing normalizePathEnvForWindows wrapper — inconsistent with sibling spawn sites

Both tool-registry.ts spawn calls now correctly wrap with normalizePathEnvForWindows(sanitizeChildEnv(process.env)), but this hookRunner.ts site applies only sanitizeChildEnv without the Windows PATH normalization. Every other explicit-env spawn site in the codebase (tool-registry.ts:72, :608, mcp-client.ts:2152, shellExecutionService.ts:768,1471) uses both wrappers.

The Windows PATH issue is pre-existing at this site (the old ...process.env spread into a plain object had the same effect), but since this PR touches this exact line and establishes the convention at the other sites, applying it here would be consistent.

Failure scenario: on Windows, process.env has case-variant PATH keys (Path, PATH, path). Spreading into a plain object loses the Proxy's case-insensitive merging. Hook commands like npx, gh, npm could fail with "command not found" while regular shell, tool discovery, tool calls, and MCP servers all resolve correctly.

Suggested change
...sanitizeChildEnv(process.env),
...normalizePathEnvForWindows(sanitizeChildEnv(process.env)),

— qwen3.7-max via Qwen Code /review

@chinesepowered chinesepowered Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding here, and the premise is off by one — hookRunner.ts is not the only site without the wrapper. Full audit on this branch:

tool-registry.ts:72             normalizePathEnvForWindows(sanitizeChildEnv(...))   ← changed by this PR
tool-registry.ts:608            normalizePathEnvForWindows(sanitizeChildEnv(...))   ← changed by this PR
mcp-client.ts:2152              normalizePathEnvForWindows(sanitizeChildEnv(...))
shellExecutionService.ts:768    normalizePathEnvForWindows(sanitizeChildEnv(...))
shellExecutionService.ts:1471   normalizePathEnvForWindows(sanitizeChildEnv(...))
monitor.ts:369                  sanitizeChildEnv(...)                               ← untouched, same shape
hookRunner.ts:590               sanitizeChildEnv(...)                               ← untouched, same shape

monitor.ts:369 spreads sanitizeChildEnv(process.env) into an explicit env object exactly like hookRunner.ts does, and both got that shape from #7256. So wrapping only hookRunner would leave monitor.ts as the outlier instead — the inconsistency doesn't get resolved, it just moves.

The distinction that decides it for me is what this PR changed. The two tool-registry.ts spawns previously passed no env at all, so Node inherited natively and Windows resolved its case-variant PATH keys itself; making env explicit is what gave that up, so restoring it belongs here. hookRunner.ts and monitor.ts already built explicit env objects before this branch and never had the native-inheritance behavior to lose — their Windows PATH exposure is identical before and after this PR.

That makes it a genuine pre-existing bug at two sites rather than a regression at one, and I'd rather fix both together in a PR that says so than half of it as a side effect of a secrets fix. Happy to open that immediately if you want it.

tool-registry + hookRunner suites 82/82; eslint clean.

GEMINI_PROJECT_DIR: input.cwd,
CLAUDE_PROJECT_DIR: input.cwd, // For compatibility
QWEN_PROJECT_DIR: input.cwd, // For Qwen Code compatibility
Expand Down
90 changes: 90 additions & 0 deletions packages/core/src/tools/tool-registry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,96 @@ describe('ToolRegistry', () => {
});
});

it('strips Qwen-internal daemon secrets from the discovery and tool-call child env (#6601)', async () => {
const originalServerToken = process.env['QWEN_SERVER_TOKEN'];
const originalDaemonToken = process.env['QWEN_DAEMON_TOKEN'];
process.env['QWEN_SERVER_TOKEN'] = 'serve-secret';
process.env['QWEN_DAEMON_TOKEN'] = 'daemon-secret';
try {
mockConfigGetToolDiscoveryCommand.mockReturnValue(
'my-discovery-command',
);
vi.spyOn(config, 'getToolCallCommand').mockReturnValue(
'my-call-command',
);

const toolDeclaration: FunctionDeclaration = {
name: 'secret-probe',
description: 'A tool',
parametersJsonSchema: { type: 'object', properties: {} },
};

const mockSpawn = vi.mocked(spawn);
const discoveryProcess = {
stdout: { on: vi.fn(), removeListener: vi.fn() },
stderr: { on: vi.fn(), removeListener: vi.fn() },
on: vi.fn(),
};
mockSpawn.mockReturnValueOnce(discoveryProcess as any);
discoveryProcess.stdout.on.mockImplementation((event, callback) => {
if (event === 'data') {
callback(
Buffer.from(
JSON.stringify([{ functionDeclarations: [toolDeclaration] }]),
),
);
}
});
discoveryProcess.on.mockImplementation((event, callback) => {
if (event === 'close') {
callback(0);
}
});

await toolRegistry.discoverAllTools();
const discoveredTool = toolRegistry.getTool('secret-probe');
expect(discoveredTool).toBeDefined();

const executionProcess = {
stdout: { on: vi.fn(), removeListener: vi.fn() },
stderr: { on: vi.fn(), removeListener: vi.fn() },
stdin: { write: vi.fn(), end: vi.fn() },
on: vi.fn(),
connected: true,
disconnect: vi.fn(),
removeListener: vi.fn(),
};
mockSpawn.mockReturnValueOnce(executionProcess as any);
executionProcess.on.mockImplementation((event, callback) => {
if (event === 'close') {
callback(0);
}
});

await (discoveredTool as DiscoveredTool)
.build({})
.execute(new AbortController().signal);

// Both the discovery command and the tool-call command are child
// processes launched on the agent's behalf, so neither may inherit
// the internal daemon secrets.
for (const call of mockSpawn.mock.calls) {
const env = (call[2] as { env: NodeJS.ProcessEnv }).env;
expect(env['QWEN_SERVER_TOKEN']).toBeUndefined();
expect(env['QWEN_DAEMON_TOKEN']).toBeUndefined();
// Benign inherited env is preserved.
expect(env['PATH']).toBeDefined();
}
expect(mockSpawn.mock.calls).toHaveLength(2);
} finally {
if (originalServerToken === undefined) {
delete process.env['QWEN_SERVER_TOKEN'];
} else {
process.env['QWEN_SERVER_TOKEN'] = originalServerToken;
}
if (originalDaemonToken === undefined) {
delete process.env['QWEN_DAEMON_TOKEN'];
} else {
process.env['QWEN_DAEMON_TOKEN'] = originalDaemonToken;
}
}
});

it('should return a DISCOVERED_TOOL_EXECUTION_ERROR on tool failure', async () => {
const discoveryCommand = 'my-discovery-command';
mockConfigGetToolDiscoveryCommand.mockReturnValue(discoveryCommand);
Expand Down
18 changes: 16 additions & 2 deletions packages/core/src/tools/tool-registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import { ToolErrorType } from './tool-error.js';
import { safeJsonStringify } from '../utils/safeJsonStringify.js';
import type { EventEmitter } from 'node:events';
import { createDebugLogger } from '../utils/debugLogger.js';
import { sanitizeChildEnv } from '../utils/sanitize-child-env.js';
import { normalizePathEnvForWindows } from '../utils/windowsPath.js';
import type { ReadResourceResult } from '@modelcontextprotocol/sdk/types.js';
import { normalizeMcpToolName } from '../utils/tool-name-utils.js';

Expand Down Expand Up @@ -61,7 +63,14 @@ class DiscoveredToolInvocation extends BaseToolInvocation<
_updateOutput?: (output: ToolResultDisplay) => void,
): Promise<ToolResult> {
const callCommand = this.config.getToolCallCommand()!;
const child = spawn(callCommand, [this.toolName]);
// The user-configured tool-call command is a child process launched on the
// agent's behalf, so it must not inherit Qwen-internal daemon secrets.
// Passing `env` explicitly loses the native inheritance that resolved
// Windows' case-insensitive PATH keys, so normalize as the shell and MCP
// spawn sites do (a no-op off win32).
const child = spawn(callCommand, [this.toolName], {
env: normalizePathEnvForWindows(sanitizeChildEnv(process.env)),
});
Comment on lines +71 to +73

@qwen-code-ci-bot qwen-code-ci-bot Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Missing normalizePathEnvForWindows wrapper — inconsistent with sibling spawn sites

Both tool-registry.ts spawn calls previously had no env option, so Node.js inherited the parent environment natively (the Windows C runtime handles case-insensitive PATH correctly). This PR introduces explicit env objects. On Windows, process.env can have multiple PATH-like keys with different casings (e.g. Path and PATH). When passed as an explicit env object without normalization, the child process may receive duplicate PATH keys, and the Windows runtime may select the wrong one.

The sibling sites in shellExecutionService.ts (lines 768, 1471) and mcp-client.ts (line 2152) both use normalizePathEnvForWindows(sanitizeChildEnv(process.env)) to prevent this.

Suggested change
const child = spawn(callCommand, [this.toolName], {
env: sanitizeChildEnv(process.env),
});
// Normalize PATH keys for Windows before passing to the child process.
const child = spawn(callCommand, [this.toolName], {
env: normalizePathEnvForWindows(sanitizeChildEnv(process.env)),
});

— qwen3.7-max via Qwen Code /review

child.stdin.write(JSON.stringify(this.params));
child.stdin.end();

Expand Down Expand Up @@ -592,7 +601,12 @@ export class ToolRegistry {
'Tool discovery command is empty or contains only whitespace.',
);
}
const proc = spawn(cmdParts[0] as string, cmdParts.slice(1) as string[]);
// Same as the tool-call command above: the discovery command is
// agent-launched, must not inherit Qwen-internal daemon secrets, and
// needs the Windows PATH normalization that comes with an explicit env.
const proc = spawn(cmdParts[0] as string, cmdParts.slice(1) as string[], {
env: normalizePathEnvForWindows(sanitizeChildEnv(process.env)),
});
Comment on lines +607 to +609

@qwen-code-ci-bot qwen-code-ci-bot Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Same normalizePathEnvForWindows gap as the tool-call spawn above — the discovery command has the same Windows PATH risk.

Suggested change
const proc = spawn(cmdParts[0] as string, cmdParts.slice(1) as string[], {
env: sanitizeChildEnv(process.env),
});
// Normalize PATH keys for Windows before passing to the child process.
const proc = spawn(cmdParts[0] as string, cmdParts.slice(1) as string[], {
env: normalizePathEnvForWindows(sanitizeChildEnv(process.env)),
});

— qwen3.7-max via Qwen Code /review

let stdout = '';
const stdoutDecoder = new StringDecoder('utf8');
let stderr = '';
Expand Down
Loading