Skip to content

fix(core): strip additional dangerous interpreter rules #4371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 7 commits into from
May 25, 2026
+128 −13
Merged

fix(core): strip additional dangerous interpreter rules #4371

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
51e7566
fix(core): strip additional dangerous interpreter rules
qqqys May 21, 2026
2be2e0b
test(core): clarify dangerous interpreter coverage
qqqys May 21, 2026
9a62aa7
chore(core): group bash.exe with windows shells
qqqys May 21, 2026
d16cab2
fix(core): normalize dangerous interpreter tokens
qqqys May 23, 2026
cb9661d
test(core): normalize dangerous exe interpreter rules
qqqys May 23, 2026
1359fe5
fix(core): detect windows interpreter path allows
qqqys May 23, 2026
e34ee85
fix(core): detect windows interpreter path allows
qqqys May 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions packages/core/src/permissions/dangerousRules.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,46 @@ describe('isDangerousBashRule', () => {
expect(isDangerousBashRule(bashRule(interp))).toBe(true);
});

it.each(['tsx -e *', 'ssh prod-host -- *', 'bunx -p dangerous-pkg *'])(
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Missing test coverage for two code paths with the new tokens:

  1. Colon-form wildcards — tsx:*, ssh:*, bunx:*, cmd:* and empty-suffix tsx:, ssh:, cmd: are untested. Existing colon-form tests only cover pre-PR tokens (python:*, node:*, bash:*).

  2. Bare-name .exe for non-listed interpreters — python.exe, node.exe, tsx.exe, bun.exe (bare name, no wildcard) are not tested. These only match via the new stripWindowsExecutableSuffix logic (they're NOT literally in DANGEROUS_BASH_INTERPRETERS), so they exercise a different code path than bash.exe/cmd.exe which 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

'flags new interpreter, remote shell, or runner wildcard %s',
(s) => {
expect(isDangerousBashRule(bashRule(s))).toBe(true);
},
);

it.each([
'tsx',
'ssh',
'bunx',
'bash.exe',
'cmd',
'cmd.exe',
'pwsh.exe',
'powershell.exe',
])('flags new interpreter, remote shell, or runner bare name %s', (s) => {
expect(isDangerousBashRule(bashRule(s))).toBe(true);
});

it.each([
'python.exe -c *',
'node.exe -e *',
'tsx.exe -e *',
'bunx.exe -p dangerous-pkg *',
'C:\\Python\\python.exe -c *',
])('flags Windows executable suffix wildcard %s', (s) => {
expect(isDangerousBashRule(bashRule(s))).toBe(true);
});

it.each([
'cmd /c *',
'cmd.exe /c *',
'bash.exe -c *',
'powershell.exe -Command *',
'pwsh.exe -Command *',
])('flags Windows shell wildcard %s', (s) => {
expect(isDangerousBashRule(bashRule(s))).toBe(true);
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

});

it.each([
'bun run *',
'deno run *',
Expand All @@ -176,6 +216,20 @@ describe('isDangerousBashRule', () => {
expect(isDangerousBashRule(bashRule(s))).toBe(true);
});

it.each([
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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

'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 .',
])('does NOT flag concrete commands using new tokens %s', (s) => {
expect(isDangerousBashRule(bashRule(s))).toBe(false);
});

it('flags Monitor allow rules with the same interpreter logic', () => {
// Monitor is a long-running shell-command runner; broad allow rules
// on it bypass the AUTO classifier just like Bash(...) ones.
Expand Down
39 changes: 30 additions & 9 deletions packages/core/src/permissions/dangerousRules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@ import type { PermissionRule } from './types.js';
/**
* Tokens that, when used as the leading command of a Bash allow rule, let the
* model execute arbitrary code under the AUTO classifier's nose. Covers
* 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,
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

* remote shells, and build/package tools that themselves run arbitrary
* scripts (`cargo run`, `npm run`, …). The exact token set is intentionally
* self-contained so AUTO-mode stripping does not depend on an external
* upstream identifier.
*/
const DANGEROUS_BASH_INTERPRETERS: readonly string[] = Object.freeze([
// Shells
// Unix shells
'bash',
'sh',
'zsh',
Expand All @@ -32,14 +34,21 @@ const DANGEROUS_BASH_INTERPRETERS: readonly string[] = Object.freeze([
'tcsh',
'dash',
'ksh',
// Windows shells
'bash.exe',
'cmd',
'cmd.exe',
'pwsh',
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

'pwsh.exe',
'powershell',
'powershell.exe',
// Scripting-language interpreters
'python',
'python3',
'python2',
'node',
'deno',
'tsx',
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

'bun',
'ruby',
'perl',
Expand Down Expand Up @@ -69,10 +78,13 @@ const DANGEROUS_BASH_INTERPRETERS: readonly string[] = Object.freeze([
// that without this list would be the cleanest way to bypass the
// classifier in AUTO mode.
'npx',
'bunx',
'pnpx',
'uvx',
'pipx',
'dlx',
// Remote shells
'ssh',
// Generic eval-y commands
'eval',
'exec',
Expand All @@ -96,6 +108,7 @@ const SHELL_LIKE_TOOLS: readonly string[] = Object.freeze([
* - absolute-path forms (`/usr/bin/python3` → trailing segment `python3`)
* - trailing-wildcard forms (`python3*`)
* - colon form (`python:`)
* - Windows executable suffixes (`python.exe`)
*/
function isInterpreterToken(rawToken: string): boolean {
if (!rawToken) return false;
Expand All @@ -108,10 +121,18 @@ function isInterpreterToken(rawToken: string): boolean {
end--;
}
const noWildcard = rawToken.slice(0, end);
const beforeColon = noWildcard.split(':')[0];
const beforeColon = /^[a-z]:[\\/]/i.test(noWildcard)
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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

? noWildcard
: noWildcard.split(':')[0];
// Last path segment so `/usr/bin/python3` → `python3`
const lastSegment = (beforeColon ?? '').split('/').pop() ?? '';
return DANGEROUS_BASH_INTERPRETERS.includes(lastSegment);
const lastSegment = (beforeColon ?? '').split(/[\\/]/).pop() ?? '';
const withoutExe = lastSegment.endsWith('.exe')
? lastSegment.slice(0, -'.exe'.length)
: lastSegment;
return (
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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' → isInterpreterToken returns false

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

DANGEROUS_BASH_INTERPRETERS.includes(lastSegment) ||
DANGEROUS_BASH_INTERPRETERS.includes(withoutExe)
);
}

/**
Expand Down Expand Up @@ -140,12 +161,12 @@ export function isDangerousBashRule(rule: PermissionRule): boolean {
// dangerous when it appears as the first token of either form
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
// 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

// (`python -c *` or `python:*`). For colon-form, the part after `:` is
// the specifier — we'll separately check whether it's concrete below.
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

const firstToken = content.split(/[\s:]/)[0] ?? '';
const firstToken = content.split(/\s/)[0] ?? '';
if (!isInterpreterToken(firstToken)) return false;

Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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

// 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;
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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


// Wildcard anywhere paired with an interpreter defeats the classifier:
// `python *`, `python -c *`, `bun run *`, `/usr/bin/python3 *`,
Expand Down
Loading