azure-sdk-for-js icon indicating copy to clipboard operation
azure-sdk-for-js copied to clipboard

[dev-tool] react to NodeJS `spawn` security fix

Open jeremymeng opened this issue 1 year ago • 4 comments

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.

jeremymeng avatar Apr 23 '24 23:04 jeremymeng

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

xirzec avatar Apr 24 '24 18:04 xirzec

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.

jeremymeng avatar Apr 24 '24 21:04 jeremymeng

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.

jeremymeng avatar Apr 24 '24 23:04 jeremymeng

Hi @jeremymeng. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

github-actions[bot] avatar Jun 28 '24 05:06 github-actions[bot]

Hi @jeremymeng. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

github-actions[bot] avatar Aug 30 '24 05:08 github-actions[bot]

Hi @jeremymeng. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.

github-actions[bot] avatar Sep 06 '24 08:09 github-actions[bot]