fix(core): refuse substitutions hidden in pattern words and heredoc bodies - #10029
fix(core): refuse substitutions hidden in pattern words and heredoc bodies#10029TianYuan1024 wants to merge 2 commits into
Conversation
…odies
Two places where tree-sitter-bash yields a single leaf node, so the
substitution walk finds nothing to collect while bash still runs what is
inside.
The pattern word of `${v%%…}`, `${v%…}`, `${v##…}`, `${v#…}`, `${v^^…}`,
`${v^…}`, `${v,,…}`, `${v,…}` is one leaf, and so is each half of
`${v/pat/rep}` and the operand of `${v:-…}`, `${v:=…}`, `${v:?…}`, `${v:+…}`.
`echo ${x%%$(rm -rf build)}` therefore classified `read-only` and would have
run unattended. Since the collection pass found nothing, an opener still
present in the expansion text is exactly that hidden channel, so the leaf is
refused on the text.
A heredoc body is one leaf too — always for `<<-`, and for `<<` whenever
nothing inside it parsed — and bash expands it before feeding it to stdin.
Expansion there follows double-quote rules, so `$(…)`, backticks and `${v@P}`
run while `<(…)` does not; the body is refused for the first three only. A
quoted delimiter (`<<'EOF'`, `<<"EOF"`, `<<\EOF`) makes the body inert and is
exempted.
`${v@P}` is included in both because a prompt expansion runs any `$(…)` held in
the variable's value, and in a pattern word or a body it is a leaf that the
`@`/`P` child-adjacency check never sees. The regex is deliberately not
anchored to a brace-free span: `${a[${b}]@p}` nests a brace, and a `[^{}]*`
bridge stops at it.
These are leaf fallbacks for sites the node walk cannot reach, so over-refusing
costs at most a prompt. Mutation-verified: neutralising the three regexes fails
28, 10 and 2 tests respectively, and dropping the quoted-delimiter exemption
fails 1.
|
Thanks for the PR! Template looks good ✓ — the Problem: observed, not theoretical. The PR names concrete commands ( Direction: aligned. The classifier's entire contract is " Size: core paths touched ( Approach: scope feels right — three leaf regexes plus a quoted-delimiter exemption, all confined to the branch that already found nothing structurally, so any command with a real substitution node keeps its existing Risk: no elevated risk signals — neither changed file matches the revert-correlated high-risk paths. Note for the reviewer: this is Tier 2 core territory, so the bar below is 100% confidence, and the behavioral claims (tree-sitter grammar quirks: leaf pattern words, Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 缺少 问题：已观测而非理论性的。PR 给出了具体命令（ 方向：对齐。分类器的全部契约就是「 规模：触及核心路径（ 方案：范围合理——三个叶子正则加一个带引号分隔符豁免，全部限制在"结构上已经一无所获"的分支内，因此凡是存在真实替换节点的命令仍走原有的 风险：无升级风险信号——两个改动文件均未命中与 revert 相关的高风险路径。提醒 reviewer：这属于 Tier 2 核心模块，下面的标准是 100% 确信；其中关于 tree-sitter 语法行为的主张（叶子模式词、 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review. Independent baseline first: given only the title and the "why", my own approach would have been exactly this shape — stay inside the zero-substitution branch of What I verified against the code:
Not verified statically: the grammar-behavior claims (leaf pattern words, Test evidence — the PR's own CI on
Sandboxed verification would settle the remaining behavioral claim end-to-end: 中文说明代码审查。 先说独立基线：只看标题和动机，我自己的方案与此完全同形——留在 对照代码核实的内容：
静态无法核实的部分：tree-sitter 语法行为（叶子模式词、 测试证据——本 PR 在
（CI 表格见上方标记区域，完成后由 finalize 工作流自动更新。） 沙箱验证可以端到端落定剩余的行为主张： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a clean, one-directional safety fix with unusually disciplined test pinning; the only reservation is CI settling (two Test jobs red from runner disk exhaustion, macOS job still queued), not the code. Stepping back: my independent proposal for this problem — fail-closed text checks at exactly the leaf sites bash expands but tree-sitter never decomposes, scoped to the zero-substitution branch — is what this PR implements, and I didn't find a simpler path it missed. The hole is real (I traced it in the base classifier), the fix can only move verdicts toward prompting, and the tests pin each spelling by expected category so future grammar or refactor drift fails loudly instead of silently reopening the hole. The comments explain the non-obvious why (the unanchored The author has other shell-parser PRs open right now; judged on its own, this one earns its place — it closes a concrete silent-execution path in Plan Mode rather than rearranging code. Why not 5/5 and why no approval this run: the PR's CI hasn't finished settling on the reviewed commit. Once it lands green on that commit the approval follows automatically; if anything lands red or the head moves, the deferral flags it instead. 中文说明置信度：4/5 —— 一个干净的、单向收紧的安全修复，测试钉住做得异常严谨；唯一的保留是 CI 尚未落定（两个 Test 任务因 runner 磁盘耗尽而红，macOS 任务仍在排队），与代码本身无关。 退一步看：我对这个问题的独立方案——在 bash 会展开但 tree-sitter 不分解的叶子位置、且仅限于零替换分支内做失败关闭的文本检查——正是这个 PR 的实现，我没有找到它遗漏的更简路径。漏洞真实存在（我在基线分类器中追踪到了），修复只会把判定推向"弹窗"方向，测试按预期类别钉住每种写法，使未来语法升级或重构导致的漂移会大声失败而不是静默重开漏洞。注释解释了非显而易见的"为什么"（不加锚定的 作者目前还有其他 shell 解析器相关的 PR 在途；单独评判，这个 PR 有其价值——它堵上的是 Plan Mode 中一条具体的静默执行通道，而不是重排代码。 为什么不是 5/5、为什么本次运行不直接批准：该 PR 的 CI 在被审查提交上尚未落定。一旦在该提交上全绿，批准会自动跟上；若有任何任务变红或 head 移动，延期机制会标记出来而不是放行。 — Qwen Code · qwen3.8-max Reviewed at |
|
Added the Also merged Ready for re-run. |
|
Thanks for the stage-3 read — the What moved it, and why it was necessary: the only new commit is a merge of The other red job you saw is the Windows lane, and it is not this diff either: Note the run on the current head was cancelled about 40 seconds in, along with most other runs in the repository between 16:05Z and 16:49Z — a capacity event, not a failure on this commit. @qwen-code /triage |
|
@qwen-code /triage (The earlier request had the mention at the end of the comment rather than the first line, so it never matched the trigger — resending it correctly.) The stage-3 read left |
2 participants
What this PR does
Closes two places where tree-sitter-bash hands back a single leaf node, so the classifier's substitution walk finds nothing to collect while bash still runs what is inside.
Expansion pattern words. The pattern of
${v%%…},${v%…},${v##…},${v#…},${v^^…},${v^…},${v,,…},${v,…}is one leaf. So is each half of${v/pat/rep}, and the operand of${v:-…},${v:=…},${v:?…},${v:+…}. A substitution written there yields no node of its own, soecho ${x%%$(rm -rf build)}classifiedread-only— and would run unattended in Plan Mode. Since the collection pass already found nothing, an opener still present in the expansion's text is exactly that hidden channel, and the leaf is refused on the text.Heredoc bodies. A body is one leaf too — always for
<<-, and for<<whenever nothing inside it parsed — and bash expands it before feeding it to stdin. Expansion there follows double-quote rules, so$(…), backticks and${v@P}run while<(…)does not; the body is refused for the first three only. A quoted delimiter (<<'EOF',<<"EOF",<<\EOF) makes the body inert and is exempted.${v@P}is handled in both, because a prompt expansion runs any$(…)held in the variable's value, and in a pattern word or a body it is a leaf the@/Pchild-adjacency check never sees. Its regex is deliberately not anchored to a brace-free span:${a[${b}]@P}nests a brace, and a[^{}]*bridge stops at it.Why it's needed
These are the inputs where the AST gives less information than bash acts on. The classifier's whole contract is that a
read-onlyverdict means nothing executes — and in Plan Mode aread-onlyverdict means the command runs with no confirmation at all.echo ${x%%$(rm -rf build)}is a deletion that never appears as a command node.Reviewer Test Plan
How to verify
Before this change, on
main:After, all three are
false. The exemptions still hold:Mutation-verified: neutralising the three leaf regexes fails 28, 10 and 2 tests respectively, and dropping the quoted-delimiter exemption fails 1.
Note:
src/permissions/permission-manager.test.tshas 2 failures onmainat this commit (resolveToolName exhaustiveness (#9827), aboutReportFindings). They are unrelated to this PR and reproduce on a cleanorigin/maincheckout.Evidence (Before & After)
N/A — no TUI change. The user-visible difference is that these forms now prompt instead of running unattended.
Tested on
Risk & Scope
$(is refused whether or not bash would expand it. They are fallbacks for sites the node walk cannot reach at all, so the cost is a prompt and the alternative is silence; but a user with an unusual pattern word will see a confirmation they did not before.permission-managerfailures noted above. The deprecated regex fallback inshellReadOnlyChecker.tsis untouched.Linked Issues
No issue to close. This is a self-contained correctness fix carved out of #9950 — the hidden-substitution leaves surfaced while that PR was under review, and it is filed separately so it can land on its own merits. Referenced without a closing keyword: #9950.
中文说明
这个 PR 做了什么
修复两处 tree-sitter-bash 只返回单个叶子节点、导致分类器的替换遍历什么都收集不到、而 bash 仍会执行其中内容的情形。
展开的模式词。
${v%%…}、${v%…}、${v##…}、${v#…}、${v^^…}、${v^…}、${v,,…}、${v,…}的模式部分是一个叶子；${v/pat/rep}的两半、以及${v:-…}、${v:=…}、${v:?…}、${v:+…}的操作数同样如此。写在那里的替换不会产生自己的节点，于是echo ${x%%$(rm -rf build)}被判为read-only，在 Plan Mode 下会无人值守执行。既然收集阶段已经一无所获，展开文本中仍然存在的开启符恰恰就是那条隐藏通道，因此按文本拒绝该叶子。heredoc body。 body 同样是一个叶子（
<<-恒为如此，<<在其内部无内容被解析时亦然），而 bash 会先展开它再送入 stdin。此处的展开遵循双引号规则：$(…)、反引号、${v@P}会执行，<(…)不会——因此只对前三者拒绝。带引号的分隔符（<<'EOF'、<<"EOF"、<<\EOF）使 body 惰性化，予以豁免。${v@P}两处都处理，因为提示符展开会运行变量值中携带的任何$(…)，而在模式词或 body 中它是一个叶子，@/P子节点相邻检查永远看不到它。其正则刻意不限定为无花括号跨度：${a[${b}]@P}内嵌了花括号，[^{}]*桥接会在那里断掉。为什么需要它
这些正是 AST 提供的信息少于 bash 实际行为的输入。分类器的全部契约是「
read-only意味着什么都不会执行」——而在 Plan Mode 下，read-only意味着命令完全不弹窗直接执行。echo ${x%%$(rm -rf build)}是一次从不以命令节点形式出现的删除。验证方式
见上文英文部分。变异测试：把三个叶子正则置空分别导致 28、10、2 个测试失败；去掉带引号分隔符的豁免导致 1 个失败。
注：本提交所基于的
main上，permission-manager.test.ts本身有 2 个与本 PR 无关的失败。风险与范围
$(字符就会被拒绝，无论 bash 是否真的会展开。它们是节点遍历根本到不了的位置的兜底，代价是弹窗，而另一个选择是静默；但使用不寻常模式词的用户会看到此前没有的确认框。main自带的失败；已废弃的正则回退路径不动。关联 Issue
没有需要关闭的 issue。这是从 #9950 中拆分出来的一处独立正确性修复——隐藏替换叶子节点是在那个 PR 评审过程中发现的，单独提出以便独立评审合入。仅作引用、不带关闭关键字：#9950。