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

[BUG] inconsistent interpretation of backslash passed to npm scripts in windows #2638

Closed
highmtworks opened this issue Feb 6, 2021 · 8 comments
Labels
Bug thing that needs fixing platform:windows is Windows-specific Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release

Comments

@highmtworks
Copy link

I'm sorry to report a minor issue.

Tools built on node tend to use npm scripts in package.json as shortcuts of various tasks.
Passing paths of files or directories to such tools is a common use case,
but it sometimes meets a difficulty when the path contains backslashes in windows.

Current Behavior:

package.json

{
  "scripts": {
    "start": "node index.js \\"
  }
}

index.js

console.log(process.argv.slice(2));

npm start commands:

(1)> npm start
(2)> npm start -- \
(3)> npm start -- "\"
(4)> npm start -- "\""
(5)> npm start -- \\
(6)> npm start -- "\\"
(7)> npm start -- \a
(8)> npm start -- "\a"
(9)> npm start -- \\a
(10)> npm start -- "\\a"

in npm 7.5.2

(1)#-> [ '\\' ]
(2)#-> [ '\\', '\\\\' ]
(3)#-> npm ERR! String is missing a " character
(4)#-> npm ERR! String is missing a " character
(5)#-> [ '\\', '\\\\\\\\' ]
(6)#-> [ '\\', '\\\\' ]
(7)#-> [ '\\', '\\\\a' ]
(8)#-> [ '\\', '\\\\a' ]
(9)#-> [ '\\', '\\\\\\\\a' ]
(10)#-> [ '\\', '\\\\\\\\a' ]

in npm 6.4.11

(1)#-> [ '\\' ]
(2)#-> [ '\\', '"' ]
(3)#-> [ '\\', '"' ]
(4)#-> [ '\\', '"' ]
(5)#-> [ '\\', '\\' ]
(6)#-> [ '\\', '"' ]
(7)#-> [ '\\', '\\a' ]
(8)#-> [ '\\', '\\a' ]
(9)#-> [ '\\', '\\\\a' ]
(10)#-> [ '\\', '\\\\a' ]

Expected Behavior:

I hope the result is the same as that of corresponding node commands:

(1)> node index.js \
(2)> node index.js \ \
(3)> node index.js \ "\"
(4)> node index.js \ "\""
(5)> node index.js \ \\
(6)> node index.js \ "\\"
(7)> node index.js \ \a
(8)> node index.js \ "\a"
(9)> node index.js \ \\a
(10)> node index.js \ "\\a"

in both npm 7.5.2 and npm 6.4.11

(1)#-> [ '\\' ]
(2)#-> [ '\\', '\\' ]
(3)#-> [ '\\', '"' ]
(4)#-> [ '\\', '"' ]
(5)#-> [ '\\', '\\\\' ]
(6)#-> [ '\\', '\\' ]
(7)#-> [ '\\', '\\a' ]
(8)#-> [ '\\', '\\a' ]
(9)#-> [ '\\', '\\\\a' ]
(10)#-> [ '\\', '\\\\a' ]

but it looks quite hard to resolve.

Steps To Reproduce:

see Current Behavior section.

Environment:

  • OS: Windows 10 Pro 2004 (19041.746)
  • Node: 15.8.0
  • npm: 7.5.2
@highmtworks highmtworks added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Feb 6, 2021
@wandyezj
Copy link

I hit a similar issue:

In the following repository: https://github.com/wandyezj/package

on windows in cmd.exe

using npm version 6.14.9

the following script works npm run prettier

which is keyed to:
./node_modules/.bin/prettier

upon upgrading to npm version 7.5.4 via npm install -g npm

running npm run prettier

now errors with: '.' is not recognized as an internal or external command,

@ljharb
Copy link
Collaborator

ljharb commented Feb 13, 2021

(while that should work, in a run-script it should just be keyed to “prettier” - hardcoding node_modules anywhere is a code smell)

@wandyezj
Copy link

wandyezj commented Feb 13, 2021

There are some significant benefits to the above approach:

  • Using a fixed prettier version (same for other programs) in the node_modules allows everyone to use the same prettier or other tool versions when they operate on the repository.
  • in azure automation workflows it only requires node and npm to be installed by default, all other scripts can simply be run after and npm install of the repository and if the tool versions are updated no changes are needed to the pipeline workflow.
  • It also avoid cluttering the global namespace with tools.

Note: the above issue is caused due to a global NPM version that varies across machines, precisely the problem the above approach avoids. NPM is somewhat of an exception since it needs to be globally installed so it can install other packages.

I could create a npm.cmd / npm.sh script that references a specific NPM version in the node_modules and falls back to the global install if npm is not present, but this seems like something that should be supported by default instead of something that needs to be hacked to work around.

What is your recommendation of how to achieve the above scenario where I want to version and run tools with the package and have these scripts present in the package scripts instead of relying on global installs of those tools?

How would you recommend I adjust my script to achieve the above scenario?

Would it be possible to add a unit test to verify that the above functionality continues to work? This significantly disrupted an existing workflow and cost me several hours to debug.

Perhaps I am misunderstanding, does npm search first in the node_modules/.bin for scripts to execute instead of the global path? (I'm not sure that would be a good practice from a security perspective as a module could overwrite common commands...)

@ljharb
Copy link
Collaborator

ljharb commented Feb 13, 2021

npx will always use the local prettier if it’s installed, so i don’t think you need to hardcore the path at all.

@wandyezj
Copy link

wandyezj commented Feb 14, 2021

I removed the absolute relative path ./node_modules/.bin/prettier and used prettier instead.

I tested with npm run prettier in the package directory. When prettier is installed as part of in the local package it does indeed call that prettier version first.

However, if a local version is not installed and a global version is installed npm install -g prettier, then the global version will be called, similarly if there is another tool called prettier on the path that tool will be called.

I'd like to avoid the possibility of calling a globally installed tool entirely, for the reasons mentioned above. I know this seems like nitpicking, but it's a real issue when working across multiple developers' environments (It would be awesome for everyone to set up local machine development environments the same way, but realistically that's not going to happen (The entire reason for the above thread is because different developers had different npm versions installed)).

Is there a way to ensure that only the version in the local node_modules/.bin is called?

@ljharb
Copy link
Collaborator

ljharb commented Feb 14, 2021

In that case no, hardcoding the path is the only way to do that afaik, but then i think you might lose platform path portability (since non-WSL windows uses the bin with “.cmd” on the end)

@wandyezj
Copy link

wandyezj commented Feb 14, 2021

Ahh... Thank you for the clarification @ljharb, I suppose I will have to wait for this issue to be resolved in the latest version of NPM, before upgrading.

note: This works fine on Windows as cmd.exe has a precedence list for executing items in the path without the extension's presence by looking for the command without the extension (i.e. hello will call hello.exe, hello.bat, hello.cmd, etc.. in that order as it searches the path)

@wraithgar
Copy link
Member

The escaping that npm does was greatly fixed in npm/run-script#22, and we have another effort in place to be more consistent across the cli in parsing and escaping things when we spawn scripts, that effort is happening in the new https://github.com/npm/exec repo.

I believe the first PR linked fixed the issue described here. If this is still an issue please feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing platform:windows is Windows-specific Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

No branches or pull requests

5 participants