fix(core): keep heredoc bodies out of permission rule splitting - #9417
fix(core): keep heredoc bodies out of permission rule splitting#9417he-yufeng wants to merge 1 commit into
Conversation
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template: looks good ✓ — all three main sections and the full Reviewer Test Plan are filled in (including the Tested-on table and an honest N/A for before/after evidence on a non-UI change). Minor: the Problem: observed bug with a concrete reproduction, not theoretical hardening. #9381 shows Direction: aligned — this makes the rule-evaluation path consistent with the file-operation path that already shipped with heredoc stripping, which is exactly the fix the issue prescribes. CHANGELOG: no direct reference, but permission rule matching is core qwen-code surface. Size: core paths touched ( Approach: the scope is exactly right. Risk: no elevated risk signals (none of the revert-correlated paths matched). One semantic note for reviewers: heredoc bodies stop being evaluated line-by-line, so a deny rule that happened to match body text (e.g. Moving on to code review. 🔍 中文说明感谢贡献! 模板:完整 ✓ —— 三个主要部分和完整的 Reviewer Test Plan 都已填写(含测试平台表格,非 UI 改动的 before/after 证据如实写了 N/A)。小问题:缺少 问题:已观测到的 bug,有具体复现,不是理论性加固。#9381 展示了 方向:对齐——使规则求值路径与已上线的、带 heredoc 剥离的文件操作路径保持一致,正是 issue 指定的修复方式。CHANGELOG 无直接条目,但权限规则匹配是 qwen-code 的核心面。 规模:触及核心路径( 方案:范围恰到好处。 风险:无升级风险信号(未命中任何与 revert 相关的高风险路径)。一个供 reviewer 留意的语义点:heredoc 正文不再逐行求值,因此恰好匹配正文文本的 deny 规则(例如针对含 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first: with the issue's root-cause analysis in hand, the fix I'd write is exactly what this PR does — reuse the heredoc stripping the file-op path already has, applied before the rule path splits. The PR matches that baseline; I didn't find a simpler path it missed. Findings, in order of what I checked:
Non-blocking observations:
Testing evidenceUnattended CI run — no PR code is built or executed by this review. Evidence below is the PR's own CI on the reviewed commit, read via the API. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The unit suite ( Sandboxed verification would settle the remaining gap: 中文说明代码审查先写独立方案:基于 issue 的根因分析,我会做的修复正是本 PR 的做法——复用文件操作路径已有的 heredoc 剥离,在规则路径拆分之前应用。PR 与该基线一致,没有找到更简单的路径。
非阻塞观察:
测试证据无人值守 CI 运行——本审查不构建、不执行 PR 代码。以上证据为本 PR 自身 CI 在被审 commit 上的结果,经 API 读取。单测套件(ubuntu)审查时仍在运行,按策略只抓取一次、不轮询;macOS/Windows 的 "Test" 为被跳过的矩阵占位,并非 PR 导致的失败。作者报告 macOS 本地 444/444 通过——为作者声明,未在此独立复跑。沙箱验证可补齐剩余缺口: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal fix for a confirmed bug; only non-blocking nits (missing Stepping back: this is what a good fork PR looks like. The issue came with a root-cause analysis, the PR does exactly the prescribed fix and nothing else — a byte-identical function move plus one application of it — and the tests fail on The one thing I'd want the merging maintainer to hold in mind is the deny-rule semantics: after this change, rule matching sees Everything else is verified: consumers are named and consistent, no import cycle, splitting around heredocs still works, here-strings and quoted 中文说明置信度:4/5 —— 对已确认 bug 的干净、最小修复;仅有非阻塞小问题(缺 整体来看:这是一个优秀 fork PR 的样子。issue 自带根因分析,PR 严格实施指定修复、毫无多余——一次逐字节一致的函数移动加一处应用——测试在 唯一希望合并维护者心里有数的是 deny 规则语义:此改动后,规则匹配看到的是 其余均已核实:消费方明确且一致、无循环依赖、heredoc 外围拆分正常、here-string 与引号内 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): src/permissions/permission-manager.test.ts — no such file or directory; src/permissions/shell-semantics.test.ts — no such file or directory.
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const commands = splitCompoundCommandSegments( | ||
| stripHeredocBodies(command), | ||
| ).map((segment) => segment.command); |
There was a problem hiding this comment.
[Critical] The heredoc lexer this diff wires into rule evaluation diverges from real bash in several corners, and in each one lines that bash executes are swallowed as "heredoc body" and never reach permission rules: deny/ask rules cannot match them, while a prefix allow rule (compiled with the s flag, so .* crosses newlines) auto-approves the whole multi-line command. Before this change the unstripped splitter returned those lines as their own segments, which were evaluated; the strip removes that last detection layer (the virtual-op pass only escalates from 'default', it never blocks an allow). Six executed entrances: (1) arithmetic — with deny: ["Bash(rm *)"], echo start + SIZE=$((1 << 20)) + rm -rf /important: the << inside $(( )) is a shift, but the lexer registers delimiter 20 and swallows the rm line — PermissionManager returns deny without the arithmetic line and allow with it; (2) comments — echo hi # <<EOF + touch /tmp/pwned + EOF: bash treats # <<EOF as a comment and runs the touch, while evaluate returns allow under Bash(echo *); (3) multi-line quotes — quote state resets per line, so <<EOF inside an unclosed double-quoted string opens a phantom heredoc that swallows a following rm bash executes; (4) continued delimiter — cat <<EO\ + F registers EO, never matches, swallows to end of input; (5) delimiter charset — cat <<A,B truncates the delimiter at ,, never matches the real A,B terminator, swallows to end of input; (6) body-start boundary — cat <<EOF \ followed by && rm -rf /important: bash folds the continuation into the opening logical line and executes the rm, but the stripper queues EOF after the physical line and swallows the rm line — verdict allow post-PR vs deny with only this wiring line reverted. The entrance space is general shell syntax the lexer does not model (comments, arithmetic, continuations, expansions, quoting), so patching entrances one by one will not converge; fail closed instead. Concretely: skip << inside arithmetic contexts ($((/((), stop heredoc scanning at word-start #, carry quote state across lines (or move body consumption into splitCompoundCommandSegments' single-pass scanner, which already tracks quotes across newlines), fold backslash-newline continuations into the opening line before body consumption, and when a delimiter cannot be resolved keep the remaining lines so they are over-evaluated (ask) instead of invisible. Longer-term, defer compound/heredoc structure to a real shell parser.
Witness (executed at this commit):
bash: SIZE=1048576, marker CREATED (the swallowed line ran)
PermissionManager: control 'echo start\nrm -rf /important' => deny
with arithmetic line => segments ["echo start","SIZE=$((1 << 20))"], verdict allow
continuation entrance: PR verdict allow; wiring line reverted =>
split ["cat <<EOF \\","rm -rf /important","body","EOF"], verdict deny
— qwen3.8-max via Qwen Code /review (v0.21.11)
| (segment) => segment.command, | ||
| ); | ||
| const commands = splitCompoundCommandSegments( | ||
| stripHeredocBodies(command), |
There was a problem hiding this comment.
[Suggestion] "Heredoc bodies are stdin, not commands" is correct for cat/grep consumers (the bug being fixed) but not for shell interpreters — bash <<EOF executes its body, and this strip makes that body invisible to every Bash-pattern rule. Measured: with deny: ["Bash(touch *)"], bash <<'EOF' + touch /tmp/r12 + EOF was deny before this change (the body line was its own segment) and is ask after it (the only segment is bash <<'EOF'), and becomes silent allow under an allow rule like Bash(bash *). The silent path itself predates the diff — bash -c 'touch' under the same deny rule returns ask on both sides, because Bash-pattern matching never unwraps wrappers — so this is the semantic note the triage already flagged for the merging maintainer rather than a blocker: either document the trade-off (deny rules cannot see heredoc bodies fed to shells, same as bash -c) or special-case shell-interpreter heredoc openers (bash, sh, zsh, dash, eval) and keep their bodies visible to rule evaluation.
Witness (executed):
BASE (wiring reverted): deny
PR: segments ["bash <<'EOF'"], verdict ask; under Bash(bash *): allow
control: bash -c 'touch' => ask on both arms
— qwen3.8-max via Qwen Code /review (v0.21.11)
| }); | ||
|
|
||
| it('handles the tab-stripping heredoc variant', async () => { | ||
| expect(splitCompoundCommand('python <<-PY\n\timport os\n\tPY')).toEqual([ |
There was a problem hiding this comment.
[Suggestion] This <<- test is vacuous against the two code paths that define the variant — the - skip in getHeredocDelimiters and the line.trim() terminator match — because nothing follows the terminator, so whether the heredoc ever closes is unobservable. Both mutants keep the test green while silently dropping trailing commands: deleting the - skip (delimiter becomes -PY) and changing line.trim() to line (tab-indented terminator no longer closes) each still pass this test, while on extended input both return ['python <<-PY'] instead of ['python <<-PY', 'echo done'] — echo done swallowed. Extend the input with a trailing segment; that single input kills both mutants:
expect(
splitCompoundCommand('python <<-PY\n\timport os\n\tPY\necho done'),
).toEqual(['python <<-PY', 'echo done']);— qwen3.8-max via Qwen Code /review (v0.21.11)
| * and the delimiter line must survive splitting untouched instead of being | ||
| * evaluated as shell segments of its own. | ||
| */ | ||
| export function stripHeredocBodies(command: string): string { |
There was a problem hiding this comment.
[Suggestion] This PR relocates and exports the heredoc stripper, but two other implementations of the same behavior remain and have already diverged: packages/cli/src/serve/daemon-git-worktree-guard.ts:1867 (regex-based, no quote state, only the first heredoc per line) and the heredoc tokenizer inside detectCommandSubstitution in packages/core/src/utils/shell-utils.ts. The divergence is executed, not hypothetical: for echo "<<EOF" followed by touch /tmp/x, this scanner correctly keeps the touch line as a segment, while the guard's regex matches delimiter EOF through the double quotes and strips it — the same command is segmented differently by the permission manager and the daemon worktree guard. A heredoc fix like this one now has to be re-derived in up to three places, and fixing only one site silently re-diverges them. Since the guard already imports from @qwen-code/qwen-code-core, re-export this function from the core package entry and replace the guard's regex copy (here or in a recorded follow-up).
— qwen3.8-max via Qwen Code /review (v0.21.11)
| * "x=$(( a & b ))" → ["x=$(( a & b ))"] (arithmetic, not async) | ||
| * "python - <<'PY'\nimport os\nPY" → ["python - <<'PY'"] (heredoc body is stdin) | ||
| */ | ||
| export function splitCompoundCommand(command: string): string[] { |
There was a problem hiding this comment.
[Suggestion] The sibling docstring this change falsifies: splitCompoundCommandSegments (~lines 833-835, unchanged text) says of this function "this is the same split, and that function is a projection of this one." After this diff the two are no longer the same split — this one strips heredoc bodies first, Segments does not — and the example list now contains the heredoc case, which Segments does not honour. Measured: Segments on "python - <<'PY'\nimport os\nPY" returns ["python - <<'PY'", "import os", "PY"] while this function returns ["python - <<'PY'"]. A future caller needing terminator info reaches for the exported Segments on a raw command, reads "the same split", and resurrects the #9381 bug class on the segment path. Amend the Segments docstring to say that callers passing raw commands must strip heredoc bodies first (stripHeredocBodies) and that this function does it for them — or apply the strip inside Segments and drop it from this wrapper.
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
|
||
| for (const line of lines) { | ||
| if (pendingDelimiters.length > 0) { | ||
| if (line.trim() === pendingDelimiters[0]) { |
There was a problem hiding this comment.
[Suggestion] line.trim() terminator matching is laxer than bash: a heredoc ends only at a line exactly equal to the delimiter (<<- strips only leading tabs), so an indented or padded delimiter-lookalike body line terminates the skip early and the remaining genuine body lines become evaluated command segments. Measured: splitCompoundCommand('cat <<EOF\n EOF\ncat data.txt\nEOF') yields ['cat <<EOF', 'cat data.txt', 'EOF'] while bash treats EOF and cat data.txt as body data — producing a spurious ask (or a deny if a data line resembles a denied pattern) for an otherwise-allowed heredoc command. This errs in the over-evaluation direction, so it is not a bypass — but it is newly reachable in the rule path via this diff and it is the same class of bash divergence the PR is fixing. Match terminators exactly (line === delimiter), stripping only leading tabs when the opener was <<-.
— qwen3.8-max via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-6 daemon-git-worktree-guard keeps a weaker regex copy of the now-exported stripHeredocBodies — already reported (comment 3806112221)
- R1-3 stale 'projection' JSDoc on splitCompoundCommandSegments — already reported (comment 3806112224)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 6 by the review time budget.
Test Plan (not a blocker): src/permissions/permission-manager.test.ts — no such file or directory; src/permissions/shell-semantics.test.ts — no such file or directory.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| import { | ||
| splitCompoundCommandSegments, | ||
| stripHeredocBodies, | ||
| } from './rule-parser.js'; |
There was a problem hiding this comment.
[Suggestion] The heredoc-strip invariant is enforced in the thin wrapper splitCompoundCommand instead of the exported primitive splitCompoundCommandSegments that actually performs the split — this import is added solely to sustain walkCompoundCommand's manual composition splitCompoundCommandSegments(stripHeredocBodies(command)).
Failure scenario: the next caller that reaches for the exported primitive (it has its own dedicated tests, advertising it as a public building block) will silently split heredoc bodies into command segments again — re-instantiating bug #9381 for that caller with no diagnostic. The strip is lossless to push down: CompoundCommandSegment carries only the trimmed command string and terminator operator.
Suggested fix: move stripHeredocBodies(command) inside splitCompoundCommandSegments, revert splitCompoundCommand to projecting the primitive's result, and drop this manual composition (and this import).
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (line.trim() === pendingDelimiters[0]) { | ||
| pendingDelimiters.shift(); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The concurrent-delimiter path of stripHeredocBodies (the pendingDelimiters queue and its shift() ordering) is never exercised by any test, though it is now wired into the permission-rule path via splitCompoundCommand. The one-line mutation shift() → pop() passes every existing test (verified: 533/533 pass under the mutation).
Failure scenario: multiple heredocs opened on one line are valid bash (cat <<A <<B). Under the mutation, for cat <<A <<B\nbodyA\nA\nbodyB\nB\nrm -rf / the terminator A pops B, nothing later matches A, and everything after — bodyB, B, and the real trailing command rm -rf / — is swallowed as heredoc body and never evaluated. Nothing in the suite detects this regression class. Current code is correct (probe: segments ["cat <<A <<B", "rm -rf /"]), so this is unpinned coverage on a safety-relevant ordering, not a live bug.
Suggested fix: add a unit test such as expect(splitCompoundCommand('cat <<A <<B\nbodyA\nA\nbodyB\nB && echo done')).toEqual(['cat <<A <<B', 'echo done']).
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const quote = line[wordStart]; | ||
| const quoted = quote === "'" || quote === '"'; |
There was a problem hiding this comment.
[Suggestion] Double-quoted heredoc delimiters (<<"TAG") are untested everywhere in the permissions suite (only <<EOF, <<'PY', and <<-PY forms appear), leaving the double-quote alternative of getHeredocDelimiters unpinned on the new rule-evaluation path. The one-line mutation dropping the || quote === '"' alternative passes all current tests (verified).
Failure scenario: under the mutation, cat <<"TAG"\nrm -rf / && ops\nTAG would no longer be stripped, so the body lines fall back into per-segment evaluation — exactly the #9381 failure mode (spurious prompts, prefix allow rules never matching the whole command) silently reintroduced for the double-quoted delimiter form. Current code handles the input correctly, so this is an unpinned-branch gap, not a live bug.
Suggested fix: extend one of the new tests, e.g. expect(splitCompoundCommand('cat <<"TAG"\nbody && ops\nTAG')).toEqual(['cat <<"TAG"']).
— qwen3.8-max via Qwen Code /review (v0.21.13)
What this PR does
Fixes #9381. A shell command with a heredoc body was split per line during permission rule evaluation, so
python - <<'PY'\nimport os\n...\nPYbecame four segments and an allow rule likeBash(python *)could never match the whole command.splitCompoundCommandnow strips heredoc bodies before splitting, the same treatment the file-operation path (walkCompoundCommand) already applied.Why it's needed
The per-line fallout was real friction: body lines like
import oswere evaluated as commands of their own, matching nothing and falling through to per-line prompts. Heredoc bodies are stdin, not shell segments, so they should not be evaluated at all.Reviewer Test Plan
How to verify
splitHeredocBodiesmoves unchanged from shell-semantics.ts to rule-parser.ts andsplitCompoundCommandstrips before it splits. The permission-manager suite covers it at both levels:splitCompoundCommandkeepspython - <<'PY'\nimport os\nprint(os.getcwd())\nPYas one segment (plus body-with-operators and<<-variants), and aBash(python *)allow rule now auto-approves that whole heredoc command end to end. The new tests fail on main without the rule-parser change and pass with it.Evidence (Before & After)
N/A (permission evaluation internals, no UI)
Tested on
npx vitest run src/permissions/permission-manager.test.ts src/permissions/shell-semantics.test.tspasses 444/444 on macOS.