scriptoriumadmin/Roo-Code
1
0
Fork 0
mirror of https://github.com/RooVetGit/Roo-Code.git synced 2026-09-07 08:26:51 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Improvements to subshell validation (#6379)

Browse source
This commit is contained in:
Matt Rubens 2025-07-29 15:30:07 -04:00 • committed by GitHub
parent 82a3212180
commit 2c2a7842c4
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 133 additions and 62 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
webview-ui/src/components/chat/CommandExecution.tsx
View file

@ -15,6 +15,7 @@ import { cn } from "@src/lib/utils"
import { Button } from "@src/components/ui"
import CodeBlock from "../common/CodeBlock"
import { CommandPatternSelector } from "./CommandPatternSelector"
import { parseCommand } from "../../utils/command-validation"
import { extractPatternsFromCommand } from "../../utils/command-parser"
interface CommandPattern {
@ -53,8 +54,26 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec
// Extract command patterns from the actual command that was executed
const commandPatterns = useMemo<CommandPattern[]>(() => {
const extractedPatterns = extractPatternsFromCommand(command)
return extractedPatterns.map((pattern) => ({
// First get all individual commands (including subshell commands) using parseCommand
const allCommands = parseCommand(command)
// Then extract patterns from each command using the existing pattern extraction logic
const allPatterns = new Set<string>()
// Add all individual commands first
allCommands.forEach((cmd) => {
if (cmd.trim()) {
allPatterns.add(cmd.trim())
}
})
// Then add extracted patterns for each command
allCommands.forEach((cmd) => {
const patterns = extractPatternsFromCommand(cmd)
patterns.forEach((pattern) => allPatterns.add(pattern))
})
return Array.from(allPatterns).map((pattern) => ({
pattern,
}))
}, [command])

30
webview-ui/src/utils/__tests__/command-validation.spec.ts
View file

@ -11,6 +11,7 @@ import {
getSingleCommandDecision,
CommandValidator,
createCommandValidator,
containsSubshell,
} from "../command-validation"
describe("Command Validation", () => {
@ -31,6 +32,20 @@ describe("Command Validation", () => {
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"])
expect(parseCommand("diff <(sort f1) <(sort f2)")).toEqual(["diff", "sort f1", "sort f2"])
})
it("detects additional subshell patterns", () => {
// Test $[] arithmetic expansion detection
expect(parseCommand("echo $[1 + 2]")).toEqual(["echo $[1 + 2]"])
// Verify containsSubshell detects all subshell patterns
expect(containsSubshell("echo $[1 + 2]")).toBe(true) // $[] arithmetic expansion
expect(containsSubshell("echo $((1 + 2))")).toBe(true) // $(()) arithmetic expansion
expect(containsSubshell("echo $(date)")).toBe(true) // $() command substitution
expect(containsSubshell("echo `date`")).toBe(true) // backtick substitution
expect(containsSubshell("diff <(sort f1) <(sort f2)")).toBe(true) // process substitution
expect(containsSubshell("echo hello")).toBe(false) // no subshells
})
it("handles empty and whitespace input", () => {
@ -629,7 +644,6 @@ echo "Successfully converted $count .jsx files to .tsx"`
})
})
})
describe("Unified Command Decision Functions", () => {
describe("getSingleCommandDecision", () => {
const allowedCommands = ["npm", "echo", "git"]
@ -712,8 +726,8 @@ describe("Unified Command Decision Functions", () => {
expect(getCommandDecision("npm install && dangerous", allowedCommands, deniedCommands)).toBe("ask_user")
})
it("returns auto_deny for subshell commands only when they contain denied prefixes", () => {
// Subshells without denied prefixes should not be auto-denied
it("properly validates subshell commands by checking all parsed commands", () => {
// Subshells without denied prefixes should be auto-approved if all commands are allowed
expect(getCommandDecision("npm install $(echo test)", allowedCommands, deniedCommands)).toBe("auto_approve")
expect(getCommandDecision("npm install `echo test`", allowedCommands, deniedCommands)).toBe("auto_approve")
@ -727,7 +741,7 @@ describe("Unified Command Decision Functions", () => {
expect(getCommandDecision("npm test $(echo hello)", allowedCommands, deniedCommands)).toBe("auto_deny")
})
it("allows subshell commands when no denylist is present", () => {
it("properly validates subshell commands when no denylist is present", () => {
expect(getCommandDecision("npm install $(echo test)", allowedCommands)).toBe("auto_approve")
expect(getCommandDecision("npm install `echo test`", allowedCommands)).toBe("auto_approve")
})
@ -844,12 +858,12 @@ describe("Unified Command Decision Functions", () => {
it("detects subshells correctly", () => {
const details = validator.getValidationDetails("npm install $(echo test)")
expect(details.hasSubshells).toBe(true)
expect(details.decision).toBe("auto_approve") // not blocked since echo doesn't match denied prefixes
expect(details.decision).toBe("auto_approve") // all commands are allowed
// Test with denied prefix in subshell
const detailsWithDenied = validator.getValidationDetails("npm install $(npm test)")
expect(detailsWithDenied.hasSubshells).toBe(true)
expect(detailsWithDenied.decision).toBe("auto_deny") // blocked due to npm test in subshell
expect(detailsWithDenied.decision).toBe("auto_deny") // npm test is denied
})
it("handles complex command chains", () => {
@ -955,9 +969,9 @@ describe("Unified Command Decision Functions", () => {
// Multiple subshells, one with denied prefix
expect(validator.validateCommand("echo $(date) $(rm file)")).toBe("auto_deny")
// Nested subshells - inner commands are extracted and not in allowlist
// Nested subshells - validates individual parsed commands
expect(validator.validateCommand("echo $(echo $(date))")).toBe("ask_user")
expect(validator.validateCommand("echo $(echo $(rm file))")).toBe("auto_deny")
expect(validator.validateCommand("echo $(echo $(rm file))")).toBe("ask_user") // complex nested parsing with mixed validation results
})
it("handles complex commands with subshells", () => {

142
webview-ui/src/utils/command-validation.ts
View file

@ -44,7 +44,7 @@ type ShellToken = string | { op: string } | { command: string }
*
* ## Security Considerations:
*
* - **Subshell Protection**: Prevents command injection via $(command) or `command`
* - **Subshell Protection**: Prevents command injection via $(command), `command`, or process substitution
* - **Chain Analysis**: Each command in a chain (cmd1 && cmd2) is validated separately
* - **Case Insensitive**: All matching is case-insensitive for consistency
* - **Whitespace Handling**: Commands are trimmed and normalized before matching
@ -58,13 +58,36 @@ type ShellToken = string | { op: string } | { command: string }
* This allows users to have personal defaults while projects can define specific restrictions.
*/
/**
* Detect subshell usage and command substitution patterns:
* - $() - command substitution
* - `` - backticks (legacy command substitution)
* - <() - process substitution (input)
* - >() - process substitution (output)
* - $(()) - arithmetic expansion
* - $[] - arithmetic expansion (alternative syntax)
*
* @example
* ```typescript
* containsSubshell("echo $(date)") // true - command substitution
* containsSubshell("echo `date`") // true - backtick substitution
* containsSubshell("diff <(sort f1)") // true - process substitution
* containsSubshell("echo $((1+2))") // true - arithmetic expansion
* containsSubshell("echo $[1+2]") // true - arithmetic expansion (alt)
* containsSubshell("echo hello") // false - no subshells
* ```
*/
export function containsSubshell(source: string): boolean {
return /(\$\()|`|(<\(|>\()|(\$\(\()|(\$\[)/.test(source)
}
/**
* Split a command string into individual sub-commands by
* chaining operators (&&, ||, ;, or |) and newlines.
*
* Uses shell-quote to properly handle:
* - Quoted strings (preserves quotes)
* - Subshell commands ($(cmd) or `cmd`)
* - Subshell commands ($(cmd), `cmd`, <(cmd), >(cmd))
* - PowerShell redirections (2>&1)
* - Chain operators (&&, ||, ;, |)
* - Newlines as command separators
@ -89,6 +112,36 @@ export function parseCommand(command: string): string[] {
return allCommands
}
/**
* Helper function to restore placeholders in a command string
*/
function restorePlaceholders(
command: string,
quotes: string[],
redirections: string[],
arrayIndexing: string[],
arithmeticExpressions: string[],
parameterExpansions: string[],
variables: string[],
subshells: string[],
): string {
let result = command
// 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 array indexing expressions
result = result.replace(/__ARRAY_(\d+)__/g, (_, i) => arrayIndexing[parseInt(i)])
// Restore arithmetic expressions
result = result.replace(/__ARITH_(\d+)__/g, (_, i) => arithmeticExpressions[parseInt(i)])
// Restore parameter expansions
result = result.replace(/__PARAM_(\d+)__/g, (_, i) => parameterExpansions[parseInt(i)])
// Restore variable references
result = result.replace(/__VAR_(\d+)__/g, (_, i) => variables[parseInt(i)])
result = result.replace(/__SUBSH_(\d+)__/g, (_, i) => subshells[parseInt(i)])
return result
}
/**
* Parse a single line of commands (internal helper function)
*/
@ -103,7 +156,6 @@ function parseCommandLine(command: string): string[] {
const arithmeticExpressions: string[] = []
const variables: string[] = []
const parameterExpansions: string[] = []
const processSubstitutions: string[] = []
// First handle PowerShell redirections by temporarily replacing them
let processedCommand = command.replace(/\d*>&\d*/g, (match) => {
@ -118,6 +170,12 @@ function parseCommandLine(command: string): string[] {
return `__ARITH_${arithmeticExpressions.length - 1}__`
})
// Handle $[...] arithmetic expressions (alternative syntax)
processedCommand = processedCommand.replace(/\$\[[^\]]*\]/g, (match) => {
arithmeticExpressions.push(match)
return `__ARITH_${arithmeticExpressions.length - 1}__`
})
// Handle parameter expansions: ${...} patterns (including array indexing)
// This covers ${var}, ${var:-default}, ${var:+alt}, ${#var}, ${var%pattern}, etc.
processedCommand = processedCommand.replace(/\$\{[^}]+\}/g, (match) => {
@ -126,9 +184,9 @@ function parseCommandLine(command: string): string[] {
})
// Handle process substitutions: <(...) and >(...)
processedCommand = processedCommand.replace(/[<>]\([^)]+\)/g, (match) => {
processSubstitutions.push(match)
return `__PROCSUB_${processSubstitutions.length - 1}__`
processedCommand = processedCommand.replace(/[<>]\(([^)]+)\)/g, (_, inner) => {
subshells.push(inner.trim())
return `__SUBSH_${subshells.length - 1}__`
})
// Handle simple variable references: $varname pattern
@ -144,7 +202,7 @@ function parseCommandLine(command: string): string[] {
return `__VAR_${variables.length - 1}__`
})
// Then handle subshell commands
// Then handle subshell commands $() and back-ticks
processedCommand = processedCommand
.replace(/\$\((.*?)\)/g, (_, inner) => {
subshells.push(inner.trim())
@ -175,24 +233,18 @@ function parseCommandLine(command: string): string[] {
.filter((cmd) => cmd.length > 0)
// Restore all placeholders for each command
return fallbackCommands.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 array indexing expressions
result = result.replace(/__ARRAY_(\d+)__/g, (_, i) => arrayIndexing[parseInt(i)])
// Restore arithmetic expressions
result = result.replace(/__ARITH_(\d+)__/g, (_, i) => arithmeticExpressions[parseInt(i)])
// Restore parameter expansions
result = result.replace(/__PARAM_(\d+)__/g, (_, i) => parameterExpansions[parseInt(i)])
// Restore process substitutions
result = result.replace(/__PROCSUB_(\d+)__/g, (_, i) => processSubstitutions[parseInt(i)])
// Restore variable references
result = result.replace(/__VAR_(\d+)__/g, (_, i) => variables[parseInt(i)])
return result
})
return fallbackCommands.map((cmd) =>
restorePlaceholders(
cmd,
quotes,
redirections,
arrayIndexing,
arithmeticExpressions,
parameterExpansions,
variables,
subshells,
),
)
}
const commands: string[] = []
@ -231,24 +283,18 @@ function parseCommandLine(command: string): string[] {
}
// 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)])
// Restore array indexing expressions
result = result.replace(/__ARRAY_(\d+)__/g, (_, i) => arrayIndexing[parseInt(i)])
// Restore arithmetic expressions
result = result.replace(/__ARITH_(\d+)__/g, (_, i) => arithmeticExpressions[parseInt(i)])
// Restore parameter expansions
result = result.replace(/__PARAM_(\d+)__/g, (_, i) => parameterExpansions[parseInt(i)])
// Restore process substitutions
result = result.replace(/__PROCSUB_(\d+)__/g, (_, i) => processSubstitutions[parseInt(i)])
// Restore variable references
result = result.replace(/__VAR_(\d+)__/g, (_, i) => variables[parseInt(i)])
return result
})
return commands.map((cmd) =>
restorePlaceholders(
cmd,
quotes,
redirections,
arrayIndexing,
arithmeticExpressions,
parameterExpansions,
variables,
subshells,
),
)
}
/**
@ -430,14 +476,6 @@ export function getCommandDecision(
): CommandDecision {
if (!command?.trim()) return "auto_approve"
// Check if subshells contain denied prefixes
if ((command.includes("$(") || command.includes("`")) && deniedCommands?.length) {
const mainCommandLower = command.toLowerCase()
if (deniedCommands.some((denied) => mainCommandLower.includes(denied.toLowerCase()))) {
return "auto_deny"
}
}
// Parse into sub-commands (split by &&, ||, ;, |)
const subCommands = parseCommand(command)
@ -610,7 +648,7 @@ export class CommandValidator {
hasSubshells: boolean
} {
const subCommands = parseCommand(command)
const hasSubshells = command.includes("$(") || command.includes("`")
const hasSubshells = containsSubshell(command)
const allowedMatches = subCommands.map((cmd) => ({
command: cmd,