Skip to content

Commit 704dd68

Browse files
TyriarTyriar
authored
Merge pull request microsoft#259770 from microsoft/tyriar/259763
Add pwsh equivalents to default auto approve list and always use case insensitivity for pwsh
2 parents 5d3254c + 6a43461 commit 704dd68

File tree

3 files changed

+138
-17
lines changed

3 files changed

+138
-17
lines changed

‎src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandLineAutoApprover.ts

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { isPowerShell } from './runInTerminalHelpers.js';
1212

1313
interface IAutoApproveRule {
1414
regex: RegExp;
15+
regexCaseInsensitive: RegExp;
1516
sourceText: string;
1617
}
1718

@@ -65,14 +66,14 @@ export class CommandLineAutoApprover extends Disposable {
6566
isCommandAutoApproved(command: string, shell: string, os: OperatingSystem): { result: ICommandApprovalResult; reason: string } {
6667
// Check the deny list to see if this command requires explicit approval
6768
for (const rule of this._denyListRules) {
68-
if (this._commandMatchesRegex(rule.regex, command, shell, os)) {
69+
if (this._commandMatchesRule(rule, command, shell, os)) {
6970
return { result: 'denied', reason: `Command '${command}' is denied by deny list rule: ${rule.sourceText}` };
7071
}
7172
}
7273

7374
// Check the allow list to see if the command is allowed to run without explicit approval
7475
for (const rule of this._allowListRules) {
75-
if (this._commandMatchesRegex(rule.regex, command, shell, os)) {
76+
if (this._commandMatchesRule(rule, command, shell, os)) {
7677
return { result: 'approved', reason: `Command '${command}' is approved by allow list rule: ${rule.sourceText}` };
7778
}
7879
}
@@ -123,15 +124,17 @@ export class CommandLineAutoApprover extends Disposable {
123124
return trimmedCommand;
124125
}
125126

126-
private _commandMatchesRegex(regex: RegExp, command: string, shell: string, os: OperatingSystem): boolean {
127+
private _commandMatchesRule(rule: IAutoApproveRule, command: string, shell: string, os: OperatingSystem): boolean {
127128
const actualCommand = this._removeEnvAssignments(command, shell, os);
129+
const isPwsh = isPowerShell(shell, os);
128130

129-
if (regex.test(actualCommand)) {
131+
// PowerShell is case insensitive regardless of platform
132+
if ((isPwsh ? rule.regexCaseInsensitive : rule.regex).test(actualCommand)) {
130133
return true;
131-
} else if (isPowerShell(shell, os) && actualCommand.startsWith('(')) {
134+
} else if (isPwsh && actualCommand.startsWith('(')) {
132135
// Allow ignoring of the leading ( for PowerShell commands as it's a command pattern to
133136
// operate on the output of a command. For example `(Get-Content README.md) ...`
134-
if (regex.test(actualCommand.slice(1))) {
137+
if (rule.regexCaseInsensitive.test(actualCommand.slice(1))) {
135138
return true;
136139
}
137140
}
@@ -160,29 +163,29 @@ export class CommandLineAutoApprover extends Disposable {
160163

161164
Object.entries(config).forEach(([key, value]) => {
162165
if (typeof value === 'boolean') {
163-
const regex = this._convertAutoApproveEntryToRegex(key);
166+
const { regex, regexCaseInsensitive } = this._convertAutoApproveEntryToRegex(key);
164167
// IMPORTANT: Only true and false are used, null entries need to be ignored
165168
if (value === true) {
166-
allowListRules.push({ regex, sourceText: key });
169+
allowListRules.push({ regex, regexCaseInsensitive, sourceText: key });
167170
} else if (value === false) {
168-
denyListRules.push({ regex, sourceText: key });
171+
denyListRules.push({ regex, regexCaseInsensitive, sourceText: key });
169172
}
170173
} else if (typeof value === 'object' && value !== null) {
171174
// Handle object format like { approve: true/false, matchCommandLine: true/false }
172175
const objectValue = value as { approve?: boolean; matchCommandLine?: boolean };
173176
if (typeof objectValue.approve === 'boolean') {
174-
const regex = this._convertAutoApproveEntryToRegex(key);
177+
const { regex, regexCaseInsensitive } = this._convertAutoApproveEntryToRegex(key);
175178
if (objectValue.approve === true) {
176179
if (objectValue.matchCommandLine === true) {
177-
allowListCommandLineRules.push({ regex, sourceText: key });
180+
allowListCommandLineRules.push({ regex, regexCaseInsensitive, sourceText: key });
178181
} else {
179-
allowListRules.push({ regex, sourceText: key });
182+
allowListRules.push({ regex, regexCaseInsensitive, sourceText: key });
180183
}
181184
} else if (objectValue.approve === false) {
182185
if (objectValue.matchCommandLine === true) {
183-
denyListCommandLineRules.push({ regex, sourceText: key });
186+
denyListCommandLineRules.push({ regex, regexCaseInsensitive, sourceText: key });
184187
} else {
185-
denyListRules.push({ regex, sourceText: key });
188+
denyListRules.push({ regex, regexCaseInsensitive, sourceText: key });
186189
}
187190
}
188191
}
@@ -197,7 +200,15 @@ export class CommandLineAutoApprover extends Disposable {
197200
};
198201
}
199202

200-
private _convertAutoApproveEntryToRegex(value: string): RegExp {
203+
private _convertAutoApproveEntryToRegex(value: string): { regex: RegExp; regexCaseInsensitive: RegExp } {
204+
const regex = this._doConvertAutoApproveEntryToRegex(value);
205+
if (regex.flags.includes('i')) {
206+
return { regex, regexCaseInsensitive: regex };
207+
}
208+
return { regex, regexCaseInsensitive: new RegExp(regex.source, regex.flags + 'i') };
209+
}
210+
211+
private _doConvertAutoApproveEntryToRegex(value: string): RegExp {
201212
// If it's wrapped in `/`, it's in regex format and should be converted directly
202213
// Support all standard JavaScript regex flags: d, g, i, m, s, u, v, y
203214
const regexMatch = value.match(/^\/(?<pattern>.+)\/(?<flags>[dgimsuvy]*)$/);
@@ -213,6 +224,7 @@ export class CommandLineAutoApprover extends Disposable {
213224
// Allow .* as users expect this would match everything
214225
if (regexPattern === '.*') {
215226
return new RegExp(regexPattern);
227+
216228
}
217229

218230
try {

‎src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,17 +86,43 @@ export const terminalChatAgentToolsConfiguration: IStringDictionary<IConfigurati
8686
},
8787
]
8888
},
89+
// There are countless dangerous commands available on the command line, the defaults here
90+
// include common ones that the user is likely to want to explicitly approve first. This is
91+
// not intended to be a catch all as the user needs to opt-in to auto-approve commands, it
92+
// provides additional safety when the commands get approved by broad rules or via LLM-based
93+
// approval
8994
default: {
95+
// Deleting files
9096
rm: false,
9197
rmdir: false,
9298
del: false,
99+
'Remove-Item': false,
100+
ri: false,
101+
rd: false,
102+
erase: false,
103+
// Killing processes, dangerous thing to do generally
93104
kill: false,
105+
'Stop-Process': false,
106+
spps: false,
107+
taskkill: false,
108+
'taskkill.exe': false,
109+
// Web requests, prompt injection concerns
94110
curl: false,
95111
wget: false,
96-
eval: false,
112+
'Invoke-RestMethod': false,
113+
'Invoke-WebRequest': false,
114+
'irm': false,
115+
'iwr': false,
116+
// File permissions and ownership, messing with these can cause hard to diagnose issues
97117
chmod: false,
98118
chown: false,
99-
'/^Remove-Item\\b/i': false,
119+
'Set-ItemProperty': false,
120+
'sp': false,
121+
'Set-Acl': false,
122+
// Eval string, can lead to anything else running
123+
eval: false,
124+
'Invoke-Expression': false,
125+
iex: false,
100126
},
101127
}
102128
};

‎src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/commandLineAutoApprover.test.ts

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,89 @@ suite('CommandLineAutoApprover', () => {
408408
ok(!isAutoApproved('[Get-Content'));
409409
ok(!isAutoApproved('foo'));
410410
});
411+
412+
test('should be case-insensitive for PowerShell commands', () => {
413+
setAutoApprove({
414+
"Get-ChildItem": true,
415+
"Get-Content": true,
416+
"Remove-Item": false
417+
});
418+
419+
ok(isAutoApproved('Get-ChildItem'));
420+
ok(isAutoApproved('get-childitem'));
421+
ok(isAutoApproved('GET-CHILDITEM'));
422+
ok(isAutoApproved('Get-childitem'));
423+
ok(isAutoApproved('get-ChildItem'));
424+
425+
ok(isAutoApproved('Get-Content file.txt'));
426+
ok(isAutoApproved('get-content file.txt'));
427+
ok(isAutoApproved('GET-CONTENT file.txt'));
428+
ok(isAutoApproved('Get-content file.txt'));
429+
430+
ok(!isAutoApproved('Remove-Item file.txt'));
431+
ok(!isAutoApproved('remove-item file.txt'));
432+
ok(!isAutoApproved('REMOVE-ITEM file.txt'));
433+
ok(!isAutoApproved('Remove-item file.txt'));
434+
});
435+
436+
test('should be case-insensitive for PowerShell aliases', () => {
437+
setAutoApprove({
438+
"ls": true,
439+
"dir": true,
440+
"rm": false,
441+
"del": false
442+
});
443+
444+
// Test case-insensitive matching for aliases
445+
ok(isAutoApproved('ls'));
446+
ok(isAutoApproved('LS'));
447+
ok(isAutoApproved('Ls'));
448+
449+
ok(isAutoApproved('dir'));
450+
ok(isAutoApproved('DIR'));
451+
ok(isAutoApproved('Dir'));
452+
453+
ok(!isAutoApproved('rm file.txt'));
454+
ok(!isAutoApproved('RM file.txt'));
455+
ok(!isAutoApproved('Rm file.txt'));
456+
457+
ok(!isAutoApproved('del file.txt'));
458+
ok(!isAutoApproved('DEL file.txt'));
459+
ok(!isAutoApproved('Del file.txt'));
460+
});
461+
462+
test('should be case-insensitive with regex patterns', () => {
463+
setAutoApprove({
464+
"/^Get-/": true,
465+
"/Remove-Item|rm/": false
466+
});
467+
468+
ok(isAutoApproved('Get-ChildItem'));
469+
ok(isAutoApproved('get-childitem'));
470+
ok(isAutoApproved('GET-PROCESS'));
471+
ok(isAutoApproved('Get-Location'));
472+
473+
ok(!isAutoApproved('Remove-Item file.txt'));
474+
ok(!isAutoApproved('remove-item file.txt'));
475+
ok(!isAutoApproved('rm file.txt'));
476+
ok(!isAutoApproved('RM file.txt'));
477+
});
478+
479+
test('should handle case-insensitive PowerShell commands on different OS', () => {
480+
setAutoApprove({
481+
"Get-Process": true,
482+
"Stop-Process": false
483+
});
484+
485+
for (const currnetOS of [OperatingSystem.Windows, OperatingSystem.Linux, OperatingSystem.Macintosh]) {
486+
os = currnetOS;
487+
ok(isAutoApproved('Get-Process'), `os=${os}`);
488+
ok(isAutoApproved('get-process'), `os=${os}`);
489+
ok(isAutoApproved('GET-PROCESS'), `os=${os}`);
490+
ok(!isAutoApproved('Stop-Process'), `os=${os}`);
491+
ok(!isAutoApproved('stop-process'), `os=${os}`);
492+
}
493+
});
411494
});
412495

413496
suite('isCommandLineAutoApproved - matchCommandLine functionality', () => {

0 commit comments

Comments
 (0)