Add shell=true when spawning cmd files in windows #4969
Conversation
| @@ -21,6 +21,9 @@ function run(command, args, options) { | |||
|
|
|||
| if (process.platform === "win32" && isCmdOnWindows.includes(command)) { | |||
| command += ".cmd"; | |||
| // When spawning .bat or .cmd files on windows, node will now error with EINVAL if shell is not set to true. | |||
| // https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2 | |||
| options.shell = true; | |||
There was a problem hiding this comment.
Be careful with this, you need to escape the args yourself if you pass some
There was a problem hiding this comment.
hmm... how deep do you think the change will go then?
There was a problem hiding this comment.
there is 3 options:
- One this isn't used anywhere where it is a problem so we can keep it
- Anywhere it is a problem we escape manually
- We use
cross-spawnpacakge instead but autorest still using rush it is very anoying to have top level dev dependency for scripts.
There was a problem hiding this comment.
was there something in particular that broke here?
There was a problem hiding this comment.
Should we push the escaping requirement to the caller and error here if they haven't already set shell=true?
There was a problem hiding this comment.
was there something in particular that broke here?
Autorest regeneration fails consistently on a single project in azure-sdk-for-net:
fatal | Process() cancelled due to failure
EXEC : error | error : spawn EINVAL [D:\a\_work\1\s\azure-sdk\sdk\resourcehealth\Azure.ResourceManager.ResourceHealth\src\Azure.ResourceManager.ResourceHealth.csproj]
error | Autorest completed with an error. If you think the error message is unclear, or is a bug, please declare an issues at https://github.com/Azure/autorest/issues with the error message you are seeing.
There was a problem hiding this comment.
Should we push the escaping requirement to the caller and error here if they haven't already set shell=true?
yeah we could do that,
so I doubt this will fix the error above. This file is only used for intenral scripts
|
You may test this build by running Add the following CLI flags
Or with all autorest --version:https://tinyurl.com/2639ambr --use:https://tinyurl.com/2dj6wno3or use the following in your autorest configuration: # For changes to autorest core
version: "https://tinyurl.com/2639ambr"
# For changes to modelerfour
use-extension:
"@autorest/modelerfour": "https://tinyurl.com/2dj6wno3"If this build is good for you, give this comment a thumbs up. (👍) And you should run `autorest --reset` again once you're finished testing to remove it. |
When spawning .bat or .cmd files on windows, node will now error with EINVAL if shell is not set to true.
https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2