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

[dev-tool] react to NodeJS spawn security fix #29414

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jeremymeng
Copy link
Contributor

@jeremymeng jeremymeng commented Apr 23, 2024

Node made a breaking change in a security release for 18/20 where spawn() no longer loads .cmd files by default. nodejs/node#52554.

This PR sets the shell: true option when running vendored command on Windows.

Node made a breaking change in a security release for 18/20 where `spawn()` no
longer loads `.cmd` files by default. nodejs/node#52554.

This PR adds the `shell: true` option to execute command in shell.
@jeremymeng jeremymeng marked this pull request as ready for review April 23, 2024 23:44
@xirzec
Copy link
Member

xirzec commented Apr 24, 2024

The only downside to this change is when shell: true is set if there are strings passed from input/etc it opens us up to injection attacks.

Rush has some code to harden these kinds of calls that maybe we could consider porting over? https://github.com/microsoft/rushstack/blob/5d9c506caec86b1d2b979703c893b67481451bb5/libraries/node-core-library/src/Executable.ts#L674

@jeremymeng
Copy link
Contributor Author

The only downside to this change is when shell: true is set if there are strings passed from input/etc it opens us up to injection attacks.

Rush has some code to harden these kinds of calls that maybe we could consider porting over?

Good point! I'd thought these are only internal tools maybe low risk but safer is always better.

revert other changes as they are not affected.
@jeremymeng
Copy link
Contributor Author

Looks that only run vendored command is affected (.cmd and Windows). I revert changes to other files, and adopted the fix that rush did for their install-run.ts
in https://github.com/microsoft/rushstack/pull/4637/files

I took a look at https://github.com/microsoft/rushstack/blob/5d9c506caec86b1d2b979703c893b67481451bb5/libraries/node-core-library/src/Executable.ts#L710. It looks more generic, in addition to adding options, it also spawns cmd.exe. I think it's too much for our limited scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants