From bbef66ca359dea64cfa7a200f4d810ecf8e815e2 Mon Sep 17 00:00:00 2001 From: Soumyajit2288 Date: Sat, 11 Jul 2026 13:00:17 +0530 Subject: [PATCH 1/2] fix: bound recursive command parsing --- src/command-manager.ts | 45 +++++++++++++++++++++++++++--- test/test-command-parser-limits.js | 36 ++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 4 deletions(-) create mode 100644 test/test-command-parser-limits.js diff --git a/src/command-manager.ts b/src/command-manager.ts index a477374e..59c4a38c 100644 --- a/src/command-manager.ts +++ b/src/command-manager.ts @@ -2,14 +2,40 @@ import path from 'path'; import {configManager} from './config-manager.js'; import {capture} from "./utils/capture.js"; +const MAX_COMMAND_PARSE_DEPTH = 32; +const MAX_COMMAND_PARSE_CHARS = 4 * 1024 * 1024; + +class CommandParsingLimitError extends Error { + constructor(limit: 'depth' | 'budget') { + super(`Command parsing ${limit} limit exceeded`); + this.name = 'CommandParsingLimitError'; + } +} + +interface CommandParsingBudget { + remainingChars: number; +} + class CommandManager { getBaseCommand(command: string) { return command.split(' ')[0].toLowerCase().trim(); } - extractCommands(commandString: string): string[] { + extractCommands( + commandString: string, + depth: number = 0, + budget: CommandParsingBudget = { remainingChars: MAX_COMMAND_PARSE_CHARS } + ): string[] { try { + if (depth > MAX_COMMAND_PARSE_DEPTH) { + throw new CommandParsingLimitError('depth'); + } + if (commandString.length > budget.remainingChars) { + throw new CommandParsingLimitError('budget'); + } + budget.remainingChars -= commandString.length; + // Trim any leading/trailing whitespace commandString = commandString.trim(); @@ -67,7 +93,7 @@ class CommandManager { } if (j <= commandString.length && openParens === 0) { const subContent = commandString.substring(i + 2, j - 1); - const subCommands = this.extractCommands(subContent); + const subCommands = this.extractCommands(subContent, depth + 1, budget); commands.push(...subCommands); i = j - 1; if (!inQuote) { @@ -88,7 +114,7 @@ class CommandManager { } if (j < commandString.length) { const subContent = commandString.substring(i + 1, j); - const subCommands = this.extractCommands(subContent); + const subCommands = this.extractCommands(subContent, depth + 1, budget); commands.push(...subCommands); i = j; if (!inQuote) { @@ -121,7 +147,7 @@ class CommandManager { if (j <= commandString.length && openParens === 0) { const subshellContent = commandString.substring(i + 1, j - 1); // Recursively extract commands from the subshell - const subCommands = this.extractCommands(subshellContent); + const subCommands = this.extractCommands(subshellContent, depth + 1, budget); commands.push(...subCommands); // Move position past the subshell @@ -162,6 +188,14 @@ class CommandManager { // Remove duplicates and return return [...new Set(commands)]; } catch (error) { + if (error instanceof CommandParsingLimitError) { + if (depth === 0) { + capture('command_parser_limit_exceeded', { + error: error.message + }); + } + throw error; + } // If anything goes wrong, log the error but return the basic command to not break execution capture('server_request_error', { error: 'Error extracting commands' @@ -251,6 +285,9 @@ class CommandManager { // No commands were blocked return true; } catch (error) { + if (error instanceof CommandParsingLimitError) { + return false; + } console.error('Error validating command:', error); capture('server_validate_command_error', { error: error instanceof Error ? error.message : String(error) diff --git a/test/test-command-parser-limits.js b/test/test-command-parser-limits.js new file mode 100644 index 00000000..a6818270 --- /dev/null +++ b/test/test-command-parser-limits.js @@ -0,0 +1,36 @@ +import assert from 'assert'; +import { commandManager } from '../dist/command-manager.js'; + +async function run() { + const nestedCommand = `${'$('.repeat(8)}echo safe${')'.repeat(8)}`; + assert.deepStrictEqual(commandManager.extractCommands(nestedCommand), ['echo']); + + const excessiveDepth = `${'$('.repeat(64)}echo safe${')'.repeat(64)}`; + assert.throws( + () => commandManager.extractCommands(excessiveDepth), + /depth limit exceeded/ + ); + assert.strictEqual( + await commandManager.validateCommand(excessiveDepth), + false, + 'commands that exceed parser depth must fail closed' + ); + + const oversizedNestedCommand = `$(${`x`.repeat(4 * 1024 * 1024)})`; + assert.throws( + () => commandManager.extractCommands(oversizedNestedCommand), + /budget limit exceeded/ + ); + assert.strictEqual( + await commandManager.validateCommand(oversizedNestedCommand), + false, + 'commands that exceed the parsing budget must fail closed' + ); + + console.log('PASS: command parser depth and work limits fail closed'); +} + +run().catch(error => { + console.error(`FAIL: ${error.stack || error.message}`); + process.exit(1); +}); From 6b4ee0e95c4990977a32eeb1813f8f398f5d6945 Mon Sep 17 00:00:00 2001 From: Soumyajit2288 Date: Sat, 11 Jul 2026 13:14:47 +0530 Subject: [PATCH 2/2] fix: close command parser bypasses --- src/command-manager.ts | 48 +++++++++++++++++++++++++++--- test/test-command-parser-limits.js | 16 ++++++++++ 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/src/command-manager.ts b/src/command-manager.ts index 59c4a38c..aba75d0b 100644 --- a/src/command-manager.ts +++ b/src/command-manager.ts @@ -12,6 +12,13 @@ class CommandParsingLimitError extends Error { } } +class UnsafeDynamicCommandError extends Error { + constructor() { + super('Dynamic shell expansion cannot be validated as an executable'); + this.name = 'UnsafeDynamicCommandError'; + } +} + interface CommandParsingBudget { remainingChars: number; } @@ -40,7 +47,7 @@ class CommandManager { commandString = commandString.trim(); // Define command separators - these are the operators that can chain commands - const separators = [';', '&&', '||', '|', '&']; + const separators = ['\r\n', '\n', '\r', ';', '&&', '||', '|', '&']; // This will store our extracted commands const commands: string[] = []; @@ -132,6 +139,24 @@ class CommandManager { continue; } + // Process substitutions execute their contents in a subshell. + if ((char === '<' || char === '>') && commandString[i + 1] === '(') { + let openParens = 1; + let j = i + 2; + while (j < commandString.length && openParens > 0) { + if (commandString[j] === '(') openParens++; + if (commandString[j] === ')') openParens--; + j++; + } + if (openParens === 0) { + const subContent = commandString.substring(i + 2, j - 1); + const subCommands = this.extractCommands(subContent, depth + 1, budget); + commands.push(...subCommands); + i = j - 1; + continue; + } + } + // Handle subshells - if we see an opening parenthesis, we need to find its matching closing parenthesis if (char === '(') { // Find the matching closing parenthesis @@ -188,9 +213,11 @@ class CommandManager { // Remove duplicates and return return [...new Set(commands)]; } catch (error) { - if (error instanceof CommandParsingLimitError) { + if (error instanceof CommandParsingLimitError || error instanceof UnsafeDynamicCommandError) { if (depth === 0) { - capture('command_parser_limit_exceeded', { + capture(error instanceof CommandParsingLimitError + ? 'command_parser_limit_exceeded' + : 'command_parser_dynamic_executable_rejected', { error: error.message }); } @@ -221,6 +248,10 @@ class CommandManager { // Find the first valid token (skip variables) for (let i = 0; i < tokens.length; i++) { const token = tokens[i]; + + if (token.startsWith('${')) { + throw new UnsafeDynamicCommandError(); + } // Skip dollar-prefixed tokens (variables) but not $() command substitutions if (token.startsWith('$') && !token.startsWith('$(')) { @@ -241,6 +272,12 @@ class CommandManager { return null; } + // The executable cannot be known until shell expansion. Reject it + // instead of skipping the token and validating a later argument. + if (firstToken.startsWith('${')) { + throw new UnsafeDynamicCommandError(); + } + // handle $() command substitution - extract the inner command if (firstToken.startsWith('$(') && firstToken.endsWith(')')) { const inner = firstToken.slice(2, -1).trim(); @@ -255,6 +292,9 @@ class CommandManager { const baseName = path.basename(firstToken); return baseName.toLowerCase(); } catch (error) { + if (error instanceof UnsafeDynamicCommandError) { + throw error; + } capture('Error extracting base command'); return null; } @@ -285,7 +325,7 @@ class CommandManager { // No commands were blocked return true; } catch (error) { - if (error instanceof CommandParsingLimitError) { + if (error instanceof CommandParsingLimitError || error instanceof UnsafeDynamicCommandError) { return false; } console.error('Error validating command:', error); diff --git a/test/test-command-parser-limits.js b/test/test-command-parser-limits.js index a6818270..dee2197d 100644 --- a/test/test-command-parser-limits.js +++ b/test/test-command-parser-limits.js @@ -27,6 +27,22 @@ async function run() { 'commands that exceed the parsing budget must fail closed' ); + assert.deepStrictEqual( + commandManager.extractCommands('echo safe\nsudo echo blocked'), + ['echo', 'sudo'], + 'newlines must separate shell commands' + ); + assert.deepStrictEqual( + commandManager.extractCommands('cat <(rm /tmp/file)'), + ['rm', 'cat'], + 'process substitutions must be recursively parsed' + ); + assert.strictEqual( + await commandManager.validateCommand('${SUDO:-sudo} echo blocked'), + false, + 'dynamic executable expansion must fail closed' + ); + console.log('PASS: command parser depth and work limits fail closed'); }