Passing args to a child process with shell option true is deprecated

[zepumph]

As of node 24, a primary way that we are supporting windows users in our build tools is deprecated.

(node:39656) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.

I am unsure how long we will be able to limp along, but I thought I'd get a head start on this since it involves changing the majority of our code tooling. The primary issues are in spawning grunt, running lint, and calling execute. We have been using the *.cmd string + shell:true to support windows, but that is no longer possible. Instead, the solution that I found requires you to actually pass the .cmd to cmd.exe. Here is an example:

From:
execute( 'grunt.cmd', ['lint'] );

To:
`execute( 'cmd.exe', [ '/d', '/s', '/c', 'grunt.cmd lint' ]

While this is a bit of a scary change, because of all the spots it may touch, it isn't too complicated, as we can factor out this mapping code to only run on windows (with the presence of .cmd) at the source, not the usage:

  // Windows support. `.cmd` files are supposed to be run with `cmd.exe` on windows, otherwise you fail without shell:true (a deprecated option now).
  if ( cmd.endsWith( '.cmd' ) ) {
    args = [ '/d', '/s', '/c', [ cmd, ...args ].join( ' ' ) ];
    cmd = 'cmd.exe';
  }

This patch isn't working perfectly across all usages or anything, but serves as a proof of concept for some stuff in perennial. These commands are working:

grunt lint
grunt lint --repo=chipper
grunt type-check
grunt sync

Details

Subject: [PATCH] fdsa
---
Index: perennial-alias/js/eslint/lint.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/perennial-alias/js/eslint/lint.ts b/perennial-alias/js/eslint/lint.ts
--- a/perennial-alias/js/eslint/lint.ts	(revision f749d9bf2f5096a715fa492a0897563bda833d20)
+++ b/perennial-alias/js/eslint/lint.ts	(date 1769558851339)
@@ -66,7 +66,6 @@
           `--fix=${options.fix}`
         ], {
           stdio: [ 'ignore', 'pipe', 'pipe' ],
-          shell: process.platform.startsWith( 'win' ),
           env: {
             ...process.env, // eslint-disable-line phet/no-object-spread-on-non-literals
 
Index: perennial/js/eslint/lint.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/perennial/js/eslint/lint.ts b/perennial/js/eslint/lint.ts
--- a/perennial/js/eslint/lint.ts	(revision f749d9bf2f5096a715fa492a0897563bda833d20)
+++ b/perennial/js/eslint/lint.ts	(date 1769559680073)
@@ -58,15 +58,21 @@
   // spawn node lint-main.js for each batch and wait for all to complete using child process
   const promises = repoBatches.map( batch => {
     return new Promise<void>( ( resolve, reject ) => {
-
-      const child = spawn( tsxCommand, [
-          lintMainPath,
-          `--repos=${batch.join( ',' )}`,
-          `--clean=${options.clean}`,
-          `--fix=${options.fix}`
-        ], {
+      // Windows support. `.cmd` files are supposed to be run with `cmd.exe` on windows, otherwise you fail without shell:true (a deprecated option now).
+      let cmd = tsxCommand;
+      let args = [
+        lintMainPath,
+        `--repos=${batch.join( ',' )}`,
+        `--clean=${options.clean}`,
+        `--fix=${options.fix}`
+      ];
+      if ( cmd.endsWith( '.cmd' ) ) {
+        args = [ '/d', '/s', '/c', [ cmd, ...args ].join( ' ' ) ];
+        cmd = 'cmd.exe';
+      }
+
+      const child = spawn( cmd, args, {
           stdio: [ 'ignore', 'pipe', 'pipe' ],
-          shell: process.platform.startsWith( 'win' ),
           env: {
             ...process.env, // eslint-disable-line phet/no-object-spread-on-non-literals
 
Index: perennial/js/common/execute.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/perennial/js/common/execute.ts b/perennial/js/common/execute.ts
--- a/perennial/js/common/execute.ts	(revision f749d9bf2f5096a715fa492a0897563bda833d20)
+++ b/perennial/js/common/execute.ts	(date 1769559565488)
@@ -67,6 +67,12 @@
  */
 function execute( cmd: string, args: string[], cwd: string, providedOptions?: ExecuteOptions ): Promise<string | ExecuteResult> {
 
+  // Windows support. `.cmd` files are supposed to be run with `cmd.exe` on windows, otherwise you fail without shell:true (a deprecated option now).
+  if ( cmd.endsWith( '.cmd' ) ) {
+    args = [ '/d', '/s', '/c', [ cmd, ...args ].join( ' ' ) ];
+    cmd = 'cmd.exe';
+  }
+
   const startTime = Date.now();
 
   const options: Required<ExecuteOptions> = _.merge( {
@@ -77,10 +83,7 @@
 
       // Provide additional env variables, and they will be merged with the existing defaults.
       // eslint-disable-next-line phet/no-object-spread-on-non-literals
-      env: { ...process.env },
-
-      // options.shell value to the child_process.spawn.
-      shell: getShellOption( cmd )
+      env: { ...process.env }
     },
 
     onStdout: null,
@@ -96,6 +99,7 @@
     let stdout = ''; // to be appended to
     let stderr = '';
 
+    console.log( cmd, args );
     const childProcess = child_process.spawn( cmd, args, _.assignIn( {
         cwd: cwd
       }, options.childProcessOptions
@@ -162,11 +166,6 @@
   } );
 }
 
-// shell:true is required for a NodeJS security update, see https://github.com/phetsims/perennial/issues/359
-// In this case, only bash scripts fail with an EINVAL error, so we don't need to worry about node/git (and in
-// fact don't want the overhead of a new shell).
-export const getShellOption = ( cmd: string ): boolean => cmd !== 'node' && cmd !== 'git' && cmd !== 'bash' && process.platform.startsWith( 'win' );
-
 class ExecuteError extends Error {
 
   public constructor(
Index: perennial/js/grunt/commonjs/gruntSpawn.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/perennial/js/grunt/commonjs/gruntSpawn.js b/perennial/js/grunt/commonjs/gruntSpawn.js
--- a/perennial/js/grunt/commonjs/gruntSpawn.js	(revision f749d9bf2f5096a715fa492a0897563bda833d20)
+++ b/perennial/js/grunt/commonjs/gruntSpawn.js	(date 1769559565458)
@@ -14,14 +14,20 @@
  * @author Jonathan Olson (PhET Interactive Simulations)
  */
 const child_process = require( 'child_process' );
-const isWindows = /^win/.test( process.platform );
 
 module.exports = function spawn( grunt, command, args, cwd, preHook ) {
   const done = grunt.task.current.async();
   const argsString = args.map( arg => `"${arg}"` ).join( ' ' );
+
+  // Windows support. `.cmd` files are supposed to be run with `cmd.exe` on windows, otherwise you fail without shell:true (a deprecated option now).
+  if ( command.endsWith( '.cmd' ) ) {
+    args = [ '/d', '/s', '/c', [ command, ...args ].join( ' ' ) ];
+    command = 'cmd.exe';
+  }
+
+  console.log( command, args );
   const spawned = child_process.spawn( command, args, {
     cwd: cwd,
-    shell: isWindows, // shell is required for a NodeJS security update, see https://github.com/phetsims/perennial/issues/359
     env: Object.create( process.env )
   } );
   preHook && preHook( argsString );
Index: perennial-alias/js/grunt/commonjs/gruntSpawn.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/perennial-alias/js/grunt/commonjs/gruntSpawn.js b/perennial-alias/js/grunt/commonjs/gruntSpawn.js
--- a/perennial-alias/js/grunt/commonjs/gruntSpawn.js	(revision f749d9bf2f5096a715fa492a0897563bda833d20)
+++ b/perennial-alias/js/grunt/commonjs/gruntSpawn.js	(date 1769558871995)
@@ -14,14 +14,12 @@
  * @author Jonathan Olson (PhET Interactive Simulations)
  */
 const child_process = require( 'child_process' );
-const isWindows = /^win/.test( process.platform );
 
 module.exports = function spawn( grunt, command, args, cwd, preHook ) {
   const done = grunt.task.current.async();
   const argsString = args.map( arg => `"${arg}"` ).join( ' ' );
   const spawned = child_process.spawn( command, args, {
     cwd: cwd,
-    shell: isWindows, // shell is required for a NodeJS security update, see https://github.com/phetsims/perennial/issues/359
     env: Object.create( process.env )
   } );
   preHook && preHook( argsString );