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

feat: adds environment variable to allow PowerShell executable to be specified #2512

Closed
wants to merge 1 commit into from

Conversation

brianpursley
Copy link

@brianpursley brianpursley commented Oct 5, 2021

Checklist
Description of change

The Add-Type powershell command in findVisualStudio2017OrNewer() can fail with early versions of PowerShell (<=5.x) in a restricted corporate environment due to permission denied on a temp folder in ~\AppData\Local\Temp. This is not a problem in node-gyp itself, but rather a limitation of legacy versions of PowerShell in a restricted environment.

Newer versions of PowerShell (7.0 for example) do not have this problem and are able to perform the Add-Type command without any problems in a restricted environment.

This PR adds a new check for a new environment variable (NODE_GYP_POWERSHELL_PATH) which can be used to specify a different PowerShell executable to use, if you want.

Example usage:

$env:NODE_GYP_POWERSHELL_PATH = "C:\Program Files\PowerShell\7\pwsh.exe"
npm i

NOTE: This is different than the problem addressed by #2449. I'm looking for a way to use Powershell 7, which is an entirely different executable than powershell.exe.

@cclauss
Copy link
Contributor

cclauss commented Oct 6, 2021

Are there new security exposures as a result of the proposed changes?

@cclauss cclauss requested a review from joaocgreis October 6, 2021 04:46
@brianpursley
Copy link
Author

brianpursley commented Oct 6, 2021

Are there new security exposures as a result of the proposed changes?

I don’t think there are any new security exposures, as this is similar to how node-gyp uses the PYTHON environment variable.

It does allow someone to potentially put any arbitrary executable’s path in this variable and node-gyp will execute it while searching for visual studio. I don’t think it would be an executable they can’t already execute themselves though.

Would you consider this to be a vulnerability?

As an alternative, I could change the new environment variable to something like NODE_GYP_POWERSHELL_VERSION and let you set it to something like 6 or 7 instead of the full path, and then the code would only look for the executable in a well known, whitelisted, location. A little bigger change, but it would be more restrictive.

Another alternative might be to not add any new environment variable and change node-gyp to search for powershell like it does for Python, prefer a newer version of powershell if it is found, and fallback to the current powershell.exe if nothing is found. I don’t see a problem with this approach personally, but I was hesitant to suggest this as it would change the default behavior of node-gyp. Edit: well I don’t think the observed behavior would be different, just that this would be the new default way of working under the hood.

@cclauss cclauss added the Windows label Oct 6, 2021
@brianpursley
Copy link
Author

brianpursley commented Oct 12, 2021

The more I think about it, the more I wonder if this should just be some sort of automatic fallback instead.

I originally thought the environment variable would be a good idea because it would let people decide if they want to use a different version of powershell, but node-gyp already has quite a few environment variables and it does make things more complicated for people.

What do you think about something like this?

  1. Try to execute the command using powershell.exe like it does currently
  2. If powershell.exe fails, then automatically try to execute the same thing using pwsh.exe
  3. If both fail, then print the message, like it does today when powershell.exe fails.

Edit to add: I probably should have just opened an issue on this instead of going straight to a PR, so sorry about that.

I am happy to update this PR if there is a preferred approach, or if you'd like me to close this and open an issue, that's fine too, so just let me know what is best here.

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

Successfully merging this pull request may close these issues.

2 participants