fix(core): strip additional dangerous interpreter rules#4371
Conversation
📋 Review SummaryThis PR adds coverage for additional dangerous interpreter tokens ( 🔍 General Feedback
🎯 Specific Feedback🔵 Low
✅ Highlights
|
Qwen Code Review (LIGHT)What this PR does: Adds Findings:
No other issues in the visible diff. Tier: LIGHT. Reply Reviewed by |
| 'python2', | ||
| 'node', | ||
| 'deno', | ||
| 'tsx', |
There was a problem hiding this comment.
[Critical] .exe suffix bypass for scripting interpreters — The PR adds .exe variants for Windows shells (bash.exe, cmd.exe, pwsh.exe, powershell.exe) but does not apply the same treatment to scripting-language interpreters and runners. On Windows, Bash(python.exe *) or Bash(node.exe -e *) bypasses the AUTO-mode strip because isInterpreterToken does an exact includes() match: "python.exe" !== "python".
Consider stripping a trailing .exe (case-insensitive) inside isInterpreterToken before the list lookup. This normalizes python.exe → python, Node.EXE → node, etc., and makes the per-token .exe entries for shells unnecessary:
let segment = (beforeColon ?? '').split('/').pop() ?? '';
if (segment.toLowerCase().endsWith('.exe')) {
segment = segment.slice(0, -4);
}
return DANGEROUS_BASH_INTERPRETERS.includes(segment);— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| expect(isDangerousBashRule(bashRule(interp))).toBe(true); | ||
| }); | ||
|
|
||
| it.each(['tsx -e *', 'ssh prod-host -- *', 'bunx -p dangerous-pkg *'])( |
There was a problem hiding this comment.
[Suggestion] Missing bare-name tests for the 8 new tokens — The new tokens are only tested in wildcard forms (e.g., tsx -e *, cmd /c *). The bare-name branch (firstToken === content at dangerousRules.ts:163) is a distinct code path that is not exercised for any of: tsx, ssh, bunx, bash.exe, cmd, cmd.exe, pwsh.exe, powershell.exe. Every pre-existing interpreter token has bare-name coverage.
it.each(['tsx', 'ssh', 'bunx', 'bash.exe', 'cmd', 'cmd.exe', 'pwsh.exe', 'powershell.exe'])(
'flags new interpreter %s as bare name',
(interp) => {
expect(isDangerousBashRule(bashRule(interp))).toBe(true);
},
);— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| }); | ||
|
|
||
| it.each(['tsx -e *', 'ssh prod-host -- *', 'bunx -p dangerous-pkg *'])( | ||
| 'flags Unix shell or runner wildcard %s', |
There was a problem hiding this comment.
[Suggestion] Misleading test description — 'flags Unix shell or runner wildcard %s' is inaccurate: tsx is a scripting-language interpreter (categorized under // Scripting-language interpreters in the source), ssh is a remote shell (// Remote shells), and only bunx loosely fits "runner." Consider a more accurate description:
'flags new interpreter, remote shell, or runner wildcard %s',— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| 'powershell.exe -Command *', | ||
| 'pwsh.exe -Command *', | ||
| ])('flags Windows shell wildcard %s', (s) => { | ||
| expect(isDangerousBashRule(bashRule(s))).toBe(true); |
There was a problem hiding this comment.
[Suggestion] Missing negative tests for concrete commands using new tokens — The existing test file has negative tests verifying that concrete commands like python script.py are NOT flagged. No equivalent assertions exist for the new tokens. A regression could over-flag legitimate concrete allow rules (e.g., tsx script.tsx, ssh prod-host -- ls, cmd /c script.bat) in AUTO mode.
it('does NOT flag concrete commands using new interpreters', () => {
expect(isDangerousBashRule(bashRule('tsx script.tsx'))).toBe(false);
expect(isDangerousBashRule(bashRule('ssh prod-host -- ls'))).toBe(false);
expect(isDangerousBashRule(bashRule('cmd /c script.bat'))).toBe(false);
expect(isDangerousBashRule(bashRule('powershell.exe -File deploy.ps1'))).toBe(false);
});— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| * shell interpreters, scripting-language interpreters, and build/package | ||
| * tools that themselves run arbitrary scripts (`cargo run`, `npm run`, …). | ||
| * Mirrors and extends ClaudeCode's `DANGEROUS_BASH_PATTERNS`. | ||
| * Unix and Windows shell interpreters, scripting-language interpreters, |
There was a problem hiding this comment.
[Suggestion] The JSDoc claims this list "Mirrors Claude Code's shell and scripting-interpreter checks" but the referenced upstream identifier (DANGEROUS_BASH_PATTERNS) cannot be located in any public Claude Code source. This creates a phantom sync obligation — future maintainers will waste time trying to find and diff against a non-existent reference.
Consider replacing with a self-contained description:
* Originally inspired by Claude Code's dangerous-command checks.
* Maintained independently — add any token that lets the leading
* command of an allow rule execute arbitrary code.— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| const withoutExe = lastSegment.endsWith('.exe') | ||
| ? lastSegment.slice(0, -'.exe'.length) | ||
| : lastSegment; | ||
| return ( |
There was a problem hiding this comment.
[Suggestion] withoutExe fallback is unidirectional — it strips the .exe suffix and checks whether the stripped base name is in DANGEROUS_BASH_INTERPRETERS, but never does the reverse. If a future maintainer adds only python.exe to the list (without also adding python), a bare Bash(python) rule would NOT be flagged:
lastSegment = 'python',withoutExe = 'python'- Neither
'python'nor'python'matches'python.exe'→isInterpreterTokenreturnsfalse
The current list avoids this by duplicating every entry in both forms (bash/bash.exe, cmd/cmd.exe, pwsh/pwsh.exe, etc.), but that duplication is itself a signal of the design asymmetry — and a maintenance hazard.
Consider one of: (a) make the fallback bidirectional (check both lastSegment and lastSegment + '.exe' against the list, then remove redundant .exe entries), or (b) document the invariant that both forms must always be added in a comment above the list, enforced by a unit test that verifies every .exe entry has a matching non-.exe counterpart.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Bare interpreter name (`python`, `/usr/bin/python3`) — caller decides | ||
| // what to do, classifier never sees it. Dangerous. | ||
| if (firstToken === content) return true; | ||
| if (firstToken === content && !content.includes(':')) return true; |
There was a problem hiding this comment.
[Critical] Windows drive-letter colon bypass — Bare Windows interpreter paths like C:\Python\python.exe are NOT flagged as dangerous.
Root cause: content.includes(':') returns true for drive letters (the C: at position 1), so the bare-name guard is skipped. The colon-form check then treats the drive-letter : as a colon-form delimiter, sees afterColon = '\python\python.exe' (non-empty), and returns false.
Trace for Bash(C:\Python\python.exe):
content = 'c:\python\python.exe'
firstToken === content → true
!content.includes(':') → false ← drive letter trips this
content.includes('*') → false
content.includes(':') → true, afterColon = '\python\python.exe' ≠ '' → false
→ returns false (NOT flagged)
Compare Unix: Bash(/usr/bin/python3) → !content.includes(':') → true → correctly flagged.
Also: the !content.includes(':') guard is load-bearing (without it, colon-form rules like python:run-tests would be mis-flagged) but has no comment explaining why. Consider documenting this.
| if (firstToken === content && !content.includes(':')) return true; | |
| // Bare interpreter name (`python`, `/usr/bin/python3`, | |
| // `C:\Python\python.exe`) — caller decides what to do, | |
| // classifier never sees it. Dangerous. | |
| // The `!includes(':')` guard is load-bearing: since firstToken | |
| // is split on whitespace only, colon-form inputs like | |
| // `python:run-tests` reach here with firstToken === content. | |
| if (firstToken === content) { | |
| if (!content.includes(':')) return true; | |
| // Windows drive-letter path — the colon is part of the drive | |
| // specifier, not a colon-form delimiter. Still dangerous. | |
| if (/^[a-z]:[\\/]/i.test(content)) return true; | |
| } |
— qwen3.7-max via Qwen Code /review
| 'bash.exe', | ||
| 'cmd', | ||
| 'cmd.exe', | ||
| 'pwsh', |
There was a problem hiding this comment.
[Suggestion] Dead .exe entries — bash.exe, cmd.exe, pwsh.exe, powershell.exe are unreachable because stripWindowsExecutableSuffix is applied bidirectionally in isInterpreterToken. The bare names (bash, cmd, pwsh, powershell) always match first via stripping. Other interpreters (python, node, tsx) correctly omit .exe variants.
Consider removing the four redundant entries and adding a note:
// Windows shells (bare names only — .exe variants are matched
// by stripWindowsExecutableSuffix in isInterpreterToken)
'cmd',
'pwsh',
'powershell',— qwen3.7-max via Qwen Code /review
| @@ -140,12 +163,12 @@ export function isDangerousBashRule(rule: PermissionRule): boolean { | |||
| // dangerous when it appears as the first token of either form | |||
There was a problem hiding this comment.
[Suggestion] Stale comment — says "Treat both whitespace and : as token delimiters" but the split regex was changed from /[\s:]/ to /\s/. Colon is no longer a delimiter at this stage; it's handled by isInterpreterToken and the colon-form check below.
| // dangerous when it appears as the first token of either form | |
| // Split on whitespace only. Colon-form specifiers (e.g. | |
| // `python:run-tests`) arrive as a single first token; | |
| // isInterpreterToken handles colon extraction internally. | |
| const firstToken = content.split(/\s/)[0] ?? ''; |
— qwen3.7-max via Qwen Code /review
| expect(isDangerousBashRule(bashRule(interp))).toBe(true); | ||
| }); | ||
|
|
||
| it.each(['tsx -e *', 'ssh prod-host -- *', 'bunx -p dangerous-pkg *'])( |
There was a problem hiding this comment.
[Suggestion] Missing test coverage for two code paths with the new tokens:
-
Colon-form wildcards —
tsx:*,ssh:*,bunx:*,cmd:*and empty-suffixtsx:,ssh:,cmd:are untested. Existing colon-form tests only cover pre-PR tokens (python:*,node:*,bash:*). -
Bare-name
.exefor non-listed interpreters —python.exe,node.exe,tsx.exe,bun.exe(bare name, no wildcard) are not tested. These only match via the newstripWindowsExecutableSuffixlogic (they're NOT literally inDANGEROUS_BASH_INTERPRETERS), so they exercise a different code path thanbash.exe/cmd.exewhich are in the list.
it.each(['tsx:*', 'ssh:*', 'bunx:*', 'cmd:*'])(
'flags colon-form wildcard %s for new tokens',
(s) => {
expect(isDangerousBashRule(bashRule(s))).toBe(true);
},
);
it.each(['python.exe', 'node.exe', 'tsx.exe', 'bun.exe'])(
'flags bare-name .exe for non-listed interpreters %s',
(s) => {
expect(isDangerousBashRule(bashRule(s))).toBe(true);
},
);— qwen3.7-max via Qwen Code /review
| } | ||
| const noWildcard = rawToken.slice(0, end); | ||
| const beforeColon = noWildcard.split(':')[0]; | ||
| const beforeColon = /^[a-z]:[\\/]/i.test(noWildcard) |
There was a problem hiding this comment.
[Critical] Windows drive-letter + matcher-colon bypass (Layer 1 of 2) — When content starts with a drive letter (e.g. C:\Python\python.exe:*), the drive-letter regex suppresses colon splitting here, so the matcher colon fuses onto the last path segment. For C:\Python\python.exe:*, noWildcard becomes c:\python\python.exe:, the regex matches and keeps the full string, then split(/[\\/]/).pop() yields python.exe: (with trailing colon). stripWindowsExecutableSuffix('python.exe:') is a no-op (ends with :, not .exe), no interpreter matches, and isInterpreterToken returns false — short-circuiting isDangerousBashRule before the wildcard or colon-form checks.
Verified: C:\Python\python.exe:* → false, C:\Python\python:* → false. Unix equivalents (/usr/bin/python:* → true, python:* → true) are correctly caught.
| const beforeColon = /^[a-z]:[\\/]/i.test(noWildcard) | |
| let beforeColon: string; | |
| if (/^[a-z]:[\\/]/i.test(noWildcard)) { | |
| // Windows absolute path — the first colon is a drive letter. | |
| // A second colon (if present) is a matcher colon; split on it. | |
| const matcherColon = noWildcard.indexOf(':', 2); | |
| beforeColon = matcherColon === -1 | |
| ? noWildcard | |
| : noWildcard.slice(0, matcherColon); | |
| } else { | |
| beforeColon = noWildcard.split(':')[0]; | |
| } |
— qwen3.7-max via Qwen Code /review
| if (!isInterpreterToken(firstToken)) return false; | ||
| const hasMatcherColon = | ||
| content.includes(':') && !/^[a-z]:[\\/]/i.test(content); | ||
|
|
There was a problem hiding this comment.
[Critical] Windows drive-letter + matcher-colon bypass (Layer 2 of 2) — The same drive-letter regex negates the colon check here, setting hasMatcherColon = false even when a second colon exists that IS a matcher colon. For C:\Python\python.exe:*, content.includes(':') is true but /^[a-z]:[\\/]/i.test(content) is also true, so hasMatcherColon becomes false — the colon-form handler at line 182 is skipped entirely.
Even if Layer 1 (isInterpreterToken at line 124) is fixed, this second guard independently sustains the bypass. Both must be corrected.
| const hasMatcherColon = (() => { | |
| const colonIdx = content.indexOf(':'); | |
| if (colonIdx < 0) return false; | |
| // If the first colon is part of a drive letter (position 1), | |
| // look for a second colon (the matcher colon). | |
| if (colonIdx === 1 && /^[a-z]:[\\/]/i.test(content)) { | |
| return content.indexOf(':', colonIdx + 1) >= 0; | |
| } | |
| return true; | |
| })(); |
— qwen3.7-max via Qwen Code /review
| // An interpreter is dangerous when it appears as the first token of either | ||
| // form | ||
| // (`python -c *` or `python:*`). For colon-form, the part after `:` is | ||
| // the specifier — we'll separately check whether it's concrete below. |
There was a problem hiding this comment.
[Suggestion] Windows paths with spaces bypass detection — split(/\s/) truncates at the first space. C:\Program Files\Python\python.exe (the default Windows install location for Python, Node.js, and many others) yields c:\program as the first token, which never matches any interpreter.
Verified: C:\Program Files\Python\python.exe → false, C:\Program Files\Python\python.exe * → false, C:\Program Files\Python\python.exe -c * → false.
Consider extracting the leading path up to (and including) the .exe boundary when the content starts with a Windows drive letter, or using quote-aware token splitting.
— qwen3.7-max via Qwen Code /review
| expect(isDangerousBashRule(bashRule(s))).toBe(true); | ||
| }); | ||
|
|
||
| it.each([ |
There was a problem hiding this comment.
[Suggestion] Missing negative test for Windows absolute-path + concrete command — The concrete-command negative tests cover bare .exe forms (python.exe script.py) but never a full Windows absolute path like C:\Python\python.exe script.py. This is the only scenario that integrates all four new Windows features (drive-letter guard, backslash splitting, .exe stripping, hasMatcherColon guard) with the concrete-command fallthrough. A future refactor breaking this interaction would have no test to catch it.
| it.each([ | |
| it.each([ | |
| 'tsx script.tsx', | |
| 'ssh prod-host -- ls', | |
| 'cmd /c script.bat', | |
| 'cmd.exe /c script.bat', | |
| 'pwsh.exe -File script.ps1', | |
| 'powershell.exe -File script.ps1', | |
| 'python.exe script.py', | |
| 'node.exe script.js', | |
| 'bunx eslint .', | |
| 'C:\\Python\\python.exe script.py', | |
| 'C:\\nodejs\\node.exe server.js', | |
| ])('does NOT flag concrete commands using new tokens %s', (s) => { |
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R4 Critical (Windows drive-letter + matcher-colon bypass) is fully addressed by matcherColonIndex. The R4 Suggestions (paths with spaces, missing negative tests) are also addressed for the common .exe case. Build clean, 110/110 tests pass. LGTM! ✅ — qwen3.7-max via Qwen Code /review
pomelo-nwu
left a comment
pomelo-nwu
left a comment
There was a problem hiding this comment.
LGTM. Harden AUTO mode security classifier by adding coverage for tsx, ssh, bunx, and Windows shell executable variants that could bypass dangerous-command detection.
本地验证报告（maintainer review）把本 PR 的 7 个 commit cherry-pick 到当前 验证矩阵
Pre-existing 失败归因（与本 PR 无关）
建议作为独立 issue 跟进，不阻塞本 PR。 代码评审要点
风险与遗留
结论建议合并。所有可归因于本 PR 的检查全部通过，逻辑收窄了 AUTO-mode 危险 allow-rule 的盲区且不影响 concrete 命令的合法白名单。 验证环境：macOS Darwin 25.4.0，Node v22.17.0，npm 11.8.0。tmux 多 worktree 并行（PR-as-is + cherry-picked-on-main 对照）。 |
…4500) Pulls 5 main commits since #4469 (2026-05-24): - #4464 fix(weixin): send decryptable image payloads - #4465 fix(weixin): allow Windows image paths inside workspace - #4470 fix(cli): resolve stale closure race in text buffer submit handler - #4468 feat(skills): add memory-leak-debug skill for heap snapshot diagnosis - #4288 feat(cli): do not append trailing space for directory completions (#4092) 11 manual conflicts resolved + 2 add/add conflicts taken from main wholesale: Manual UU (12, all daemon-side preferred except text-buffer.ts): - packages/acp-bridge/package.json — kept HEAD's fuller description (F1 lift expanded the package surface; main has stale pre-F1 wording). - packages/cli/src/acp-integration/acpAgent.ts — kept HEAD's WorkspaceMcpBudget import (F2 needs it). - packages/cli/src/acp-integration/acpAgent.worktree.test.ts (AA): kept HEAD's superset of mocks (MCP_BUDGET_WARN_FRACTION, getMCPDiscoveryState, MCPServerStatus, McpTransportPool, WorkspaceMcpBudget, workspace/debug/mcp config mocks). HEAD already includes main-side SessionStartSource + SessionEndReason mocks. - packages/cli/src/ui/commands/directoryCommand.tsx — pure formatting (HEAD wrapped vs main inline). Kept HEAD. - packages/cli/src/ui/commands/directoryCommand.test.tsx — pure formatting. Kept HEAD. - packages/cli/src/ui/commands/skillsCommand.ts — pure formatting. Kept HEAD. - packages/cli/src/ui/hooks/useCommandCompletion.tsx — pure formatting. Kept HEAD. - packages/cli/src/ui/hooks/useCommandCompletion.test.ts — pure formatting. Kept HEAD. - packages/cli/src/ui/hooks/useSlashCompletion.test.ts — pure formatting. Kept HEAD. - packages/core/src/config/config.test.ts — kept HEAD's TrustGateError import (daemon-added). text-buffer.ts (4 zones — took MAIN wholesale for #4470's stale-closure fix): - Import: useRef instead of useReducer (daemon side had useReducer as a dead import — file uses dispatch via useCallback, not useReducer; verified via grep). useRef is needed for stateRef + #4470's currentText capture. - writeFileSync zone: use stateRef.current.lines.join('\n') instead of stale closure-captured `text`. Fixes #4470's bug. - text comparison: `newText !== currentText` not `newText !== text`. - dep array: `[dispatch, ...]` not `[text, ...]` (callback reads from ref now, doesn't need to re-bind on text change). AA (2, main wholesale via git checkout --theirs): - packages/core/src/permissions/dangerousRules.ts + .test.ts Original #4151 Auto-mode added these on main, came into daemon via #4469 squash. Main then landed #4371 ("strip additional dangerous interpreter rules") as a follow-up that daemon side never saw. Take main's evolved version wholesale. Verification: - packages/core tsc: 50 errors PRE-merge, 50 errors POST-merge (pre-existing baseline — none introduced by this sync). - packages/acp-bridge tsc: clean. - 5 spot-test runs on conflict-resolved files: 132 + 17 + 24 + 30 + 1 = 204 tests pass (text-buffer / directoryCommand / useCommandCompletion / useSlashCompletion / skillsCommand). Mirrors #4469's pattern (squash merge daemon_mode_b_main-side). Unblocks #4490 daemon_mode_b_main → main reverse integration merge (currently CONFLICTING precisely because of these 5 main commits).
4 participants
Summary
tsx,ssh,bunx, and Windows shell executable variants.Validation
dangerousRules.test.tspassed with 78 tests.git diff --checkpassed.npm run typecheckdid not complete because the local dependency tree could not resolve existing project dependencies such as@google/genai, MCP SDK packages, andundici.Scope / Risk
Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Closes #4370