diff --git a/webview-ui/src/components/chat/CommandExecution.tsx b/webview-ui/src/components/chat/CommandExecution.tsx index a2b63ec5bf..560424546d 100644 --- a/webview-ui/src/components/chat/CommandExecution.tsx +++ b/webview-ui/src/components/chat/CommandExecution.tsx @@ -6,7 +6,6 @@ import { CommandExecutionStatus, commandExecutionStatusSchema } from "@roo-code/ import { ExtensionMessage } from "@roo/ExtensionMessage" import { safeJsonParse } from "@roo/safeJsonParse" -import { COMMAND_OUTPUT_STRING } from "@roo/combineCommandSequences" import { vscode } from "@src/utils/vscode" import { useExtensionState } from "@src/context/ExtensionStateContext" @@ -42,15 +41,8 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec output: parsedOutput, suggestions, } = useMemo(() => { - // First try our enhanced parser - const enhanced = parseCommandAndOutputUtil(text || "") - // If it found a command, use it, otherwise fall back to the original parser - if (enhanced.command && enhanced.command !== text) { - return enhanced - } - // Fall back to original parser - const original = parseCommandAndOutput(text) - return { ...original, suggestions: [] } + // Use the enhanced parser from commandPatterns + return parseCommandAndOutputUtil(text || "") }, [text]) // If we aren't opening the VSCode terminal for this command then we default @@ -230,20 +222,3 @@ const OutputContainerInternal = ({ isExpanded, output }: { isExpanded: boolean; ) const OutputContainer = memo(OutputContainerInternal) - -const parseCommandAndOutput = (text: string | undefined) => { - if (!text) { - return { command: "", output: "" } - } - - const index = text.indexOf(COMMAND_OUTPUT_STRING) - - if (index === -1) { - return { command: text, output: "" } - } - - return { - command: text.slice(0, index), - output: text.slice(index + COMMAND_OUTPUT_STRING.length), - } -} diff --git a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx index eafbde59aa..c356aabdc1 100644 --- a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx @@ -274,4 +274,143 @@ describe("CommandExecution", () => { expect(vscode.postMessage).toHaveBeenCalledWith({ type: "allowedCommands", commands: ["npm", "rm"] }) expect(vscode.postMessage).toHaveBeenCalledWith({ type: "deniedCommands", commands: [] }) }) + + describe("integration with CommandPatternSelector", () => { + it("should extract patterns from complex commands with multiple operators", () => { + render( + + + , + ) + + const selector = screen.getByTestId("command-pattern-selector") + expect(selector).toBeInTheDocument() + expect(screen.getByText("npm")).toBeInTheDocument() + expect(screen.getByText("npm install")).toBeInTheDocument() + expect(screen.getByText("npm test")).toBeInTheDocument() + expect(screen.getByText("echo")).toBeInTheDocument() + }) + + it("should handle commands with malformed suggestions gracefully", () => { + const commandWithMalformedSuggestions = `npm install +Output: +Suggested patterns: npm, , npm install, +Other output here` + + render( + + icon} + title={Run Command} + /> + , + ) + + const selector = screen.getByTestId("command-pattern-selector") + expect(selector).toBeInTheDocument() + // Should still show valid patterns + expect(screen.getAllByText("npm")[0]).toBeInTheDocument() + expect(screen.getAllByText("npm install")[0]).toBeInTheDocument() + }) + + it("should handle commands with subshells by not including them in patterns", () => { + render( + + + , + ) + + const selector = screen.getByTestId("command-pattern-selector") + expect(selector).toBeInTheDocument() + expect(screen.getByText("echo")).toBeInTheDocument() + expect(screen.getByText("git")).toBeInTheDocument() + expect(screen.getByText("git status")).toBeInTheDocument() + // Should not include subshell content + expect(screen.queryByText("whoami")).not.toBeInTheDocument() + }) + + it("should handle commands with backtick subshells", () => { + render( + + + , + ) + + const selector = screen.getByTestId("command-pattern-selector") + expect(selector).toBeInTheDocument() + expect(screen.getByText("git")).toBeInTheDocument() + expect(screen.getByText("git commit")).toBeInTheDocument() + // Should not include subshell content + expect(screen.queryByText("date")).not.toBeInTheDocument() + }) + + it("should handle pattern changes for commands with special characters", () => { + render( + + + , + ) + + const selector = screen.getByTestId("command-pattern-selector") + expect(selector).toBeInTheDocument() + expect(screen.getByText("cd")).toBeInTheDocument() + expect(screen.getByText("npm")).toBeInTheDocument() + expect(screen.getByText("npm start")).toBeInTheDocument() + }) + + it("should handle commands with mixed content including output and suggestions", () => { + const commandWithMixedContent = `npm test +Output: +Running tests... +✓ Test 1 passed +✓ Test 2 passed + +Suggested patterns: npm, npm test, npm run +- npm +- npm test +- npm run test` + + render( + + icon} + title={Run Command} + /> + , + ) + + const selector = screen.getByTestId("command-pattern-selector") + expect(selector).toBeInTheDocument() + // Should show patterns from suggestions + expect(screen.getAllByText("npm")[0]).toBeInTheDocument() + expect(screen.getAllByText("npm test")[0]).toBeInTheDocument() + expect(screen.getAllByText("npm run")[0]).toBeInTheDocument() + }) + + it("should update both allowed and denied lists when patterns conflict", () => { + const conflictState = { + ...mockExtensionState, + allowedCommands: ["git"], + deniedCommands: ["git push"], + } + + render( + + + , + ) + + // Click to allow "git push" + const allowButton = screen.getByText("Allow git push") + fireEvent.click(allowButton) + + // Should add to allowed and remove from denied + expect(conflictState.setAllowedCommands).toHaveBeenCalledWith(["git", "git push"]) + expect(conflictState.setDeniedCommands).toHaveBeenCalledWith([]) + }) + }) }) diff --git a/webview-ui/src/i18n/locales/pl/chat.json b/webview-ui/src/i18n/locales/pl/chat.json index 807a2da644..b0a9d16e6a 100644 --- a/webview-ui/src/i18n/locales/pl/chat.json +++ b/webview-ui/src/i18n/locales/pl/chat.json @@ -197,7 +197,7 @@ "pid": "PID: {{pid}}", "exited": "Zakończono ({{exitCode}})", "manageCommands": "Zarządzaj uprawnieniami poleceń", - "commandManagementDescription": "Zarządzaj uprawnieniami poleceń: Kliknij ✓, aby zezwolić na automatyczne wykonanie, ✗, aby odmówić wykonania. Wzorce można włączać/wyłączać lub usuwać z list. Zobacz wszystkie ustawienia", + "commandManagementDescription": "Zarządzaj uprawnieniami poleceń: Kliknij ✓, aby zezwolić na automatyczne wykonanie, ✗, aby odmówić wykonania. Wzorce można włączać/wyłączać lub usuwać z listy. Zobacz wszystkie ustawienia", "addToAllowed": "Dodaj do listy dozwolonych", "removeFromAllowed": "Usuń z listy dozwolonych", "addToDenied": "Dodaj do listy odrzuconych", diff --git a/webview-ui/src/utils/__tests__/commandPatterns.spec.ts b/webview-ui/src/utils/__tests__/commandPatterns.spec.ts index 2c16ab68a7..b3d5ffacdd 100644 --- a/webview-ui/src/utils/__tests__/commandPatterns.spec.ts +++ b/webview-ui/src/utils/__tests__/commandPatterns.spec.ts @@ -1,5 +1,10 @@ import { describe, it, expect } from "vitest" -import { extractCommandPatterns, getPatternDescription, parseCommandAndOutput } from "../commandPatterns" +import { + extractCommandPatterns, + getPatternDescription, + parseCommandAndOutput, + detectSecurityIssues, +} from "../commandPatterns" describe("extractCommandPatterns", () => { it("should extract simple command", () => { @@ -99,16 +104,16 @@ describe("extractCommandPatterns", () => { }) describe("getPatternDescription", () => { - it("should return descriptions for common commands", () => { - expect(getPatternDescription("cd")).toBe("directory navigation") + it("should return pattern followed by commands", () => { + expect(getPatternDescription("cd")).toBe("cd commands") expect(getPatternDescription("npm")).toBe("npm commands") expect(getPatternDescription("npm install")).toBe("npm install commands") expect(getPatternDescription("git")).toBe("git commands") expect(getPatternDescription("git push")).toBe("git push commands") - expect(getPatternDescription("python")).toBe("python scripts") + expect(getPatternDescription("python")).toBe("python commands") }) - it("should return default description for unknown commands", () => { + it("should handle any command pattern", () => { expect(getPatternDescription("unknowncommand")).toBe("unknowncommand commands") expect(getPatternDescription("custom-tool")).toBe("custom-tool commands") }) @@ -116,13 +121,13 @@ describe("getPatternDescription", () => { it("should handle package managers", () => { expect(getPatternDescription("yarn")).toBe("yarn commands") expect(getPatternDescription("pnpm")).toBe("pnpm commands") - expect(getPatternDescription("bun")).toBe("bun scripts") + expect(getPatternDescription("bun")).toBe("bun commands") }) it("should handle build tools", () => { - expect(getPatternDescription("make")).toBe("build automation") - expect(getPatternDescription("cmake")).toBe("CMake build system") - expect(getPatternDescription("cargo")).toBe("Rust cargo commands") + expect(getPatternDescription("make")).toBe("make commands") + expect(getPatternDescription("cmake")).toBe("cmake commands") + expect(getPatternDescription("cargo")).toBe("cargo commands") expect(getPatternDescription("go build")).toBe("go build commands") }) }) @@ -273,3 +278,77 @@ Installing...` expect(result.output).toBe("First output\nOutput: Second output") }) }) + +describe("detectSecurityIssues", () => { + it("should detect subshell execution with $()", () => { + const warnings = detectSecurityIssues("echo $(malicious)") + expect(warnings).toHaveLength(1) + expect(warnings[0].type).toBe("subshell") + expect(warnings[0].message).toContain("subshell execution") + }) + + it("should detect subshell execution with backticks", () => { + const warnings = detectSecurityIssues("echo `malicious`") + expect(warnings).toHaveLength(1) + expect(warnings[0].type).toBe("subshell") + expect(warnings[0].message).toContain("subshell execution") + }) + + it("should detect nested subshells", () => { + const warnings = detectSecurityIssues("echo $(echo $(date))") + expect(warnings).toHaveLength(1) + expect(warnings[0].type).toBe("subshell") + }) + + it("should detect subshells in complex commands", () => { + const warnings = detectSecurityIssues("npm install && echo $(whoami) || git push") + expect(warnings).toHaveLength(1) + expect(warnings[0].type).toBe("subshell") + }) + + it("should not detect issues in safe commands", () => { + const warnings = detectSecurityIssues("npm install express") + expect(warnings).toHaveLength(0) + }) + + it("should handle empty commands", () => { + const warnings = detectSecurityIssues("") + expect(warnings).toHaveLength(0) + }) + + it("should detect multiple subshell patterns", () => { + const warnings = detectSecurityIssues("echo $(date) && echo `whoami`") + expect(warnings).toHaveLength(1) // Should still be 1 warning for subshell presence + expect(warnings[0].type).toBe("subshell") + }) + + it("should detect subshells in quoted strings", () => { + const warnings = detectSecurityIssues('echo "Current user: $(whoami)"') + expect(warnings).toHaveLength(1) + expect(warnings[0].type).toBe("subshell") + }) +}) + +describe("security integration with extractCommandPatterns", () => { + it("should not include subshell content in patterns", () => { + const patterns = extractCommandPatterns("echo $(malicious)") + expect(patterns).toContain("echo") + expect(patterns).not.toContain("$(malicious)") + expect(patterns).not.toContain("malicious") + }) + + it("should handle commands with subshells properly", () => { + const patterns = extractCommandPatterns("npm install && echo $(whoami)") + expect(patterns).toContain("npm") + expect(patterns).toContain("npm install") + expect(patterns).toContain("echo") + expect(patterns).not.toContain("whoami") + }) + + it("should extract patterns from commands with backtick subshells", () => { + const patterns = extractCommandPatterns("git commit -m `date`") + expect(patterns).toContain("git") + expect(patterns).toContain("git commit") + expect(patterns).not.toContain("date") + }) +}) diff --git a/webview-ui/src/utils/commandPatterns.ts b/webview-ui/src/utils/commandPatterns.ts index f6c2b48294..1dd2836ff0 100644 --- a/webview-ui/src/utils/commandPatterns.ts +++ b/webview-ui/src/utils/commandPatterns.ts @@ -5,13 +5,23 @@ export interface CommandPattern { description?: string } +export interface SecurityWarning { + type: "subshell" | "injection" + message: string +} + export function extractCommandPatterns(command: string): string[] { if (!command?.trim()) return [] const patterns = new Set() try { - const parsed = parse(command) + // First, remove subshell expressions to avoid extracting their contents + const cleanedCommand = command + .replace(/\$\([^)]*\)/g, "") // Remove $() subshells + .replace(/`[^`]*`/g, "") // Remove backtick subshells + + const parsed = parse(cleanedCommand) const commandSeparators = new Set(["|", "&&", "||", ";"]) let current: any[] = [] @@ -54,73 +64,26 @@ function processCommand(cmd: any[], patterns: Set) { } } -export function getPatternDescription(pattern: string): string { - // Generate human-readable descriptions for common patterns - const descriptions: Record = { - cd: "directory navigation", - ls: "list directory contents", - pwd: "print working directory", - mkdir: "create directories", - rm: "remove files/directories", - cp: "copy files/directories", - mv: "move/rename files", - cat: "display file contents", - echo: "display text", - npm: "npm commands", - "npm install": "npm install commands", - "npm run": "all npm run scripts", - "npm test": "npm test commands", - "npm start": "npm start commands", - "npm build": "npm build commands", - yarn: "yarn commands", - "yarn install": "yarn install commands", - "yarn run": "all yarn run scripts", - pnpm: "pnpm commands", - "pnpm install": "pnpm install commands", - "pnpm run": "all pnpm run scripts", - git: "git commands", - "git add": "git add commands", - "git commit": "git commit commands", - "git push": "git push commands", - "git pull": "git pull commands", - "git clone": "git clone commands", - "git checkout": "git checkout commands", - "git branch": "git branch commands", - "git merge": "git merge commands", - "git status": "git status commands", - "git log": "git log commands", - python: "python scripts", - python3: "python3 scripts", - node: "node.js scripts", - deno: "deno scripts", - bun: "bun scripts", - docker: "docker commands", - "docker run": "docker run commands", - "docker build": "docker build commands", - "docker compose": "docker compose commands", - curl: "HTTP requests", - wget: "download files", - grep: "search text patterns", - find: "find files/directories", - sed: "stream editor", - awk: "text processing", - make: "build automation", - cmake: "CMake build system", - go: "go commands", - "go run": "go run commands", - "go build": "go build commands", - "go test": "go test commands", - cargo: "Rust cargo commands", - "cargo run": "cargo run commands", - "cargo build": "cargo build commands", - "cargo test": "cargo test commands", - dotnet: ".NET commands", - "dotnet run": "dotnet run commands", - "dotnet build": "dotnet build commands", - "dotnet test": "dotnet test commands", +export function detectSecurityIssues(command: string): SecurityWarning[] { + const warnings: SecurityWarning[] = [] + + // Check for subshell execution attempts + if (command.includes("$(") || command.includes("`")) { + warnings.push({ + type: "subshell", + message: "Command contains subshell execution which could bypass restrictions", + }) } - return descriptions[pattern] || `${pattern} commands` + return warnings +} + +/** + * Get a human-readable description for a command pattern. + * Simply returns the pattern followed by "commands". + */ +export function getPatternDescription(pattern: string): string { + return `${pattern} commands` } export function parseCommandAndOutput(text: string): {