Skip to content

child_process: validate options.shell and correctly enforce shell invocation in exec/execSync #56761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

child_process: validate options.shell and correctly enforce shell invocation in exec/execSync #56761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,12 @@ function normalizeExecArgs(command, options, callback) {

// Make a shallow copy so we don't clobber the user's options object.
options = { __proto__: null, ...options };
options.shell = typeof options.shell === 'string' ? options.shell : true;

// Validate the shell, if present, and ensure a truthy value.
if (options.shell != null) {
validateString(options.shell, 'options.shell');
}
options.shell ||= true;
Comment on lines +203 to +204
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why would we want to accept the empty string a valid value

Suggested change
}
options.shell ||= true;
} else {
options.shell = true;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggested change would maintain the existing edge case behaviour when { shell: '' } is passed to exec.

  • It is not null, and validates as a string, so is passed through unchanged.
  • When this reaches spawn() downstream, because it is not a truthy value, a shell is not invoked.

exec() must always invoke a shell, and therefore anything that passes this validation needs to be a truthy value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make more sense to throw on empty string IMO

Copy link
Contributor Author

@Renegade334 Renegade334 Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree, but this would then diverge from the behaviour of the other spawning child_process functions, unless we change them all? (Everywhere else considers { shell: '' } equivalent to { shell: undefined }.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could start by deprecating that, sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, two separate PRs eve, one for doc-only deprecation, and one that adds a runtime warning. Would you like to take up that effort?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While deprecated, should the behaviour of exec(..., { shell: '' }) be

  1. its current behaviour (attempt to spawn the target directly, equivalent to execFile(..., { shell: false })), or
  2. this proposal's behaviour (spawn a default shell, the same as exec(..., { shell: undefined })?

My vote would be for the latter – the whole point of exec is that it always invokes a shell, so I can't see that anyone would be relying on 1 as intended behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it should be the current behavior until we move forward with the deprecation at which point it will throw. I don't see the point of introducing an intermediary breaking change only to land another breaking change


return {
file: command,
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-child-process-exec-any-shells-windows.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const testCopy = (shellName, shellPath) => {
const system32 = `${process.env.SystemRoot}\\System32`;

// Test CMD
test(true);
test();
test('cmd');
testCopy('cmd.exe', `${system32}\\cmd.exe`);
test('cmd.exe');
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-child-process-exec-enforce-shell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const { exec, execSync } = require('child_process');

const invalidArgTypeError = {
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError'
};

exec('echo should-be-passed-as-argument', { shell: '' }, common.mustSucceed((stdout, stderr) => {
assert.match(stdout, /should-be-passed-as-argument/);
assert.ok(!stderr);
}));

{
const ret = execSync('echo should-be-passed-as-argument', { encoding: 'utf-8', shell: '' });
assert.match(ret, /should-be-passed-as-argument/);
}

for (const fn of [exec, execSync]) {
assert.throws(() => fn('should-throw-on-boolean-shell-option', { shell: false }), invalidArgTypeError);
}
Loading