Skip to content

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

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

Open
iclanton wants to merge 4 commits into
base: main
Choose a base branch
from
+19 −6
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa3d1e2
Start testing with Node 24.
iclanton Sep 28, 2025
185621c
Use Node 22 during publish.
iclanton Sep 28, 2025
91388ed
fixup! Start testing with Node 24.
iclanton Dec 7, 2025
e169c53
Bump Node support in rush-lib and rush init.
iclanton Dec 7, 2025
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: 5 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,11 @@ jobs:
- NodeVersion: 22.19.x
NodeVersionDisplayName: 22
OS: ubuntu-latest
- NodeVersion: 22.19.x
NodeVersionDisplayName: 22
- NodeVersion: 24.11.x
NodeVersionDisplayName: 24
OS: ubuntu-latest
Copy link
Collaborator

@octogonz octogonz Dec 2, 2025

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

@octogonz octogonz Dec 2, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@octogonz octogonz Dec 2, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@dmichon-msft dmichon-msft Dec 3, 2025

Choose a reason for hiding this comment

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

Or as an alternative phrasing, we should still set shell: false and invoke the shell ourselves rather than telling Node to implicitly do so.

- NodeVersion: 24.11.x
NodeVersionDisplayName: 24
OS: windows-latest
Copy link
Collaborator

@octogonz octogonz Dec 2, 2025

Choose a reason for hiding this comment

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

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.

name: Node.js v${{ matrix.NodeVersionDisplayName }} (${{ matrix.OS }})
runs-on: ${{ matrix.OS }}
Expand Down
10 changes: 10 additions & 0 deletions common/changes/@microsoft/rush/node-24_2025-12-07-09-31.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@microsoft/rush",
"comment": "Add support for Node 24 and bump the `rush init` template to default to Node 24.",
"type": "none"
}
],
"packageName": "@microsoft/rush"
}
2 changes: 1 addition & 1 deletion common/config/azure-pipelines/templates/install-node.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
parameters:
- name: NodeMajorVersion
type: number
default: 20
default: 22

steps:
- task: NodeTool@0
Expand Down
2 changes: 1 addition & 1 deletion libraries/rush-lib/assets/rush-init/rush.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
* LTS schedule: https://nodejs.org/en/about/releases/
* LTS versions: https://nodejs.org/en/download/releases/
*/
"nodeSupportedVersionRange": ">=22.20.0 <23.0.0",
"nodeSupportedVersionRange": ">=24.11.1 <25.0.0",

/**
* If the version check above fails, Rush will display a message showing the current
Expand Down
2 changes: 1 addition & 1 deletion libraries/rush-lib/src/logic/NodeJsCompatibility.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { RushConstants } from './RushConstants';
* LTS schedule: https://nodejs.org/en/about/releases/
* LTS versions: https://nodejs.org/en/download/releases/
*/
const UPCOMING_NODE_LTS_VERSION: number = 22;
const UPCOMING_NODE_LTS_VERSION: number = 24;
const nodeVersion: string = process.versions.node;
const nodeMajorVersion: number = semver.major(nodeVersion);

Expand Down
2 changes: 1 addition & 1 deletion rush.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
* LTS schedule: https://nodejs.org/en/about/releases/
* LTS versions: https://nodejs.org/en/download/releases/
*/
"nodeSupportedVersionRange": ">=18.15.0 <19.0.0 || >=20.9.0 <21.0.0 || >=22.12.0 <23.0.0",
"nodeSupportedVersionRange": ">=18.15.0 <19.0.0 || >=20.9.0 <21.0.0 || >=22.12.0 <23.0.0 || >=24.11.1 <25.0.0",

/**
* If the version check above fails, Rush will display a message showing the current
Expand Down
Loading