From a1402d4347823aac624c80ec997f542e53016461 Mon Sep 17 00:00:00 2001 From: pragnyanramtha Date: Sun, 17 May 2026 03:46:48 +0000 Subject: [PATCH] Fix terminal substitution quoting checks --- .../src/evaluateTerminalCommandSecurity.ts | 168 ++++++++++++------ .../test/terminalCommandSecurity.test.ts | 38 +++- 2 files changed, 153 insertions(+), 53 deletions(-) diff --git a/packages/terminal-security/src/evaluateTerminalCommandSecurity.ts b/packages/terminal-security/src/evaluateTerminalCommandSecurity.ts index 693eef25f9c..ee69c0a7c13 100644 --- a/packages/terminal-security/src/evaluateTerminalCommandSecurity.ts +++ b/packages/terminal-security/src/evaluateTerminalCommandSecurity.ts @@ -19,6 +19,11 @@ interface CommentToken { type ParsedToken = string | ShellOperator | GlobPattern | CommentToken; +interface CommandSubstitutionScan { + hasSubstitution: boolean; + commands: string[]; +} + /** * Evaluates the security policy for a terminal command. * @@ -244,9 +249,9 @@ function evaluateTokens( // Also check for command substitution in the original command // (shell-quote doesn't parse these as separate tokens) - if (hasCommandSubstitution(originalCommand)) { - const substitutedCommands = extractSubstitutedCommands(originalCommand); - for (const subCmd of substitutedCommands) { + const commandSubstitutions = scanCommandSubstitutions(originalCommand); + if (commandSubstitutions.hasSubstitution) { + for (const subCmd of commandSubstitutions.commands) { const nestedPolicy = evaluateTerminalCommandSecurity(basePolicy, subCmd); mostRestrictivePolicy = getMostRestrictive( mostRestrictivePolicy, @@ -1095,68 +1100,131 @@ function isSafeCommand(baseCommand: string, args: string[]): boolean { } /** - * Checks for command substitution patterns + * Scans shell text for command substitution syntax that can execute nested + * commands. Single-quoted text is literal shell text, while substitutions + * remain active in double-quoted text. */ -function hasCommandSubstitution(command: string): boolean { - // Backticks - if (command.includes("`")) { - return true; - } +function scanCommandSubstitutions(command: string): CommandSubstitutionScan { + const commands: string[] = []; + let hasSubstitution = false; + let inSingleQuote = false; + let inDoubleQuote = false; - // $() substitution - if (command.includes("$(")) { - return true; + for (let idx = 0; idx < command.length; idx++) { + const char = command[idx]; + const nextChar = command[idx + 1]; + + if (!inSingleQuote && char === "\\") { + idx++; + continue; + } + + if (!inDoubleQuote && char === "'") { + inSingleQuote = !inSingleQuote; + continue; + } + + if (!inSingleQuote && char === '"') { + inDoubleQuote = !inDoubleQuote; + continue; + } + + if (inSingleQuote) { + continue; + } + + if (char === "`") { + hasSubstitution = true; + const endIdx = findClosingBacktick(command, idx + 1); + if (endIdx !== -1) { + commands.push(command.slice(idx + 1, endIdx)); + idx = endIdx; + } + continue; + } + + if ( + (char === "$" || (!inDoubleQuote && (char === "<" || char === ">"))) && + nextChar === "(" + ) { + hasSubstitution = true; + const endIdx = findClosingParen(command, idx + 2); + if (endIdx !== -1) { + commands.push(command.slice(idx + 2, endIdx)); + idx = endIdx; + } + } } - // Process substitution <() or >() - if (command.match(/<\(|>\(/)) { - return true; + return { hasSubstitution, commands }; +} + +function findClosingBacktick(command: string, startIdx: number): number { + for (let idx = startIdx; idx < command.length; idx++) { + if (command[idx] === "\\") { + idx++; + continue; + } + if (command[idx] === "`") { + return idx; + } } - return false; + return -1; } -/** - * Extracts commands from substitution patterns - */ -function extractSubstitutedCommands(command: string): string[] { - const commands: string[] = []; +function findClosingParen(command: string, startIdx: number): number { + let depth = 1; + let inSingleQuote = false; + let inDoubleQuote = false; + + for (let idx = startIdx; idx < command.length; idx++) { + const char = command[idx]; + const nextChar = command[idx + 1]; - // Extract from backticks - const backtickMatches = command.match(/`([^`]+)`/g); - if (backtickMatches) { - commands.push(...backtickMatches.map((m) => m.slice(1, -1))); - } - - // Extract from $() - handle nested parentheses - let idx = 0; - while (idx < command.length) { - const dollarParen = command.indexOf("$(", idx); - if (dollarParen === -1) break; - - // Find matching closing parenthesis - let depth = 1; - let endIdx = dollarParen + 2; - while (endIdx < command.length && depth > 0) { - if (command[endIdx] === "(") depth++; - else if (command[endIdx] === ")") depth--; - endIdx++; + if (!inSingleQuote && char === "\\") { + idx++; + continue; } - if (depth === 0) { - // Extract the command inside $() - const extracted = command.slice(dollarParen + 2, endIdx - 1); - commands.push(extracted); - // Also check for nested substitutions within this command - if (extracted.includes("$(") || extracted.includes("`")) { - commands.push(...extractSubstitutedCommands(extracted)); - } + if (!inDoubleQuote && char === "'") { + inSingleQuote = !inSingleQuote; + continue; + } + + if (!inSingleQuote && char === '"') { + inDoubleQuote = !inDoubleQuote; + continue; + } + + if (inSingleQuote) { + continue; + } + + if ( + (char === "$" || (!inDoubleQuote && (char === "<" || char === ">"))) && + nextChar === "(" + ) { + depth++; + idx++; + continue; } - idx = endIdx; + if (inDoubleQuote) { + continue; + } + + if (char === "(") { + depth++; + } else if (char === ")") { + depth--; + if (depth === 0) { + return idx; + } + } } - return commands; + return -1; } /** diff --git a/packages/terminal-security/test/terminalCommandSecurity.test.ts b/packages/terminal-security/test/terminalCommandSecurity.test.ts index 7d2e484765e..1e411529f6a 100644 --- a/packages/terminal-security/test/terminalCommandSecurity.test.ts +++ b/packages/terminal-security/test/terminalCommandSecurity.test.ts @@ -959,12 +959,12 @@ describe("evaluateTerminalCommandSecurity", () => { expect(result).toBe("disabled"); }); - it("should allow backticks in quotes", () => { + it("should allow backticks in single quotes", () => { const result = evaluateTerminalCommandSecurity( - "allowedWithPermission", + "allowedWithoutPermission", "echo 'This is a `backtick` in quotes'", ); - expect(result).toBe("allowedWithPermission"); + expect(result).toBe("allowedWithoutPermission"); }); it("should detect command substitution with $()", () => { @@ -975,6 +975,22 @@ describe("evaluateTerminalCommandSecurity", () => { expect(result).toBe("disabled"); }); + it("should allow $() text in single quotes", () => { + const result = evaluateTerminalCommandSecurity( + "allowedWithoutPermission", + "echo 'literal $(name)'", + ); + expect(result).toBe("allowedWithoutPermission"); + }); + + it("should detect command substitution in double quotes", () => { + const result = evaluateTerminalCommandSecurity( + "allowedWithoutPermission", + 'echo "$(sudo cat /etc/shadow)"', + ); + expect(result).toBe("disabled"); + }); + it("should detect nested command substitution", () => { const result = evaluateTerminalCommandSecurity( "allowedWithoutPermission", @@ -990,6 +1006,22 @@ describe("evaluateTerminalCommandSecurity", () => { ); expect(result).toBe("allowedWithPermission"); }); + + it("should detect dangerous commands inside process substitution", () => { + const result = evaluateTerminalCommandSecurity( + "allowedWithoutPermission", + "cat <(sudo cat /etc/shadow)", + ); + expect(result).toBe("disabled"); + }); + + it("should allow process substitution text in double quotes", () => { + const result = evaluateTerminalCommandSecurity( + "allowedWithoutPermission", + 'echo "<(date)"', + ); + expect(result).toBe("allowedWithoutPermission"); + }); }); describe("Path Traversal and Directory Navigation", () => {