azure-sdk-for-js
azure-sdk-for-js copied to clipboard
[dev-tool] react to NodeJS `spawn` security fix
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.
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
The only downside to this change is when
shell: trueis 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.
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.
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.
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.
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.