Skip to content
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<ToolConfirmationMessage
confirmationDetails={confirmationDetails}
config={mockConfig}
availableTerminalHeight={30}
contentWidth={80}
compactMode={true}
/>,
);

expect(lastFrame()).toContain('Contains command_substitution');
});

it('renders MCP server and tool name for mcp confirmations', () => {
const confirmationDetails: ToolCallConfirmationDetails = {
type: 'mcp',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,11 @@ export const ToolConfirmationMessage: React.FC<
</Box>
</MaxSizedBox>
</Box>
{executionProps.securityWarnings?.map((warning) => (
<Box key={warning} paddingX={1} marginLeft={1} marginTop={1}>
<Text color={theme.status.warning}>{warning}</Text>
</Box>
))}
</Box>
);
} else if (confirmationDetails.type === 'plan') {
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/confirmation-bus/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export type SerializableConfirmationDetails =
rootCommand: string;
rootCommands: string[];
commands?: string[];
securityWarnings?: string[];
}
| {
type: 'mcp';
Expand Down
16 changes: 16 additions & 0 deletions packages/core/src/permissions/permission-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
8 changes: 4 additions & 4 deletions packages/core/src/permissions/permission-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
*
Expand Down Expand Up @@ -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
Expand Down
16 changes: 16 additions & 0 deletions packages/core/src/tools/shell.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
6 changes: 6 additions & 0 deletions packages/core/src/tools/shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import {
getCommandRoot,
getCommandRoots,
getShellConfiguration,
detectCommandSubstitution,
type ShellConfiguration,
type ShellType,
splitCommands,
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 3 additions & 1 deletion packages/core/src/tools/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down
Loading