fix(triage): actually restrict the CI review agent's tools - #7647
Conversation
|
Thanks for the PR! Template looks good ✓ — headings are non-standard but all required content is covered. Problem: observed bug with clear evidence. The run logs show Direction: squarely within scope — fixing a misconfiguration that left the CI review agent unrestricted. The layered approach (deny rules as defense-in-depth, ephemeral runners for fork PRs, git exec vector cleanup) is the right shape for this problem. The PR is honest that the denylist is not a security boundary under Size: not applicable — single workflow file, no core paths. Approach: scope feels right. Each change serves the stated goal — rename the input, update to the current schema, add the deny rules, route fork PRs to ephemeral runners, wipe stale git exec vectors. No drive-by changes. Dependency on #7646 is now resolved (merged 2026-07-24). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 标题非标准格式，但所有必需内容均已覆盖。 问题：已观测到的 bug，有明确证据。运行日志显示 方向：完全在范围内 —— 修复一个导致 CI 审查 agent 不受限制的错误配置。分层方案（deny 规则作为纵深防御、fork PR 用临时 runner、清理 git 执行向量）是正确的问题形态。PR 诚实地说明 denylist 在 规模：不适用 —— 单个 workflow 文件，未触及核心路径。 方案：范围合理。每项改动都服务于既定目标 —— 重命名输入、更新到当前 schema、添加 deny 规则、将 fork PR 路由到临时 runner、清除陈旧的 git 执行向量。无夹带改动。对 #7646 的依赖已解决（2026-07-24 已合并）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal (before reading the diff): given "settings_json doesn't exist, agent runs unrestricted", I'd (1) rename to Comparison with the diff: the PR does exactly this, plus adds git exec vector cleanup and thorough inline documentation. No simpler path missed. Findings — no blockers. The three changes are each correct:
The inline comments are extensive but earn their place — this is security-sensitive configuration, and the comments document the threat model, the honest limitations of a denylist under CI Test EvidenceCI checks for commit
No failures. The skipped checks are expected: this PR modifies only Not verified: runtime behavior of the deny rules in a live triage run — this requires the workflow to actually execute on a PR event. The PR author (maintainer/collaborator) reports 38/38 attack-shaped commands hard-denied and 34/34 legitimate triage commands allowed through the real 中文说明代码审查独立方案（读 diff 前）：给定"settings_json 不存在，agent 不受限制"的问题，我会 (1) 改名为 与 diff 对比： PR 完全按此方案执行，另外增加了 git 执行向量清理和详尽的内联文档。没有遗漏更简路径。 发现 —— 无阻塞项。 三项改动均正确：
内联注释很详尽但物有所值 —— 这是安全敏感配置，注释记录了威胁模型、denylist 在 CI 测试证据提交
无失败。跳过的检查是预期的：本 PR 仅修改 未验证：deny 规则在实际 triage 运行中的运行时行为 —— 这需要 workflow 在 PR 事件上实际执行。PR 作者（维护者/协作者）报告 38/38 攻击形态命令被硬拒、34/34 合法 triage 命令通过真实 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; this fixes a real, observed security gap and the implementation matches the problem exactly. Stepping back: the bug is unambiguous — The PR is refreshingly honest about what the denylist does and doesn't do — it's a speed bump, not a boundary, and the real controls are the no-PR-code rule, runner isolation, and exec vector cleanup. The residual risk (write PAT exfiltration under successful injection) is documented with a clear path to the structural fix (token isolation). That's the right way to ship security hardening: name the layers, name the gaps, name the follow-up. The dependency on #7646 is resolved (merged). The squashed commit is clean. CI shows no failures (workflow-only change, all source checks skipped as expected). Already approved by @yiliang114. Ships it. ✅ 中文说明置信度：5/5 —— 每个阶段都干净；这修复了一个真实的、已观测到的安全缺口，实现与问题完全匹配。 退一步看：bug 是明确的 —— PR 对 denylist 能做和不能做什么坦诚得令人耳目一新 —— 它是减速带，不是边界，真正的控制是"不执行 PR 代码"规则、runner 隔离和执行向量清理。残余风险（成功注入下写权限 PAT 被外带）已记录，并给出了结构性修复（token 隔离）的清晰路径。这是交付安全加固的正确方式：命名层次、命名缺口、命名后续项。 对 #7646 的依赖已解决（已合并）。压缩后的提交干净。CI 无失败（仅 workflow 变更，所有源代码检查按预期跳过）。已获 @yiliang114 批准。 可以合并 ✅ — 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. ✅
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. Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.
— qwen3.7-max via Qwen Code /review
| # PRs), go to ephemeral hosted runners so a steered agent cannot persist | ||
| # anything across runs. Forks of this repo fall back to ubuntu-latest as | ||
| # before. | ||
| runs-on: "${{ (github.repository == 'QwenLM/qwen-code' && vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true' && (github.event_name == 'issues' || (github.event.pull_request && github.event.pull_request.head.repo.full_name == github.repository))) && fromJSON('[\"self-hosted\", \"linux\", \"x64\", \"ecs-qwen\"]') || fromJSON('[\"ubuntu-latest\"]') }}" |
There was a problem hiding this comment.
[Suggestion] No regression test for the fork-PR-to-ephemeral-runner routing decision.
The runs-on expression is the only control preventing fork PR code from executing on persistent ECS runners (which carry write PATs and reuse workspace state across runs). There are no workflow-level assertions on head.repo.full_name, ecs-qwen, or the ubuntu-latest fallback. A future PR that simplifies or restructures this expression — removing the fork check, or adding issue_comment to the ECS path — would route fork PRs to persistent runners without any test catching it.
Consider adding assertions in the existing workflow test file verifying: (a) head.repo.full_name == github.repository is present (same-repo gate), (b) ecs-qwen is present (persistent pool label), and (c) the ubuntu-latest fallback exists.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Valid — deferring, not dismissing. There's no workflow-YAML assertion harness in the repo today (the .github/scripts/*.test.mjs files test scripts, not this workflow). A harness asserting the head.repo.full_name / ecs-qwen / ubuntu-latest routing invariants is worth doing deliberately in its own PR rather than bolting a bespoke grep-test onto this change. Tracking as a follow-up.
— via Claude Code (Opus 4.8)
| settings: |- | ||
| { |
There was a problem hiding this comment.
[Suggestion] No regression test for the settings input name that replaces the silently-dropped settings_json.
The comment on lines 305–309 documents that the previous settings_json: input was silently ignored by the action, causing the agent to run with the full default toolset and no deny list. No test asserts that the Run Qwen Triage step uses the correct input name settings:. The exact bug this diff fixes — wrong input name silently dropped, full toolset exposed — has no regression guard.
Consider adding a test asserting that the step contains settings: |- and does not contain settings_json:.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Agreed this is the highest-value guard (it's the exact bug this PR fixes). Deferring only because there's no workflow-YAML test harness yet — the .github/scripts/*.test.mjs suite covers scripts, not this YAML. A small harness asserting settings:-not-settings_json:, the deny entries, and the tools.core allowlist is the right home for it, as its own PR. Tracked as a follow-up.
— via Claude Code (Opus 4.8)
| git config --local --unset-all core.hooksPath 2>/dev/null || true | ||
| git config --local --unset-all core.fsmonitor 2>/dev/null || true | ||
| git config --local --remove-section alias 2>/dev/null || true |
There was a problem hiding this comment.
[Suggestion] No regression test for the new git exec vector cleanup.
These lines defend against persistent exec vectors on ECS runners (hooks/config/aliases planted by a prior run). The existing workflow test file asserts on QWEN_HOME, rm -rf, and workspace cleanup, but has no assertions on hooksPath, fsmonitor, alias, HOOKS_DIR, or hook deletion. A future PR that removes any of these cleanup operations would leave persistent exec vectors intact on the ECS workspace without test detection.
Consider extending the cleanup-isolation test with assertions that the clean step contains core.hooksPath, core.fsmonitor, --remove-section alias, and HOOKS_DIR.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Valid — deferring to the same workflow-test-harness follow-up. No YAML assertion harness exists today; a dedicated one asserting the cleanup knobs (core.hooksPath/fsmonitor/pager/editor, the include/includeIf unset, and the symlink-aware find) is worth adding deliberately rather than inline here.
— via Claude Code (Opus 4.8)
d6f7c54 to
f0cc2b5
Compare
|
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）为单个提交。 |
|
Thanks for the review — addressed in the latest push. Fixed (real hardening gaps — good catches):
Deferred — worth their own PR:
— via Claude Code (Opus 4.8) |
|
@qwen-code /triage |
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. ✅
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. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
| for k in core.hooksPath core.fsmonitor core.pager pager core.editor \ | ||
| sequence.editor core.sshCommand diff.external; do | ||
| git config --local --unset-all "$k" 2>/dev/null || true | ||
| done |
There was a problem hiding this comment.
[Suggestion] No regression test verifies the git exec vector cleanup commands are present. The existing 'isolates agent state per run' test only checks QWEN_HOME, rm -rf, mkdir -p — none of the new git config --unset-all / --remove-section / find -delete commands are asserted. A future cleanup of the step body could drop core.hooksPath or the symlink-aware find, and no test would catch it.
Also: filter.<name>.smudge, diff.<name>.command, and merge.<name>.driver sections are not cleaned — they are git-native code-execution vectors (smudge filters fire on git checkout). Consider either enumerating them or stripping all sections except core/remote/branch.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Same theme as the cleanup-test thread above — deferred to a dedicated workflow-test-harness PR (no YAML assertion harness exists yet). Tracked together.
— via Claude Code (Opus 4.8)
| settings: |- | ||
| { | ||
| "coreTools": [ | ||
| "run_shell_command", | ||
| "write_file", | ||
| "read_file", | ||
| "grep_search", | ||
| "glob", | ||
| "agent", | ||
| "enter_worktree", | ||
| "exit_worktree" | ||
| ], | ||
| "sandbox": false | ||
| "tools": { | ||
| "core": [ | ||
| "run_shell_command", |
There was a problem hiding this comment.
[Suggestion] No regression test verifies that the input key is settings (not the silently-dropped settings_json), that key deny entries are present, or that the tools.core allowlist is correct. The exact regression this PR fixes — settings_json being silently ignored — could recur if a future edit renames the key back, and no assertion would catch it.
The autofix workflow tests (scripts/tests/qwen-autofix-workflow.test.js) already follow this pattern — asserting toContain for specific deny entries and not.toContain for the wrong input name. A parallel test here would be consistent with existing practice.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Same as the settings-input-name test thread above — deferred to the same workflow-test-harness follow-up. Tracked together.
— via Claude Code (Opus 4.8)
f0cc2b5 to
4d99603
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
The `settings_json:` input on the Qwen Code action does not exist — the action
reads `settings:`. The block was silently dropped ("Unexpected input(s)
'settings_json'" in the run logs), so the triage agent ran with the full
default toolset and no restrictions in a job that carries a write PAT.
- Rename to `settings:` and express it in the current schema (tools.core +
permissions.deny); verified to load through the real settings pipeline
(11 tools registered, 106 deny rules active).
- Deny interpreters, build tools, shells, network binaries, path execution, and
the git/gh write subcommands that execute configured commands or materialize
PR code. This is defense-in-depth, NOT a boundary: under the action's --yolo a
command denylist cannot be one (`$(...)` is not reliably deny-matchable, and
exec can hide in other commands) — see the in-file note. The real controls are
the skill's no-PR-code rule, ephemeral runners for fork PRs, and the git
exec-vector cleanup added here. Residual token-exfil-under-injection risk is
documented, with token isolation flagged as the structural follow-up.
- Route fork PRs (and comment/dispatch reruns, which may target fork PRs) to
ephemeral hosted runners so a steered agent cannot persist on the shared ECS
pool. Issue triage and same-repo PRs keep the ECS pool.
- Wipe stale git hooks/config/aliases each run before checkout.
Depends on the skill change that switches test evidence to the CI API and the
CHANGELOG fetch to `gh api`; merge that first so the npm/curl denials are
harmless.
4d99603 to
5ca0876
Compare
| # runs, filter.*.{clean,smudge,process} (run on checkout via | ||
| # .gitattributes), credential.helper, url.*.insteadOf, init.templateDir, | ||
| # aliases, and include/includeIf which pull any of them back in from an | ||
| # arbitrary file. Enumerate the actual keys and --unset-all each: |
There was a problem hiding this comment.
[Suggestion] The git config cleanup covers hookspath, fsmonitor, pager, editor, sshCommand, filter., credential., url.*, init.templateDir, alias, include, includeIf. The hooks dir sweep also covers symlinks (-type f -o -type l). Good defense-in-depth against planted exec vectors. Consider also unsetting core.alternateRefsCommand and core.pager (already covered) — but diff.guitool and merge.tool are not in the denylist and could fire on git diff/git merge invocations. Minor gap.
— qwen3.8-max-preview via Qwen Code /review
| # command is a gh-API call, a read-only git command, or text | ||
| # processing — none touch the PR's tree in an executable way). | ||
| # 2. Fork PRs run on ephemeral hosted runners (see the triage job | ||
| # `runs-on`), bounding persistence/lateral movement. |
There was a problem hiding this comment.
[Suggestion] The deny list is comprehensive (106 rules) but correctly documented as NOT a security boundary under --yolo. The residual risk (agent holds write PAT, could exfiltrate under successful injection) is well-documented. The structural fix (token isolation — agent runs read-only, separate step publishes with PAT) is the right end-state. Tracked as follow-up.
— qwen3.8-max-preview via Qwen Code /review
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Critical security fix — settings_json: was silently dropped, agent ran with full toolset + write PAT. Fix correctly renames to settings: with 11 core tools + 106 deny rules. Defense-in-depth approach is well-documented: deny list is NOT a security boundary under --yolo, real controls are (1) no PR code execution, (2) fork PRs on ephemeral runners, (3) stale git vectors wiped. Residual risk (PAT exfiltration under injection) correctly tracked as follow-up for token isolation. Two inline suggestions on minor gaps (diff.guitool/merge.tool, core.alternateRefsCommand).
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| "run_shell_command(git merge)", | ||
| "run_shell_command(git push)", | ||
| "run_shell_command(patch)", | ||
| "run_shell_command(git -c)", |
There was a problem hiding this comment.
[Suggestion] The deny list blocks git -c (lowercase, sets config key=value) but not git -C (uppercase, changes working directory), git --git-dir, or git --work-tree. These global git flags prefix any subcommand before the denied pattern, bypassing all git <subcommand> deny rules.
Failure scenario: git -C . config --local core.hooksPath /tmp/evil — prefix matching checks startsWith('git config ') which fails because the command starts with git -C .. Similarly, git -C . rebase --exec 'malicious' bypasses run_shell_command(git rebase). Verified in matchesCommandPattern (rule-parser.ts:769-772).
| "run_shell_command(git -c)", | |
| "run_shell_command(git -c)", | |
| "run_shell_command(git -C)", | |
| "run_shell_command(git --git-dir)", | |
| "run_shell_command(git --work-tree)", |
— qwen3.7-max via Qwen Code /review
| "run_shell_command(ksh)", | ||
| "run_shell_command(su)", | ||
| "run_shell_command(sudo)", | ||
| "run_shell_command(env)", |
There was a problem hiding this comment.
[Suggestion] The deny list blocks run_shell_command(env) but not run_shell_command(printenv) — a direct synonym that dumps all environment variables. The step's env carries GH_TOKEN (write PAT) and OPENAI_API_KEY.
Failure scenario: A prompt injection steers the agent to call printenv. Prefix word-boundary matching ("printenv".startsWith("env ") → false) lets it through. Secrets are dumped in plaintext and can be exfiltrated via gh pr comment (the agent has legitimate comment-posting capability).
| "run_shell_command(env)", | |
| "run_shell_command(env)", | |
| "run_shell_command(printenv)", |
— qwen3.7-max via Qwen Code /review
| # a keep-known-safe config allowlist is the exhaustive end-state, tracked | ||
| # with the workflow-test-harness follow-up since it needs a checkout test. | ||
| git config --local --name-only --list 2>/dev/null \ | ||
| | grep -iE '^(core\.(hookspath|fsmonitor|pager|editor|sshcommand)|sequence\.editor|diff\.external|filter\.|credential\.|url\.|init\.templatedir|pager\.|alias\.|include\.|includeif\.)' \ |
There was a problem hiding this comment.
[Suggestion] The cleanup grep regex omits gpg\., a direct code-execution config knob: git runs the value of gpg.program as an external command when verifying signatures (git log --show-signature, git verify-commit).
Failure scenario: A prior run on the persistent ECS pool plants git config --local gpg.program /tmp/evil. The cleanup grep doesn't match gpg.program, so it survives. A subsequent triage run invokes git log --show-signature (not in the deny list) and git executes /tmp/evil --status-fd ... --verify ... with the agent's full environment.
| | grep -iE '^(core\.(hookspath|fsmonitor|pager|editor|sshcommand)|sequence\.editor|diff\.external|filter\.|credential\.|url\.|init\.templatedir|pager\.|alias\.|include\.|includeif\.)' \ | |
| | grep -iE '^(core\.(hookspath|fsmonitor|pager|editor|sshcommand)|sequence\.editor|diff\.external|filter\.|credential\.|url\.|init\.templatedir|pager\.|alias\.|include\.|includeif\.|gpg\.)' \ |
— qwen3.7-max via Qwen Code /review
| "write_file(/.git/hooks/**)", | ||
| "write_file(/.git/config)", |
There was a problem hiding this comment.
[Suggestion] All file-path deny rules use write_file(...) as the tool name, but sed -i via run_shell_command produces virtual edit operations (shell-semantics.ts:1596), and toolMatchesRuleToolName('write_file', 'edit') returns false (rule-parser.ts:230-256). The edit rule direction matches write_file contexts, but not the reverse.
Failure scenario: The agent runs sed -i 's/.*/malicious/' ~/.bashrc. Shell semantics extracts a virtual edit operation for ~/.bashrc. The deny rule write_file(~/.bashrc) is checked via toolMatchesRuleToolName('write_file', 'edit') → false. The virtual-op pass returns default, and the command is allowed. This applies to every write_file(...) path in the deny list.
Suggested fix: Use edit(...) instead of write_file(...) for all file-path deny rules. An edit(...) rule matches both edit and write_file contexts via EDIT_TOOLS (rule-parser.ts:184).
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
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. ✅
The bug
The
settings_json:input on theQwenLM/qwen-code-actionstep does not exist — the action readssettings:. The block was silently dropped (the run logs showUnexpected input(s) 'settings_json'), so the triage agent has been running with the full default toolset and no restrictions — in a job whose environment carries a write PAT.Change
settings:and express it in the current schema (tools.core+permissions.deny). Verified to load through the real settings pipeline: 11 tools registered, 106 deny rules active.git/ghwrite subcommands that execute configured commands or materialize PR code.core.hooksPath/core.fsmonitor/ aliases) each run before checkout.What this is — and is not
A command denylist is not a security boundary under the action's
--yolo, and this PR does not treat it as one.$(...)substitution is not reliably deny-matchable, and even with it blocked, exec can hide elsewhere. The denylist is defense-in-depth against the normal failure (the agent runningnpm/nodebecause a prompt implied it) and low-effort injection.The real controls are, in order:
Residual risk is documented inline: the agent still holds a write PAT, so a successful prompt-injection could still exfiltrate it. The structural fix is token isolation (agent runs read-only; a separate PR-code-free step publishes its drafts, as
publish-tmuxalready does) — flagged as a follow-up, not wired here.Verification
settingsJSON was fed through the realPermissionManager: 38/38 attack-shaped commands hard-denied, 34/34 legitimate triage commands allowed, write-path rules correct (git -cdenied,git -Callowed).echoexecute andnode -e …hard-denied.Depends on #7646
#7646 switches test evidence to the CI API and the CHANGELOG fetch to
gh api. Merge #7646 first so thenpm/curldenials introduced here are harmless.中文说明
Bug
QwenLM/qwen-code-actionstep 上的settings_json:输入根本不存在 —— action 读的是settings:。这个块被静默丢弃了(运行日志里能看到Unexpected input(s) 'settings_json'),所以 triage agent 一直在用全量默认工具集、零限制运行 —— 而该 job 的环境里带着有写权限的 PAT。改动
settings:,并用当前 schema 表达(tools.core+permissions.deny)。已验证能通过真实设置加载链路生效:注册 11 个工具,106 条 deny 规则激活。git/gh写子命令。core.hooksPath/core.fsmonitor/ alias)。它是什么,不是什么
在 action 的
--yolo下,命令 denylist 不是安全边界,本 PR 也没把它当边界。$(...)替换无法可靠地被 deny 匹配,即便挡住,执行也能藏在别处。denylist 是针对常见失误(agent 因为 prompt 暗示就去跑npm/node)和低成本注入的纵深防御。真正的控制,按强度排序:
残余风险已在文件内注明:agent 仍持有写权限 PAT,成功的 prompt 注入仍可能把它外带。结构性修复是 token 隔离(agent 用只读 token,由单独的、不接触 PR 代码的步骤发布其草稿,正如
publish-tmux已经在做的)—— 已标为后续项,本 PR 未接线。验证
settingsJSON 喂进真实PermissionManager:38/38 攻击形态命令被硬拒,34/34 合法 triage 命令放行,写路径规则正确(git -c拒、git -C放)。echo执行、node -e …被硬拒。依赖 #7646
#7646 把测试证据切到 CI API、CHANGELOG 拉取切到
gh api。请先合并 #7646,这样本 PR 引入的npm/curldeny 才不会造成困扰。