feat(shell): Enable prefix matching for flexible command validation (#2653)

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

@@ -168,4 +168,171 @@ describe('ShellTool', () => {
     const isAllowed = shellTool.isCommandAllowed('rm -rf /');
     expect(isAllowed).toBe(false);
   });
+
+  it('should allow a command that starts with an allowed command prefix', async () => {
+    const config = {
+      getCoreTools: () => ['ShellTool(gh issue edit)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed(
+      'gh issue edit 1 --add-label "kind/feature"',
+    );
+    expect(isAllowed).toBe(true);
+  });
+
+  it('should allow a command that starts with an allowed command prefix using the public-facing name', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(gh issue edit)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed(
+      'gh issue edit 1 --add-label "kind/feature"',
+    );
+    expect(isAllowed).toBe(true);
+  });
+
+  it('should not allow a command that starts with an allowed command prefix but is chained with another command', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(gh issue edit)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('gh issue edit&&rm -rf /');
+    expect(isAllowed).toBe(false);
+  });
+
+  it('should not allow a command that is a prefix of an allowed command', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(gh issue edit)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('gh issue');
+    expect(isAllowed).toBe(false);
+  });
+
+  it('should not allow a command that is a prefix of a blocked command', async () => {
+    const config = {
+      getCoreTools: () => [],
+      getExcludeTools: () => ['run_shell_command(gh issue edit)'],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('gh issue');
+    expect(isAllowed).toBe(true);
+  });
+
+  it('should not allow a command that is chained with a pipe', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(gh issue list)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('gh issue list | rm -rf /');
+    expect(isAllowed).toBe(false);
+  });
+
+  it('should not allow a command that is chained with a semicolon', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(gh issue list)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('gh issue list; rm -rf /');
+    expect(isAllowed).toBe(false);
+  });
+
+  it('should block a chained command if any part is blocked', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(echo "hello")'],
+      getExcludeTools: () => ['run_shell_command(rm)'],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('echo "hello" && rm -rf /');
+    expect(isAllowed).toBe(false);
+  });
+
+  it('should block a command if its prefix is on the blocklist, even if the command itself is on the allowlist', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(git push)'],
+      getExcludeTools: () => ['run_shell_command(git)'],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('git push');
+    expect(isAllowed).toBe(false);
+  });
+
+  it('should be case-sensitive in its matching', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(echo)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('ECHO "hello"');
+    expect(isAllowed).toBe(false);
+  });
+
+  it('should correctly handle commands with extra whitespace around chaining operators', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(ls -l)'],
+      getExcludeTools: () => ['run_shell_command(rm)'],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('ls -l  ;  rm -rf /');
+    expect(isAllowed).toBe(false);
+  });
+
+  it('should allow a chained command if all parts are allowed', async () => {
+    const config = {
+      getCoreTools: () => [
+        'run_shell_command(echo)',
+        'run_shell_command(ls -l)',
+      ],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('echo "hello" && ls -l');
+    expect(isAllowed).toBe(true);
+  });
+
+  it('should block a command with command substitution using backticks', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(echo)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('echo `rm -rf /`');
+    expect(isAllowed).toBe(false);
+  });
+
+  it('should block a command with command substitution using $()', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(echo)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('echo $(rm -rf /)');
+    expect(isAllowed).toBe(false);
+  });
+
+  it('should allow a command with I/O redirection', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(echo)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('echo "hello" > file.txt');
+    expect(isAllowed).toBe(true);
+  });
+
+  it('should not allow a command that is chained with a double pipe', async () => {
+    const config = {
+      getCoreTools: () => ['run_shell_command(gh issue list)'],
+      getExcludeTools: () => [],
+    } as unknown as Config;
+    const shellTool = new ShellTool(config);
+    const isAllowed = shellTool.isCommandAllowed('gh issue list || rm -rf /');
+    expect(isAllowed).toBe(false);
+  });
 });

packages/core/src/tools/shell.ts

@@ -99,17 +99,39 @@ Process Group PGID: Process group started or \`(none)\``,
   }
 
   isCommandAllowed(command: string): boolean {
-    const normalize = (cmd: string) => cmd.trim().replace(/\s+/g, ' ');
+    // 0. Disallow command substitution
+    if (command.includes('$(') || command.includes('`')) {
+      return false;
+    }
 
+    const SHELL_TOOL_NAMES = [ShellTool.name, ShellTool.Name];
+
+    const normalize = (cmd: string): string => cmd.trim().replace(/\s+/g, ' ');
+
+    /**
+     * Checks if a command string starts with a given prefix, ensuring it's a
+     * whole word match (i.e., followed by a space or it's an exact match).
+     * e.g., `isPrefixedBy('npm install', 'npm')` -> true
+     * e.g., `isPrefixedBy('npm', 'npm')` -> true
+     * e.g., `isPrefixedBy('npminstall', 'npm')` -> false
+     */
+    const isPrefixedBy = (cmd: string, prefix: string): boolean => {
+      if (!cmd.startsWith(prefix)) {
+        return false;
+      }
+      return cmd.length === prefix.length || cmd[prefix.length] === ' ';
+    };
+
+    /**
+     * Extracts and normalizes shell commands from a list of tool strings.
+     * e.g., 'ShellTool("ls -l")' becomes 'ls -l'
+     */
     const extractCommands = (tools: string[]): string[] =>
       tools.flatMap((tool) => {
-        if (tool.startsWith(`${ShellTool.name}(`) && tool.endsWith(')')) {
-          return [normalize(tool.slice(ShellTool.name.length + 1, -1))];
-        } else if (
-          tool.startsWith(`${ShellTool.Name}(`) &&
-          tool.endsWith(')')
-        ) {
-          return [normalize(tool.slice(ShellTool.Name.length + 1, -1))];
+        for (const toolName of SHELL_TOOL_NAMES) {
+          if (tool.startsWith(`${toolName}(`) && tool.endsWith(')')) {
+            return [normalize(tool.slice(toolName.length + 1, -1))];
+          }
         }
         return [];
       });
@@ -117,41 +139,44 @@ Process Group PGID: Process group started or \`(none)\``,
     const coreTools = this.config.getCoreTools() || [];
     const excludeTools = this.config.getExcludeTools() || [];
 
-    if (
-      excludeTools.includes(ShellTool.name) ||
-      excludeTools.includes(ShellTool.Name)
-    ) {
+    // 1. Check if the shell tool is globally disabled.
+    if (SHELL_TOOL_NAMES.some((name) => excludeTools.includes(name))) {
       return false;
     }
 
-    const blockedCommands = extractCommands(excludeTools);
-    const normalizedCommand = normalize(command);
+    const blockedCommands = new Set(extractCommands(excludeTools));
+    const allowedCommands = new Set(extractCommands(coreTools));
 
-    if (blockedCommands.includes(normalizedCommand)) {
-      return false;
-    }
-
-    const hasSpecificCommands = coreTools.some(
-      (tool) =>
-        (tool.startsWith(`${ShellTool.name}(`) && tool.endsWith(')')) ||
-        (tool.startsWith(`${ShellTool.Name}(`) && tool.endsWith(')')),
+    const hasSpecificAllowedCommands = allowedCommands.size > 0;
+    const isWildcardAllowed = SHELL_TOOL_NAMES.some((name) =>
+      coreTools.includes(name),
     );
 
-    if (hasSpecificCommands) {
-      // If the generic `ShellTool` is also present, it acts as a wildcard,
-      // allowing all commands (that are not explicitly blocked).
-      if (
-        coreTools.includes(ShellTool.name) ||
-        coreTools.includes(ShellTool.Name)
-      ) {
-        return true;
+    const commandsToValidate = command.split(/&&|\|\||\||;/).map(normalize);
+
+    for (const cmd of commandsToValidate) {
+      // 2. Check if the command is on the blocklist.
+      const isBlocked = [...blockedCommands].some((blocked) =>
+        isPrefixedBy(cmd, blocked),
+      );
+      if (isBlocked) {
+        return false;
       }
 
-      // Otherwise, we are in strict allow-list mode.
-      const allowedCommands = extractCommands(coreTools);
-      return allowedCommands.includes(normalizedCommand);
+      // 3. If in strict allow-list mode, check if the command is permitted.
+      const isStrictAllowlist =
+        hasSpecificAllowedCommands && !isWildcardAllowed;
+      if (isStrictAllowlist) {
+        const isAllowed = [...allowedCommands].some((allowed) =>
+          isPrefixedBy(cmd, allowed),
+        );
+        if (!isAllowed) {
+          return false;
+        }
+      }
     }
 
+    // 4. If all checks pass, the command is allowed.
     return true;
   }