fix: align shell allowlist handling (#11510) (#11813)

integration-tests/run_shell_command.test.ts

@@ -61,6 +61,28 @@ function getDisallowedFileReadCommand(testFile: string): {
   }
 }
 
+function getChainedEchoCommand(): { allowPattern: string; command: string } {
+  const secondCommand = getAllowedListCommand();
+  switch (shell) {
+    case 'powershell':
+      return {
+        allowPattern: 'Write-Output',
+        command: `Write-Output "foo" && ${secondCommand}`,
+      };
+    case 'cmd':
+      return {
+        allowPattern: 'echo',
+        command: `echo "foo" && ${secondCommand}`,
+      };
+    case 'bash':
+    default:
+      return {
+        allowPattern: 'echo',
+        command: `echo "foo" && ${secondCommand}`,
+      };
+  }
+}
+
 describe('run_shell_command', () => {
   it('should be able to run a shell command', async () => {
     const rig = new TestRig();
@@ -405,6 +427,35 @@ describe('run_shell_command', () => {
     expect(failureLog!.toolRequest.success).toBe(false);
   });
 
+  it('should reject chained commands when only the first segment is allowlisted in non-interactive mode', async () => {
+    const rig = new TestRig();
+    await rig.setup(
+      'should reject chained commands when only the first segment is allowlisted',
+    );
+
+    const chained = getChainedEchoCommand();
+    const shellInjection = `!{${chained.command}}`;
+
+    await rig.run(
+      {
+        stdin: `${shellInjection}\n`,
+        yolo: false,
+      },
+      `--allowed-tools=ShellTool(${chained.allowPattern})`,
+    );
+
+    // CLI should refuse to execute the chained command without scheduling run_shell_command.
+    const toolLogs = rig
+      .readToolLogs()
+      .filter((log) => log.toolRequest.name === 'run_shell_command');
+
+    // Success is false because tool is in the scheduled state.
+    for (const log of toolLogs) {
+      expect(log.toolRequest.success).toBe(false);
+      expect(log.toolRequest.args).toContain('&&');
+    }
+  });
+
   it('should allow all with "ShellTool" and other specific tools', async () => {
     const rig = new TestRig();
     await rig.setup(

packages/core/src/tools/shell.test.ts

@@ -480,6 +480,18 @@ describe('ShellTool', () => {
           invocation.shouldConfirmExecute(new AbortController().signal),
         ).rejects.toThrow('wc');
       });
+
+      it('should require all segments of a chained command to be allowlisted', async () => {
+        (mockConfig.getAllowedTools as Mock).mockReturnValue([
+          'ShellTool(echo)',
+        ]);
+        const invocation = shellTool.build({ command: 'echo "foo" && ls -l' });
+        await expect(
+          invocation.shouldConfirmExecute(new AbortController().signal),
+        ).rejects.toThrow(
+          'Command "echo "foo" && ls -l" is not in the list of allowed tools for non-interactive mode.',
+        );
+      });
     });
   });
 

packages/core/src/tools/shell.ts

@@ -9,6 +9,7 @@ import path from 'node:path';
 import os, { EOL } from 'node:os';
 import crypto from 'node:crypto';
 import type { Config } from '../config/config.js';
+import type { AnyToolInvocation } from '../index.js';
 import { ToolErrorType } from './tool-error.js';
 import type {
   ToolInvocation,
@@ -36,10 +37,9 @@ import {
   getCommandRoots,
   initializeShellParsers,
   isCommandAllowed,
-  SHELL_TOOL_NAMES,
+  isShellInvocationAllowlisted,
   stripShellWrapper,
 } from '../utils/shell-utils.js';
-import { doesToolInvocationMatch } from '../utils/tool-utils.js';
 import { SHELL_TOOL_NAME } from './tool-names.js';
 
 export const OUTPUT_UPDATE_INTERVAL_MS = 1000;
@@ -90,9 +90,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
       !this.config.isInteractive() &&
       this.config.getApprovalMode() !== ApprovalMode.YOLO
     ) {
-      const allowedTools = this.config.getAllowedTools() || [];
-      const [SHELL_TOOL_NAME] = SHELL_TOOL_NAMES;
-      if (doesToolInvocationMatch(SHELL_TOOL_NAME, command, allowedTools)) {
+      if (this.isInvocationAllowlisted(command)) {
         // If it's an allowed shell command, we don't need to confirm execution.
         return false;
       }
@@ -324,6 +322,16 @@ export class ShellToolInvocation extends BaseToolInvocation<
       }
     }
   }
+
+  private isInvocationAllowlisted(command: string): boolean {
+    const allowedTools = this.config.getAllowedTools() || [];
+    if (allowedTools.length === 0) {
+      return false;
+    }
+
+    const invocation = { params: { command } } as unknown as AnyToolInvocation;
+    return isShellInvocationAllowlisted(invocation, allowedTools);
+  }
 }
 
 function getShellToolDescription(): string {