fix(core): strip daemon secrets from hook and tool-discovery child env - #7527
Conversation
Follow-up to QwenLM#7256, which introduced sanitizeChildEnv and applied it to the shell, monitor, and MCP stdio spawn paths. Three agent-launched child processes were left inheriting the full environment: - hooks/hookRunner.ts spreads process.env into the hook command's env - tools/tool-registry.ts spawns the configured tool-call command and the tool-discovery command with no env option, so both inherit implicitly All three run user- or config-supplied commands on the agent's behalf and have no need for QWEN_SERVER_TOKEN / QWEN_DAEMON_TOKEN, so they are the same credential-exposure gap QwenLM#6601 describes. Route each through sanitizeChildEnv(process.env); benign inherited env is unchanged.
|
Thanks for the PR! Template looks good ✓ Problem: observed security gap, not theoretical. This is a direct follow-up to the already-merged #7256, completing the same fix (#6601) for three spawn sites that were missed — hook commands, tool-discovery commands, and tool-call commands. All three currently leak Direction: clearly aligned — this finishes an accepted security pattern. Size: 18 production lines (hookRunner.ts: 4+1, tool-registry.ts: 11+2), 133 test lines. Well under any threshold. Approach: minimal and mirrors #7256 exactly — import Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的安全缺口，非理论性问题。这是已合并的 #7256 的直接后续，为被遗漏的三处 spawn 点补上相同的修复（#6601）——hook 命令、工具发现命令和工具调用命令。这三处目前都会将 方向：明确对齐——完成一个已被接受的安全模式。 规模：18 行生产代码（hookRunner.ts: 4+1, tool-registry.ts: 11+2），133 行测试代码。远低于任何阈值。 方案：最小化改动，与 #7256 完全一致——导入 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given three spawn sites missing env sanitization, I'd import Comparison with the diff: the PR does exactly this. No simpler path missed.
No critical blockers. No convention violations. Typecheck and lint clean. Real-Scenario TestingNon-user-visible change (env-var stripping in child processes). Project-level hooks didn't trigger in headless mode (trust requirement), so I verified with a direct before/after process-level test and the full unit test suite. Before/After: env var strippingBefore: both daemon secrets leak to the child process. After: stripped, while Unit tests (82/82 pass)Headless CLI run (no regression)中文说明代码审查独立方案： 针对三处缺少 env 净化的 spawn 点，导入 与 diff 对比： PR 完全按此实现，无更简路径被遗漏。
无关键阻塞项。无规范违反。Typecheck 和 lint 均干净。 真实场景测试非用户可见变更（子进程 env 变量剥离）。项目级 hook 在 headless 模式下未触发（信任要求），因此通过直接的 before/after 进程级测试和完整单元测试套件验证。 修复前后：env 变量剥离修复前：两个守护进程秘密泄露到子进程。修复后：被剥离， 单元测试（82/82 通过）Headless CLI 运行（无回归）CLI 正常执行 shell 命令，输出 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean, minimal security fix completing an established pattern; would merge without hesitation. This is exactly the kind of follow-up that should be easy to review and ship. The problem is real and already accepted (#6601, #7256) — three spawn sites were missed when The before/after test confirms the leak and the fix: If I had to maintain this in six months, I'd thank the author — it's the same one-line pattern already used at four other spawn sites, with tests that prove both the leak and the fix. Nothing to untangle. 中文说明置信度：5/5 —— 干净、最小化的安全修复，完成已有模式；毫不犹豫即可合并。 这正是那种应该容易审查和发布的后续修复。问题是真实的且已被接受（#6601、#7256）—— before/after 测试确认了泄露和修复： 如果六个月后我要维护这段代码，我会感谢作者——它与已在其他四处 spawn 点使用的单行模式完全一致，测试同时证明了泄露和修复。无需解开任何纠缠。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Heads-up on the red Bisected to #7460 ( |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| const child = spawn(callCommand, [this.toolName], { | ||
| env: sanitizeChildEnv(process.env), | ||
| }); |
There was a problem hiding this comment.
[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.
| 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
| const proc = spawn(cmdParts[0] as string, cmdParts.slice(1) as string[], { | ||
| env: sanitizeChildEnv(process.env), | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Same normalizePathEnvForWindows gap as the tool-call spawn above — the discovery command has the same Windows PATH risk.
| 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
Both tool-registry spawns previously passed no env option, so Node inherited the parent environment natively and Windows resolved its case-insensitive PATH keys itself. Passing env explicitly gives that up: on Windows process.env can carry both Path and PATH, and the child may pick the wrong one. Route them through normalizePathEnvForWindows, matching the shell (shellExecutionService.ts) and MCP stdio (mcp-client.ts) spawn sites that already pair it with sanitizeChildEnv. It returns env untouched off win32, so nothing changes on other platforms. hookRunner.ts is left as-is: it already built an explicit env before this branch, so it does not regain inheritance semantics here.
|
Good catch — this is a regression the PR itself introduced, so it's worth fixing here. Both I left
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| ...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), |
There was a problem hiding this comment.
[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.
| ...sanitizeChildEnv(process.env), | |
| ...normalizePathEnvForWindows(sanitizeChildEnv(process.env)), |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
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.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
Review & Local Verification Report代码审查设计评价：安全边界的正确收紧。 本 PR 修复了 #6601——hook 命令和工具发现子进程继承了 daemon 密钥（ 修复策略：
安全分析：
测试： 两个测试分别验证 hook 和 tool-registry 的子进程 env 中不含 daemon 密钥，同时保留良性变量。 结论LGTM。 最小化的安全修复，共享 |
Maintainer Local Verification ReportBranch: 1. Unit Tests — 82/82 PASS2. Fail-Before / Pass-After (proven in isolated git worktree)Reverted only the two source files (
Both failures match the PR description exactly — the hook test leaks the token, the registry test has no 3. Real E2E Test — Hook
|
| Check | Status |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ✅ pass (32m18s) |
| Test (macos-latest, Node 22.x) | ⏭️ skipping (fork PR) |
| Test (windows-latest, Node 22.x) | ⏭️ skipping (fork PR) |
Verdict
The change is narrow, correct, and well-tested. All three agent-launched child process spawn sites (hookRunner.ts command hook, tool-registry.ts discovery command, tool-registry.ts tool-call command) now route through sanitizeChildEnv(), matching the existing shell/monitor/MCP-stdio pattern from #7256. The Windows PATH normalization (normalizePathEnvForWindows) on the two registry spawns is a correct companion fix since passing env explicitly loses Node's native case-insensitive PATH resolution. No regressions observed.
中文验证报告
维护者本地验证报告
分支： fix/sanitize-child-env-hooks-registry @ 53f891b0ca
环境： macOS (darwin), Node v22.22.2
1. 单元测试 — 82/82 通过
✓ src/hooks/hookRunner.test.ts (37 tests) 58ms
✓ src/tools/tool-registry.test.ts (45 tests) 39ms
Test Files 2 passed (2)
Tests 82 passed (82)
2. Fail-Before / Pass-After（在隔离 git worktree 中验证）
仅将两个源文件（hookRunner.ts、tool-registry.ts）还原到 main，保留 PR 的测试文件：
| 测试 | main 源文件（fail-before） | PR 源文件（pass-after） |
|---|---|---|
hookRunner > strips Qwen-internal daemon secrets (#6601) |
❌ expected 'serve-secret' to be undefined |
✅ 通过 |
tool-registry > strips Qwen-internal daemon secrets (#6601) |
❌ Cannot read properties of undefined (reading 'env') |
✅ 通过 |
两个失败与 PR 描述完全一致——hook 测试泄露了 token，registry 测试根本没有传 env 选项。
3. 真实 E2E 测试 — Hook printenv 探测
创建测试项目，配置 PreToolUse command hook 执行 printenv | grep -E 'QWEN_SERVER_TOKEN|QWEN_DAEMON_TOKEN|PATH' 并写入文件。启动 CLI：
QWEN_SERVER_TOKEN=serve-secret-12345 QWEN_DAEMON_TOKEN=daemon-secret-67890 node dist/cli.js --prompt "read the file ..." --yoloHook 子进程环境输出（修复后）：
MANPATH=/Users/wenshao/.nvm/versions/node/v22.22.2/share/man:...
PATH=/Users/wenshao/.local/bin:...
✅ QWEN_SERVER_TOKEN — 不存在（已剥离）
✅ QWEN_DAEMON_TOKEN — 不存在（已剥离）
✅ PATH — 存在（无害环境变量保留）
4. 真实 E2E 测试 — sanitizeChildEnv + normalizePathEnvForWindows 直接对比
分别以旧/新方式 spawn 子进程：
=== 旧行为（不经过 sanitizeChildEnv）===
{"QWEN_SERVER_TOKEN":"serve-secret-12345","QWEN_DAEMON_TOKEN":"daemon-secret-67890","PATH":"DEFINED"}
=== 新行为（经过 sanitizeChildEnv）===
{"QWEN_SERVER_TOKEN":"UNDEFINED","QWEN_DAEMON_TOKEN":"UNDEFINED","PATH":"DEFINED"}
5. 类型检查 & Lint
tsc --noEmit → 通过（0 错误）
eslint → 通过（0 警告）
6. CI 状态
| 检查项 | 状态 |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ✅ 通过 (32m18s) |
| Test (macos-latest, Node 22.x) | ⏭️ 跳过（fork PR） |
| Test (windows-latest, Node 22.x) | ⏭️ 跳过（fork PR） |
结论
改动范围窄、正确、测试充分。三处 agent 启动的子进程 spawn 点（hookRunner.ts command hook、tool-registry.ts discovery 命令、tool-registry.ts tool-call 命令）现在都经过 sanitizeChildEnv()，与 #7256 中 shell/monitor/MCP-stdio 的模式一致。两处 registry spawn 上的 Windows PATH 规范化（normalizePathEnvForWindows）是正确的伴随修复，因为显式传入 env 会失去 Node 原生的大小写不敏感 PATH 解析。未观察到回归。
Maintainer E2E Verification ReportEnvironment: macOS (darwin arm64), Node v22.22.2, built from PR branch Test 1: Hook child env — real CLI E2EConfigured a BEFORE (main branch): Both daemon tokens leaked into the hook child process:
AFTER (PR 7527): Both tokens are stripped; benign env (
Test 2: Tool-registry spawn — direct code-path verificationRan a script that imports the built
Test 3: Unit tests (CI)CI on Ubuntu passes all tests including the two new ones:
Summary
Verdict: The fix works as intended. Daemon tokens are stripped from all three child-process spawn sites (hook command, tool-call command, tool-discovery command) while benign environment variables are fully preserved. 中文版本维护者 E2E 验证报告环境： macOS (darwin arm64)，Node v22.22.2，从 PR 分支 测试 1：Hook 子进程环境 — 真实 CLI E2E配置了一个运行 修复前（main 分支）： 两个 daemon token 均泄露到 hook 子进程中：
修复后（PR 7527）： 两个 token 均被剥离；无害环境变量（
测试 2：Tool-registry spawn — 直接代码路径验证运行脚本导入构建后的
测试 3：单元测试（CI）CI 在 Ubuntu 上通过所有测试，包括两个新增测试：
总结
结论： 修复按预期工作。Daemon token 在全部三处子进程 spawn 点（hook 命令、工具调用命令、工具发现命令）均被剥离，同时无害环境变量完整保留。 |
4 participants
What this PR does
Follow-up to the just-merged #7256. That PR added
sanitizeChildEnv()and applied it to the shell, monitor, and MCP stdio spawn paths. This routes the three remaining agent-launched child processes through it as well:hooks/hookRunner.ts...process.envinto the hook command's envtools/tool-registry.ts(tool-call command)spawn(callCommand, …)with noenvoption → implicit full inheritancetools/tool-registry.ts(discovery command)Why it's needed
All three run a user- or config-supplied command on the agent's behalf, which is exactly the category
sanitizeChildEnvwas written for — its doc comment names "shell commands, the monitor tool, or a stdio MCP server", and hooks and the tool-discovery/tool-call commands are the same shape of thing. None of them has any need forQWEN_SERVER_TOKENorQWEN_DAEMON_TOKEN, so leaving them inherited is the same credential-exposure gap #6601 describes: a hook or discovery script that runsprintenvsees the daemon bearer tokens.The two
tool-registry.tssites are easy to miss because they pass noenvoption at all — the inheritance is implicit rather than a visible...process.env.This is deliberately narrow and mirrors #7256 exactly:
sanitizeChildEnvstrips only the two Qwen-internal vars and leaves third-party credentials (GH_TOKEN,AWS_*, …) alone, so hooks and discovery scripts that legitimately depend on them keep working.Reviewer Test Plan
How to verify
npx vitest run --root packages/core src/hooks/hookRunner.test.ts src/tools/tool-registry.test.ts→ 82/82.HookRunner > executeHook > strips Qwen-internal daemon secrets from the hook child env (#6601)— fails onmainwithexpected 'serve-secret' to be undefined.ToolRegistry > discoverTools > strips Qwen-internal daemon secrets from the discovery and tool-call child env (#6601)— asserts over both spawn calls; fails onmainwithCannot read properties of undefined (reading 'env'), i.e. noenvoption was passed at all.PATHstill defined,QWEN_PROJECT_DIRstill/test), so this cannot silently blank a child's environment.Evidence (Before & After)
Not user-visible; verified by the deterministic unit tests above.
QWEN_SERVER_TOKEN/QWEN_DAEMON_TOKEN.Tested on
macOS:
hookRunner+tool-registry(82) pass locally with proven fail-before/pass-after; typecheck and eslint clean on all four touched files. The change is an env-object transformation with no platform-dependent behavior and the assertions are deterministic, so no manual QA is required; CI covers Windows/Linux.Environment (optional)
Node v24;
@qwen-code/qwen-code-coreworkspace; vitest 3.2.Risk & Scope
tool-registry.tsspawns now pass an explicitenvwhere they previously passed none.sanitizeChildEnv(process.env)is a full shallow copy minus the two internal vars, so the child sees the same environment it did before apart from those — verified by thePATHassertion in the new test.QWEN_SERVER_TOKENbeing present will no longer see it. That is the intended fix.Linked Issues
Follow-up to #7256; same root cause as #6601.
中文说明
本 PR 的作用
这是刚刚合并的 #7256 的后续。那个 PR 引入了
sanitizeChildEnv()并将其应用于 shell、monitor 和 MCP stdio 的 spawn 路径。本 PR 将其余三处由 agent 启动的子进程也接入该函数：hooks/hookRunner.ts...process.env展开进 hook 命令的 envtools/tool-registry.ts（工具调用命令）spawn(callCommand, …)未传env选项 → 隐式完整继承tools/tool-registry.ts（工具发现命令）为什么需要
这三处都是代表 agent执行用户或配置提供的命令，正是
sanitizeChildEnv所针对的场景——其文档注释点名了「shell 命令、monitor 工具、stdio MCP server」，而 hook 与工具发现/调用命令属于同一类。它们都不需要QWEN_SERVER_TOKEN或QWEN_DAEMON_TOKEN，因此继续继承就是 #6601 所描述的同一个凭据泄露缺口：一个执行printenv的 hook 或发现脚本即可看到守护进程的 bearer token。tool-registry.ts的两处很容易被漏掉，因为它们完全没有传env选项——继承是隐式的，而非可见的...process.env。本改动刻意保持窄范围，与 #7256 完全一致：
sanitizeChildEnv只剥离两个 Qwen 内部变量，不动第三方凭据（GH_TOKEN、AWS_*等），因此合理依赖这些变量的 hook 与发现脚本仍可正常工作。复核测试计划
如何验证
npx vitest run --root packages/core src/hooks/hookRunner.test.ts src/tools/tool-registry.test.ts→ 82/82 通过。HookRunner > executeHook > strips Qwen-internal daemon secrets from the hook child env (#6601)——在main上失败：expected 'serve-secret' to be undefined。ToolRegistry > discoverTools > strips Qwen-internal daemon secrets from the discovery and tool-call child env (#6601)——对两次 spawn 调用同时断言；在main上失败：Cannot read properties of undefined (reading 'env')，即根本没有传入env选项。PATH仍存在、QWEN_PROJECT_DIR仍为/test），因此不会悄悄清空子进程环境。证据（修复前后对比）
非用户可见；由上述确定性单元测试验证。
QWEN_SERVER_TOKEN/QWEN_DAEMON_TOKEN。测试环境
macOS：
hookRunner+tool-registry（82）本地通过，并验证了 fail-before/pass-after；四个改动文件的 typecheck 与 eslint 均干净。该改动是对 env 对象的变换，无平台相关行为，断言均为确定性，因此无需人工 QA；Windows/Linux 由 CI 覆盖。运行环境（可选）
Node v24；
@qwen-code/qwen-code-core工作区；vitest 3.2。风险与影响范围
tool-registry.ts的两处 spawn 现在显式传入env，而此前完全不传。sanitizeChildEnv(process.env)是去掉两个内部变量后的完整浅拷贝，因此除这两者外子进程看到的环境与之前相同——新增测试中的PATH断言已验证这一点。QWEN_SERVER_TOKEN存在，它将不再能看到该变量。这正是本次修复的预期效果。关联 Issue
#7256 的后续；与 #6601 同源。