fix: improve command pattern extraction security

- Fix npm/yarn/pnpm commands with additional arguments to avoid overly broad wildcards
- Improve handling of chained commands with proper quote-aware parsing
- Add security limits for deeply nested command chains
- Enhance detection of command injection patterns
- Update tests to reflect more secure pattern extraction behavior

This addresses the critical security issue where commands like 'npm run build:prod -- --env=production' were creating patterns that were too permissive.

Author: hannesrudolph

webview-ui/src/utils/__tests__/extract-command-pattern.spec.ts

@@ -11,11 +11,17 @@ describe("extractCommandPattern", () => {
 
 	describe("npm/yarn/pnpm/bun commands", () => {
 		it("extracts npm run patterns", () => {
-			expect(extractCommandPattern("npm run build")).toBe("npm run")
+			expect(extractCommandPattern("npm run build")).toBe("npm run build")
 			expect(extractCommandPattern("npm run test:unit")).toBe("npm run *")
-			expect(extractCommandPattern("yarn run dev")).toBe("yarn run")
-			expect(extractCommandPattern("pnpm run lint")).toBe("pnpm run")
-			expect(extractCommandPattern("bun run start")).toBe("bun run")
+			expect(extractCommandPattern("yarn run dev")).toBe("yarn run dev")
+			expect(extractCommandPattern("pnpm run lint")).toBe("pnpm run lint")
+			expect(extractCommandPattern("bun run start")).toBe("bun run start")
+		})
+
+		it("handles npm run with additional arguments", () => {
+			expect(extractCommandPattern("npm run build:prod -- --env=production")).toBe("npm run build:prod")
+			expect(extractCommandPattern("npm run test -- --coverage")).toBe("npm run test")
+			expect(extractCommandPattern("npm run build:dev --watch")).toBe("npm run *")
 		})
 
 		it("extracts npm script patterns", () => {
@@ -79,7 +85,7 @@ describe("extractCommandPattern", () => {
 			expect(extractCommandPattern("cd /path && npm install")).toBe("cd * && npm install")
 			expect(extractCommandPattern("npm test || echo 'failed'")).toBe("npm test || echo")
 			expect(extractCommandPattern("git pull; npm install; npm run build")).toBe(
-				"git pull ; npm install ; npm run",
+				"git pull ; npm install ; npm run build",
 			)
 			expect(extractCommandPattern("echo 'start' | grep start")).toBe("echo | grep")
 		})
@@ -137,11 +143,31 @@ describe("extractCommandPattern", () => {
 			expect(extractCommandPattern("cd /home/user/project")).toBe("cd *")
 			expect(extractCommandPattern("cd ..")).toBe("cd *")
 			expect(extractCommandPattern("cd")).toBe("cd *")
+			expect(extractCommandPattern("cd /usr/local/bin")).toBe("cd *")
 		})
 
 		it("handles commands with environment variables", () => {
-			expect(extractCommandPattern("NODE_ENV=production npm start")).toBe("NODE_ENV=production")
-			expect(extractCommandPattern("PORT=3000 node server.js")).toBe("PORT=3000")
+			expect(extractCommandPattern("NODE_ENV=production npm start")).toBe("NODE_ENV=*")
+			expect(extractCommandPattern("PORT=3000 node server.js")).toBe("PORT=*")
+		})
+
+		it("handles dangerous commands more carefully", () => {
+			expect(extractCommandPattern("rm -rf /")).toBe("rm")
+			expect(extractCommandPattern("rm -rf node_modules")).toBe("rm")
+			expect(extractCommandPattern("find . -name '*.log' -delete")).toBe("find")
+		})
+
+		it("handles command injection patterns", () => {
+			expect(extractCommandPattern("echo $(whoami)")).toBe("echo")
+			expect(extractCommandPattern("echo `date`")).toBe("echo")
+			expect(extractCommandPattern("bash -c 'echo hello'")).toBe("bash")
+		})
+
+		it("limits deeply chained commands", () => {
+			const deepChain = "echo 1 && echo 2 && echo 3 && echo 4 && echo 5"
+			const result = extractCommandPattern(deepChain)
+			// Should stop at some point to prevent overly complex patterns
+			expect(result).toBe("echo")
 		})
 	})
 })

webview-ui/src/utils/extract-command-pattern.ts

@@ -19,13 +19,69 @@ export function extractCommandPattern(command: string): string {
 	const trimmedCommand = command.trim()
 
 	// Check if this is a chained command
-	const chainMatch = trimmedCommand.match(/^(.+?)\s*(&&|\|\||;|\|)\s*(.+)$/)
-	if (chainMatch) {
-		// Handle chained commands by processing each part
-		const [, firstPart, operator, restPart] = chainMatch
-		const firstPattern = extractSingleCommandPattern(firstPart.trim())
-		const restPattern = extractCommandPattern(restPart.trim())
-		return `${firstPattern} ${operator} ${restPattern}`
+	// Use a more robust regex that handles nested quotes properly
+	const operators = ["&&", "||", ";", "|"]
+	let chainOperator: string | null = null
+	let splitIndex = -1
+
+	// Find the first unquoted operator
+	let inSingleQuote = false
+	let inDoubleQuote = false
+	let escapeNext = false
+
+	for (let i = 0; i < trimmedCommand.length; i++) {
+		const char = trimmedCommand[i]
+
+		if (escapeNext) {
+			escapeNext = false
+			continue
+		}
+
+		if (char === "\\") {
+			escapeNext = true
+			continue
+		}
+
+		if (char === "'" && !inDoubleQuote) {
+			inSingleQuote = !inSingleQuote
+			continue
+		}
+
+		if (char === '"' && !inSingleQuote) {
+			inDoubleQuote = !inDoubleQuote
+			continue
+		}
+
+		// Only look for operators outside of quotes
+		if (!inSingleQuote && !inDoubleQuote) {
+			for (const op of operators) {
+				if (trimmedCommand.substring(i, i + op.length) === op) {
+					chainOperator = op
+					splitIndex = i
+					break
+				}
+			}
+			if (chainOperator) break
+		}
+	}
+
+	if (chainOperator && splitIndex > 0) {
+		const firstPart = trimmedCommand.substring(0, splitIndex).trim()
+		const restPart = trimmedCommand.substring(splitIndex + chainOperator.length).trim()
+
+		// Process each part separately
+		const firstPattern = extractSingleCommandPattern(firstPart)
+		const restPattern = extractCommandPattern(restPart)
+
+		// For security, limit the depth of chained commands
+		// Count existing operators in the pattern to prevent deeply nested chains
+		const operatorCount = (restPattern.match(/&&|\|\||;|\|/g) || []).length
+		if (operatorCount >= 3) {
+			// Too many chained commands, return a more restrictive pattern
+			return firstPattern
+		}
+
+		return `${firstPattern} ${chainOperator} ${restPattern}`
 	}
 
 	// Not a chained command, process normally
@@ -95,14 +151,24 @@ function extractSingleCommandPattern(command: string): string {
 	// 1. npm/yarn/pnpm commands - include subcommand with wildcards for scripts
 	if (["npm", "yarn", "pnpm", "bun"].includes(baseCommand) && tokens.length > 1) {
 		const subCommand = tokens[1]
-		// For "run" commands, include "run" with wildcard for script names
+		// For "run" commands, be more specific about patterns
 		if (subCommand === "run" && tokens.length > 2) {
-			// Check if the script name contains special characters like colons
 			const scriptName = tokens[2]
+			// Check if there are additional arguments after the script name
+			const hasDoubleDash = tokens.includes("--")
+			const doubleDashIndex = hasDoubleDash ? tokens.indexOf("--") : -1
+
+			// For scripts with colons or complex names
 			if (scriptName && (scriptName.includes(":") || scriptName.includes("-"))) {
+				// If there's a double dash with additional args, include the full script name
+				if (hasDoubleDash && doubleDashIndex > 2) {
+					return `${baseCommand} run ${scriptName}`
+				}
+				// Otherwise use wildcard for flexibility
 				return `${baseCommand} run *`
 			}
-			return `${baseCommand} run`
+			// For simple script names, include the script name itself
+			return `${baseCommand} run ${scriptName}`
 		}
 		// For direct scripts like "npm test", "npm build", include the script name
 		if (!subCommand.startsWith("-")) {
@@ -139,7 +205,7 @@ function extractSingleCommandPattern(command: string): string {
 		return baseCommand
 	}
 
-	// 6. cd command - include wildcard for paths
+	// 6. cd command - use wildcard for flexibility
 	if (baseCommand === "cd") {
 		return "cd *"
 	}
@@ -160,6 +226,23 @@ function extractSingleCommandPattern(command: string): string {
 		}
 	}
 
+	// 9. Environment variables - handle with care
+	if (baseCommand.includes("=")) {
+		// This might be an environment variable like NODE_ENV=production
+		const envMatch = baseCommand.match(/^([A-Z_]+)=/)
+		if (envMatch) {
+			// Don't include the value, just the variable name pattern
+			return `${envMatch[1]}=*`
+		}
+	}
+
+	// 10. Commands with suspicious patterns - be restrictive
+	// Check for potential command injection patterns
+	if (baseCommand.includes("$") || baseCommand.includes("`") || baseCommand.includes("(")) {
+		// These could be command substitutions or variables, be very restrictive
+		return baseCommand.split(/[$`(]/)[0].trim() || "echo"
+	}
+
 	// Default: just return the base command
 	return baseCommand
 }