Error: spawn EINVAL on Windows

[samuelmaddock]

The latest versions of Node include a security vulnerability fix which now requires calling spawn() with shell: true on Windows (Node security release blog).

node-pre-gyp info using node-pre-gyp@1.0.11
node-pre-gyp info using node@20.13.0 | win32 | x64
node-pre-gyp ERR! UNCAUGHT EXCEPTION 
node-pre-gyp ERR! stack Error: spawn EINVAL
node-pre-gyp ERR! stack     at ChildProcess.spawn (node:internal/child_process:421:11)
node-pre-gyp ERR! stack     at Object.spawn (node:child_process:761:9)
node-pre-gyp ERR! stack     at module.exports.run_gyp (C:\Users\circleci\project\node_modules\@mapbox\node-pre-gyp\lib\util\compile.js:80:18)
node-pre-gyp ERR! stack     at C:\Users\circleci\project\node_modules\@mapbox\node-pre-gyp\lib\configure.js:44:15
node-pre-gyp ERR! stack     at handle_gyp_opts (C:\Users\circleci\project\node_modules\@mapbox\node-pre-gyp\lib\util\handle_gyp_opts.js:101:10)
node-pre-gyp ERR! stack     at configure (C:\Users\circleci\project\node_modules\@mapbox\node-pre-gyp\lib\configure.js:12:3)
node-pre-gyp ERR! stack     at self.commands.<computed> [as configure] (C:\Users\circleci\project\node_modules\@mapbox\node-pre-gyp\lib\node-pre-gyp.js:86:37)
node-pre-gyp ERR! stack     at run (C:\Users\circleci\project\node_modules\@mapbox\node-pre-gyp\lib\main.js:81:30)
node-pre-gyp ERR! stack     at Object.<anonymous> (C:\Users\circleci\project\node_modules\@mapbox\node-pre-gyp\lib\main.js:125:1)
node-pre-gyp ERR! stack     at Module._compile (node:internal/modules/cjs/loader:1358:14)

I'm currently working around this by using patch-package with the following patch:

diff --git a/node_modules/@mapbox/node-pre-gyp/lib/util/compile.js b/node_modules/@mapbox/node-pre-gyp/lib/util/compile.js
index 956e5aa..0051fce 100644
--- a/node_modules/@mapbox/node-pre-gyp/lib/util/compile.js
+++ b/node_modules/@mapbox/node-pre-gyp/lib/util/compile.js
@@ -77,7 +77,9 @@ module.exports.run_gyp = function(args, opts, callback) {
     }
   }
   const final_args = cmd_args.concat(args);
-  const cmd = cp.spawn(shell_cmd, final_args, { cwd: undefined, env: process.env, stdio: [0, 1, 2] });
+  // Add 'shell' on Windows due to security vulnerability
+  // https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2
+  const cmd = cp.spawn(shell_cmd, final_args, { cwd: undefined, env: process.env, stdio: [0, 1, 2], shell: process.platform === 'win32' });
   cmd.on('error', (err) => {
     if (err) {
       return callback(new Error("Failed to execute '" + shell_cmd + ' ' + final_args.join(' ') + "' (" + err + ')'));

[Stanzilla]

@benmccann any chance this could be picked up for 2.0?

[cclauss]

Does this happen on v2.0.0-rc.0? https://github.com/mapbox/node-pre-gyp/tags

[benmccann]

Should we just always set shell: true? I'm afraid I don't know the implications of doing so. I see the patch in the issue description does it only on Windows, but I'm wary about creating an inconsistency between platforms

[Stanzilla]

Should we just always set shell: true? I'm afraid I don't know the implications of doing so. I see the patch in the issue description does it only on Windows, but I'm wary about creating an inconsistency between platforms

The change in node was specifically just for Windows and bat/cmd files so imo doing it just for Windows, like it was proposed, is fine

[benmccann]

The proposal does it for non-bat/cmd files in Windows though. If we're going to do it as minimally as possible perhaps we should check for that too

[Stanzilla]

Does this happen on v2.0.0-rc.0? https://github.com/mapbox/node-pre-gyp/tags

I don't have a Windows system with access to the codebase where this happens, sorry

[polar2031]

The patch saves my day, but it will not work when node is installed in path containing space like Program File
https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows
Simply make shell_cmd quoted should fix it.

[benmccann]

It seems when using spawn: true you need to escape the command and it's arguments. Some details here: https://www.npmjs.com/package/cross-spawn

[Stanzilla]

So one could just always escape the commands and arguments yet only set shell to true as in the patch suggested above, no?

[Stanzilla]

@benmccann 🙏 anything I can do to help move this forward?

[benmccann]

Perhaps we should just use cross-spawn. I'm not sure I want to try rebuilding the logic they already have