diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 936c1f77..f33d3f03 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -15,8 +15,8 @@ describe('ShellTool', () => { getExcludeTools: () => undefined, } as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('ls -l'); - expect(isAllowed).toBe(true); + const result = shellTool.isCommandAllowed('ls -l'); + expect(result.allowed).toBe(true); }); it('should allow a command if it is in the allowed list', async () => { @@ -25,8 +25,8 @@ describe('ShellTool', () => { getExcludeTools: () => undefined, } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('ls -l'); - expect(isAllowed).toBe(true); + const result = shellTool.isCommandAllowed('ls -l'); + expect(result.allowed).toBe(true); }); it('should block a command if it is not in the allowed list', async () => { @@ -35,8 +35,11 @@ describe('ShellTool', () => { getExcludeTools: () => undefined, } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('rm -rf /'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is not in the allowed commands list", + ); }); it('should block a command if it is in the blocked list', async () => { @@ -45,8 +48,11 @@ describe('ShellTool', () => { getExcludeTools: () => ['ShellTool(rm -rf /)'], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('rm -rf /'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is blocked by configuration", + ); }); it('should allow a command if it is not in the blocked list', async () => { @@ -55,8 +61,8 @@ describe('ShellTool', () => { getExcludeTools: () => ['ShellTool(rm -rf /)'], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('ls -l'); - expect(isAllowed).toBe(true); + const result = shellTool.isCommandAllowed('ls -l'); + expect(result.allowed).toBe(true); }); it('should block a command if it is in both the allowed and blocked lists', async () => { @@ -65,8 +71,11 @@ describe('ShellTool', () => { getExcludeTools: () => ['ShellTool(rm -rf /)'], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('rm -rf /'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is blocked by configuration", + ); }); it('should allow any command when ShellTool is in coreTools without specific commands', async () => { @@ -75,8 +84,8 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('any command'); - expect(isAllowed).toBe(true); + const result = shellTool.isCommandAllowed('any command'); + expect(result.allowed).toBe(true); }); it('should block any command when ShellTool is in excludeTools without specific commands', async () => { @@ -85,8 +94,11 @@ describe('ShellTool', () => { getExcludeTools: () => ['ShellTool'], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('any command'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('any command'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + 'Shell tool is globally disabled in configuration', + ); }); it('should allow a command if it is in the allowed list using the public-facing name', async () => { @@ -95,8 +107,8 @@ describe('ShellTool', () => { getExcludeTools: () => undefined, } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('ls -l'); - expect(isAllowed).toBe(true); + const result = shellTool.isCommandAllowed('ls -l'); + expect(result.allowed).toBe(true); }); it('should block a command if it is in the blocked list using the public-facing name', async () => { @@ -105,8 +117,11 @@ describe('ShellTool', () => { getExcludeTools: () => ['run_shell_command(rm -rf /)'], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('rm -rf /'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is blocked by configuration", + ); }); it('should block any command when ShellTool is in excludeTools using the public-facing name', async () => { @@ -115,8 +130,11 @@ describe('ShellTool', () => { getExcludeTools: () => ['run_shell_command'], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('any command'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('any command'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + 'Shell tool is globally disabled in configuration', + ); }); it('should block any command if coreTools contains an empty ShellTool command list using the public-facing name', async () => { @@ -125,8 +143,11 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('any command'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('any command'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'any command' is not in the allowed commands list", + ); }); it('should block any command if coreTools contains an empty ShellTool command list', async () => { @@ -135,8 +156,11 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('any command'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('any command'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'any command' is not in the allowed commands list", + ); }); it('should block a command with extra whitespace if it is in the blocked list', async () => { @@ -145,8 +169,11 @@ describe('ShellTool', () => { getExcludeTools: () => ['ShellTool(rm -rf /)'], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed(' rm -rf / '); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed(' rm -rf / '); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is blocked by configuration", + ); }); it('should allow any command when ShellTool is present with specific commands', async () => { @@ -155,8 +182,8 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('any command'); - expect(isAllowed).toBe(true); + const result = shellTool.isCommandAllowed('any command'); + expect(result.allowed).toBe(true); }); it('should block a command on the blocklist even with a wildcard allow', async () => { @@ -165,8 +192,11 @@ describe('ShellTool', () => { getExcludeTools: () => ['ShellTool(rm -rf /)'], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('rm -rf /'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is blocked by configuration", + ); }); it('should allow a command that starts with an allowed command prefix', async () => { @@ -175,10 +205,10 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed( + const result = shellTool.isCommandAllowed( 'gh issue edit 1 --add-label "kind/feature"', ); - expect(isAllowed).toBe(true); + expect(result.allowed).toBe(true); }); it('should allow a command that starts with an allowed command prefix using the public-facing name', async () => { @@ -187,10 +217,10 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed( + const result = shellTool.isCommandAllowed( 'gh issue edit 1 --add-label "kind/feature"', ); - expect(isAllowed).toBe(true); + expect(result.allowed).toBe(true); }); it('should not allow a command that starts with an allowed command prefix but is chained with another command', async () => { @@ -199,8 +229,11 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('gh issue edit&&rm -rf /'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('gh issue edit&&rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is not in the allowed commands list", + ); }); it('should not allow a command that is a prefix of an allowed command', async () => { @@ -209,8 +242,11 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('gh issue'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('gh issue'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'gh issue' is not in the allowed commands list", + ); }); it('should not allow a command that is a prefix of a blocked command', async () => { @@ -219,8 +255,8 @@ describe('ShellTool', () => { 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); + const result = shellTool.isCommandAllowed('gh issue'); + expect(result.allowed).toBe(true); }); it('should not allow a command that is chained with a pipe', async () => { @@ -229,8 +265,11 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('gh issue list | rm -rf /'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('gh issue list | rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is not in the allowed commands list", + ); }); it('should not allow a command that is chained with a semicolon', async () => { @@ -239,8 +278,11 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('gh issue list; rm -rf /'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('gh issue list; rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is not in the allowed commands list", + ); }); it('should block a chained command if any part is blocked', async () => { @@ -249,8 +291,11 @@ describe('ShellTool', () => { 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); + const result = shellTool.isCommandAllowed('echo "hello" && rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is blocked by configuration", + ); }); it('should block a command if its prefix is on the blocklist, even if the command itself is on the allowlist', async () => { @@ -259,8 +304,11 @@ describe('ShellTool', () => { getExcludeTools: () => ['run_shell_command(git)'], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('git push'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('git push'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'git push' is blocked by configuration", + ); }); it('should be case-sensitive in its matching', async () => { @@ -269,8 +317,11 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('ECHO "hello"'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('ECHO "hello"'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + 'Command \'ECHO "hello"\' is not in the allowed commands list', + ); }); it('should correctly handle commands with extra whitespace around chaining operators', async () => { @@ -279,8 +330,11 @@ describe('ShellTool', () => { 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); + const result = shellTool.isCommandAllowed('ls -l ; rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is blocked by configuration", + ); }); it('should allow a chained command if all parts are allowed', async () => { @@ -292,8 +346,8 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('echo "hello" && ls -l'); - expect(isAllowed).toBe(true); + const result = shellTool.isCommandAllowed('echo "hello" && ls -l'); + expect(result.allowed).toBe(true); }); it('should block a command with command substitution using backticks', async () => { @@ -302,8 +356,11 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('echo `rm -rf /`'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('echo `rm -rf /`'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + 'Command substitution using backticks is not allowed for security reasons', + ); }); it('should block a command with command substitution using $()', async () => { @@ -312,8 +369,11 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('echo $(rm -rf /)'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('echo $(rm -rf /)'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + 'Command substitution using $() is not allowed for security reasons', + ); }); it('should allow a command with I/O redirection', async () => { @@ -322,8 +382,8 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('echo "hello" > file.txt'); - expect(isAllowed).toBe(true); + const result = shellTool.isCommandAllowed('echo "hello" > file.txt'); + expect(result.allowed).toBe(true); }); it('should not allow a command that is chained with a double pipe', async () => { @@ -332,7 +392,10 @@ describe('ShellTool', () => { getExcludeTools: () => [], } as unknown as Config; const shellTool = new ShellTool(config); - const isAllowed = shellTool.isCommandAllowed('gh issue list || rm -rf /'); - expect(isAllowed).toBe(false); + const result = shellTool.isCommandAllowed('gh issue list || rm -rf /'); + expect(result.allowed).toBe(false); + expect(result.reason).toBe( + "Command 'rm -rf /' is not in the allowed commands list", + ); }); }); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index f0a30a9c..4954e055 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -112,12 +112,23 @@ Process Group PGID: Process group started or \`(none)\``, * the tool's configuration including allowlists and blocklists. * * @param command The shell command string to validate - * @returns True if the command is allowed to execute, false otherwise + * @returns An object with 'allowed' boolean and optional 'reason' string if not allowed */ - isCommandAllowed(command: string): boolean { + isCommandAllowed(command: string): { allowed: boolean; reason?: string } { // 0. Disallow command substitution - if (command.includes('$(') || command.includes('`')) { - return false; + if (command.includes('$(')) { + return { + allowed: false, + reason: + 'Command substitution using $() is not allowed for security reasons', + }; + } + if (command.includes('`')) { + return { + allowed: false, + reason: + 'Command substitution using backticks is not allowed for security reasons', + }; } const SHELL_TOOL_NAMES = [ShellTool.name, ShellTool.Name]; @@ -157,7 +168,10 @@ Process Group PGID: Process group started or \`(none)\``, // 1. Check if the shell tool is globally disabled. if (SHELL_TOOL_NAMES.some((name) => excludeTools.includes(name))) { - return false; + return { + allowed: false, + reason: 'Shell tool is globally disabled in configuration', + }; } const blockedCommands = new Set(extractCommands(excludeTools)); @@ -170,35 +184,51 @@ Process Group PGID: Process group started or \`(none)\``, const commandsToValidate = command.split(/&&|\|\||\||;/).map(normalize); + const blockedCommandsArr = [...blockedCommands]; + for (const cmd of commandsToValidate) { // 2. Check if the command is on the blocklist. - const isBlocked = [...blockedCommands].some((blocked) => + const isBlocked = blockedCommandsArr.some((blocked) => isPrefixedBy(cmd, blocked), ); if (isBlocked) { - return false; + return { + allowed: false, + reason: `Command '${cmd}' is blocked by configuration`, + }; } // 3. If in strict allow-list mode, check if the command is permitted. const isStrictAllowlist = hasSpecificAllowedCommands && !isWildcardAllowed; + const allowedCommandsArr = [...allowedCommands]; if (isStrictAllowlist) { - const isAllowed = [...allowedCommands].some((allowed) => + const isAllowed = allowedCommandsArr.some((allowed) => isPrefixedBy(cmd, allowed), ); if (!isAllowed) { - return false; + return { + allowed: false, + reason: `Command '${cmd}' is not in the allowed commands list`, + }; } } } // 4. If all checks pass, the command is allowed. - return true; + return { allowed: true }; } validateToolParams(params: ShellToolParams): string | null { - if (!this.isCommandAllowed(params.command)) { - return `Command is not allowed: ${params.command}`; + const commandCheck = this.isCommandAllowed(params.command); + if (!commandCheck.allowed) { + if (!commandCheck.reason) { + console.error( + 'Unexpected: isCommandAllowed returned false without a reason', + ); + return `Command is not allowed: ${params.command}`; + } + return commandCheck.reason; } if ( !SchemaValidator.validate(