fix: harden recursive command parsing

src/command-manager.ts

@@ -2,19 +2,52 @@ 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';
+    }
+}
+
+class UnsafeDynamicCommandError extends Error {
+    constructor() {
+        super('Dynamic shell expansion cannot be validated as an executable');
+        this.name = 'UnsafeDynamicCommandError';
+    }
+}
+
+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();
 
             // 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[] = [];
@@ -67,7 +100,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 +121,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) {
@@ -106,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
@@ -121,7 +172,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 +213,16 @@ class CommandManager {
             // Remove duplicates and return
             return [...new Set(commands)];
         } catch (error) {
+            if (error instanceof CommandParsingLimitError || error instanceof UnsafeDynamicCommandError) {
+                if (depth === 0) {
+                    capture(error instanceof CommandParsingLimitError
+                        ? 'command_parser_limit_exceeded'
+                        : 'command_parser_dynamic_executable_rejected', {
+                        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'
@@ -187,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('$(')) {
@@ -207,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();
@@ -221,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;
         }
@@ -251,6 +325,9 @@ class CommandManager {
             // No commands were blocked
             return true;
         } catch (error) {
+            if (error instanceof CommandParsingLimitError || error instanceof UnsafeDynamicCommandError) {
+                return false;
+            }
             console.error('Error validating command:', error);
             capture('server_validate_command_error', {
                 error: error instanceof Error ? error.message : String(error)

test/test-command-parser-limits.js

@@ -0,0 +1,52 @@
+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'
+  );
+
+  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');
+}
+
+run().catch(error => {
+  console.error(`FAIL: ${error.stack || error.message}`);
+  process.exit(1);
+});