npm / cli

the package manager for JavaScript
https://docs.npmjs.com/cli/
Other
8.34k stars 3.07k forks source link

[BUG] $EDITOR environment variable broken on Windows #6716

Open rotu opened 1 year ago

rotu commented 1 year ago

Is there an existing issue for this?

This issue exists in the latest npm version

Current Behavior

On Windows, if the EDITOR environment variable is set, npm config edit does not resolve it as either a literal path nor a shell command.

Expected Behavior

I expect either:

  1. Editor to be a path to an executable
  2. Normal shell-like resolution applies to the command (including escaping spaces)

Steps To Reproduce

  1. On windows 11, using PowerShell or the Environment Variable Editor, set the value of EDITOR
  2. Call npm config edit

Environment

prefix = "C:\Users\dan\AppData\Roaming\npm"

; "user" config from C:\Users\dan.npmrc

@cs:registry = "http://reg.example.com" //registry.npmjs.org/:_authToken = (protected)

; node bin location = C:\Program Files\nodejs\node.exe ; node version = v18.17.1 ; npm local prefix = C:\Users\dan\Source\Cerulean\SonarView ; npm version = 9.8.1 ; cwd = C:\Users\dan\Source\Cerulean\SonarView ; HOME = C:\Users\dan ; Run npm config ls -l to show all defaults.

rotu commented 1 year ago

I think this is related to a deeper issue with @npmcli/run-script. spawnWithShell seems VERY suspect:

  1. It finds the executable by looking at the string and naively scanning for quotes. This doesn't respect ^-escaped spaces.
  2. This quote-scanning doesn't match types of quotes. For instance, "'my double quoted path'" will open quotes at the first " character and then say the quotes have ended at the ' character.
  3. Finally, the path is passed to which.sync with the quotes intact. So it's looking for an executable with those quotes in its path.
  4. The isCmd regex searches for \ but / is a legal alternative path separator on windows.
  5. Shell can be passed in. It's not unlikely that, if the user is on Windows, they're using PowerShell. In that case, I don't know what's supposed to happen.

https://github.com/npm/promise-spawn/blob/d0e078944659d34ac883c8e108e01aeeb8d7e18a/lib/index.js#L65-L124