From 83141d375a9d1282927a4397f6638ffd2d3a804a Mon Sep 17 00:00:00 2001 From: Sander Declerck Date: Wed, 24 Sep 2025 14:29:49 +0200 Subject: [PATCH] Escape args before running spawn --- .../src/packagemanager/pnpm/runPnpmCommand.js | 10 +- packages/safe-chain/src/utils/safeSpawn.js | 38 ++++++ .../safe-chain/src/utils/safeSpawn.spec.js | 109 ++++++++++++++++++ 3 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 packages/safe-chain/src/utils/safeSpawn.js create mode 100644 packages/safe-chain/src/utils/safeSpawn.spec.js diff --git a/packages/safe-chain/src/packagemanager/pnpm/runPnpmCommand.js b/packages/safe-chain/src/packagemanager/pnpm/runPnpmCommand.js index 5683a62..0d5133f 100644 --- a/packages/safe-chain/src/packagemanager/pnpm/runPnpmCommand.js +++ b/packages/safe-chain/src/packagemanager/pnpm/runPnpmCommand.js @@ -1,15 +1,18 @@ -import { execSync } from "child_process"; import { ui } from "../../environment/userInteraction.js"; +import { safeSpawnSync } from "../../utils/safeSpawn.js"; export function runPnpmCommand(args, toolName = "pnpm") { try { + let result; if (toolName === "pnpm") { - execSync(`pnpm ${args.join(" ")}`, { stdio: "inherit" }); + result = safeSpawnSync("pnpm", args, { stdio: "inherit" }); } else if (toolName === "pnpx") { - execSync(`pnpx ${args.join(" ")}`, { stdio: "inherit" }); + result = safeSpawnSync("pnpx", args, { stdio: "inherit" }); } else { throw new Error(`Unsupported tool name for aikido-pnpm: ${toolName}`); } + + return { status: result.status }; } catch (error) { if (error.status) { return { status: error.status }; @@ -18,5 +21,4 @@ export function runPnpmCommand(args, toolName = "pnpm") { return { status: 1 }; } } - return { status: 0 }; } diff --git a/packages/safe-chain/src/utils/safeSpawn.js b/packages/safe-chain/src/utils/safeSpawn.js new file mode 100644 index 0000000..826ab7d --- /dev/null +++ b/packages/safe-chain/src/utils/safeSpawn.js @@ -0,0 +1,38 @@ +import { spawnSync, spawn } from "child_process"; + +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('"', '\\"') + '"'; + } + return arg; +} + +function buildCommand(command, args) { + const escapedArgs = args.map(escapeArg); + return `${command} ${escapedArgs.join(" ")}`; +} + +export function safeSpawnSync(command, args, options = {}) { + const fullCommand = buildCommand(command, args); + return spawnSync(fullCommand, { ...options, shell: true }); +} + +export async function safeSpawn(command, args, options = {}) { + const fullCommand = buildCommand(command, args); + return new Promise((resolve, reject) => { + const child = spawn(fullCommand, { ...options, shell: true }); + + child.on("close", (code) => { + resolve({ + status: code, + stdout: Buffer.from(""), + stderr: Buffer.from(""), + }); + }); + + child.on("error", (error) => { + reject(error); + }); + }); +} diff --git a/packages/safe-chain/src/utils/safeSpawn.spec.js b/packages/safe-chain/src/utils/safeSpawn.spec.js new file mode 100644 index 0000000..d325f8a --- /dev/null +++ b/packages/safe-chain/src/utils/safeSpawn.spec.js @@ -0,0 +1,109 @@ +import { describe, it, beforeEach, afterEach, mock } from "node:test"; +import assert from "node:assert"; + +describe("safeSpawn", () => { + let safeSpawnSync, safeSpawn; + let spawnCalls = []; + + beforeEach(async () => { + spawnCalls = []; + + // Mock child_process module to capture what command string gets built + mock.module("child_process", { + namedExports: { + spawnSync: (command, options) => { + spawnCalls.push({ command, options }); + return { + status: 0, + stdout: Buffer.from(""), + stderr: Buffer.from(""), + }; + }, + spawn: (command, options) => { + spawnCalls.push({ command, options }); + return { + on: (event, callback) => { + if (event === 'close') { + // Simulate immediate success + setTimeout(() => callback(0), 0); + } + } + }; + }, + }, + }); + + // Import after mocking + const safeSpawnModule = await import("./safeSpawn.js"); + safeSpawnSync = safeSpawnModule.safeSpawnSync; + safeSpawn = safeSpawnModule.safeSpawn; + }); + + afterEach(() => { + mock.reset(); + }); + + // Helper to run either sync or async variant + async function runSafeSpawn(variant, command, args, options) { + if (variant === "sync") { + return safeSpawnSync(command, args, options); + } else { + return await safeSpawn(command, args, options); + } + } + + for (let variant of ["sync", "async"]) { + it(`should pass basic command and arguments correctly (${variant})`, async () => { + await runSafeSpawn(variant, "echo", ["hello"]); + + assert.strictEqual(spawnCalls.length, 1); + assert.strictEqual(spawnCalls[0].command, "echo hello"); + assert.strictEqual(spawnCalls[0].options.shell, true); + }); + + it(`should escape arguments containing spaces (${variant})`, async () => { + await runSafeSpawn(variant, "echo", ["hello world"]); + + assert.strictEqual(spawnCalls.length, 1); + // Argument should be escaped to prevent shell interpretation + assert.strictEqual(spawnCalls[0].command, 'echo "hello world"'); + assert.strictEqual(spawnCalls[0].options.shell, true); + }); + + it(`should prevent shell injection attacks (${variant})`, async () => { + await runSafeSpawn(variant, "ls", ["; rm test123.txt"]); + + assert.strictEqual(spawnCalls.length, 1); + // Malicious command should be escaped to prevent execution + assert.strictEqual(spawnCalls[0].command, 'ls "; rm test123.txt"'); + assert.strictEqual(spawnCalls[0].options.shell, true); + }); + + it(`should escape single quotes in arguments (${variant})`, async () => { + await runSafeSpawn(variant, "echo", ["don't break"]); + + assert.strictEqual(spawnCalls.length, 1); + // Single quote should be properly escaped with double quotes + assert.strictEqual(spawnCalls[0].command, 'echo "don\'t break"'); + assert.strictEqual(spawnCalls[0].options.shell, true); + }); + + it(`should handle double quotes with simpler escaping (${variant})`, async () => { + await runSafeSpawn(variant, "echo", ['say "hello"']); + + assert.strictEqual(spawnCalls.length, 1); + // If we switch to double quotes, this should be: "say \"hello\"" + assert.strictEqual(spawnCalls[0].command, 'echo "say \\"hello\\""'); + assert.strictEqual(spawnCalls[0].options.shell, true); + }); + + it(`should not escape arguments with only safe characters (${variant})`, async () => { + await runSafeSpawn(variant, "npm", ["install", "axios", "--save"]); + + assert.strictEqual(spawnCalls.length, 1); + // Safe arguments (alphanumeric, dash, underscore, dot, slash) shouldn't be quoted + assert.strictEqual(spawnCalls[0].command, "npm install axios --save"); + assert.strictEqual(spawnCalls[0].options.shell, true); + }); + } +}); \ No newline at end of file