Start testing with Node 24 and use Node 22 during publish.

[iclanton]

No description provided.

[dmichon-msft]

(node:11789) [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.

[cssatkmd]

Doesn't the file "rushstack/libraries/rush-lib/assets/rush-init/rush.json" need to be updated as well?
I'm referring to the "nodeSupportedVersionRange", which is a template for the created rush.json file when running "rush init".

Also, side note: Is NodeJs version 22.12.x supported for now?

[iclanton]

@cssatkmd - we'll update that when 24 enters LTS, sometime in mid-October.

22 has been supported for about a year.

[daniel-kuhn-burda]

any news about node 24? it is LTS by now

[octogonz]

CI is failing with this message, which is printed to STDERR and therefore Rush interprets it as a failure:

(node:11990) [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.
(Use `node --trace-deprecation ...` to show where the warning was created)

[octogonz]

Invoking Rush with --trace-deprecation shows this callstack:

(node:8616) [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.
    at normalizeSpawnArguments (node:child_process:644:15)
    at Object.spawn (node:child_process:789:13)
    at Utilities._executeCommandInternalAsync (C:\Users\Owner\.rush\node-v24.11.1\rush-5.162.0\node_modules\@microsoft\rush-lib\dist\commons.js:11959:78)
    at Utilities.executeCommandAndCaptureOutputAsync (C:\Users\Owner\.rush\node-v24.11.1\rush-5.162.0\node_modules\@microsoft\rush-lib\dist\commons.js:11632:44)
    at Git._executeGitCommandAndCaptureOutputAsync (C:\Users\Owner\.rush\node-v24.11.1\rush-5.162.0\node_modules\@microsoft\rush-lib\dist\commons.js:2117:87)
    at Git._tryGetGitEmailAsync (C:\Users\Owner\.rush\node-v24.11.1\rush-5.162.0\node_modules\@microsoft\rush-lib\dist\commons.js:2063:41)
    at Git.tryGetGitEmailAsync (C:\Users\Owner\.rush\node-v24.11.1\rush-5.162.0\node_modules\@microsoft\rush-lib\dist\commons.js:2051:39)
    at Module.validateAsync (C:\Users\Owner\.rush\node-v24.11.1\rush-5.162.0\node_modules\@microsoft\rush-lib\dist\commons.js:497:31)
    at Module.validatePolicyAsync (C:\Users\Owner\.rush\node-v24.11.1\rush-5.162.0\node_modules\@microsoft\rush-lib\dist\commons.js:24022:60)
    at WorkspaceInstallManager.prepareAsync (C:\Users\Owner\.rush\node-v24.11.1\rush-5.162.0\node_modules\@microsoft\rush-lib\dist\chunks\InstallManagerFactory.js:636:69)

...which is referencing this code:

rushstack/libraries/rush-lib/src/utilities/Utilities.ts

Lines 821 to 843 in fb6faf2

	// This is needed since we specify shell=true below.
	// NOTE: On Windows if we escape "NPM", the spawnSync() function runs something like this:
	// [ 'C:\\Windows\\system32\\cmd.exe', '/s', '/c', '""NPM" "install""' ]
	//
	// Due to a bug with Windows cmd.exe, the npm.cmd batch file's "%~dp0" variable will
	// return the current working directory instead of the batch file's directory.
	// The workaround is to not escape, npm, i.e. do this instead:
	// [ 'C:\\Windows\\system32\\cmd.exe', '/s', '/c', '"npm "install""' ]
	//
	// We will come up with a better solution for this when we promote executeCommand()
	// into node-core-library, but for now this hack will unblock people:
	// Only escape the command if it actually contains spaces:
	const escapedCommand: string =
	command.indexOf(' ') < 0 ? command : Utilities.escapeShellParameter(command);
	const escapedArgs: string[] = args.map((x) => Utilities.escapeShellParameter(x));
	const childProcess: child_process.ChildProcess = child_process.spawn(
	escapedCommand,
	escapedArgs,
	options
	);

Of course we ARE correctly escaping the shell parameters in this code, so the warning is spurious.

[octogonz]

This situation is discussed in nodejs/node#58763 (comment) and the recommended fix is easy (albeit poorly documented):

Using shell isn't deprecated, args + shell is. A shell takes a string that can have one or more commands, metacharacters, newlines, etc. so the correct interface is to accept a single string as done with exec (which mirrors the C system call).

child_process.spawn("command", ["arg1", "arg2"], { shell: true, stdio: ["ignore", "pipe", "pipe"] })

You can just do child_process.spawn("command arg1 arg2", { shell: true, stdio: ["ignore", "pipe", "pipe"] }) instead.

The args parameter was deprecated because it gave the mistaken impression that it's safe to pass arguments this way. The args are not really arguments, they're just text concatenated with spaces i.e., the following would delete your root directory after running command:

child_process.spawn("command", ["&&", "rm -rf /"], { shell: true, stdio: ["ignore", "pipe", "pipe"] })

The issue was people were flicking shell to true and immediately introducing a vulnerability.

[octogonz]

The same warning is also printed for this code in _executeLifecycleCommandInternal():

rushstack/libraries/rush-lib/src/utilities/Utilities.ts

Line 682 in fb6faf2

return spawnFunction(shellCommand, [commandFlags, command], spawnOptions);

However in this case the API is not being used correctly:

let commandFlags: string = '/d /s /c';   🤦‍♂️

[octogonz]

If the above two spawn() warnings are fixed, then rush install and rush build will complete without printing any deprecation warnings, which should be sufficient to merge this PR.

@iclanton However, but note that our CI uses the last published Rush. So we would need to merge+publish the code fix prior to merging the updated CI YAML file. In other words, these two issues should probably be fixed in a separate PR that gets merged before PR 5379.

[octogonz]

Searching our codebase, I can find lots of other calls to .spawnSync() or .spawn() with shell: true, which will produce the same Node.js warning. Many of them look like genuine negligence in escaping (which is understandable, because that spawn() API signature does misleadingly imply that it is escaping, which is why Node.js has deprecated it). When fixing those cases, we should try to add proper escaping, but it can be a bit tricky because shell invokes different shells with different rules -- a deeply problematic requirement. That was the motivation for our Executable API. But some of these calls appear in contexts that cannot easily access the Executable API.