diff --git a/.changeset/sharp-apes-stare.md b/.changeset/sharp-apes-stare.md new file mode 100644 index 0000000..d9e99fe --- /dev/null +++ b/.changeset/sharp-apes-stare.md @@ -0,0 +1,5 @@ +--- +"roo-cline": patch +--- + +Improved logic for auto-approving chained commands diff --git a/webview-ui/.npmrc b/webview-ui/.npmrc new file mode 100644 index 0000000..5660f81 --- /dev/null +++ b/webview-ui/.npmrc @@ -0,0 +1 @@ +registry=https://registry.npmjs.org/ \ No newline at end of file diff --git a/webview-ui/package-lock.json b/webview-ui/package-lock.json index ade601f..cdaf3b4 100644 --- a/webview-ui/package-lock.json +++ b/webview-ui/package-lock.json @@ -28,12 +28,14 @@ "react-virtuoso": "^4.7.13", "rehype-highlight": "^7.0.0", "rewire": "^7.0.0", + "shell-quote": "^1.8.2", "styled-components": "^6.1.13", "typescript": "^4.9.5", "web-vitals": "^2.1.4" }, "devDependencies": { "@babel/plugin-proposal-private-property-in-object": "^7.21.11", + "@types/shell-quote": "^1.7.5", "@types/vscode-webview": "^1.57.5", "eslint": "^8.57.0" } @@ -3449,6 +3451,12 @@ "@types/send": "*" } }, + "node_modules/@types/shell-quote": { + "version": "1.7.5", + "resolved": "https://registry.npmjs.org/@types/shell-quote/-/shell-quote-1.7.5.tgz", + "integrity": "sha512-+UE8GAGRPbJVQDdxi16dgadcBfQ+KG2vgZhV1+3A1XmHbmwcdwhCUwIdy+d3pAGrbvgRoVSjeI9vOWyq376Yzw==", + "dev": true + }, "node_modules/@types/sockjs": { "version": "0.3.36", "license": "MIT", @@ -13349,7 +13357,8 @@ }, "node_modules/shell-quote": { "version": "1.8.2", - "license": "MIT", + "resolved": "https://registry.npmjs.org/shell-quote/-/shell-quote-1.8.2.tgz", + "integrity": "sha512-AzqKpGKjrj7EM6rKVQEPpB288oCfnrEIuyoT9cyF4nmGa7V8Zk6f7RRqYisX8X9m+Q7bd632aZW4ky7EhbQztA==", "engines": { "node": ">= 0.4" }, diff --git a/webview-ui/package.json b/webview-ui/package.json index 2ede80f..ef1013d 100644 --- a/webview-ui/package.json +++ b/webview-ui/package.json @@ -23,6 +23,7 @@ "react-virtuoso": "^4.7.13", "rehype-highlight": "^7.0.0", "rewire": "^7.0.0", + "shell-quote": "^1.8.2", "styled-components": "^6.1.13", "typescript": "^4.9.5", "web-vitals": "^2.1.4" @@ -54,6 +55,7 @@ }, "devDependencies": { "@babel/plugin-proposal-private-property-in-object": "^7.21.11", + "@types/shell-quote": "^1.7.5", "@types/vscode-webview": "^1.57.5", "eslint": "^8.57.0" }, diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index f4d26d2..f4be356 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -26,6 +26,7 @@ import ChatRow from "./ChatRow" import ChatTextArea from "./ChatTextArea" import TaskHeader from "./TaskHeader" import { AudioType } from "../../../../src/shared/WebviewMessage" +import { validateCommand } from "../../utils/command-validation" interface ChatViewProps { isHidden: boolean @@ -515,23 +516,10 @@ const ChatView = ({ isHidden, showAnnouncement, hideAnnouncement, showHistoryVie return false }, [mcpServers]) - const isAllowedCommand = useCallback((message: ClineMessage | undefined) => { - if (message?.type === "ask") { - const command = message.text - if (!command) { - return true - } - - // Split command by chaining operators - const commands = command.split(/&&|\|\||;|(? cmd.trim()) - - // Check if all individual commands are allowed - return commands.every((cmd) => { - const trimmedCommand = cmd.toLowerCase() - return allowedCommands?.some((prefix) => trimmedCommand.startsWith(prefix.toLowerCase())) - }) - } - return false + // Check if a command message is allowed + const isAllowedCommand = useCallback((message: ClineMessage | undefined): boolean => { + if (message?.type !== "ask") return false + return validateCommand(message.text || '', allowedCommands || []) }, [allowedCommands]) const isAutoApproved = useCallback( diff --git a/webview-ui/src/utils/__tests__/command-validation.test.ts b/webview-ui/src/utils/__tests__/command-validation.test.ts new file mode 100644 index 0000000..b77227c --- /dev/null +++ b/webview-ui/src/utils/__tests__/command-validation.test.ts @@ -0,0 +1,101 @@ +import { parseCommand, isAllowedSingleCommand, validateCommand } from '../command-validation' + +describe('Command Validation', () => { + describe('parseCommand', () => { + it('splits commands by chain operators', () => { + expect(parseCommand('npm test && npm run build')).toEqual(['npm test', 'npm run build']) + expect(parseCommand('npm test || npm run build')).toEqual(['npm test', 'npm run build']) + expect(parseCommand('npm test; npm run build')).toEqual(['npm test', 'npm run build']) + expect(parseCommand('npm test | npm run build')).toEqual(['npm test', 'npm run build']) + }) + + it('preserves quoted content', () => { + expect(parseCommand('npm test "param with | inside"')).toEqual(['npm test "param with | inside"']) + expect(parseCommand('echo "hello | world"')).toEqual(['echo "hello | world"']) + expect(parseCommand('npm test "param with && inside"')).toEqual(['npm test "param with && inside"']) + }) + + it('handles subshell patterns', () => { + expect(parseCommand('npm test $(echo test)')).toEqual(['npm test', 'echo test']) + expect(parseCommand('npm test `echo test`')).toEqual(['npm test', 'echo test']) + }) + + it('handles empty and whitespace input', () => { + expect(parseCommand('')).toEqual([]) + expect(parseCommand(' ')).toEqual([]) + expect(parseCommand('\t')).toEqual([]) + }) + + it('handles PowerShell specific patterns', () => { + expect(parseCommand('npm test 2>&1 | Select-String "Error"')).toEqual(['npm test 2>&1', 'Select-String "Error"']) + expect(parseCommand('npm test | Select-String -NotMatch "node_modules" | Select-String "FAIL|Error"')) + .toEqual(['npm test', 'Select-String -NotMatch "node_modules"', 'Select-String "FAIL|Error"']) + }) + }) + + describe('isAllowedSingleCommand', () => { + const allowedCommands = ['npm test', 'npm run', 'echo'] + + it('matches commands case-insensitively', () => { + expect(isAllowedSingleCommand('NPM TEST', allowedCommands)).toBe(true) + expect(isAllowedSingleCommand('npm TEST --coverage', allowedCommands)).toBe(true) + expect(isAllowedSingleCommand('ECHO hello', allowedCommands)).toBe(true) + }) + + it('matches command prefixes', () => { + expect(isAllowedSingleCommand('npm test --coverage', allowedCommands)).toBe(true) + expect(isAllowedSingleCommand('npm run build', allowedCommands)).toBe(true) + expect(isAllowedSingleCommand('echo "hello world"', allowedCommands)).toBe(true) + }) + + it('rejects non-matching commands', () => { + expect(isAllowedSingleCommand('npmtest', allowedCommands)).toBe(false) + expect(isAllowedSingleCommand('dangerous', allowedCommands)).toBe(false) + expect(isAllowedSingleCommand('rm -rf /', allowedCommands)).toBe(false) + }) + + it('handles undefined/empty allowed commands', () => { + expect(isAllowedSingleCommand('npm test', undefined as any)).toBe(false) + expect(isAllowedSingleCommand('npm test', [])).toBe(false) + }) + }) + + describe('validateCommand', () => { + const allowedCommands = ['npm test', 'npm run', 'echo', 'Select-String'] + + it('validates simple commands', () => { + expect(validateCommand('npm test', allowedCommands)).toBe(true) + expect(validateCommand('npm run build', allowedCommands)).toBe(true) + expect(validateCommand('dangerous', allowedCommands)).toBe(false) + }) + + it('validates chained commands', () => { + expect(validateCommand('npm test && npm run build', allowedCommands)).toBe(true) + expect(validateCommand('npm test && dangerous', allowedCommands)).toBe(false) + expect(validateCommand('npm test | Select-String "Error"', allowedCommands)).toBe(true) + expect(validateCommand('npm test | rm -rf /', allowedCommands)).toBe(false) + }) + + it('handles quoted content correctly', () => { + expect(validateCommand('npm test "param with | inside"', allowedCommands)).toBe(true) + expect(validateCommand('echo "hello | world"', allowedCommands)).toBe(true) + expect(validateCommand('npm test "param with && inside"', allowedCommands)).toBe(true) + }) + + it('handles subshell execution attempts', () => { + expect(validateCommand('npm test $(echo dangerous)', allowedCommands)).toBe(false) + expect(validateCommand('npm test `rm -rf /`', allowedCommands)).toBe(false) + }) + + it('handles PowerShell patterns', () => { + expect(validateCommand('npm test 2>&1 | Select-String "Error"', allowedCommands)).toBe(true) + expect(validateCommand('npm test | Select-String -NotMatch "node_modules" | Select-String "FAIL|Error"', allowedCommands)).toBe(true) + expect(validateCommand('npm test | Select-String | dangerous', allowedCommands)).toBe(false) + }) + + it('handles empty input', () => { + expect(validateCommand('', allowedCommands)).toBe(true) + expect(validateCommand(' ', allowedCommands)).toBe(true) + }) + }) +}) \ No newline at end of file diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts new file mode 100644 index 0000000..0ffea2a --- /dev/null +++ b/webview-ui/src/utils/command-validation.ts @@ -0,0 +1,126 @@ +import { parse } from 'shell-quote' + +type ShellToken = string | { op: string } | { command: string } + +/** + * Split a command string into individual sub-commands by + * chaining operators (&&, ||, ;, or |). + * + * Uses shell-quote to properly handle: + * - Quoted strings (preserves quotes) + * - Subshell commands ($(cmd) or `cmd`) + * - PowerShell redirections (2>&1) + * - Chain operators (&&, ||, ;, |) + */ +export function parseCommand(command: string): string[] { + if (!command?.trim()) return [] + + // First handle PowerShell redirections by temporarily replacing them + const redirections: string[] = [] + let processedCommand = command.replace(/\d*>&\d*/g, (match) => { + redirections.push(match) + return `__REDIR_${redirections.length - 1}__` + }) + + // Then handle subshell commands + const subshells: string[] = [] + processedCommand = processedCommand + .replace(/\$\((.*?)\)/g, (_, inner) => { + subshells.push(inner.trim()) + return `__SUBSH_${subshells.length - 1}__` + }) + .replace(/`(.*?)`/g, (_, inner) => { + subshells.push(inner.trim()) + return `__SUBSH_${subshells.length - 1}__` + }) + + // Then handle quoted strings + const quotes: string[] = [] + processedCommand = processedCommand.replace(/"[^"]*"/g, (match) => { + quotes.push(match) + return `__QUOTE_${quotes.length - 1}__` + }) + + const tokens = parse(processedCommand) as ShellToken[] + const commands: string[] = [] + let currentCommand: string[] = [] + + for (const token of tokens) { + if (typeof token === 'object' && 'op' in token) { + // Chain operator - split command + if (['&&', '||', ';', '|'].includes(token.op)) { + if (currentCommand.length > 0) { + commands.push(currentCommand.join(' ')) + currentCommand = [] + } + } else { + // Other operators (>, &) are part of the command + currentCommand.push(token.op) + } + } else if (typeof token === 'string') { + // Check if it's a subshell placeholder + const subshellMatch = token.match(/__SUBSH_(\d+)__/) + if (subshellMatch) { + if (currentCommand.length > 0) { + commands.push(currentCommand.join(' ')) + currentCommand = [] + } + commands.push(subshells[parseInt(subshellMatch[1])]) + } else { + currentCommand.push(token) + } + } + } + + // Add any remaining command + if (currentCommand.length > 0) { + commands.push(currentCommand.join(' ')) + } + + // Restore quotes and redirections + return commands.map(cmd => { + let result = cmd + // Restore quotes + result = result.replace(/__QUOTE_(\d+)__/g, (_, i) => quotes[parseInt(i)]) + // Restore redirections + result = result.replace(/__REDIR_(\d+)__/g, (_, i) => redirections[parseInt(i)]) + return result + }) +} + +/** + * Check if a single command is allowed based on prefix matching. + */ +export function isAllowedSingleCommand( + command: string, + allowedCommands: string[] +): boolean { + if (!command || !allowedCommands?.length) return false + const trimmedCommand = command.trim().toLowerCase() + return allowedCommands.some(prefix => + trimmedCommand.startsWith(prefix.toLowerCase()) + ) +} + +/** + * Check if a command string is allowed based on the allowed command prefixes. + * This version also blocks subshell attempts by checking for `$(` or `` ` ``. + */ +export function validateCommand(command: string, allowedCommands: string[]): boolean { + if (!command?.trim()) return true + + // Block subshell execution attempts + if (command.includes('$(') || command.includes('`')) { + return false + } + + // Parse into sub-commands (split by &&, ||, ;, |) + const subCommands = parseCommand(command) + + // Then ensure every sub-command starts with an allowed prefix + return subCommands.every(cmd => { + // Remove simple PowerShell-like redirections (e.g. 2>&1) before checking + const cmdWithoutRedirection = cmd.replace(/\d*>&\d*/, '').trim() + return isAllowedSingleCommand(cmdWithoutRedirection, allowedCommands) + }) +} \ No newline at end of file