From 58bff0e380c063e9074c8068abf6b6e806510c50 Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Mon, 12 May 2025 20:09:02 -0700 Subject: [PATCH 1/2] fix: command validation failing on shell array indexing Previously command validation would fail with 'Bad substitution' error when encountering shell array indexing expressions like ${array[var]}. Added parsing support for array indexing expressions before they reach shell-quote parser to prevent the error. This allows proper validation of shell commands that use array indexing. Test cases added to verify handling of: - Simple array indexing - Array indexing with variables - Complex array indexing expressions Disabled ESLint rules no-template-curly-in-string and no-useless-escape specifically for command-validation test file since they conflict with shell command syntax testing. Fixes: #3529 Signed-off-by: Eric Wheeler --- webview-ui/.eslintrc.json | 11 +- .../utils/command-validation.test.ts | 158 ++++++++++++++++++ webview-ui/src/utils/command-validation.ts | 17 +- 3 files changed, 182 insertions(+), 4 deletions(-) create mode 100644 webview-ui/src/__tests__/utils/command-validation.test.ts diff --git a/webview-ui/.eslintrc.json b/webview-ui/.eslintrc.json index 8ca877d87cc..48d548b4d83 100644 --- a/webview-ui/.eslintrc.json +++ b/webview-ui/.eslintrc.json @@ -4,5 +4,14 @@ "rules": { "no-unused-vars": "off", "@typescript-eslint/no-unused-vars": ["error", { "varsIgnorePattern": "^_", "argsIgnorePattern": "^_" }] - } + }, + "overrides": [ + { + "files": ["webview-ui/src/__tests__/utils/command-validation.test.ts"], + "rules": { + "no-template-curly-in-string": "off", + "no-useless-escape": "off" + } + } + ] } diff --git a/webview-ui/src/__tests__/utils/command-validation.test.ts b/webview-ui/src/__tests__/utils/command-validation.test.ts new file mode 100644 index 00000000000..f9e01eee6c2 --- /dev/null +++ b/webview-ui/src/__tests__/utils/command-validation.test.ts @@ -0,0 +1,158 @@ +import { parseCommand, validateCommand } from "../../utils/command-validation" + +/** + * Tests for the command-validation module + * + * These tests include a reproduction of a bug where the shell-quote library + * used in parseCommand throws an error when parsing commands that contain + * the Bash $RANDOM variable in array indexing expressions. + * + * Error: "Bad substitution: levels[$RANDOM" + * + * The issue occurs specifically with complex expressions like: + * ${levels[$RANDOM % ${#levels[@]}]} + */ +describe("command-validation", () => { + describe("parseCommand", () => { + it("should correctly parse simple commands", () => { + const result = parseCommand("echo hello") + expect(result).toEqual(["echo hello"]) + }) + + it("should correctly parse commands with chaining operators", () => { + const result = parseCommand("echo hello && echo world") + expect(result).toEqual(["echo hello", "echo world"]) + }) + + it("should correctly parse commands with quotes", () => { + const result = parseCommand('echo "hello world"') + expect(result).toEqual(['echo "hello world"']) + }) + + it("should correctly parse commands with redirections", () => { + const result = parseCommand("echo hello 2>&1") + expect(result).toEqual(["echo hello 2>&1"]) + }) + + it("should correctly parse commands with subshells", () => { + const result = parseCommand("echo $(date)") + expect(result).toEqual(["echo", "date"]) + }) + + it("should not throw an error when parsing commands with simple array indexing", () => { + // Simple array indexing works fine + const commandWithArrayIndex = "value=${array[$index]}" + + expect(() => { + parseCommand(commandWithArrayIndex) + }).not.toThrow() + }) + + it("should throw an error when parsing commands with $RANDOM in array index", () => { + // This test reproduces the specific bug reported in the error + const commandWithRandom = "level=${levels[$RANDOM % ${#levels[@]}]}" + + expect(() => { + parseCommand(commandWithRandom) + }).not.toThrow() + }) + + it("should not throw an error with simple $RANDOM variable", () => { + // Simple $RANDOM usage works fine + const commandWithRandom = "echo $RANDOM" + + expect(() => { + parseCommand(commandWithRandom) + }).not.toThrow() + }) + + it("should not throw an error with simple array indexing using $RANDOM", () => { + // Simple array indexing with $RANDOM works fine + const commandWithRandomIndex = "echo ${array[$RANDOM]}" + + expect(() => { + parseCommand(commandWithRandomIndex) + }).not.toThrow() + }) + + it("should throw an error with complex array indexing using $RANDOM and arithmetic", () => { + // This test reproduces the exact expression from the original error + const commandWithComplexRandom = "echo ${levels[$RANDOM % ${#levels[@]}]}" + + expect(() => { + parseCommand(commandWithComplexRandom) + }).not.toThrow("Bad substitution") + }) + + it("should throw an error when parsing the full log generator command", () => { + // This is the exact command from the original error message + const logGeneratorCommand = `while true; do \\ + levels=(INFO WARN ERROR DEBUG); \\ + msgs=("User logged in" "Connection timeout" "Processing request" "Cache miss" "Database query"); \\ + level=\${levels[$RANDOM % \${#levels[@]}]}; \\ + msg=\${msgs[$RANDOM % \${#msgs[@]}]}; \\ + echo "\$(date '+%Y-%m-%d %H:%M:%S') [$level] $msg"; \\ + sleep 1; \\ +done` + + // This reproduces the original error + expect(() => { + parseCommand(logGeneratorCommand) + }).not.toThrow("Bad substitution: levels[$RANDOM") + }) + + it("should throw an error when parsing just the problematic part", () => { + // This isolates just the part mentioned in the error message + const problematicPart = "level=${levels[$RANDOM" + + expect(() => { + parseCommand(problematicPart) + }).not.toThrow("Bad substitution") + }) + }) + + describe("validateCommand", () => { + it("should validate allowed commands", () => { + const result = validateCommand("echo hello", ["echo"]) + expect(result).toBe(true) + }) + + it("should reject disallowed commands", () => { + const result = validateCommand("rm -rf /", ["echo", "ls"]) + expect(result).toBe(false) + }) + + it("should not fail validation for commands with simple $RANDOM variable", () => { + const commandWithRandom = "echo $RANDOM" + + expect(() => { + validateCommand(commandWithRandom, ["echo"]) + }).not.toThrow() + }) + + it("should not fail validation for commands with simple array indexing using $RANDOM", () => { + const commandWithRandomIndex = "echo ${array[$RANDOM]}" + + expect(() => { + validateCommand(commandWithRandomIndex, ["echo"]) + }).not.toThrow() + }) + + it("should return false for the full log generator command due to subshell detection", () => { + // This is the exact command from the original error message + const logGeneratorCommand = `while true; do \\ + levels=(INFO WARN ERROR DEBUG); \\ + msgs=("User logged in" "Connection timeout" "Processing request" "Cache miss" "Database query"); \\ + level=\${levels[$RANDOM % \${#levels[@]}]}; \\ + msg=\${msgs[$RANDOM % \${#msgs[@]}]}; \\ + echo "\$(date '+%Y-%m-%d %H:%M:%S') [$level] $msg"; \\ + sleep 1; \\ +done` + + // validateCommand should return false due to subshell detection + // without throwing an error + const result = validateCommand(logGeneratorCommand, ["while"]) + expect(result).toBe(false) + }) + }) +}) diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index 3f8fd593b19..7ad21b1675c 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -15,15 +15,25 @@ type ShellToken = string | { op: string } | { command: string } export function parseCommand(command: string): string[] { if (!command?.trim()) return [] - // First handle PowerShell redirections by temporarily replacing them + // Storage for replaced content const redirections: string[] = [] + const subshells: string[] = [] + const quotes: string[] = [] + const arrayIndexing: string[] = [] + + // First handle PowerShell redirections by temporarily replacing them let processedCommand = command.replace(/\d*>&\d*/g, (match) => { redirections.push(match) return `__REDIR_${redirections.length - 1}__` }) + // Handle array indexing expressions: ${array[...]} pattern and partial expressions + processedCommand = processedCommand.replace(/\$\{[^}]*\[[^\]]*(\]([^}]*\})?)?/g, (match) => { + arrayIndexing.push(match) + return `__ARRAY_${arrayIndexing.length - 1}__` + }) + // Then handle subshell commands - const subshells: string[] = [] processedCommand = processedCommand .replace(/\$\((.*?)\)/g, (_, inner) => { subshells.push(inner.trim()) @@ -35,7 +45,6 @@ export function parseCommand(command: string): string[] { }) // Then handle quoted strings - const quotes: string[] = [] processedCommand = processedCommand.replace(/"[^"]*"/g, (match) => { quotes.push(match) return `__QUOTE_${quotes.length - 1}__` @@ -84,6 +93,8 @@ export function parseCommand(command: string): string[] { 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)]) return result }) } From c27a94918976dd76ba03dcb61f36e769a3a9dd66 Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Mon, 12 May 2025 21:11:11 -0700 Subject: [PATCH 2/2] test: fix command validation test descriptions Update test descriptions to accurately reflect that tests verify errors are prevented rather than thrown. This aligns the descriptions with their .not.toThrow() assertions. Signed-off-by: Eric Wheeler --- webview-ui/src/__tests__/utils/command-validation.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/webview-ui/src/__tests__/utils/command-validation.test.ts b/webview-ui/src/__tests__/utils/command-validation.test.ts index f9e01eee6c2..d1e3fc60101 100644 --- a/webview-ui/src/__tests__/utils/command-validation.test.ts +++ b/webview-ui/src/__tests__/utils/command-validation.test.ts @@ -48,7 +48,7 @@ describe("command-validation", () => { }).not.toThrow() }) - it("should throw an error when parsing commands with $RANDOM in array index", () => { + it("should not throw an error when parsing commands with $RANDOM in array index", () => { // This test reproduces the specific bug reported in the error const commandWithRandom = "level=${levels[$RANDOM % ${#levels[@]}]}" @@ -75,7 +75,7 @@ describe("command-validation", () => { }).not.toThrow() }) - it("should throw an error with complex array indexing using $RANDOM and arithmetic", () => { + it("should not throw an error with complex array indexing using $RANDOM and arithmetic", () => { // This test reproduces the exact expression from the original error const commandWithComplexRandom = "echo ${levels[$RANDOM % ${#levels[@]}]}" @@ -84,7 +84,7 @@ describe("command-validation", () => { }).not.toThrow("Bad substitution") }) - it("should throw an error when parsing the full log generator command", () => { + it("should not throw an error when parsing the full log generator command", () => { // This is the exact command from the original error message const logGeneratorCommand = `while true; do \\ levels=(INFO WARN ERROR DEBUG); \\ @@ -101,7 +101,7 @@ done` }).not.toThrow("Bad substitution: levels[$RANDOM") }) - it("should throw an error when parsing just the problematic part", () => { + it("should not throw an error when parsing just the problematic part", () => { // This isolates just the part mentioned in the error message const problematicPart = "level=${levels[$RANDOM"