cli icon indicating copy to clipboard operation
cli copied to clipboard

[BUG] $EDITOR environment variable broken on Windows

Open rotu opened this issue 2 years ago • 2 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

This issue exists in the latest npm version

  • [X] I am using the latest npm

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
  • If the value of EDITOR has a space in it, it is truncated there (even if the executable is surrounded by ", meaning you can't use a path with a space in it. None of the following work:
# any of the below values
$env:EDITOR=(gcm code).source
$env:EDITOR="C:\Users\dan\AppData\Local\Programs\Microsoft VS Code\bin\code"
$env:EDITOR='"C:\Users\dan\AppData\Local\Programs\Microsoft VS Code\bin\code"'
$env:EDITOR="C:\Users\dan\AppData\Local\Programs\Microsoft^ VS^ Code\bin\code"
npm config edit

The error message indicates that the path is truncated at the first space, e.g.

npm config edit npm ERR! code ENOENT npm ERR! syscall spawn C:\Users\dan\AppData\Local\Programs\Microsoft npm ERR! path C:\Users\dan\AppData\Local\Programs\Microsoft npm ERR! errno -4058 npm ERR! enoent spawn C:\Users\dan\AppData\Local\Programs\Microsoft ENOENT npm ERR! enoent This is related to npm not being able to find a file. npm ERR! enoent

  • normal shell resolution of a command does not work:
$env:EDITOR="code"
  • The following does work:
$env:EDITOR="code.cmd"

Environment

  • npm: 9.8.1
  • Node.js: v18.17.1
  • OS Name: Windows 11
  • System Model Name:
  • npm config:
npm config ls
; "builtin" config from C:\Users\dan\AppData\Roaming\npm\node_modules\npm\npmrc

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 avatar Aug 17 '23 20:08 rotu

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

rotu avatar Aug 19 '23 00:08 rotu

npm edit and npm config edit don't use promise-spawn. The root cause is a split of the editor config setting by spaces:

https://github.com/npm/cli/blob/62c71e5128a01283f97bd62da30ddc673bddda0b/lib/commands/config.js#L286

https://github.com/npm/cli/blob/62c71e5128a01283f97bd62da30ddc673bddda0b/lib/commands/edit.js#L52

I suggest replacing these with the following:

      // On Windows, editor setting can contain spaces in path so we don't support
      // splitting off command line arguments
      const [bin, ...args] = process.platform === 'win32' ? [e, []] : e.split(/\s+/)

mbtools avatar Oct 17 '24 01:10 mbtools