Skip to content

fix(core): Sanitize internal daemon secrets from shell subprocess environments - #6606

Open
jadelike-wine wants to merge 23 commits into
QwenLM:mainfrom
jadelike-wine:main
Open

fix(core): Sanitize internal daemon secrets from shell subprocess environments#6606
jadelike-wine wants to merge 23 commits into
QwenLM:mainfrom
jadelike-wine:main

Conversation

@jadelike-wine

@jadelike-wine jadelike-wine commented Jul 9, 2026

Copy link
Copy Markdown

What this PR does

Adds a shared environment sanitization utility for child processes and applies it to model-controlled execution paths.

The shell execution path now removes Qwen internal daemon secrets, such as QWEN_SERVER_TOKEN, before spawning child processes.

The sanitization scope intentionally only covers internal Qwen credentials. User-provided credentials required for normal workflows, such as GitHub, AWS, and npm authentication variables, remain available to shell commands.

Why it's needed

Shell subprocesses previously inherited the full daemon process environment through process.env.

This allowed model-controlled shell commands to access daemon-level credentials that should never be exposed outside the Qwen Code process boundary.

For example, QWEN_SERVER_TOKEN could be read from shell commands executed by the agent.

This change closes that defense-in-depth gap while preserving existing user workflows that rely on inherited third-party credentials.

Reviewer Test Plan

How to verify

  1. Set a fake daemon secret:

export QWEN_SERVER_TOKEN=test-secret-token

  1. Start Qwen Code and run a shell command:

printenv QWEN_SERVER_TOKEN

Expected:
The variable should not be visible in the shell subprocess environment.

  1. Verify user workflow credentials are preserved:

export GH_TOKEN=test-github-token

Run:

printenv GH_TOKEN

Expected:
GH_TOKEN should still be available to shell commands.

Evidence (Before & After)

N/A - Non-user-visible security hardening change.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ✅ tested
🐧 Linux N/A

Environment (optional)

Qwen Code v0.19.8
macOS arm64
Node.js v24.3.0

Verified with local CLI execution and unit tests.

Risk & Scope

  • Main risk or tradeoff:
    The sanitization intentionally only removes internal Qwen daemon secrets. Third-party user credentials remain available to preserve shell workflows such as gh, aws, and npm commands.

  • Not validated / out of scope:
    This PR does not change user credential handling or introduce permission prompts for shell environment access.

  • Breaking changes / migration notes:
    Commands that previously relied on accessing Qwen internal daemon credentials from the shell environment will no longer work.

Linked Issues

Fixes #6601

中文说明

本 PR 做了什么

新增共享环境变量清理逻辑,并应用到受模型控制的子进程执行路径。

Shell 子进程启动前会移除 Qwen 内部 daemon 密钥,例如 QWEN_SERVER_TOKEN。

清理范围仅针对 Qwen 内部凭据,不影响用户正常使用 GitHub、AWS、npm 等需要环境变量认证的工作流。

为什么需要

之前 shell 子进程会完整继承 daemon 的 process.env。

这导致模型执行的 shell 命令可能读取 daemon 级别的认证信息。

例如 QWEN_SERVER_TOKEN 可能通过 shell 命令暴露。

本修改关闭该安全风险,同时保持用户现有工作流兼容。

…tive with ACP child

Extract the env-scrub helper from acp-bridge into core as shared
`scrubChildEnv` and `collectSensitiveShellEnvKeys`, and use both to
strip secret-semantic env vars (API keys, tokens, credentials, passwords,
private keys, …) before spawning the shell subprocess. The model's own
commands run there — including auto-allowed read-only `printenv`/`env` — so
inherititing the daemon's env verbatim would let a `printenv
QWEN_SERVER_TOKEN` exfiltrate tokens straight into the tool result.

The ACP child deliberately keeps a NARROW scrub set because it is itself a
`qwen` agent that MUST inherit provider API keys to call models; the two
paths share one primitive but NOT one denylist — unifying them would
reintroduce the leak or break the child. `collectSensitiveShellEnvKeys`
drives the shell path's BROAD set, parametrized on the live `process.env`.
…scrub primitive(QwenLM#6601)

Drop the broad user-credential denylist (GH_TOKEN, AWS_*, NPM_TOKEN, …) from
child-env scrubbing — those belong to normal shell/MCP workflows and were
wrongly stripped. Now only daemon/internal Qwen keys are stripped, and the
scrub/collect primitive is shared between core (ACP/shell/MCP) and desktop
(session-tools-core / mcp client) subprocess paths.
@jadelike-wine jadelike-wine changed the title Sanitize internal daemon secrets from shell subprocess environments fix(core): Sanitize internal daemon secrets from shell subprocess environments Jul 9, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Qwen precheck requires maintainer approval before automated triage/review.

Head SHA: 467b43e55847a28476aefe711f148c3f8642003e

Reason:

  • sensitive_diff:secret_logging
  • secret_value:assignment

A maintainer with write access can inspect the PR and manually request a run with @qwen-code /triage or @qwen-code /review. A new push requires a fresh precheck.

Comment thread packages/core/src/tools/tool-registry.ts
Comment thread packages/desktop/packages/session-tools-core/src/runtime/sandbox-env.ts Outdated
jadelike-wine and others added 2 commits July 11, 2026 12:38
Windows environment variables are case-insensitive, so PATH and Path can both
exist. Normalize them before scrubbing so discovery + execution child
processes receive a single merged PATH and never a split/partial value.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… subprocess env

Untrusted scripts spawned by the desktop session tools were inheriting
provider API keys. Block LLM/AWS/GitHub/Google/Stripe/NPM credential keys in
the sanitized environment alongside existing daemon-internal scrubbing.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jadelike-wine
jadelike-wine requested a review from wenshao July 11, 2026 04:47
Comment thread packages/core/src/services/shellExecutionService.ts
Comment thread packages/core/src/utils/child-env-scrub.ts Outdated
Comment thread packages/desktop/packages/session-tools-core/src/runtime/sandbox-env.ts Outdated
Comment thread packages/desktop/packages/shared/src/mcp/client.ts Outdated
Comment thread packages/desktop/packages/shared/src/mcp/validation.ts Outdated
Comment thread packages/desktop/packages/shared/src/mcp/client.ts Outdated
Comment thread packages/core/src/services/shellExecutionService.ts

@wenshao wenshao left a comment

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.

[Critical] packages/desktop/packages/shared/src/mcp/validation.ts:309 — The production StdioClientTransport at line 309 uses unsanitized { ...processEnv, ...env } while the probe at line 271 (changed by this diff) correctly uses createSanitizedChildEnv. The MCP handshake subprocess inherits QWEN_SERVER_TOKEN.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/utils/child-env-scrub.test.ts
Comment thread packages/acp-bridge/src/spawnChannel.ts
@jadelike-wine
jadelike-wine requested a review from wenshao July 13, 2026 02:16
@wenshao
wenshao requested a review from Copilot July 13, 2026 03:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wenshao
wenshao requested a review from Copilot July 13, 2026 07:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wenshao
wenshao requested a review from Copilot July 13, 2026 08:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@yiliang114 yiliang114 added this to the v1.0.0 milestone Jul 13, 2026
Comment thread packages/cli/src/serve/run-qwen-serve.ts
@wenshao
wenshao requested a review from Copilot July 13, 2026 11:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 73 out of 73 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

packages/desktop/packages/shared/src/mcp/validation.test.ts:117

  • GITHUB_TOKEN is part of the desktop scrub denylist (see DESKTOP_CHILD_CREDENTIAL_ENV_KEYS). If this test is meant to validate the scrubber is applied, it should assert the token is not present in the spawned env even when provided via overrides.
    packages/desktop/packages/shared/src/mcp/validation.test.ts:145
  • QWEN_API_KEY is part of the desktop scrub policy and should be removed case-insensitively on Windows. Once validateStdioMcpConnection applies overrides via createSanitizedChildEnv(process.env, env), Qwen_Api_Key should remain scrubbed (undefined) even if provided in overrides.
    packages/desktop/packages/shared/src/mcp/validation.test.ts:113
  • After switching to createSanitizedChildEnv(process.env, env), scrubbed keys (like QWEN_SERVER_TOKEN / LLM_API_KEY) should remain absent even if provided in env overrides. These expectations currently assert the opposite, which would mask a regression in the scrubber contract.

This issue also appears in the following locations of the same file:

  • line 114
  • line 142

Comment on lines +270 to +272
// MCP child processes must not inherit daemon credentials. Reuse this
// exact env for both the probe spawn and the production stdio transport.
const sanitizedEnv = { ...createSanitizedChildEnv(process.env), ...env };
Comment on lines +63 to +66
env: {
...createSanitizedChildEnv(process.env),
...config.env,
} as Record<string, string>,

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 73 out of 73 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

packages/core/src/lsp/LspServerManager.ts:770

  • These env builders now always pass an explicit, cloned env object to spawned LSP processes. On Windows, that can drop PATH entries when both PATH/Path variants exist unless you normalize first. Other spawn sites already do this via normalizePathEnvForWindows(process.env) before scrubbing (e.g. ToolRegistry). Consider normalizing in buildProcessEnv/buildCommandProbeEnv before calling scrubChildEnv to avoid commandExists()/LSP startup regressions on win32.

Comment on lines +166 to +175
// Combine the static denylist with a pattern-based sweep of the base
// env so QWEN_CUSTOM_API_KEY_* (and any future internal Qwen secret
// pattern) is stripped even if .env/settings.env re-introduced it
// after the daemon's boot-time self-scrub. Defense-in-depth: the
// daemon already removed these from process.env, but this is the
// final spawn boundary for stdio MCP children.
const scrubbed = new Set<string>(SCRUBBED_STDIO_ENV_KEYS);
for (const key of collectSensitiveShellEnvKeys(baseEnv)) {
scrubbed.add(key);
}

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 73 out of 73 changed files in this pull request and generated no new comments.

Suppressed comments (3)

packages/cli/src/serve/voice/resolve-voice-config.ts:124

  • loadDaemonVoiceContext() builds ModelsConfig using baseEnv (without the credentialStore snapshot), but the config construction uses env for auth-type detection and CLI generation config resolution. This can make voice config resolution diverge from the merged env actually used for transcription (e.g., when credentials exist only in the credentialStore). Use mergedEnv when calling buildModelsConfig so both paths see the same effective env.
  const models = buildModelsConfig(settings, baseEnv);

packages/core/src/lsp/LspServerManager.ts:770

  • buildProcessEnv() now always returns an explicit env object (even when no overrides are provided). On Windows, passing an explicit env without normalizing PATH-like keys can lead to multiple case-variants (PATH/Path/...) being present and an unexpected effective PATH for spawned LSP processes. Consider normalizing PATH first (via normalizePathEnvForWindows) before scrubbing/merging overrides, similar to other spawn call sites.
    packages/core/src/lsp/LspServerManager.ts:803
  • buildCommandProbeEnv() also always returns an explicit env object now. For the same reason as buildProcessEnv(), consider normalizing PATH-like keys (PATH/Path/...) on Windows before scrubbing and applying the filtered overrides, so the probe spawn reliably finds the intended executable via PATH.

@wenshao

wenshao commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

Reconcile the daemon-secret sanitization work with changes that landed on main: keep the post-scrub daemonRuntimeBaseEnv snapshot (clean by construction) while folding in main's memoryProjectScope injection, thread credentialStore into the hoisted loadSettings call in workspace-skills-status, and union the CredentialStore/Storage and credentials/memoryScopes imports that both sides added at the same sites.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Merge: PR #6606 (daemon secret sanitization) ← origin/main

Root cause

origin/main advanced past the PR's merge base and touched the same
packages/cli/src/serve boot/env code:

  • Added Storage.getRuntimeBaseDir() + import to server.ts /
    acp-http/index.ts, right where the PR added a CredentialStore import.
  • Added memoryProjectScope to run-qwen-serve.ts (MEMORY_PROJECT_SCOPES
    check + QWEN_CODE_MEMORY_PROJECT_SCOPE injection into daemonRuntimeBaseEnv)
    before scrubbing — the PR moved that snapshot after scrubDaemonProcessEnv().
  • Refactored workspace-skills-status.ts to hoist loadSettings() into a
    settings var; the PR threaded credentialStore into that loadSettings().

Textual or semantic

  • server.ts, acp-http/index.ts: textual only — both sides added a
    different import at one site; the body uses both → import union.
  • run-qwen-serve.ts: semantic — both sides built daemonRuntimeBaseEnv:
    scrubDaemonProcessEnv();
    const daemonRuntimeBaseEnv = Object.freeze({
      ...process.env,
      ...(optsIn.memoryProjectScope !== undefined
        ? { QWEN_CODE_MEMORY_PROJECT_SCOPE: optsIn.memoryProjectScope }
        : {}),
    });
    (import union kept for .../credentials + .../memoryScopes).
  • workspace-skills-status.ts: semantic — kept main's
    resolveSkillSettings(settings), re-added PR's credentialStore to the
    hoisted loadSettings. Import union: Config, CredentialStore, SkillLevel.

What is load-bearing

  • run-qwen-serve.ts: the snapshot MUST stay after scrubDaemonProcessEnv()
    (clean by construction — the PR's point). The memoryProjectScope injection
    is safe there (non-secret, pre-validated).
  • workspace-skills-status.ts: credentialStore must reach the hoisted
    loadSettings, else the param is unused and the threading is lost.

Not verified

No build/typecheck/lint/tests run. Statically: all unioned imports used;
buildWorkspaceSkillsStatus is module-private (no external callers); all
createWorkspaceSkillsStatusProvider calls in run-qwen-serve.ts pass
credentialStore. The server.ts primary caller does NOT — matches both
parents (not a regression).

中文说明

根因:origin/main 越过本 PR 合并基点,改动了同样的
packages/cli/src/serve 启动/环境代码——在 server.ts/acp-http/index.ts
Storage.getRuntimeBaseDir() 及 import(正是 PR 加 CredentialStore 处);在
run-qwen-serve.tsmemoryProjectScope(校验 + 向 daemonRuntimeBaseEnv
注入 QWEN_CODE_MEMORY_PROJECT_SCOPE)且位于脱敏之前,而 PR 把快照移到
scrubDaemonProcessEnv() 之后;重构 workspace-skills-status.ts
loadSettings() 提升为 settings,PR 则向其传入 credentialStore

文本还是语义:server.tsacp-http/index.ts 仅文本(同处各加一个
import,函数体两者都用,取并集);run-qwen-serve.ts 语义(两边都构造
daemonRuntimeBaseEnv,保留 PR「脱敏后快照」并并入 main 的 memoryProjectScope
注入,代码见上);workspace-skills-status.ts 语义(保留 main 的
resolveSkillSettings(settings),加回 PR 的 credentialStore;import 并集
Config, CredentialStore, SkillLevel)。

关键承重点:run-qwen-serve.ts 快照必须留在 scrubDaemonProcessEnv() 之后
(构造上即干净,PR 核心);memoryProjectScope 放那里安全(非机密、已预校验)。
workspace-skills-status.tscredentialStore 必须传到提升后的 loadSettings

未验证:未运行 build/typecheck/lint/测试。已静态检查:并集 import 均被使用;
buildWorkspaceSkillsStatus 模块私有(无外部调用方);run-qwen-serve.ts
createWorkspaceSkillsStatusProvider 调用都传 credentialStoreserver.ts
主调用点传——与两边父提交一致(非回归)。

@wenshao
wenshao requested a review from Copilot August 2, 2026 16:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wenshao
wenshao requested a lite review from Copilot August 4, 2026 13:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Shell subprocess inherits sensitive environment variables causing credential exposure

7 participants