Skip to content

Commit fd7e38f

Browse files
author
herdiyanitdev
committed
fix(ng-dev): address alan-agius4 review — remove redundant shell default, audit call sites
- 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.
1 parent 2701597 commit fd7e38f

File tree

3 files changed

+6
-7
lines changed

3 files changed

+6
-7
lines changed

ng-dev/release/publish/external-commands.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,10 +202,12 @@ export abstract class ExternalCommands {
202202
}
203203

204204
try {
205-
// We must source nvm.sh so the shell recognizes the 'nvm' command since nvm is not a binary but a shell script.
205+
// We must source nvm.sh so the shell recognizes the 'nvm' command since nvm is not a binary
206+
// but a shell function. The dot (.) built-in and && operator require shell: true here.
206207
await ChildProcess.spawn('. ~/.nvm/nvm.sh && nvm install', [], {
207208
cwd: projectDir,
208209
mode: 'on-error',
210+
shell: true,
209211
});
210212

211213
if (!quiet) {

ng-dev/utils/child-process.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ export abstract class ChildProcess {
9999
signal,
100100
stdout,
101101
stderr,
102-
} = _spawnSync(command, args, {...options, env, encoding: 'utf8', shell: options.shell ?? false, stdio: 'pipe'});
102+
} = _spawnSync(command, args, {...options, env, encoding: 'utf8', stdio: 'pipe'});
103103

104104
/** The status of the spawn result. */
105105
const status = statusFromExitCodeAndSignal(exitCode, signal);
@@ -130,7 +130,7 @@ export abstract class ChildProcess {
130130
return processAsyncCmd(
131131
commandText,
132132
options,
133-
_spawn(command, args, {...options, env, shell: options.shell ?? false, stdio: 'pipe'}),
133+
_spawn(command, args, {...options, env, stdio: 'pipe'}),
134134
);
135135
}
136136

@@ -143,9 +143,6 @@ export abstract class ChildProcess {
143143
* rejects on command failure.
144144
*/
145145
static exec(command: string, options: ExecOptions = {}): Promise<SpawnResult> {
146-
Log.warn(
147-
'ChildProcess.exec is susceptible to command injection. Prefer ChildProcess.spawn with an explicit args array.',
148-
);
149146
const env = getEnvironmentForNonInteractiveCommand(options.env);
150147
return processAsyncCmd(command, options, _exec(command, {...options, env}));
151148
}

ng-dev/utils/repo-directory.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {ChildProcess} from './child-process.js';
1010

1111
/** Determines the repository base directory from the current working directory. */
1212
export function determineRepoBaseDirFromCwd() {
13-
const {stdout, stderr, status} = ChildProcess.spawnSync('git', ['rev-parse --show-toplevel']);
13+
const {stdout, stderr, status} = ChildProcess.spawnSync('git', ['rev-parse', '--show-toplevel']);
1414
if (status !== 0) {
1515
throw Error(
1616
`Unable to find the path to the base directory of the repository.\n` +

0 commit comments

Comments
 (0)