Merge pull request #91 from AikidoSec/escape-special-chars-in-shell

Escape special chars in shell scripts

packages/safe-chain/src/utils/safeSpawn.js

@@ -1,22 +1,77 @@
-import { spawn } from "child_process";
+import { spawn, execSync } from "child_process";
+import os from "os";
 
-function escapeArg(arg) {
-  // If argument contains spaces or quotes, wrap in double quotes and escape double quotes
-  if (arg.includes(" ") || arg.includes('"') || arg.includes("'")) {
-    return '"' + arg.replaceAll('"', '\\"') + '"';
+function sanitizeShellArgument(arg) {
+  // If argument contains shell metacharacters, wrap in double quotes
+  // and escape characters that are special even inside double quotes
+  if (hasShellMetaChars(arg)) {
+    // Inside double quotes, we need to escape: " $ ` \
+    return '"' + escapeDoubleQuoteContent(arg) + '"';
   }
   return arg;
 }
 
+function hasShellMetaChars(arg) {
+  // Shell metacharacters that need escaping
+  // These characters have special meaning in shells and need to be quoted
+  // Whenever one of these characters is present, we should quote the argument
+  // Characters: space, ", &, ', |, ;, <, >, (, ), $, `, \, !, *, ?, [, ], {, }, ~, #
+  const shellMetaChars = /[ "&'|;<>()$`\\!*?[\]{}~#]/;
+  return shellMetaChars.test(arg);
+}
+
+function escapeDoubleQuoteContent(arg) {
+  // Escape special characters for shell safety
+  // This escapes ", $, `, and \ by prefixing them with a backslash
+  return arg.replace(/(["`$\\])/g, "\\$1");
+}
+
 function buildCommand(command, args) {
-  const escapedArgs = args.map(escapeArg);
+  if (args.length === 0) {
+    return command;
+  }
+
+  const escapedArgs = args.map(sanitizeShellArgument);
+
   return `${command} ${escapedArgs.join(" ")}`;
 }
 
+function resolveCommandPath(command) {
+  // command will be "npm", "yarn", etc.
+  // Use 'command -v' to find the full path
+  const fullPath = execSync(`command -v ${command}`, {
+    encoding: "utf8",
+    shell: true,
+  }).trim();
+
+  if (!fullPath) {
+    throw new Error(`Command not found: ${command}`);
+  }
+
+  return fullPath;
+}
+
 export async function safeSpawn(command, args, options = {}) {
-  const fullCommand = buildCommand(command, args);
+  // The command is always one of our supported package managers.
+  // It should always be alphanumeric or _ or -
+  // Reject any command names with suspicious characters
+  if (!/^[a-zA-Z0-9_-]+$/.test(command)) {
+    throw new Error(`Invalid command name: ${command}`);
+  }
+
   return new Promise((resolve, reject) => {
-    const child = spawn(fullCommand, { ...options, shell: true });
+    // Windows requires shell: true because .bat and .cmd files are not executable
+    // without a terminal. On Unix/macOS, we resolve the full path first, then use
+    // array args (safer, no escaping needed).
+    // See: https://nodejs.org/api/child_process.html#child_processspawncommand-args-options
+    let child;
+    if (os.platform() === "win32") {
+      const fullCommand = buildCommand(command, args);
+      child = spawn(fullCommand, { ...options, shell: true });
+    } else {
+      const fullPath = resolveCommandPath(command);
+      child = spawn(fullPath, args, options);
+    }
 
     // When stdio is piped, we need to collect the output
     let stdout = "";

packages/safe-chain/src/utils/safeSpawn.spec.js

@@ -4,9 +4,11 @@ import assert from "node:assert";
 describe("safeSpawn", () => {
   let safeSpawn;
   let spawnCalls = [];
+  let os;
 
   beforeEach(async () => {
     spawnCalls = [];
+    os = "win32"; // Test Windows behavior by default
 
     // Mock child_process module to capture what command string gets built
     mock.module("child_process", {
@@ -15,13 +17,27 @@ describe("safeSpawn", () => {
           spawnCalls.push({ command, options });
           return {
             on: (event, callback) => {
-              if (event === 'close') {
+              if (event === "close") {
                 // Simulate immediate success
                 setTimeout(() => callback(0), 0);
               }
-            }
+            },
           };
         },
+        execSync: (cmd) => {
+          // Simulate 'command -v' returning full path
+          const match = cmd.match(/command -v (.+)/);
+          if (match) {
+            return `/usr/bin/${match[1]}\n`;
+          }
+          return "";
+        },
+      },
+    });
+
+    mock.module("os", {
+      namedExports: {
+        platform: () => os,
       },
     });
 
@@ -86,4 +102,113 @@ describe("safeSpawn", () => {
     assert.strictEqual(spawnCalls[0].command, "npm install axios --save");
     assert.strictEqual(spawnCalls[0].options.shell, true);
   });
+
+  it(`should escape ampersand character`, async () => {
+    await safeSpawn("npx", ["cypress", "run", "--env", "password=foo&bar"]);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    // & should be escaped by wrapping the arg in quotes
+    assert.strictEqual(
+      spawnCalls[0].command,
+      'npx cypress run --env "password=foo&bar"'
+    );
+    assert.strictEqual(spawnCalls[0].options.shell, true);
+  });
+
+  it("should escape dollar signs to prevent variable expansion", async () => {
+    await safeSpawn("echo", ["$HOME/test"]);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    assert.strictEqual(spawnCalls[0].command, 'echo "\\$HOME/test"');
+  });
+
+  it("should escape backticks to prevent command substitution", async () => {
+    await safeSpawn("echo", ["file`whoami`.txt"]);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    assert.strictEqual(spawnCalls[0].command, 'echo "file\\`whoami\\`.txt"');
+  });
+
+  it("should escape backslashes properly", async () => {
+    await safeSpawn("echo", ["path\\with\\backslash"]);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    assert.strictEqual(
+      spawnCalls[0].command,
+      'echo "path\\\\with\\\\backslash"'
+    );
+  });
+
+  it("should handle multiple special characters in one argument", async () => {
+    await safeSpawn("cmd", ['test "quoted" $var `cmd` & more']);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    assert.strictEqual(
+      spawnCalls[0].command,
+      'cmd "test \\"quoted\\" \\$var \\`cmd\\` & more"'
+    );
+  });
+
+  it("should handle pipe character", async () => {
+    await safeSpawn("echo", ["foo|bar"]);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    assert.strictEqual(spawnCalls[0].command, 'echo "foo|bar"');
+  });
+
+  it("should handle parentheses", async () => {
+    await safeSpawn("echo", ["(test)"]);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    assert.strictEqual(spawnCalls[0].command, 'echo "(test)"');
+  });
+
+  it("should handle angle brackets for redirection", async () => {
+    await safeSpawn("echo", ["foo>output.txt"]);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    assert.strictEqual(spawnCalls[0].command, 'echo "foo>output.txt"');
+  });
+
+  it("should handle wildcard characters", async () => {
+    await safeSpawn("echo", ["*.txt"]);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    assert.strictEqual(spawnCalls[0].command, 'echo "*.txt"');
+  });
+
+  it("should handle multiple arguments with mixed escaping needs", async () => {
+    await safeSpawn("cmd", ["safe", "needs space", "$dangerous", "also-safe"]);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    assert.strictEqual(
+      spawnCalls[0].command,
+      'cmd safe "needs space" "\\$dangerous" also-safe'
+    );
+  });
+
+  it("should reject command names with special characters", async () => {
+    await assert.rejects(async () => await safeSpawn("npm; echo hacked", []), {
+      message: "Invalid command name: npm; echo hacked",
+    });
+  });
+
+  it("should reject command names with spaces", async () => {
+    await assert.rejects(async () => await safeSpawn("npm install", []), {
+      message: "Invalid command name: npm install",
+    });
+  });
+
+  it("should reject command names with slashes", async () => {
+    await assert.rejects(async () => await safeSpawn("../../malicious", []), {
+      message: "Invalid command name: ../../malicious",
+    });
+  });
+
+  it("should accept valid command names with letters, numbers, underscores and hyphens", async () => {
+    await safeSpawn("valid_command-123", []);
+
+    assert.strictEqual(spawnCalls.length, 1);
+    assert.strictEqual(spawnCalls[0].command, "valid_command-123");
+  });
 });