Skip to content
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.

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

[rush] child_process.spawnSync on windows must set shell: true when targeting a .cmd file #4637

Merged
merged 7 commits into from
Apr 12, 2024

Conversation

ben-polinsky
Copy link
Contributor

@ben-polinsky ben-polinsky commented Apr 11, 2024

Summary

Adapt to Node.js security release, which disallows .bat and .cmd file spawning without enabling the shell option. As @dmichon-msft notes below, Rush invokes npm bin stubs which are .CMD files on Windows.

Fixes #4636

Details

Add shell: true to child_process.spawnSync options when running in Windows, and ensure any path or binstub invoked is formatted appropriately.

How it was tested

On both MacOS and Windows:

  • Before any changes, reproduced the issue on Windows locally.
  • Changes were made in rushstack repo and rebuilt.
  • Ran node ./libraries/rush-lib/dist/scripts/run-rush-install.js install and node ./libraries/rush-lib/dist/scripts/run-rush-install.js rebuild successfully.
  • On Windows, initially encountered the above path issue which was resolved by formatting all windows invoked paths.

@ben-polinsky
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Bentley Systems"

@dmichon-msft dmichon-msft changed the title [rush] child_process.spawnSync on windows must set shell: true [rush] child_process.spawnSync on windows must set shell: true when targeting a .cmd file Apr 11, 2024
Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

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

Please call out that this is expressly because we are invoking an npm bin stub, since npm bin stubs on Windows are .CMD files, and Node.js will now refuse to directly invoke a .CMD file without setting shell: true.

@ben-polinsky ben-polinsky changed the title [rush] child_process.spawnSync on windows must set shell: true when targeting a .cmd file [rush] adapt to node restricting how cmd can be invoked Apr 12, 2024
@ben-polinsky
Copy link
Contributor Author

Please call out that this is expressly because we are invoking an npm bin stub, since npm bin stubs on Windows are .CMD files, and Node.js will now refuse to directly invoke a .CMD file without setting shell: true.

Thanks - PR title and description updated.

@ben-polinsky ben-polinsky changed the title [rush] adapt to node restricting how cmd can be invoked [rush] child_process.spawnSync on windows must set shell: true when targeting a .cmd file Apr 12, 2024
@dmichon-msft dmichon-msft merged commit 34a862d into microsoft:main Apr 12, 2024
5 checks passed
@ben-polinsky ben-polinsky deleted the fix-spawnsync-windows branch April 12, 2024 20:27
fannheyward added a commit to neoclide/coc.nvim that referenced this pull request Apr 19, 2024
Closes neoclide/coc-prettier#178

Latest version of Node.js needs to set `shell: true` to spawn option

- node-red/node-red#4652
- microsoft/rushstack#4637
fannheyward added a commit to neoclide/coc.nvim that referenced this pull request Apr 19, 2024
Closes neoclide/coc-prettier#178

Latest version of Node.js needs to set `shell: true` to spawn option

- node-red/node-red#4652
- microsoft/rushstack#4637
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[rush] Latest Node LTS security patch for spawnSync breaks rush on Windows
2 participants