diff options
| -rw-r--r-- | docs/tools/shell.md | 77 | ||||
| -rw-r--r-- | packages/core/src/tools/shell.test.ts | 167 | ||||
| -rw-r--r-- | packages/core/src/tools/shell.ts | 91 |
3 files changed, 251 insertions, 84 deletions
diff --git a/docs/tools/shell.md b/docs/tools/shell.md index cc65b013..021cede1 100644 --- a/docs/tools/shell.md +++ b/docs/tools/shell.md @@ -64,96 +64,71 @@ run_shell_command(command="npm run dev &", description="Start development server You can restrict the commands that can be executed by the `run_shell_command` tool by using the `coreTools` and `excludeTools` settings in your configuration file. -- `coreTools`: If you want to restrict the `run_shell_command` tool to a specific set of commands, you can add entries to the `coreTools` list in the format `ShellTool(<command>)`. For example, `"coreTools": ["ShellTool(ls -l)"]` will only allow the `ls -l` command to be executed. If you include `ShellTool` as a general entry in the `coreTools` list, it will act as a wildcard and allow any command to be executed, even if you have other specific commands in the list. -- `excludeTools`: If you want to block specific commands, you can add entries to the `excludeTools` list in the format `ShellTool(<command>)`. For example, `"excludeTools": ["ShellTool(rm -rf /)"]` will block the `rm -rf /` command. +- `coreTools`: To restrict `run_shell_command` to a specific set of commands, add entries to the `coreTools` list in the format `run_shell_command(<command>)`. For example, `"coreTools": ["run_shell_command(git)"]` will only allow `git` commands. Including the generic `run_shell_command` acts as a wildcard, allowing any command not explicitly blocked. +- `excludeTools`: To block specific commands, add entries to the `excludeTools` list in the format `run_shell_command(<command>)`. For example, `"excludeTools": ["run_shell_command(rm)"]` will block `rm` commands. -### Command Restriction Examples - -Here are some examples of how to use the `coreTools` and `excludeTools` settings to control which commands can be executed. - -**Allow only specific commands** - -To allow only `ls -l` and `git status`, and block all other commands: - -```json -{ - "coreTools": ["ShellTool(ls -l)", "ShellTool(git status)"] -} -``` - -- `ls -l`: Allowed -- `git status`: Allowed -- `npm install`: Blocked - -**Block specific commands** - -To block `rm -rf /` and `npm install`, and allow all other commands: +The validation logic is designed to be secure and flexible: -```json -{ - "excludeTools": ["ShellTool(rm -rf /)", "ShellTool(npm install)"] -} -``` +1. **Command Chaining Disabled**: The tool automatically splits commands chained with `&&`, `||`, or `;` and validates each part separately. If any part of the chain is disallowed, the entire command is blocked. +2. **Prefix Matching**: The tool uses prefix matching. For example, if you allow `git`, you can run `git status` or `git log`. +3. **Blocklist Precedence**: The `excludeTools` list is always checked first. If a command matches a blocked prefix, it will be denied, even if it also matches an allowed prefix in `coreTools`. -- `rm -rf /`: Blocked -- `npm install`: Blocked -- `ls -l`: Allowed +### Command Restriction Examples -**Allow all commands** +**Allow only specific command prefixes** -To allow any command to be executed, you can use the `ShellTool` wildcard in `coreTools`: +To allow only `git` and `npm` commands, and block all others: ```json { - "coreTools": ["ShellTool"] + "coreTools": ["run_shell_command(git)", "run_shell_command(npm)"] } ``` -- `ls -l`: Allowed +- `git status`: Allowed - `npm install`: Allowed -- `any other command`: Allowed +- `ls -l`: Blocked -**Wildcard with specific allowed commands** +**Block specific command prefixes** -If you include the `ShellTool` wildcard along with specific commands, the wildcard takes precedence, and all commands are allowed. +To block `rm` and allow all other commands: ```json { - "coreTools": ["ShellTool", "ShellTool(ls -l)"] + "coreTools": ["run_shell_command"], + "excludeTools": ["run_shell_command(rm)"] } ``` -- `ls -l`: Allowed +- `rm -rf /`: Blocked +- `git status`: Allowed - `npm install`: Allowed -- `any other command`: Allowed -**Wildcard with a blocklist** +**Blocklist takes precedence** -You can use the `ShellTool` wildcard to allow all commands, while still blocking specific commands using `excludeTools`. +If a command prefix is in both `coreTools` and `excludeTools`, it will be blocked. ```json { - "coreTools": ["ShellTool"], - "excludeTools": ["ShellTool(rm -rf /)"] + "coreTools": ["run_shell_command(git)"], + "excludeTools": ["run_shell_command(git push)"] } ``` -- `rm -rf /`: Blocked -- `ls -l`: Allowed -- `npm install`: Allowed +- `git push origin main`: Blocked +- `git status`: Allowed **Block all shell commands** -To block all shell commands, you can add the `ShellTool` wildcard to `excludeTools`: +To block all shell commands, add the `run_shell_command` wildcard to `excludeTools`: ```json { - "excludeTools": ["ShellTool"] + "excludeTools": ["run_shell_command"] } ``` - `ls -l`: Blocked -- `npm install`: Blocked - `any other command`: Blocked ## Security Note for `excludeTools` diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 2cbd0ff4..936c1f77 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/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); + }); }); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index a2fa5ce4..bbb998d5 100644 --- a/packages/core/src/tools/shell.ts +++ b/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; } |
