From d2a0390f5dd4d24eb108174122dafca28bb280d2 Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Tue, 1 Jul 2025 16:44:42 -0600 Subject: [PATCH] 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. --- .../__tests__/extract-command-pattern.spec.ts | 40 +++++-- .../src/utils/extract-command-pattern.ts | 105 ++++++++++++++++-- 2 files changed, 127 insertions(+), 18 deletions(-) diff --git a/webview-ui/src/utils/__tests__/extract-command-pattern.spec.ts b/webview-ui/src/utils/__tests__/extract-command-pattern.spec.ts index 31452a42c0..b6715fcc4e 100644 --- a/webview-ui/src/utils/__tests__/extract-command-pattern.spec.ts +++ b/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") }) }) }) diff --git a/webview-ui/src/utils/extract-command-pattern.ts b/webview-ui/src/utils/extract-command-pattern.ts index 9bfbe60768..83f0628fb5 100644 --- a/webview-ui/src/utils/extract-command-pattern.ts +++ b/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 }