Skip to content

Commit 1356c05

Browse files
fix(permissions): match env-prefixed shell commands against saved permission rules (#2850)
* fix(permissions): match env-prefixed shell commands Fixes #2846 * fix(core): improve shell command parsing for env vars and multiline commands - Add dotAll flag to matchesCommandPattern for matching commands with embedded newlines - Support newline operators in SHELL_OPERATORS for splitCompoundCommand - Refactor getCommandRoot to skip leading VAR=value assignments - Add test coverage for multiline commands and env var prefixed commands Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(permissions): tighten shell command parsing Handle env-prefixed commands and quoted Windows paths consistently. Keep newline splitting heredoc-aware and avoid false heredoc detection in comments or arithmetic expressions. * refactor(permissions): simplify fix by reverting splitCompoundCommand rewrite Remove ~350 lines of heredoc/comment/arithmetic parsing from splitCompoundCommand that were not needed to fix #2846. Revert to the original main version, keeping only the core env-var stripping logic in matchesCommandPattern and getCommandRoot. This addresses both reviewer concerns: - heredoc breakage: no longer an issue since splitCompoundCommand is unchanged - Windows quoted paths: handled correctly by shell-quote parse in getCommandRoot --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
1 parent 3e8b3c6 commit 1356c05

File tree

4 files changed

+112
-29
lines changed

4 files changed

+112
-29
lines changed

packages/core/src/permissions/permission-manager.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,24 @@ describe('matchesCommandPattern', () => {
228228
expect(matchesCommandPattern('npm run *', 'npm run build')).toBe(true);
229229
});
230230

231+
it('matches commands with leading env var assignments', async () => {
232+
expect(
233+
matchesCommandPattern(
234+
'python3 *',
235+
'PYTHONPATH=/tmp/lib python3 -c "print(1)"',
236+
),
237+
).toBe(true);
238+
});
239+
240+
it('matches commands containing embedded newlines (dotAll)', async () => {
241+
expect(
242+
matchesCommandPattern(
243+
'python3 *',
244+
'python3 -c "\nimport sys\nprint(sys.version)\n"',
245+
),
246+
).toBe(true);
247+
});
248+
231249
it('space-star requires word boundary (ls * does not match lsof)', async () => {
232250
expect(matchesCommandPattern('ls *', 'ls -la')).toBe(true);
233251
expect(matchesCommandPattern('ls *', 'lsof')).toBe(false);

packages/core/src/permissions/rule-parser.ts

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import path from 'node:path';
88
import os from 'node:os';
99
import picomatch from 'picomatch';
10+
import { parse } from 'shell-quote';
1011

1112
/**
1213
* Normalize a filesystem path to use POSIX-style forward slashes.
@@ -606,6 +607,7 @@ export function matchesCommandPattern(
606607
): boolean {
607608
// This function matches a single pattern against a single simple command.
608609
// Compound command splitting is handled by the caller (PermissionManager).
610+
const normalizedCommand = stripLeadingVariableAssignments(command);
609611

610612
// Special case: lone `*` matches any single command
611613
if (pattern === '*') {
@@ -616,7 +618,10 @@ export function matchesCommandPattern(
616618
// No wildcards: prefix matching (backward compat).
617619
// "git commit" matches "git commit" and "git commit -m test"
618620
// but NOT "gitcommit".
619-
return command === pattern || command.startsWith(pattern + ' ');
621+
return (
622+
normalizedCommand === pattern ||
623+
normalizedCommand.startsWith(pattern + ' ')
624+
);
620625
}
621626

622627
// Build regex from glob pattern with word-boundary semantics.
@@ -665,9 +670,9 @@ export function matchesCommandPattern(
665670
regex += '$';
666671

667672
try {
668-
return new RegExp(regex).test(command);
673+
return new RegExp(regex, 's').test(normalizedCommand);
669674
} catch {
670-
return command === pattern;
675+
return normalizedCommand === pattern;
671676
}
672677
}
673678

@@ -678,6 +683,44 @@ function escapeRegex(s: string): string {
678683
return s.replace(/[.+?^${}()|[\]\\]/g, '\\$&');
679684
}
680685

686+
const ENV_ASSIGNMENT_REGEX = /^[A-Za-z_][A-Za-z0-9_]*=/;
687+
688+
function stripLeadingVariableAssignments(command: string): string {
689+
const trimmed = command.trim();
690+
if (!trimmed) {
691+
return trimmed;
692+
}
693+
694+
try {
695+
const tokens: string[] = [];
696+
697+
for (const token of parse(trimmed)) {
698+
if (typeof token === 'string') {
699+
tokens.push(token);
700+
} else if (
701+
token &&
702+
typeof token === 'object' &&
703+
'op' in token &&
704+
typeof token.op === 'string'
705+
) {
706+
tokens.push(token.op);
707+
}
708+
}
709+
710+
let firstCommandToken = 0;
711+
while (
712+
firstCommandToken < tokens.length &&
713+
ENV_ASSIGNMENT_REGEX.test(tokens[firstCommandToken]!)
714+
) {
715+
firstCommandToken++;
716+
}
717+
718+
return tokens.slice(firstCommandToken).join(' ');
719+
} catch {
720+
return trimmed;
721+
}
722+
}
723+
681724
// ─────────────────────────────────────────────────────────────────────────────
682725
// File path matching (gitignore-style)
683726
// ─────────────────────────────────────────────────────────────────────────────

packages/core/src/utils/shell-utils.test.ts

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,15 @@ vi.mock('os', () => ({
2929
}));
3030

3131
const mockQuote = vi.hoisted(() => vi.fn());
32-
const mockParse = vi.hoisted(() => vi.fn());
33-
vi.mock('shell-quote', () => ({
34-
quote: mockQuote,
35-
parse: mockParse,
36-
}));
32+
vi.mock('shell-quote', async () => {
33+
const actual =
34+
await vi.importActual<typeof import('shell-quote')>('shell-quote');
35+
36+
return {
37+
...actual,
38+
quote: mockQuote,
39+
};
40+
});
3741

3842
let config: Config;
3943

@@ -42,7 +46,6 @@ beforeEach(() => {
4246
mockQuote.mockImplementation((args: string[]) =>
4347
args.map((arg) => `'${arg}'`).join(' '),
4448
);
45-
mockParse.mockImplementation((cmd: string) => cmd.split(' '));
4649
config = {
4750
getCoreTools: () => [],
4851
getPermissionsDeny: () => [],
@@ -453,6 +456,20 @@ describe('getCommandRoots', () => {
453456
const result = getCommandRoots('echo hello >| out.txt');
454457
expect(result).toEqual(['echo']);
455458
});
459+
460+
it('should skip leading env var assignments', async () => {
461+
expect(
462+
getCommandRoots(
463+
'PYTHONPATH=/Users/jinjing/.qwen/skills/scripts python3 -c "print(1)"',
464+
),
465+
).toEqual(['python3']);
466+
});
467+
468+
it('should preserve quoted Windows paths with spaces', async () => {
469+
expect(getCommandRoots('"C:\\Program Files\\foo\\bar.exe" arg1')).toEqual([
470+
'bar.exe',
471+
]);
472+
});
456473
});
457474

458475
describe('stripShellWrapper', () => {

packages/core/src/utils/shell-utils.ts

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import type { AnyToolInvocation } from '../index.js';
88
import type { Config } from '../config/config.js';
99
import os from 'node:os';
1010
import path from 'node:path';
11-
import { quote } from 'shell-quote';
11+
import { parse, quote } from 'shell-quote';
1212
import { doesToolInvocationMatch } from './tool-utils.js';
1313
import { isShellCommandReadOnly } from './shellReadOnlyChecker.js';
1414
import {
@@ -40,6 +40,7 @@ export interface ShellConfiguration {
4040
}
4141

4242
let cachedBashPath: string | undefined;
43+
const ENV_ASSIGNMENT_REGEX = /^[A-Za-z_][A-Za-z0-9_]*=/;
4344

4445
/**
4546
* Attempts to find the Git Bash executable path on Windows.
@@ -289,34 +290,38 @@ export function splitCommands(command: string): string[] {
289290

290291
/**
291292
* Extracts the root command from a given shell command string.
292-
* This is used to identify the base command for permission checks.
293-
* @param command The shell command string to parse
294-
* @returns The root command name, or undefined if it cannot be determined
295-
* @example getCommandRoot("ls -la /tmp") returns "ls"
296-
* @example getCommandRoot("git status && npm test") returns "git"
293+
* Skips leading env var assignments (VAR=value) so that
294+
* `PYTHONPATH=/tmp python3 -c "..."` returns `python3`.
297295
*/
298296
export function getCommandRoot(command: string): string | undefined {
299297
const trimmedCommand = command.trim();
300298
if (!trimmedCommand) {
301299
return undefined;
302300
}
303301

304-
// This regex is designed to find the first "word" of a command,
305-
// while respecting quotes. It looks for a sequence of non-whitespace
306-
// characters that are not inside quotes.
307-
const match = trimmedCommand.match(/^"([^"]+)"|^'([^']+)'|^(\S+)/);
308-
if (match) {
309-
// The first element in the match array is the full match.
310-
// The subsequent elements are the capture groups.
311-
// We prefer a captured group because it will be unquoted.
312-
const commandRoot = match[1] || match[2] || match[3];
313-
if (commandRoot) {
314-
// If the command is a path, return the last component.
315-
return commandRoot.split(/[\\/]/).pop();
302+
try {
303+
const tokens = parse(trimmedCommand).filter(
304+
(token): token is string => typeof token === 'string',
305+
);
306+
307+
let idx = 0;
308+
while (idx < tokens.length && ENV_ASSIGNMENT_REGEX.test(tokens[idx]!)) {
309+
idx++;
316310
}
317-
}
318311

319-
return undefined;
312+
const firstToken = tokens[idx];
313+
return firstToken ? firstToken.split(/[\\/]/).pop() : undefined;
314+
} catch {
315+
const match = trimmedCommand.match(/^"([^"]+)"|^'([^']+)'|^(\S+)/);
316+
if (match) {
317+
const commandRoot = match[1] || match[2] || match[3];
318+
if (commandRoot) {
319+
return commandRoot.split(/[\\/]/).pop();
320+
}
321+
}
322+
323+
return undefined;
324+
}
320325
}
321326

322327
export function getCommandRoots(command: string): string[] {

0 commit comments

Comments
 (0)