Improved logic for auto-approving chained commands

.changeset/sharp-apes-stare.md

@@ -0,0 +1,5 @@
+---
+"roo-cline": patch
+---
+
+Improved logic for auto-approving chained commands

webview-ui/.npmrc

@@ -0,0 +1 @@
+registry=https://registry.npmjs.org/

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"
 			},

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"
 	},

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) => {
+	// Check if a command message is allowed
-		if (message?.type === "ask") {
+	const isAllowedCommand = useCallback((message: ClineMessage | undefined): boolean => {
-			const command = message.text
+		if (message?.type !== "ask") return false
-			if (!command) {
+		return validateCommand(message.text || '', allowedCommands || [])
-				return true
-			}
-
-			// Split command by chaining operators
-			const commands = command.split(/&&|\|\||;|(?<!"[^"]*)\|(?![^"]*")|\$\(|`/).map(cmd => 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
 	}, [allowedCommands])
 
 	const isAutoApproved = useCallback(

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)
+		})
+	})
+})

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)
+	})
+}