Skip to content

Commit 6ae6190

Browse files
herdiyana256alan-agius4
authored andcommitted
fixup! fix(ng-dev): prevent OS command injection in ChildProcess wrappers
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.
1 parent 8325de8 commit 6ae6190

1 file changed

Lines changed: 13 additions & 19 deletions

File tree

ng-dev/utils/child-process.ts

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,10 @@ export interface CommonCmdOpts {
3131

3232
/** Interface describing the options for spawning a process synchronously. */
3333
export interface SpawnSyncOptions
34-
extends CommonCmdOpts, Omit<_SpawnSyncOptions, 'shell' | 'stdio' | 'input'> {}
34+
extends CommonCmdOpts, Omit<_SpawnSyncOptions, 'stdio' | 'input'> {}
3535

3636
/** Interface describing the options for spawning a process. */
37-
export interface SpawnOptions extends CommonCmdOpts, Omit<_SpawnOptions, 'shell' | 'stdio'> {}
37+
export interface SpawnOptions extends CommonCmdOpts, Omit<_SpawnOptions, 'stdio'> {}
3838

3939
/** Interface describing the options for exec-ing a process. */
4040
export interface ExecOptions extends CommonCmdOpts, Omit<_ExecOptions, 'shell' | 'stdio'> {}
@@ -85,11 +85,10 @@ 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.
91-
// Note: shell: false may affect .cmd/.bat execution on Windows, but ng-dev
92-
// targets Linux/macOS CI environments where this is not a concern.
88+
// Default shell to false to prevent OS command injection: with shell: true, Node.js
89+
// internally joins command + args into a single string evaluated by /bin/sh, making
90+
// shell metacharacters in args exploitable. Callers that genuinely require shell
91+
// features (e.g. sourcing shell scripts) may explicitly pass shell: true.
9392
const commandText = `${command} ${args.join(' ')}`;
9493
const env = getEnvironmentForNonInteractiveCommand(options.env);
9594

@@ -100,7 +99,7 @@ export abstract class ChildProcess {
10099
signal,
101100
stdout,
102101
stderr,
103-
} = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: false, stdio: 'pipe'});
102+
} = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: options.shell ?? false, stdio: 'pipe'});
104103

105104
/** The status of the spawn result. */
106105
const status = statusFromExitCodeAndSignal(exitCode, signal);
@@ -121,18 +120,17 @@ export abstract class ChildProcess {
121120
* rejects on command failure.
122121
*/
123122
static spawn(command: string, args: string[], options: SpawnOptions = {}): Promise<SpawnResult> {
124-
// Pass args as a proper array with shell: false to prevent OS command injection.
125-
// When shell: true is used, Node.js internally joins command + args into a single
126-
// string evaluated by /bin/sh, making shell metacharacters in args exploitable.
127-
// Note: shell: false may affect .cmd/.bat execution on Windows, but ng-dev
128-
// targets Linux/macOS CI environments where this is not a concern.
123+
// Default shell to false to prevent OS command injection: with shell: true, Node.js
124+
// internally joins command + args into a single string evaluated by /bin/sh, making
125+
// shell metacharacters in args exploitable. Callers that genuinely require shell
126+
// features (e.g. sourcing shell scripts) may explicitly pass shell: true.
129127
const commandText = `${command} ${args.join(' ')}`;
130128
const env = getEnvironmentForNonInteractiveCommand(options.env);
131129

132130
return processAsyncCmd(
133131
commandText,
134132
options,
135-
_spawn(command, args, {...options, env, shell: false, stdio: 'pipe'}),
133+
_spawn(command, args, {...options, env, shell: options.shell ?? false, stdio: 'pipe'}),
136134
);
137135
}
138136

@@ -143,14 +141,10 @@ export abstract class ChildProcess {
143141
*
144142
* @returns a Promise resolving with captured stdout and stderr on success. The promise
145143
* rejects on command failure.
146-
* @deprecated Use ChildProcess.spawn with an explicit args array instead.
147-
* exec() passes the command string directly to the shell, making it susceptible
148-
* to command injection via shell metacharacters in the command string.
149144
*/
150145
static exec(command: string, options: ExecOptions = {}): Promise<SpawnResult> {
151146
Log.warn(
152-
`ChildProcess.exec is discouraged as it is susceptible to command injection ` +
153-
`(command: ${command}). Prefer ChildProcess.spawn with an array of arguments.`,
147+
'ChildProcess.exec is susceptible to command injection. Prefer ChildProcess.spawn with an explicit args array.',
154148
);
155149
const env = getEnvironmentForNonInteractiveCommand(options.env);
156150
return processAsyncCmd(command, options, _exec(command, {...options, env}));

0 commit comments

Comments
 (0)