Skip to content

fix(permissions): preserve substitution-bearing env prefixes - #10204

Closed
SLP-DEV1 wants to merge 3 commits into
QwenLM:mainfrom
SLP-DEV1:fix/10192-env-assignment-substitution
Closed

fix(permissions): preserve substitution-bearing env prefixes#10204
SLP-DEV1 wants to merge 3 commits into
QwenLM:mainfrom
SLP-DEV1:fix/10192-env-assignment-substitution

Conversation

@SLP-DEV1

@SLP-DEV1 SLP-DEV1 commented Aug 26, 2026

Copy link
Copy Markdown

What this PR does

This PR prevents a saved concrete Bash allow rule from matching after Qwen strips a leading environment assignment when the original command contains shell command or process substitution. Static environment prefixes remain compatible with existing permission matching, but substitution-bearing prefixes keep their full command identity and therefore cannot collapse to the trusted main command.

Why it's needed

A command such as X=$(printf hidden) npm --version can execute the substitution before npm --version, while the previous permission normalization could strip X=$(...) and match a saved Bash(npm --version) allow rule. The shell safety layer already classifies substitution conservatively, but the saved allow rule could override that result. This crosses the confirmation boundary described in #10192.

Reviewer Test Plan

How to verify

Run npx vitest run packages/core/src/permissions/env-assignment-substitution.test.ts packages/core/src/permissions/permission-manager.test.ts packages/core/src/utils/shell-utils.test.ts. Confirm that a static prefix such as FOO=bar npm --version still matches Bash(npm --version), while X=$(printf hidden) npm --version, backtick substitution, and quoted command substitution do not. The end-to-end PermissionManager regression should return ask for X=$(printf hidden) npm --version when only Bash(npm --version) is allowed.

Evidence (Before & After)

N/A — this is a non-UI permission-matching change. Before the fix, substitution-bearing leading environment assignments could be stripped before Bash rule matching. After the fix, the regression verifies that the same invocation remains ask, while static environment-prefix compatibility is preserved.

Tested on

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ⚠️ not tested
🐧 Linux ✅ tested

Environment (optional)

GitHub Actions Ubuntu runner with Node.js 22. The focused Vitest command above completed successfully on the fork.

Risk & Scope

  • Main risk or tradeoff: Commands containing shell substitution no longer benefit from leading environment-assignment normalization when matching Bash permission rules; this is intentional so additional shell execution cannot be hidden behind a trusted command identity.
  • Not validated / out of scope: security: static loader environment assignments can bypass concrete Bash allow rules #10197, which covers completely static but runtime-sensitive environment variables such as NODE_OPTIONS and GIT_CONFIG_*, is intentionally unchanged by this PR.
  • Breaking changes / migration notes: None for static environment prefixes such as FOO=bar npm --version; those continue to match the underlying concrete Bash rule as before.

Linked Issues

Closes #10192

中文说明

此 PR 的作用

此 PR 防止在原始命令包含 shell 命令替换或进程替换时,Qwen 先移除前导环境变量赋值、再错误命中已保存的具体 Bash allow 规则。静态环境变量前缀仍保持现有权限匹配兼容性,但包含替换语义的前缀会保留完整命令身份,因此不能再被归一化成受信任的主命令。

为什么需要它

类似 X=$(printf hidden) npm --version 的命令会在执行 npm --version 之前先执行替换内容,而之前的权限归一化可能移除 X=$(...),随后命中已保存的 Bash(npm --version) allow 规则。Shell 安全层本来已经会保守地处理命令替换,但已保存的 allow 规则可能覆盖这一结果。这会跨越 #10192 中描述的确认边界。

Reviewer 测试计划

如何验证

运行 npx vitest run packages/core/src/permissions/env-assignment-substitution.test.ts packages/core/src/permissions/permission-manager.test.ts packages/core/src/utils/shell-utils.test.ts。确认像 FOO=bar npm --version 这样的静态前缀仍然可以命中 Bash(npm --version),而 X=$(printf hidden) npm --version、反引号替换以及带引号的命令替换都不能命中。端到端 PermissionManager 回归测试应确认:当唯一 allow 规则是 Bash(npm --version) 时,X=$(printf hidden) npm --version 的结果为 ask

证据(修改前与修改后)

N/A —— 这是非 UI 的权限匹配修改。修复前,包含替换语义的前导环境变量赋值可能在 Bash 规则匹配前被移除。修复后,回归测试验证同一调用保持为 ask,同时继续保留静态环境变量前缀的兼容行为。

测试平台

OS 状态
🍏 macOS ⚠️ 未测试
🪟 Windows ⚠️ 未测试
🐧 Linux ✅ 已测试

环境(可选)

GitHub Actions Ubuntu runner,Node.js 22。上面的聚焦 Vitest 命令已在 fork 上成功完成。

风险与范围

  • 主要风险或权衡:包含 shell 替换的命令在匹配 Bash 权限规则时不再享受前导环境变量赋值归一化;这是有意设计,以防额外的 shell 执行被隐藏在受信任的命令身份之后。
  • 未验证 / 不在范围内:security: static loader environment assignments can bypass concrete Bash allow rules #10197 涉及完全静态、但会影响运行时启动行为的环境变量,例如 NODE_OPTIONSGIT_CONFIG_*,本 PR 有意不改变该问题。
  • 破坏性变更 / 迁移说明:对 FOO=bar npm --version 这类静态环境变量前缀没有破坏性变更;它们仍会像以前一样命中底层具体 Bash 规则。

关联 Issue

Closes #10192

@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Aug 26, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with @qwen-code /review. See workflow logs.

Copy link
Copy Markdown
Author

Superseded by #10212. The broader fail-safe env-prefix policy now covers #10192 as well, including substitution-bearing prefixes, while also fixing #10197's static runtime-sensitive prefixes (NODE_OPTIONS, GIT_CONFIG_*, etc.). Closing this narrower conflicting implementation so maintainers have one coherent permission-policy direction to review.

@SLP-DEV1 SLP-DEV1 closed this Aug 26, 2026
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.

security: Bash allow rules can be bypassed by command substitution hidden in leading environment assignments

2 participants