diff --git a/webview-ui/src/components/chat/CommandExecution.tsx b/webview-ui/src/components/chat/CommandExecution.tsx index 8bbb7d9041..673bcca00b 100644 --- a/webview-ui/src/components/chat/CommandExecution.tsx +++ b/webview-ui/src/components/chat/CommandExecution.tsx @@ -1,6 +1,6 @@ import { useCallback, useState, memo, useMemo } from "react" import { useEvent } from "react-use" -import { ChevronDown, Skull, AlertTriangle } from "lucide-react" +import { ChevronDown, Skull } from "lucide-react" import { CommandExecutionStatus, commandExecutionStatusSchema } from "@roo-code/types" @@ -18,7 +18,6 @@ import { getPatternDescription, parseCommandAndOutput, CommandPattern, - detectSecurityIssues, } from "../../utils/commandPatterns" interface CommandExecutionProps { @@ -71,11 +70,6 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec return patterns }, [command]) - // Detect security issues in the command - const securityWarnings = useMemo(() => { - return detectSecurityIssues(command) - }, [command]) - // Handle pattern changes const handleAllowPatternChange = (pattern: string) => { const isAllowed = allowedCommands.includes(pattern) @@ -186,21 +180,6 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec
- {securityWarnings.length > 0 && ( -
-
- -
-
Security Warning
- {securityWarnings.map((warning, index) => ( -
- {warning.message} -
- ))} -
-
-
- )}
{commandPatterns.length > 0 && ( diff --git a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx index 7b60a87b4d..7b584d75e7 100644 --- a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx @@ -365,18 +365,6 @@ Other output here` expect(screen.queryByText("whoami")).not.toBeInTheDocument() }) - it("should display security warning for commands with subshells", () => { - render( - - - , - ) - - // Should show security warning - expect(screen.getByText("Security Warning")).toBeInTheDocument() - expect(screen.getByText(/subshell execution/)).toBeInTheDocument() - }) - it("should handle commands with backtick subshells", () => { render( diff --git a/webview-ui/src/components/chat/__tests__/CommandPatternSelector.spec.tsx b/webview-ui/src/components/chat/__tests__/CommandPatternSelector.spec.tsx index 4dd69e3969..d48d16c2d2 100644 --- a/webview-ui/src/components/chat/__tests__/CommandPatternSelector.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/CommandPatternSelector.spec.tsx @@ -1,50 +1,29 @@ import React from "react" import { render, screen, fireEvent } from "@testing-library/react" -import { describe, it, expect, vi, beforeEach } from "vitest" +import { describe, it, expect, vi } from "vitest" import { CommandPatternSelector } from "../CommandPatternSelector" import { CommandPattern } from "../../../utils/commandPatterns" +import { TooltipProvider } from "../../../components/ui/tooltip" // Mock react-i18next vi.mock("react-i18next", () => ({ useTranslation: () => ({ t: (key: string) => key, }), - Trans: ({ i18nKey, components }: any) => { - if (i18nKey === "chat:commandExecution.commandManagementDescription") { - return ( - - Manage command permissions: Click ✓ to allow auto-execution, ✗ to deny execution. Patterns can be - toggled on/off or removed from lists. {components.settingsLink} - - ) - } - return {i18nKey} - }, + Trans: ({ i18nKey, children }: any) => {i18nKey || children}, })) // Mock VSCodeLink vi.mock("@vscode/webview-ui-toolkit/react", () => ({ VSCodeLink: ({ children, onClick }: any) => ( - {children || "View all settings"} + {children} ), })) -// Mock StandardTooltip -vi.mock("../../ui/standard-tooltip", () => ({ - StandardTooltip: ({ children, content }: any) => ( -
- {children} - {/* Render the content to make it testable */} -
{content}
-
- ), -})) - -// Mock window.postMessage -const mockPostMessage = vi.fn() -window.postMessage = mockPostMessage +// Wrapper component with TooltipProvider +const TestWrapper = ({ children }: { children: React.ReactNode }) => {children} describe("CommandPatternSelector", () => { const mockPatterns: CommandPattern[] = [ @@ -61,192 +40,53 @@ describe("CommandPatternSelector", () => { onDenyPatternChange: vi.fn(), } - beforeEach(() => { - vi.clearAllMocks() - }) + it("should render with unique pattern keys", () => { + const { container } = render( + + + , + ) - it("should render collapsed by default", () => { - render() + // The component should render without errors + expect(container).toBeTruthy() - expect(screen.getByText("chat:commandExecution.manageCommands")).toBeInTheDocument() - expect(screen.queryByText("npm commands")).not.toBeInTheDocument() - }) - - it("should expand when clicked", () => { - render() - - const expandButton = screen.getByRole("button", { name: "chat:commandExecution.expandManagement" }) + // Click to expand the component + const expandButton = screen.getByRole("button", { name: /chat:commandExecution.expandManagement/i }) fireEvent.click(expandButton) - // Check for the patterns themselves + // Check that patterns are rendered expect(screen.getByText("npm")).toBeInTheDocument() expect(screen.getByText("npm install")).toBeInTheDocument() expect(screen.getByText("git")).toBeInTheDocument() - - // Check for the descriptions - expect(screen.getByText("- npm commands")).toBeInTheDocument() - expect(screen.getByText("- npm install commands")).toBeInTheDocument() - expect(screen.getByText("- git commands")).toBeInTheDocument() }) - it("should collapse when clicked again", () => { - render() + it("should handle duplicate patterns gracefully", () => { + // Test with duplicate patterns to ensure keys are still unique + const duplicatePatterns: CommandPattern[] = [ + { pattern: "npm", description: "npm commands" }, + { pattern: "npm", description: "duplicate npm commands" }, // Duplicate pattern + { pattern: "git", description: "git commands" }, + ] - const expandButton = screen.getByRole("button", { name: "chat:commandExecution.expandManagement" }) - fireEvent.click(expandButton) - - const collapseButton = screen.getByRole("button", { name: "chat:commandExecution.collapseManagement" }) - fireEvent.click(collapseButton) - - expect(screen.queryByText("npm commands")).not.toBeInTheDocument() - }) - - it("should show correct status for patterns", () => { - render() - - const expandButton = screen.getByRole("button", { name: "chat:commandExecution.expandManagement" }) - fireEvent.click(expandButton) - - // Check that npm has allowed styling (green) - const npmAllowButton = screen.getAllByRole("button", { name: "chat:commandExecution.removeFromAllowed" })[0] - expect(npmAllowButton).toHaveClass("bg-green-500/20") - - // Check that git has denied styling (red) - const gitDenyButton = screen.getAllByRole("button", { name: "chat:commandExecution.removeFromDenied" })[0] - expect(gitDenyButton).toHaveClass("bg-red-500/20") - }) - - it("should call onAllowPatternChange when allow button is clicked", () => { - render() - - const expandButton = screen.getByRole("button", { name: "chat:commandExecution.expandManagement" }) - fireEvent.click(expandButton) - - // Find all allow buttons with the "add to allowed" label - const allowButtons = screen.getAllByRole("button", { name: "chat:commandExecution.addToAllowed" }) - - // The second one should be for npm install (first is npm which is already allowed) - fireEvent.click(allowButtons[0]) - - expect(defaultProps.onAllowPatternChange).toHaveBeenCalledWith("npm install") - }) - - it("should call onDenyPatternChange when deny button is clicked", () => { - render() - - const expandButton = screen.getByRole("button", { name: "chat:commandExecution.expandManagement" }) - fireEvent.click(expandButton) - - // Find all deny buttons with the "add to denied" label - const denyButtons = screen.getAllByRole("button", { name: "chat:commandExecution.addToDenied" }) - - // The second one should be for npm install (first is npm, third is git which is already denied) - fireEvent.click(denyButtons[1]) - - expect(defaultProps.onDenyPatternChange).toHaveBeenCalledWith("npm install") - }) - - it("should toggle allowed pattern when clicked", () => { - render() - - const expandButton = screen.getByRole("button", { name: "chat:commandExecution.expandManagement" }) - fireEvent.click(expandButton) - - // Find the allow button for npm (which is already allowed) - const npmAllowButton = screen.getAllByRole("button", { name: "chat:commandExecution.removeFromAllowed" })[0] - fireEvent.click(npmAllowButton) - - expect(defaultProps.onAllowPatternChange).toHaveBeenCalledWith("npm") - }) - - it("should toggle denied pattern when clicked", () => { - render() - - const expandButton = screen.getByRole("button", { name: "chat:commandExecution.expandManagement" }) - fireEvent.click(expandButton) - - // Find the deny button for git (which is already denied) - const gitDenyButton = screen.getAllByRole("button", { name: "chat:commandExecution.removeFromDenied" })[0] - fireEvent.click(gitDenyButton) - - expect(defaultProps.onDenyPatternChange).toHaveBeenCalledWith("git") - }) - - it("should have tooltip with settings link", () => { - const { container } = render() - - // The info icon should have a tooltip - const tooltipWrapper = container.querySelector('[title="tooltip"]') - expect(tooltipWrapper).toBeTruthy() - - // The tooltip content includes a settings link (mocked as VSCodeLink) - // It's rendered in a hidden div for testing purposes - const settingsLink = container.querySelector('a[href="#"]') - expect(settingsLink).toBeTruthy() - expect(settingsLink?.textContent).toBe("View all settings") - - // Test that clicking the link posts the correct message - if (settingsLink) { - fireEvent.click(settingsLink) - - expect(mockPostMessage).toHaveBeenCalledWith( - { - type: "action", - action: "settingsButtonClicked", - values: { section: "autoApprove" }, - }, - "*", - ) + const props = { + ...defaultProps, + patterns: duplicatePatterns, } - }) - it("should render with empty patterns", () => { - render() + // This should not throw an error even with duplicate patterns + const { container } = render( + + + , + ) + expect(container).toBeTruthy() - const expandButton = screen.getByRole("button", { name: "chat:commandExecution.expandManagement" }) + // Click to expand the component + const expandButton = screen.getByRole("button", { name: /chat:commandExecution.expandManagement/i }) fireEvent.click(expandButton) - // The expanded view should exist but be empty since there are no patterns - const expandedContent = screen - .getByRole("button", { name: "chat:commandExecution.collapseManagement" }) - .parentElement?.querySelector(".px-3.pb-3") - expect(expandedContent).toBeInTheDocument() - expect(expandedContent?.children.length).toBe(0) - }) - - it("should render patterns without descriptions", () => { - const patternsWithoutDesc: CommandPattern[] = [{ pattern: "custom-command" }] - - render() - - const expandButton = screen.getByRole("button", { name: "chat:commandExecution.expandManagement" }) - fireEvent.click(expandButton) - - expect(screen.getByText("custom-command")).toBeInTheDocument() - }) - - it("should always show info icon with tooltip", () => { - const { container } = render() - - // Info icon should always be visible (not just when expanded) - // Look for the Info icon which is wrapped in StandardTooltip - const infoIcon = container.querySelector(".ml-1") - expect(infoIcon).toBeTruthy() - }) - - it("should apply correct classes for chevron rotation", () => { - const { container } = render() - - // Initially collapsed - chevron should be rotated - let chevron = container.querySelector(".size-3.transition-transform") - expect(chevron).toHaveClass("-rotate-90") - - // Click to expand - const expandButton = screen.getByRole("button", { name: "chat:commandExecution.expandManagement" }) - fireEvent.click(expandButton) - - // When expanded - chevron should not be rotated - chevron = container.querySelector(".size-3.transition-transform") - expect(chevron).toHaveClass("rotate-0") + // Both instances of "npm" should be rendered + const npmElements = screen.getAllByText("npm") + expect(npmElements).toHaveLength(2) }) }) diff --git a/webview-ui/src/utils/__tests__/command-parser.spec.ts b/webview-ui/src/utils/__tests__/command-parser.spec.ts index 4063e7a1ac..05303f87fc 100644 --- a/webview-ui/src/utils/__tests__/command-parser.spec.ts +++ b/webview-ui/src/utils/__tests__/command-parser.spec.ts @@ -1,77 +1,5 @@ import { describe, it, expect } from "vitest" -import { parseCommandString, extractPatternsFromCommand, detectCommandSecurityIssues } from "../command-parser" - -describe("parseCommandString", () => { - it("should parse simple command", () => { - const result = parseCommandString("ls -la") - expect(result.subCommands).toEqual(["ls -la"]) - expect(result.hasSubshells).toBe(false) - expect(result.subshellCommands).toEqual([]) - }) - - it("should parse command with && operator", () => { - const result = parseCommandString("npm install && npm test") - expect(result.subCommands).toEqual(["npm install", "npm test"]) - expect(result.hasSubshells).toBe(false) - }) - - it("should parse command with || operator", () => { - const result = parseCommandString("npm test || npm run test:ci") - expect(result.subCommands).toEqual(["npm test", "npm run test:ci"]) - expect(result.hasSubshells).toBe(false) - }) - - it("should parse command with pipe", () => { - const result = parseCommandString("ls -la | grep test") - expect(result.subCommands).toEqual(["ls -la", "grep test"]) - expect(result.hasSubshells).toBe(false) - }) - - it("should detect and extract subshells with $()", () => { - const result = parseCommandString("echo $(date)") - expect(result.subCommands).toEqual(["echo", "date"]) - expect(result.hasSubshells).toBe(true) - expect(result.subshellCommands).toEqual(["date"]) - }) - - it("should detect and extract subshells with backticks", () => { - const result = parseCommandString("echo `whoami`") - expect(result.subCommands).toEqual(["echo", "whoami"]) - expect(result.hasSubshells).toBe(true) - expect(result.subshellCommands).toEqual(["whoami"]) - }) - - it("should handle PowerShell redirections", () => { - const result = parseCommandString("command 2>&1") - expect(result.subCommands).toEqual(["command 2>&1"]) - expect(result.hasSubshells).toBe(false) - }) - - it("should handle quoted strings", () => { - const result = parseCommandString('echo "hello world"') - expect(result.subCommands).toEqual(['echo "hello world"']) - expect(result.hasSubshells).toBe(false) - }) - - it("should handle array indexing expressions", () => { - const result = parseCommandString("echo ${array[0]}") - expect(result.subCommands).toEqual(["echo ${array[0]}"]) - expect(result.hasSubshells).toBe(false) - }) - - it("should handle empty command", () => { - const result = parseCommandString("") - expect(result.subCommands).toEqual([]) - expect(result.hasSubshells).toBe(false) - expect(result.subshellCommands).toEqual([]) - }) - - it("should handle complex command with multiple operators", () => { - const result = parseCommandString("npm install && npm test | grep success || echo 'failed'") - expect(result.subCommands).toEqual(["npm install", "npm test", "grep success", "echo failed"]) - expect(result.hasSubshells).toBe(false) - }) -}) +import { extractPatternsFromCommand } from "../command-parser" describe("extractPatternsFromCommand", () => { it("should extract simple command pattern", () => { @@ -79,9 +7,9 @@ describe("extractPatternsFromCommand", () => { expect(patterns).toEqual(["ls"]) }) - it("should extract command with arguments", () => { - const patterns = extractPatternsFromCommand("npm install express") - expect(patterns).toEqual(["npm", "npm install", "npm install express"]) + it("should extract command with subcommand", () => { + const patterns = extractPatternsFromCommand("git push origin main") + expect(patterns).toEqual(["git", "git push", "git push origin"]) }) it("should stop at flags", () => { @@ -94,17 +22,24 @@ describe("extractPatternsFromCommand", () => { expect(patterns).toEqual(["cd"]) }) - it("should handle piped commands", () => { + it("should handle pipes", () => { const patterns = extractPatternsFromCommand("ls -la | grep test") - expect(patterns).toContain("ls") - expect(patterns).toContain("grep") - expect(patterns).toContain("grep test") + expect(patterns).toEqual(["grep", "grep test", "ls"]) }) - it("should remove subshells before extracting patterns", () => { - const patterns = extractPatternsFromCommand("echo $(malicious)") - expect(patterns).toEqual(["echo"]) - expect(patterns).not.toContain("malicious") + it("should handle && operator", () => { + const patterns = extractPatternsFromCommand("npm install && git push origin main") + expect(patterns).toEqual(["git", "git push", "git push origin", "npm", "npm install"]) + }) + + it("should handle || operator", () => { + const patterns = extractPatternsFromCommand("npm test || npm run test:ci") + expect(patterns).toEqual(["npm", "npm run", "npm test"]) + }) + + it("should handle semicolon separator", () => { + const patterns = extractPatternsFromCommand("cd src; npm install") + expect(patterns).toEqual(["cd", "cd src", "npm", "npm install"]) }) it("should skip numeric commands", () => { @@ -112,50 +47,91 @@ describe("extractPatternsFromCommand", () => { expect(patterns).toEqual([]) }) - it("should skip common output words", () => { - const patterns = extractPatternsFromCommand("error") - expect(patterns).toEqual([]) - }) - it("should handle empty command", () => { const patterns = extractPatternsFromCommand("") expect(patterns).toEqual([]) }) + it("should handle null/undefined", () => { + expect(extractPatternsFromCommand(null as any)).toEqual([]) + expect(extractPatternsFromCommand(undefined as any)).toEqual([]) + }) + + it("should handle scripts", () => { + const patterns = extractPatternsFromCommand("./script.sh --verbose") + expect(patterns).toEqual(["./script.sh"]) + }) + + it("should handle paths with dots", () => { + const patterns = extractPatternsFromCommand("git add .") + expect(patterns).toEqual(["git", "git add"]) + }) + + it("should handle paths with tilde", () => { + const patterns = extractPatternsFromCommand("cd ~/projects") + expect(patterns).toEqual(["cd"]) + }) + + it("should handle colons in arguments", () => { + const patterns = extractPatternsFromCommand("docker run image:tag") + expect(patterns).toEqual(["docker", "docker run"]) + }) + it("should return sorted patterns", () => { + const patterns = extractPatternsFromCommand("npm run build && git push") + expect(patterns).toEqual(["git", "git push", "npm", "npm run", "npm run build"]) + }) + + it("should handle complex command with multiple operators", () => { + const patterns = extractPatternsFromCommand("npm install && npm test | grep success || echo 'failed'") + expect(patterns).toContain("npm") + expect(patterns).toContain("npm install") + expect(patterns).toContain("npm test") + expect(patterns).toContain("grep") + expect(patterns).toContain("echo") + }) + + it("should handle malformed commands gracefully", () => { + const patterns = extractPatternsFromCommand("echo 'unclosed quote") + expect(patterns).toContain("echo") + }) + + it("should not treat package managers specially", () => { const patterns = extractPatternsFromCommand("npm run build") expect(patterns).toEqual(["npm", "npm run", "npm run build"]) - }) -}) - -describe("detectCommandSecurityIssues", () => { - it("should detect subshell with $()", () => { - const warnings = detectCommandSecurityIssues("echo $(malicious)") - expect(warnings).toHaveLength(1) - expect(warnings[0].type).toBe("subshell") - expect(warnings[0].message).toContain("subshell execution") - }) - - it("should detect subshell with backticks", () => { - const warnings = detectCommandSecurityIssues("echo `malicious`") - expect(warnings).toHaveLength(1) - expect(warnings[0].type).toBe("subshell") - expect(warnings[0].message).toContain("subshell execution") - }) - - it("should detect multiple subshell patterns", () => { - const warnings = detectCommandSecurityIssues("echo $(date) && echo `whoami`") - expect(warnings).toHaveLength(1) // Still one warning for subshell presence - expect(warnings[0].type).toBe("subshell") - }) - - it("should not detect issues in safe commands", () => { - const warnings = detectCommandSecurityIssues("npm install express") - expect(warnings).toHaveLength(0) - }) - - it("should handle empty command", () => { - const warnings = detectCommandSecurityIssues("") - expect(warnings).toHaveLength(0) + // Now includes "npm run build" with 3-level extraction + }) + + it("should extract at most 3 levels", () => { + const patterns = extractPatternsFromCommand("git push origin main --force") + expect(patterns).toEqual(["git", "git push", "git push origin"]) + // Should NOT include deeper levels beyond 3 + }) + + it("should handle multi-level commands like gh pr", () => { + const patterns = extractPatternsFromCommand("gh pr checkout 123") + expect(patterns).toEqual(["gh", "gh pr", "gh pr checkout"]) + }) + + it("should extract 3 levels for git remote add", () => { + const patterns = extractPatternsFromCommand("git remote add origin https://github.com/user/repo.git") + expect(patterns).toEqual(["git", "git remote", "git remote add"]) + }) + + it("should extract 3 levels for npm run build", () => { + const patterns = extractPatternsFromCommand("npm run build --production") + expect(patterns).toEqual(["npm", "npm run", "npm run build"]) + }) + + it("should stop at file extensions even at third level", () => { + const patterns = extractPatternsFromCommand("node scripts test.js") + expect(patterns).toEqual(["node", "node scripts"]) + // Should NOT include "node scripts test.js" because of .js + }) + + it("should stop at flags at any level", () => { + const patterns = extractPatternsFromCommand("docker run -it ubuntu") + expect(patterns).toEqual(["docker", "docker run"]) + // Stops at -it flag }) }) diff --git a/webview-ui/src/utils/__tests__/commandPatterns.spec.ts b/webview-ui/src/utils/__tests__/commandPatterns.spec.ts index 92feda0412..ff563be1ac 100644 --- a/webview-ui/src/utils/__tests__/commandPatterns.spec.ts +++ b/webview-ui/src/utils/__tests__/commandPatterns.spec.ts @@ -1,10 +1,5 @@ import { describe, it, expect } from "vitest" -import { - extractCommandPatterns, - getPatternDescription, - parseCommandAndOutput, - detectSecurityIssues, -} from "../commandPatterns" +import { extractCommandPatterns, getPatternDescription, parseCommandAndOutput } from "../commandPatterns" describe("extractCommandPatterns", () => { it("should extract simple command", () => { @@ -37,7 +32,7 @@ describe("extractCommandPatterns", () => { expect(patterns).toContain("npm") expect(patterns).toContain("npm test") expect(patterns).toContain("npm run") - expect(patterns).toContain("npm run test:ci") + expect(patterns).not.toContain("npm run test:ci") }) it("should handle semicolon separated commands", () => { @@ -94,12 +89,12 @@ describe("extractCommandPatterns", () => { const patterns = extractCommandPatterns("git add .") expect(patterns).toContain("git") expect(patterns).toContain("git add") - expect(patterns).not.toContain("git add .") + expect(patterns).not.toContain("git add .") // dot is excluded }) it("should return sorted patterns", () => { const patterns = extractCommandPatterns("npm run build && git push") - expect(patterns).toEqual([...patterns].sort()) + expect(patterns).toEqual(["git", "git push", "npm", "npm run", "npm run build"]) }) it("should handle numeric input like '0 total'", () => { @@ -354,80 +349,6 @@ drwxr-xr-x 20 user staff 640 Jan 22 11:00 ..` }) }) -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") - }) -}) - describe("integration: parseCommandAndOutput with extractCommandPatterns", () => { it("should not extract patterns from output text", () => { const text = `wc -l *.go *.java diff --git a/webview-ui/src/utils/command-parser.ts b/webview-ui/src/utils/command-parser.ts index a3fb6d92e2..d8ad9eebbc 100644 --- a/webview-ui/src/utils/command-parser.ts +++ b/webview-ui/src/utils/command-parser.ts @@ -1,289 +1,67 @@ import { parse } from "shell-quote" -type ShellToken = string | { op: string } | { command: string } - /** - * Shared command parsing utility that consolidates parsing logic - * from both command-validation.ts and commandPatterns.ts - */ - -/** - * Parse a command string and handle special cases like subshells, - * redirections, and quoted strings. - * - * @param command - The command string to parse - * @returns Object containing parsed information - */ -export function parseCommandString(command: string): { - subCommands: string[] - hasSubshells: boolean - subshellCommands: string[] -} { - if (!command?.trim()) { - return { - subCommands: [], - hasSubshells: false, - subshellCommands: [], - } - } - - try { - // First split by newlines (including all types: \n, \r\n, \r) to handle multi-line commands - const lines = command.split(/\r\n|\r|\n/) - const allCommands: string[] = [] - - for (const line of lines) { - const trimmedLine = line.trim() - if (!trimmedLine) continue // Skip empty lines - - // Storage for replaced content - const redirections: string[] = [] - const subshells: string[] = [] - const quotes: string[] = [] - const arrayIndexing: string[] = [] - const arithmeticExpressions: string[] = [] - const variables: string[] = [] - - // First handle PowerShell redirections by temporarily replacing them - let processedCommand = trimmedLine.replace(/\d*>&\d*/g, (match) => { - redirections.push(match) - return `__REDIR_${redirections.length - 1}__` - }) - - // Handle arithmetic expressions: $((...)) pattern - // Match the entire arithmetic expression including nested parentheses - processedCommand = processedCommand.replace(/\$\(\([^)]*(?:\)[^)]*)*\)\)/g, (match) => { - arithmeticExpressions.push(match) - return `__ARITH_${arithmeticExpressions.length - 1}__` - }) - - // Handle array indexing expressions: ${array[...]} pattern and partial expressions - processedCommand = processedCommand.replace(/\$\{[^}]*\[[^\]]*(\]([^}]*\})?)?/g, (match) => { - arrayIndexing.push(match) - return `__ARRAY_${arrayIndexing.length - 1}__` - }) - - // Handle simple variable references: $varname pattern - // This prevents shell-quote from splitting $count into separate tokens - processedCommand = processedCommand.replace(/\$[a-zA-Z_][a-zA-Z0-9_]*/g, (match) => { - variables.push(match) - return `__VAR_${variables.length - 1}__` - }) - - // Handle special bash variables: $?, $!, $#, $$, $@, $*, $-, $0-$9 - processedCommand = processedCommand.replace(/\$[?!#$@*\-0-9]/g, (match) => { - variables.push(match) - return `__VAR_${variables.length - 1}__` - }) - - // Then handle subshell commands - store them for security analysis - const _hasSubshells = trimmedLine.includes("$(") || trimmedLine.includes("`") - - processedCommand = processedCommand - .replace(/\$\(((?!\().*?)\)/g, (_, inner) => { - const trimmedInner = inner.trim() - subshells.push(trimmedInner) - return `__SUBSH_${subshells.length - 1}__` - }) - .replace(/`(.*?)`/g, (_, inner) => { - const trimmedInner = inner.trim() - subshells.push(trimmedInner) - return `__SUBSH_${subshells.length - 1}__` - }) - - // Then handle quoted strings - 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 (let i = 0; i < tokens.length; i++) { - const token = tokens[i] - - 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, redirections, arithmetic expressions, variables, and array indexing - const restoredCommands = 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)]) - // Restore arithmetic expressions - result = result.replace(/__ARITH_(\d+)__/g, (_, i) => arithmeticExpressions[parseInt(i)]) - // Restore variables - result = result.replace(/__VAR_(\d+)__/g, (_, i) => variables[parseInt(i)]) - // Restore array indexing expressions - result = result.replace(/__ARRAY_(\d+)__/g, (_, i) => arrayIndexing[parseInt(i)]) - return result - }) - - allCommands.push(...restoredCommands) - } - - // Check if any line has subshells - const hasSubshells = command.includes("$(") || command.includes("`") - const subshellCommands: string[] = [] - - // Extract subshell commands for security analysis - let match: RegExpExecArray | null - const subshellRegex1 = /\$\(((?!\().*?)\)/g - const subshellRegex2 = /`(.*?)`/g - - while ((match = subshellRegex1.exec(command)) !== null) { - if (match[1]) { - subshellCommands.push(match[1].trim()) - } - } - - while ((match = subshellRegex2.exec(command)) !== null) { - if (match[1]) { - subshellCommands.push(match[1].trim()) - } - } - - return { - subCommands: allCommands, - hasSubshells, - subshellCommands, - } - } catch (_error) { - // If shell-quote fails, fall back to simple splitting - const fallbackCommands = command - .split(/\n/) - .map((line) => line.trim()) - .filter((line) => line.length > 0) - - return { - subCommands: fallbackCommands.length > 0 ? fallbackCommands : [command], - hasSubshells: command.includes("$(") || command.includes("`"), - subshellCommands: [], - } - } -} - -/** - * Extract command patterns for permission management. - * This is a simplified version that focuses on extracting - * the main command and its subcommands for pattern matching. - * - * @param command - The command string to extract patterns from - * @returns Array of command patterns + * Extract command patterns from a command string. + * Returns at most 3 levels: base command, command + first argument, and command + first two arguments. + * Stops at flags (-), paths (/\~), file extensions (.ext), or special characters (:). */ export function extractPatternsFromCommand(command: string): string[] { if (!command?.trim()) return [] - // First, remove subshells for security - we don't want to extract patterns from subshell contents - const cleanedCommand = command - .replace(/\$\([^)]*\)/g, "") // Remove $() subshells - .replace(/`[^`]*`/g, "") // Remove backtick subshells - const patterns = new Set() - const parsed = parse(cleanedCommand) as ShellToken[] - const commandSeparators = new Set(["|", "&&", "||", ";"]) - let current: string[] = [] + try { + const parsed = parse(command) + const commandSeparators = new Set(["|", "&&", "||", ";"]) + let currentTokens: string[] = [] - for (const token of parsed) { - if (typeof token === "object" && "op" in token && commandSeparators.has(token.op)) { - if (current.length) processCommandForPatterns(current, patterns) - current = [] - } else { - current.push(String(token)) + for (const token of parsed) { + if (typeof token === "object" && "op" in token && commandSeparators.has(token.op)) { + // Process accumulated tokens as a command + if (currentTokens.length > 0) { + extractFromTokens(currentTokens, patterns) + currentTokens = [] + } + } else if (typeof token === "string") { + currentTokens.push(token) + } } - } - if (current.length) processCommandForPatterns(current, patterns) + // Process any remaining tokens + if (currentTokens.length > 0) { + extractFromTokens(currentTokens, patterns) + } + } catch (_error) { + // Fallback: just extract the first word + const firstWord = command.trim().split(/\s+/)[0] + if (firstWord) patterns.add(firstWord) + } return Array.from(patterns).sort() } -/** - * Process a single command to extract patterns - */ -function processCommandForPatterns(cmd: string[], patterns: Set): void { - if (!cmd.length || typeof cmd[0] !== "string") return +function isValidToken(token: string): boolean { + return !!token && !token.match(/[/\\~:]/) && token !== "." && !token.match(/\.\w+$/) +} - const mainCmd = cmd[0] +function extractFromTokens(tokens: string[], patterns: Set): void { + if (tokens.length === 0) return - // Skip if it's just a number (like "0" from "0 total") + const mainCmd = tokens[0] + + // Skip numeric commands like "0" from "0 total" if (/^\d+$/.test(mainCmd)) return - // Skip common output patterns that aren't commands - const skipWords = ["total", "error", "warning", "failed", "success", "done"] - if (skipWords.includes(mainCmd.toLowerCase())) return + // Build patterns progressively up to 3 levels + let pattern = mainCmd + patterns.add(pattern) - patterns.add(mainCmd) - - const breakingExps = [/^-/, /[\\/.~]/] - - for (let i = 1; i < cmd.length; i++) { - const arg = cmd[i] - - if (typeof arg !== "string" || breakingExps.some((re) => re.test(arg))) break - - const pattern = cmd.slice(0, i + 1).join(" ") - patterns.add(pattern) + for (let i = 1; i < Math.min(tokens.length, 3); i++) { + if (isValidToken(tokens[i])) { + pattern += ` ${tokens[i]}` + patterns.add(pattern) + } else { + break // Stop at first invalid token + } } } - -/** - * Security analysis for commands - */ -export interface SecurityWarning { - type: "subshell" | "injection" - message: string -} - -/** - * Detect security issues in a command - * - * @param command - The command to analyze - * @returns Array of security warnings - */ -export function detectCommandSecurityIssues(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 warnings -} diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index 2e69fe8b74..6396eef1a4 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -1,4 +1,4 @@ -import { parseCommandString } from "./command-parser" +import { parse } from "shell-quote" /** * # Command Denylist Feature - Longest Prefix Match Strategy @@ -68,8 +68,40 @@ import { parseCommandString } from "./command-parser" * - Newlines as command separators */ export function parseCommand(command: string): string[] { - const { subCommands } = parseCommandString(command) - return subCommands + if (!command?.trim()) return [] + + try { + const parsed = parse(command) + const commands: string[] = [] + let currentCommand: string[] = [] + + for (const token of parsed) { + 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") { + currentCommand.push(token) + } + } + + // Add any remaining command + if (currentCommand.length > 0) { + commands.push(currentCommand.join(" ")) + } + + return commands + } catch (_error) { + // If shell-quote fails, fall back to simple splitting + return [command] + } } /** diff --git a/webview-ui/src/utils/commandPatterns.ts b/webview-ui/src/utils/commandPatterns.ts index 4ae05b17c8..e68a468b22 100644 --- a/webview-ui/src/utils/commandPatterns.ts +++ b/webview-ui/src/utils/commandPatterns.ts @@ -1,21 +1,14 @@ -import { extractPatternsFromCommand, detectCommandSecurityIssues, SecurityWarning } from "./command-parser" +import { extractPatternsFromCommand } from "./command-parser" export interface CommandPattern { pattern: string description?: string } -// Re-export SecurityWarning type from command-parser -export type { SecurityWarning } - export function extractCommandPatterns(command: string): string[] { return extractPatternsFromCommand(command) } -export function detectSecurityIssues(command: string): SecurityWarning[] { - return detectCommandSecurityIssues(command) -} - /** * Get a human-readable description for a command pattern. * Simply returns the pattern followed by "commands".