From 486a4b8f680f60100cebd1dcd0aa750e3924229b Mon Sep 17 00:00:00 2001 From: Sander Declerck Date: Mon, 6 Oct 2025 16:25:12 +0200 Subject: [PATCH 1/7] Escape special chars in shell scripts --- packages/safe-chain/src/utils/safeSpawn.js | 12 +++++++++--- packages/safe-chain/src/utils/safeSpawn.spec.js | 9 +++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/safe-chain/src/utils/safeSpawn.js b/packages/safe-chain/src/utils/safeSpawn.js index 826ab7d..f45e4ff 100644 --- a/packages/safe-chain/src/utils/safeSpawn.js +++ b/packages/safe-chain/src/utils/safeSpawn.js @@ -1,9 +1,15 @@ 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('"', '\\"') + '"'; + // Shell metacharacters that need escaping + // These characters have special meaning in shells and need to be quoted + const shellMetaChars = /[ "&'|;<>()$`\\!*?[\]{}~#]/; + + // If argument contains shell metacharacters, wrap in double quotes + // and escape characters that are special even inside double quotes + if (shellMetaChars.test(arg)) { + // Inside double quotes, we need to escape: " $ ` \ + return '"' + arg.replace(/(["`$\\])/g, '\\$1') + '"'; } return arg; } diff --git a/packages/safe-chain/src/utils/safeSpawn.spec.js b/packages/safe-chain/src/utils/safeSpawn.spec.js index d325f8a..020b59a 100644 --- a/packages/safe-chain/src/utils/safeSpawn.spec.js +++ b/packages/safe-chain/src/utils/safeSpawn.spec.js @@ -105,5 +105,14 @@ describe("safeSpawn", () => { assert.strictEqual(spawnCalls[0].command, "npm install axios --save"); assert.strictEqual(spawnCalls[0].options.shell, true); }); + + it(`should escape ampersand character (${variant})`, async () => { + await runSafeSpawn(variant, "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); + }); } }); \ No newline at end of file From 7e72ae7d3d281a248afd5d926b9f2afaa41dd2e9 Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Thu, 9 Oct 2025 16:33:40 +0200 Subject: [PATCH 2/7] On Unix/macOS, pass args to `spawn` to avoid escaping issues --- packages/safe-chain/src/utils/safeSpawn.js | 33 ++++++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/packages/safe-chain/src/utils/safeSpawn.js b/packages/safe-chain/src/utils/safeSpawn.js index f45e4ff..410f455 100644 --- a/packages/safe-chain/src/utils/safeSpawn.js +++ b/packages/safe-chain/src/utils/safeSpawn.js @@ -1,4 +1,4 @@ -import { spawnSync, spawn } from "child_process"; +import { spawn, execSync } from "child_process"; function escapeArg(arg) { // Shell metacharacters that need escaping @@ -16,18 +16,39 @@ function escapeArg(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 }); +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); 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 (process.platform === "win32") { + const fullCommand = buildCommand(command, args); + child = spawn(fullCommand, { ...options, shell: true }); + } else { + const fullPath = resolveCommandPath(command); + child = spawn(fullPath, args, options); + } child.on("close", (code) => { resolve({ From c74c23b0ffada047507409d4cd39535b30d8f040 Mon Sep 17 00:00:00 2001 From: Sander Declerck Date: Thu, 23 Oct 2025 10:52:03 +0200 Subject: [PATCH 3/7] Fix unit tests --- packages/safe-chain/src/utils/safeSpawn.js | 3 ++- packages/safe-chain/src/utils/safeSpawn.spec.js | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/safe-chain/src/utils/safeSpawn.js b/packages/safe-chain/src/utils/safeSpawn.js index 32669b3..96c0603 100644 --- a/packages/safe-chain/src/utils/safeSpawn.js +++ b/packages/safe-chain/src/utils/safeSpawn.js @@ -1,4 +1,5 @@ import { spawn, execSync } from "child_process"; +import os from "os"; function escapeArg(arg) { // Shell metacharacters that need escaping @@ -42,7 +43,7 @@ export async function safeSpawn(command, args, options = {}) { // array args (safer, no escaping needed). // See: https://nodejs.org/api/child_process.html#child_processspawncommand-args-options let child; - if (process.platform === "win32") { + if (os.platform() === "win32") { const fullCommand = buildCommand(command, args); child = spawn(fullCommand, { ...options, shell: true }); } else { diff --git a/packages/safe-chain/src/utils/safeSpawn.spec.js b/packages/safe-chain/src/utils/safeSpawn.spec.js index 6d8dd26..4ad005e 100644 --- a/packages/safe-chain/src/utils/safeSpawn.spec.js +++ b/packages/safe-chain/src/utils/safeSpawn.spec.js @@ -33,6 +33,12 @@ describe("safeSpawn", () => { }, }); + mock.module("os", { + namedExports: { + platform: () => "win32", + }, + }); + // Import after mocking const safeSpawnModule = await import("./safeSpawn.js"); safeSpawn = safeSpawnModule.safeSpawn; From 08c1328b521cb5fb0872e8107be62932e736c2e2 Mon Sep 17 00:00:00 2001 From: Sander Declerck Date: Thu, 23 Oct 2025 13:23:08 +0200 Subject: [PATCH 4/7] Cleanup code, add some tests --- packages/safe-chain/src/utils/safeSpawn.js | 27 ++++--- .../safe-chain/src/utils/safeSpawn.spec.js | 72 +++++++++++++++++++ 2 files changed, 91 insertions(+), 8 deletions(-) diff --git a/packages/safe-chain/src/utils/safeSpawn.js b/packages/safe-chain/src/utils/safeSpawn.js index 96c0603..c85b91e 100644 --- a/packages/safe-chain/src/utils/safeSpawn.js +++ b/packages/safe-chain/src/utils/safeSpawn.js @@ -1,22 +1,33 @@ import { spawn, execSync } from "child_process"; import os from "os"; -function escapeArg(arg) { - // Shell metacharacters that need escaping - // These characters have special meaning in shells and need to be quoted - const shellMetaChars = /[ "&'|;<>()$`\\!*?[\]{}~#]/; - +function sanitizeShellArgument(arg) { // If argument contains shell metacharacters, wrap in double quotes // and escape characters that are special even inside double quotes - if (shellMetaChars.test(arg)) { + if (hasShellMetaChars(arg)) { // Inside double quotes, we need to escape: " $ ` \ - return '"' + arg.replace(/(["`$\\])/g, "\\$1") + '"'; + 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); + const escapedArgs = args.map(sanitizeShellArgument); return `${command} ${escapedArgs.join(" ")}`; } diff --git a/packages/safe-chain/src/utils/safeSpawn.spec.js b/packages/safe-chain/src/utils/safeSpawn.spec.js index 4ad005e..cf7bd41 100644 --- a/packages/safe-chain/src/utils/safeSpawn.spec.js +++ b/packages/safe-chain/src/utils/safeSpawn.spec.js @@ -112,4 +112,76 @@ describe("safeSpawn", () => { ); 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' + ); + }); }); From 7a55be49f4a35a9fca262c82e588996ebf4b46a0 Mon Sep 17 00:00:00 2001 From: Sander Declerck Date: Thu, 23 Oct 2025 13:29:14 +0200 Subject: [PATCH 5/7] Fix linting error --- packages/safe-chain/src/utils/safeSpawn.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/safe-chain/src/utils/safeSpawn.spec.js b/packages/safe-chain/src/utils/safeSpawn.spec.js index cf7bd41..cbdb7cb 100644 --- a/packages/safe-chain/src/utils/safeSpawn.spec.js +++ b/packages/safe-chain/src/utils/safeSpawn.spec.js @@ -22,7 +22,7 @@ describe("safeSpawn", () => { }, }; }, - execSync: (cmd, opts) => { + execSync: (cmd) => { // Simulate 'command -v' returning full path const match = cmd.match(/command -v (.+)/); if (match) { From f5f3b91b40dc1bad0050ef1d4339672ecd2cfc5f Mon Sep 17 00:00:00 2001 From: Sander Declerck Date: Fri, 24 Oct 2025 17:36:51 +0200 Subject: [PATCH 6/7] Test if command is safe to execute --- packages/safe-chain/src/utils/safeSpawn.js | 9 ++++++ .../safe-chain/src/utils/safeSpawn.spec.js | 29 ++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/packages/safe-chain/src/utils/safeSpawn.js b/packages/safe-chain/src/utils/safeSpawn.js index c85b91e..8642b07 100644 --- a/packages/safe-chain/src/utils/safeSpawn.js +++ b/packages/safe-chain/src/utils/safeSpawn.js @@ -27,6 +27,10 @@ function escapeDoubleQuoteContent(arg) { } function buildCommand(command, args) { + if (args.length === 0) { + return command; + } + const escapedArgs = args.map(sanitizeShellArgument); return `${command} ${escapedArgs.join(" ")}`; @@ -48,6 +52,11 @@ function resolveCommandPath(command) { } export async function safeSpawn(command, args, options = {}) { + // command should always be alphanumeric or _ or - to avoid injection + if (!/^[a-zA-Z0-9_-]+$/.test(command)) { + throw new Error(`Invalid command name: ${command}`); + } + return new Promise((resolve, reject) => { // 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 diff --git a/packages/safe-chain/src/utils/safeSpawn.spec.js b/packages/safe-chain/src/utils/safeSpawn.spec.js index cbdb7cb..d4180b7 100644 --- a/packages/safe-chain/src/utils/safeSpawn.spec.js +++ b/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", { @@ -35,7 +37,7 @@ describe("safeSpawn", () => { mock.module("os", { namedExports: { - platform: () => "win32", + platform: () => os, }, }); @@ -184,4 +186,29 @@ describe("safeSpawn", () => { '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"); + }); }); From 0029a7e1c1fa8f9a16ce3ef3d913c46cea360c63 Mon Sep 17 00:00:00 2001 From: Sander Declerck Date: Mon, 27 Oct 2025 10:49:26 +0100 Subject: [PATCH 7/7] Add extra comments for regex clarification --- packages/safe-chain/src/utils/safeSpawn.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/safe-chain/src/utils/safeSpawn.js b/packages/safe-chain/src/utils/safeSpawn.js index 8642b07..c398ac2 100644 --- a/packages/safe-chain/src/utils/safeSpawn.js +++ b/packages/safe-chain/src/utils/safeSpawn.js @@ -52,7 +52,9 @@ function resolveCommandPath(command) { } export async function safeSpawn(command, args, options = {}) { - // command should always be alphanumeric or _ or - to avoid injection + // 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}`); }