diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx index bb18d421ccf..4171dda0abe 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx @@ -282,6 +282,29 @@ describe('ToolConfirmationMessage', () => { expect(frame).not.toContain('Always allow for this user'); }); + it('renders exec security warnings', () => { + const confirmationDetails: ToolCallConfirmationDetails = { + type: 'exec', + title: 'Confirm Execution', + command: 'python3 -c "print($(echo hello))"', + rootCommand: 'python3', + securityWarnings: ['Contains command_substitution'], + onConfirm: vi.fn(), + }; + + const { lastFrame } = renderWithProviders( + , + ); + + expect(lastFrame()).toContain('Contains command_substitution'); + }); + it('renders MCP server and tool name for mcp confirmations', () => { const confirmationDetails: ToolCallConfirmationDetails = { type: 'mcp', diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index 07b66b2dd25..dfff8fb2549 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -273,6 +273,11 @@ export const ToolConfirmationMessage: React.FC< + {executionProps.securityWarnings?.map((warning) => ( + + {warning} + + ))} ); } else if (confirmationDetails.type === 'plan') { diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index e44fb8d940f..fe0bb0d51ce 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -78,6 +78,7 @@ export type SerializableConfirmationDetails = rootCommand: string; rootCommands: string[]; commands?: string[]; + securityWarnings?: string[]; } | { type: 'mcp'; diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 32d529d5a24..d399035525c 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -1256,6 +1256,22 @@ describe('PermissionManager', () => { ).toBe('ask'); }); + it('one sub-command has command substitution → ask with user confirmation', async () => { + pm = new PermissionManager( + makeConfig({ + permissionsAllow: ['Bash(echo *)'], + }), + ); + pm.initialize(); + + expect( + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'echo hello && python3 -c "print($(echo hello))"', + }), + ).toBe('ask'); + }); + it('one sub-command denied → deny', async () => { pm = new PermissionManager( makeConfig({ diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index 20bd0f4d278..be5c464b182 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -348,7 +348,7 @@ export class PermissionManager { * * When a sub-command returns 'default' (no rule matches), it is resolved to * the actual default permission using AST analysis: - * - Command substitution detected → 'deny' + * - Command substitution detected → 'ask' * - Read-only command (cd, ls, git status, etc.) → 'allow' * - Otherwise → 'ask' * @@ -403,14 +403,14 @@ export class PermissionManager { * This mirrors the logic in ShellToolInvocation.getDefaultPermission(). * * @param command - The shell command to analyze. - * @returns 'deny' for command substitution, 'allow' for read-only, 'ask' otherwise. + * @returns 'ask' for command substitution, 'allow' for read-only, 'ask' otherwise. */ private async resolveDefaultPermission( command: string, ): Promise<'allow' | 'ask' | 'deny'> { - // Security: command substitution ($(), ``, <(), >()) → deny + // Security: command substitution ($(), ``, <(), >()) needs explicit user confirmation. if (detectCommandSubstitution(command)) { - return 'deny'; + return 'ask'; } // AST-based read-only detection diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 706b16fdb10..4ac0b099c36 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -4873,6 +4873,22 @@ describe('ShellTool', () => { expect(details.type).toBe('exec'); }); + it('should warn when a shell confirmation contains command substitution', async () => { + const invocation = shellTool.build({ + command: 'python3 -c "print($(echo hello))"', + is_background: false, + }); + + const details = await invocation.getConfirmationDetails( + new AbortController().signal, + ); + + expect(details).toMatchObject({ + type: 'exec', + securityWarnings: ['Contains command_substitution'], + }); + }); + it('should exclude read-only sub-commands from confirmation details in compound commands', async () => { // "cd" is read-only, "npm run build" is not const params = { diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 095c2a4b41e..3150f2233c1 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -47,6 +47,7 @@ import { getCommandRoot, getCommandRoots, getShellConfiguration, + detectCommandSubstitution, type ShellConfiguration, type ShellType, splitCommands, @@ -1464,12 +1465,17 @@ export class ShellToolInvocation extends BaseToolInvocation< debugLogger.warn('Failed to extract command rules:', e); } + const securityWarnings = detectCommandSubstitution(command) + ? ['Contains command_substitution'] + : undefined; + const confirmationDetails: ToolExecuteConfirmationDetails = { type: 'exec', title: 'Confirm Shell Command', command: this.params.command, rootCommand: rootCommands.join(', '), permissionRules, + securityWarnings, onConfirm: async ( _outcome: ToolConfirmationOutcome, _payload?: ToolConfirmationPayload, diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 827eb2a86b5..6991ea2d40d 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -44,7 +44,7 @@ export interface ToolInvocation< * * - `'allow'` — inherently safe (e.g., read-only commands, `cat`, `ls`). * - `'ask'` — may have side effects, needs user or PM confirmation. - * - `'deny'` — security violation (e.g., command substitution in shell). + * - `'deny'` — explicit security or policy violation. * * The coreToolScheduler uses this as the *default* permission which may be * overridden by PermissionManager rules at L4. @@ -692,6 +692,8 @@ export interface ToolExecuteConfirmationDetails { rootCommand: string; /** Permission rules extracted by extractCommandRules(), used for display and persistence. */ permissionRules?: string[]; + /** Warnings surfaced to the confirmation UI before the user approves execution. */ + securityWarnings?: string[]; } export interface ToolMcpConfirmationDetails {