child_process: add shell option to spawn() #4598

cjihrig · 2016-01-09T01:38:11Z

This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for
compatibility with exec()'s shell option.

Closes #1009

bnoordhuis · 2016-01-09T14:11:31Z

lib/child_process.js

-  if (options && options.shell)
-    file = options.shell;
+  // Make a shallow copy so we don't clobber the user's options object
+  options = util._extend({}, options);


#4593 is about deprecating util._extend(). Maybe use Object.assign() here?

EDIT: Just a suggestion, feel free to ignore.

I'd prefer to use Object.assign(). I was waiting to see how #4593 played out, but the reaction seems to be good, so I'll replace this.

cjihrig · 2016-01-11T17:55:19Z

@bnoordhuis I've addressed your comments.

jasnell · 2016-01-11T18:42:17Z

LGTM

cjihrig · 2016-01-11T20:28:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1195/

EDIT: Test failed on Windows. Trying again. CI: https://ci.nodejs.org/job/node-test-pull-request/1198/

cjihrig · 2016-01-11T22:10:31Z

CI is all green, but I'd like sign off from @bnoordhuis

cjihrig · 2016-01-18T15:54:25Z

@bnoordhuis hope you're feeling better. Ping for a review.

cjihrig · 2016-01-26T17:22:49Z

@bnoordhuis sorry, one more ping.

bnoordhuis · 2016-01-26T17:40:19Z

lib/child_process.js

+  options = Object.assign({}, options);
+
+  if (options.shell) {
+    const command = [file].concat(args).join(' ');


Shouldn't double quotes be escaped on Windows?

Probably. I'll add a test for it too.

Actually, maybe not:

C:\Users\Colin>cmd.exe /s /c "echo "foo"" "foo" C:\Users\Colin>cmd.exe /s /c "echo \"foo\"" \"foo\"

Huh, I wonder what cmd.exe's algorithm for that is. Counting quotes?

Ah, looks like the /s enables that behavior - http://stackoverflow.com/questions/9866962/what-is-cmd-s-for

Learned something new today. Thanks.

bnoordhuis · 2016-01-26T17:50:46Z

Sorry for the delay. Left some comments.

cjihrig · 2016-01-26T18:26:40Z

@bnoordhuis, I think I've addressed all of your comments.

bnoordhuis · 2016-01-26T22:13:45Z

LGTM if the CI is happy.

This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for compatibility with exec()'s shell option. Fixes: nodejs#1009 PR-URL: nodejs#4598 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2016-12-29T17:40:53Z

/cc @nodejs/lts
Would we want to include this in a future LTS release for v4.x?

sam-github · 2016-12-29T18:32:01Z

I'm +1 for back-port, it improves compatibility between the LTS versions.

This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for compatibility with exec()'s shell option. Fixes: #1009 PR-URL: #4598 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes: * child_process: add shell option to spawn() (cjihrig) #4598 * crypto: * add ALPN Support (Shigeki Ohtsu) #2564 * allow adding extra certs to well-known CAs (Sam Roberts) #9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) #4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) #5333 * process: * add `externalMemory` to `process` (Fedor Indutny) #9587 * add process.cpuUsage() (Patrick Mueller) #10796

Notable Changes: * child_process: add shell option to spawn() (cjihrig) #4598 * crypto: * add ALPN Support (Shigeki Ohtsu) #2564 * allow adding extra certs to well-known CAs (Sam Roberts) #9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) #4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) #5333 * process: * add `externalMemory` to `process` (Fedor Indutny) #9587 * add process.cpuUsage() (Patrick Mueller) #10796 PR-URL: #10973

Notable Changes: * child_process: add shell option to spawn() (cjihrig) nodejs/node#4598 * crypto: * add ALPN Support (Shigeki Ohtsu) nodejs/node#2564 * allow adding extra certs to well-known CAs (Sam Roberts) nodejs/node#9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) nodejs/node#4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) nodejs/node#5333 * process: * add `externalMemory` to `process` (Fedor Indutny) nodejs/node#9587 * add process.cpuUsage() (Patrick Mueller) nodejs/node#10796 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Use the shell option of spawn introduced in Node.js 4.8 (see nodejs/node#4598) to pass the command to the OS shell. Supersedes kentcdodds#77. BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted by the shell.

* feat(spawn): add support for quoted scripts Use the shell option of spawn introduced in Node.js 4.8 (see nodejs/node#4598) to pass the command to the OS shell. Supersedes #77. * docs(readme): add gotchas Add Gotchas paragraph about passing variables to multiple scripts. * docs(readme): add required node version badge Replace the Prerequisite paragraph by a badge showing the require version of Node.js required to run cross-env. * test(spawn): add test for quoted scripts See #89 (review). BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted by the shell.

mohd-akram · 2025-02-19T07:09:01Z

I have no idea why this and #18237 were added. It seems really dangerous to give the impression that, since these functions accept an array of arguments, that they will be passed as-is (safely) to the command. The user should concatenate the arguments themselves to make clear that no escaping/isolation of arguments will happen. Right now all it takes is changing shell to true to eg. workaround the Windows .bat/.cmd issue and break proper handling of arguments at best or introduce a gaping security hole at worst.

mscdex added the child_process Issues and PRs related to the child_process subsystem. label Jan 9, 2016

bnoordhuis reviewed Jan 9, 2016
View reviewed changes

janicduplessis mentioned this pull request Jan 11, 2016

Fix launching text editor from a redbox stacktrace on windows facebook/react-native#5238

Closed

cjihrig force-pushed the shell branch 3 times, most recently from ce56f29 to a7c5918 Compare January 11, 2016 17:52

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 11, 2016

cjihrig force-pushed the shell branch from a7c5918 to aea316e Compare January 11, 2016 21:24

bnoordhuis reviewed Jan 26, 2016
View reviewed changes

cjihrig force-pushed the shell branch from aea316e to 78f8a5f Compare January 26, 2016 18:00

cjihrig force-pushed the shell branch from 78f8a5f to f2e70a5 Compare January 26, 2016 18:07

MylesBorins added lts-agenda labels Dec 29, 2016

This was referenced Jan 4, 2017

spawn() fails to properly exercise terminal applications nodejs/citgm#136

Closed

Potential Semver Minor Backports nodejs/Release#177

Closed

MylesBorins added land-on-v4.x and removed lts-agenda labels Jan 13, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v4.8.0 proposal #10973

Merged

This was referenced Feb 28, 2017

Switches to cross-spawn to fix issues with Windows shells workshopper/learn-sass#50

Merged

child_process.spawn shell option is available in Node v5 and v4 too moxystudio/node-cross-spawn#68

Closed

chexxor mentioned this pull request Mar 14, 2017

Initialize .purs-repl file. purescript-contrib/pulp#260

Merged

bnoordhuis mentioned this pull request Mar 29, 2017

child_process#spawn breaks backward compatibility in node v4.8.0 #12117

Closed

Vanuan mentioned this pull request Sep 12, 2017

Error: Couldn't find the binary node install.js in 1.0.1 yarnpkg/yarn#4337

Closed

idanpa mentioned this pull request Apr 11, 2018

child_process.spawn adds flags that might cause non-native shells to fail on windows #19947

Closed

Uh oh!

child_process: add shell option to spawn() #4598

child_process: add shell option to spawn() #4598

Uh oh!

Conversation

cjihrig commented Jan 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Jan 11, 2016

Uh oh!

jasnell commented Jan 11, 2016

Uh oh!

cjihrig commented Jan 11, 2016

Uh oh!

cjihrig commented Jan 11, 2016

Uh oh!

cjihrig commented Jan 18, 2016

Uh oh!

cjihrig commented Jan 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jan 26, 2016

Uh oh!

cjihrig commented Jan 26, 2016

Uh oh!

bnoordhuis commented Jan 26, 2016

Uh oh!

MylesBorins commented Dec 29, 2016

Uh oh!

sam-github commented Dec 29, 2016

Uh oh!

mohd-akram commented Feb 19, 2025

Uh oh!

Uh oh!