Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
73 changes: 73 additions & 0 deletions packages/core/src/permissions/dangerousRules.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,63 @@ describe('isDangerousBashRule', () => {
expect(isDangerousBashRule(bashRule(interp))).toBe(true);
});

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

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

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 *',
'C:\\Python\\python.exe:*',
'C:\\Python\\python:*',
'C:\\Program Files\\Python\\python.exe -c *',
])('flags Windows executable suffix wildcard %s', (s) => {
expect(isDangerousBashRule(bashRule(s))).toBe(true);
});

it.each([
'C:\\Python\\python.exe',
'C:\\nodejs\\node.exe',
'C:\\Users\\me\\bin\\tsx.exe',
'C:\\Program Files\\Python\\python.exe',
])('flags bare Windows interpreter path %s', (s) => {
expect(isDangerousBashRule(bashRule(s))).toBe(true);
});

it('normalizes Windows executable suffixes in both directions', () => {
expect(isDangerousBashRule(bashRule('cmd *'))).toBe(true);
expect(isDangerousBashRule(bashRule('cmd.exe *'))).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

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 +233,22 @@ describe('isDangerousBashRule', () => {
expect(isDangerousBashRule(bashRule(s))).toBe(true);
});

it.each([
Copy link
Copy Markdown
Collaborator

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 .',
'C:\\Python\\python.exe script.py',
'C:\\Program Files\\Python\\python.exe script.py',
])('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
68 changes: 55 additions & 13 deletions packages/core/src/permissions/dangerousRules.ts
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

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,6 +34,8 @@ const DANGEROUS_BASH_INTERPRETERS: readonly string[] = Object.freeze([
'tcsh',
'dash',
'ksh',
// Windows shells
'cmd',
'pwsh',
Copy link
Copy Markdown
Collaborator

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

'powershell',
// Scripting-language interpreters
Expand All @@ -40,6 +44,7 @@ const DANGEROUS_BASH_INTERPRETERS: readonly string[] = Object.freeze([
'python2',
'node',
'deno',
'tsx',
Copy link
Copy Markdown
Collaborator

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,16 +74,42 @@ 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',
'source',
]);

function stripWindowsExecutableSuffix(token: string): string {
return token.endsWith('.exe') ? token.slice(0, -'.exe'.length) : token;
}

function matcherColonIndex(content: string): number {
const firstColon = content.indexOf(':');
if (firstColon < 0) return -1;
if (/^[a-z]:[\\/]/i.test(content)) {
return content.indexOf(':', 2);
}
return firstColon;
}

function leadingCommandToken(content: string): string {
if (/^[a-z]:[\\/]/i.test(content)) {
const exeIndex = content.indexOf('.exe');
if (exeIndex >= 0) {
return content.slice(0, exeIndex + '.exe'.length);
}
}
return content.split(/\s/)[0] ?? '';
}

/**
* Tools whose allow rules carry shell-like risk. `monitor` is a long-running
* shell-command runner and should be treated the same as `shell` for the
Expand All @@ -96,6 +127,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 +140,16 @@ function isInterpreterToken(rawToken: string): boolean {
end--;
}
const noWildcard = rawToken.slice(0, end);
const beforeColon = noWildcard.split(':')[0];
const colonIndex = matcherColonIndex(noWildcard);
const beforeColon =
colonIndex >= 0 ? noWildcard.slice(0, colonIndex) : noWildcard;
// 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 normalizedSegment = stripWindowsExecutableSuffix(lastSegment);
return DANGEROUS_BASH_INTERPRETERS.some(
(interpreter) =>
stripWindowsExecutableSuffix(interpreter) === normalizedSegment,
);
}

/**
Expand All @@ -136,16 +174,20 @@ export function isDangerousBashRule(rule: PermissionRule): boolean {
const content = rule.specifier.trim().toLowerCase();
if (content === '' || content === '*') return true;

// Treat both whitespace and `:` as token delimiters: an interpreter is
// dangerous when it appears as the first token of either form
// Treat whitespace as the first-token delimiter; matcher-colon form is
// handled separately below because Windows drive letters also contain `:`.
// 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.
Copy link
Copy Markdown
Collaborator

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 = leadingCommandToken(content);
if (!isInterpreterToken(firstToken)) return false;
const colonIndex = matcherColonIndex(content);
const hasMatcherColon = colonIndex >= 0;

Copy link
Copy Markdown
Collaborator

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 && !hasMatcherColon) return true;

// Wildcard anywhere paired with an interpreter defeats the classifier:
// `python *`, `python -c *`, `bun run *`, `/usr/bin/python3 *`,
Expand All @@ -159,8 +201,8 @@ export function isDangerousBashRule(rule: PermissionRule): boolean {
// rules β€” same shape as `Bash(npm run test)`, which the docstring above
// commits to NOT flagging. Strip them and we'd silently disable
// intentional user allow lists in AUTO.
if (content.includes(':')) {
const afterColon = content.slice(content.indexOf(':') + 1).trim();
if (hasMatcherColon) {
const afterColon = content.slice(colonIndex + 1).trim();
return afterColon === '';
}

Expand Down
Loading