From 70a99af1d3190e782580912a542246e9d1145c83 Mon Sep 17 00:00:00 2001 From: cornmander Date: Mon, 20 Oct 2025 19:18:00 -0400 Subject: [PATCH] Fix shell auto-approval parsing for chained commands (#11527) --- .../core/src/core/coreToolScheduler.test.ts | 106 ++++++++++++++++++ packages/core/src/core/coreToolScheduler.ts | 29 ++++- packages/core/src/utils/shell-utils.test.ts | 49 ++++++++ packages/core/src/utils/shell-utils.ts | 72 ++++++++++++ 4 files changed, 250 insertions(+), 6 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 16e3b2d90c..e1e6aa2430 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -19,6 +19,7 @@ import type { ToolResult, Config, ToolRegistry, + AnyToolInvocation, } from '../index.js'; import { DEFAULT_TRUNCATE_TOOL_OUTPUT_LINES, @@ -37,6 +38,7 @@ import { } from '../test-utils/mock-tool.js'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; +import { isShellInvocationAllowlisted } from '../utils/shell-utils.js'; vi.mock('fs/promises', () => ({ writeFile: vi.fn(), @@ -1192,6 +1194,110 @@ describe('CoreToolScheduler request queueing', () => { } }); + it('should require approval for a chained shell command even when prefix is allowlisted', async () => { + expect( + isShellInvocationAllowlisted( + { + params: { command: 'git status && rm -rf /tmp/should-not-run' }, + } as unknown as AnyToolInvocation, + ['run_shell_command(git)'], + ), + ).toBe(false); + + const executeFn = vi.fn().mockResolvedValue({ + llmContent: 'Shell command executed', + returnDisplay: 'Shell command executed', + }); + + const mockShellTool = new MockTool({ + name: 'run_shell_command', + shouldConfirmExecute: (params) => + Promise.resolve({ + type: 'exec', + title: 'Confirm Shell Command', + command: String(params['command'] ?? ''), + rootCommand: 'git', + onConfirm: async () => {}, + }), + execute: () => executeFn({}), + }); + + const toolRegistry = { + getTool: () => mockShellTool, + getToolByName: () => mockShellTool, + getFunctionDeclarations: () => [], + tools: new Map(), + discovery: {}, + registerTool: () => {}, + getToolByDisplayName: () => mockShellTool, + getTools: () => [], + discoverTools: async () => {}, + getAllTools: () => [], + getToolsByServer: () => [], + }; + + const onAllToolCallsComplete = vi.fn(); + const onToolCallsUpdate = vi.fn(); + + const mockConfig = { + getSessionId: () => 'test-session-id', + getUsageStatisticsEnabled: () => true, + getDebugMode: () => false, + getApprovalMode: () => ApprovalMode.DEFAULT, + getAllowedTools: () => ['run_shell_command(git)'], + getContentGeneratorConfig: () => ({ + model: 'test-model', + authType: 'oauth-personal', + }), + getShellExecutionConfig: () => ({ + terminalWidth: 80, + terminalHeight: 24, + }), + getTerminalWidth: vi.fn(() => 80), + getTerminalHeight: vi.fn(() => 24), + storage: { + getProjectTempDir: () => '/tmp', + }, + getTruncateToolOutputThreshold: () => + DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD, + getTruncateToolOutputLines: () => DEFAULT_TRUNCATE_TOOL_OUTPUT_LINES, + getToolRegistry: () => toolRegistry, + getUseSmartEdit: () => false, + getUseModelRouter: () => false, + getGeminiClient: () => null, + getEnableMessageBusIntegration: () => false, + getMessageBus: () => null, + getPolicyEngine: () => null, + } as unknown as Config; + + const scheduler = new CoreToolScheduler({ + config: mockConfig, + onAllToolCallsComplete, + onToolCallsUpdate, + getPreferredEditor: () => 'vscode', + onEditorClose: vi.fn(), + }); + + const abortController = new AbortController(); + const request = { + callId: 'shell-1', + name: 'run_shell_command', + args: { command: 'git status && rm -rf /tmp/should-not-run' }, + isClientInitiated: false, + prompt_id: 'prompt-shell-auto-approved', + }; + + await scheduler.schedule([request], abortController.signal); + + const statusUpdates = onToolCallsUpdate.mock.calls + .map((call) => (call[0][0] as ToolCall)?.status) + .filter(Boolean); + + expect(statusUpdates).toContain('awaiting_approval'); + expect(executeFn).not.toHaveBeenCalled(); + expect(onAllToolCallsComplete).not.toHaveBeenCalled(); + }); + it('should handle two synchronous calls to schedule', async () => { const executeFn = vi.fn().mockResolvedValue({ llmContent: 'Tool executed', diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index e878e4e70d..31265690dc 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -38,6 +38,10 @@ import { import * as Diff from 'diff'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; +import { + isShellInvocationAllowlisted, + SHELL_TOOL_NAMES, +} from '../utils/shell-utils.js'; import { doesToolInvocationMatch } from '../utils/tool-utils.js'; import levenshtein from 'fast-levenshtein'; import { ShellToolInvocation } from '../tools/shell.js'; @@ -728,7 +732,8 @@ export class CoreToolScheduler { continue; } - const { request: reqInfo, invocation } = toolCall; + const validatingCall = toolCall as ValidatingToolCall; + const { request: reqInfo, invocation } = validatingCall; try { if (signal.aborted) { @@ -752,11 +757,7 @@ export class CoreToolScheduler { continue; } - const allowedTools = this.config.getAllowedTools() || []; - if ( - this.config.getApprovalMode() === ApprovalMode.YOLO || - doesToolInvocationMatch(toolCall.tool, invocation, allowedTools) - ) { + if (this.isAutoApproved(validatingCall)) { this.setToolCallOutcome( reqInfo.callId, ToolConfirmationOutcome.ProceedAlways, @@ -1190,6 +1191,22 @@ export class CoreToolScheduler { }); } + private isAutoApproved(toolCall: ValidatingToolCall): boolean { + if (this.config.getApprovalMode() === ApprovalMode.YOLO) { + return true; + } + + const allowedTools = this.config.getAllowedTools() || []; + const { tool, invocation } = toolCall; + const toolName = typeof tool === 'string' ? tool : tool.name; + + if (SHELL_TOOL_NAMES.includes(toolName)) { + return isShellInvocationAllowlisted(invocation, allowedTools); + } + + return doesToolInvocationMatch(tool, invocation, allowedTools); + } + private async autoApproveCompatiblePendingTools( signal: AbortSignal, triggeringCallId: string, diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 14fa1c0c37..e2c80bc9a2 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -21,8 +21,10 @@ import { isCommandAllowed, initializeShellParsers, stripShellWrapper, + isShellInvocationAllowlisted, } from './shell-utils.js'; import type { Config } from '../config/config.js'; +import type { AnyToolInvocation } from '../index.js'; const mockPlatform = vi.hoisted(() => vi.fn()); const mockHomedir = vi.hoisted(() => vi.fn()); @@ -420,6 +422,53 @@ describe('stripShellWrapper', () => { }); }); +describe('isShellInvocationAllowlisted', () => { + function createInvocation(command: string): AnyToolInvocation { + return { params: { command } } as unknown as AnyToolInvocation; + } + + it('should return false when any chained command segment is not allowlisted', () => { + const invocation = createInvocation( + 'git status && rm -rf /tmp/should-not-run', + ); + expect( + isShellInvocationAllowlisted(invocation, ['run_shell_command(git)']), + ).toBe(false); + }); + + it('should return true when every segment is explicitly allowlisted', () => { + const invocation = createInvocation( + 'git status && rm -rf /tmp/should-run && git diff', + ); + expect( + isShellInvocationAllowlisted(invocation, [ + 'run_shell_command(git)', + 'run_shell_command(rm -rf)', + ]), + ).toBe(true); + }); + + it('should return true when the allowlist contains a wildcard shell entry', () => { + const invocation = createInvocation('git status && rm -rf /tmp/should-run'); + expect( + isShellInvocationAllowlisted(invocation, ['run_shell_command']), + ).toBe(true); + }); + + it('should treat piped commands as separate segments that must be allowlisted', () => { + const invocation = createInvocation('git status | tail -n 1'); + expect( + isShellInvocationAllowlisted(invocation, ['run_shell_command(git)']), + ).toBe(false); + expect( + isShellInvocationAllowlisted(invocation, [ + 'run_shell_command(git)', + 'run_shell_command(tail)', + ]), + ).toBe(true); + }); +}); + describe('escapeShellArg', () => { describe('POSIX (bash)', () => { it('should use shell-quote for escaping', () => { diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 20611bf413..1130deaa47 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -738,3 +738,75 @@ export function isCommandAllowed( } return { allowed: false, reason: blockReason }; } + +/** + * Determines whether a shell invocation should be auto-approved based on an allowlist. + * + * This reuses the same parsing logic as command-permission enforcement so that + * chained commands must be individually covered by the allowlist. + * + * @param invocation The shell tool invocation being evaluated. + * @param allowedPatterns The configured allowlist patterns (e.g. `run_shell_command(git)`). + * @returns True if every parsed command segment is allowed by the patterns; false otherwise. + */ +export function isShellInvocationAllowlisted( + invocation: AnyToolInvocation, + allowedPatterns: string[], +): boolean { + if (!allowedPatterns.length) { + return false; + } + + const hasShellWildcard = allowedPatterns.some((pattern) => + SHELL_TOOL_NAMES.includes(pattern), + ); + const hasShellSpecificPattern = allowedPatterns.some((pattern) => + SHELL_TOOL_NAMES.some((name) => pattern.startsWith(`${name}(`)), + ); + + if (!hasShellWildcard && !hasShellSpecificPattern) { + return false; + } + + if (hasShellWildcard) { + return true; + } + + if ( + !('params' in invocation) || + typeof invocation.params !== 'object' || + invocation.params === null || + !('command' in invocation.params) + ) { + return false; + } + + const commandValue = (invocation.params as { command?: unknown }).command; + if (typeof commandValue !== 'string' || !commandValue.trim()) { + return false; + } + + const command = commandValue.trim(); + + const parseResult = parseCommandDetails(command); + if (!parseResult || parseResult.hasError) { + return false; + } + + const normalize = (cmd: string): string => cmd.trim().replace(/\s+/g, ' '); + const commandsToValidate = parseResult.details + .map((detail) => normalize(detail.text)) + .filter(Boolean); + + if (commandsToValidate.length === 0) { + return false; + } + + return commandsToValidate.every((commandSegment) => + doesToolInvocationMatch( + SHELL_TOOL_NAMES[0], + { params: { command: commandSegment } } as AnyToolInvocation, + allowedPatterns, + ), + ); +}