search

zkat / npx

execute npm package binaries (moved)
https://github.com/npm/npx
Other
2.63k stars 105 forks source link

fix(exec): fixed unix binary pathing issues #120

Closed chrmoritz closed 7 years ago

chrmoritz commented 7 years ago

with special characters, which were incorrectly escaped with surrounding quotation marks causing child_process.spawn to throw an ENOENT error.

While upgrading npm in our homebrew formulaes to v5.4.2 we found out, that our npx test started to regress on our versioned formulas (node@6 and node@4) with the following error: spawn '/usr/local/Cellar/node@6/6.11.4/bin/node' ENOENT (Refs: https://github.com/Homebrew/homebrew-core/pull/19348#issuecomment-336063778). After bisecting npx versions I've found out that the regression was introduced between npx@9.2.0 and npx@9.2.1 in https://github.com/zkat/npx/commit/761dfe9bd73e0aca8170d23187e4fbce0ef7a647. This commit changed the path passed as the (first) command argument to child_process.spawn in installPackages from being the path to npm to being the path to the node executable.

This issue was only reproducible for our versioned formulas because only these contain a special character in their path to node (e.g. /usr/local/Cellar/node@6/6.11.4/bin/node) and only in this case child.escapeArg will surround the path with single quotation marks (e.g.npmPath === '\'/usr/local/Cellar/node@6/6.11.4/bin/node\''). Unfortunately child_process.spawn doesn't accept paths surrounded with quotation marks (') on Unix and throws an spawn '/usr/local/Cellar/node@6/6.11.4/bin/node' ENOENT error (Note that the ' do not mark the beginning or ending of the path string here, but are actual considered part of the actual path.)

This PR fixes the issue by only running child.escapeArg on Windows (where it is needed to support for examples paths containing spaces, but on Unix even spaces in the path string aren't a problem for child_process.spawn).

An alternative fix would be to remove the quotations marks in line 84 in child.escapeArg, but because child.escapeArg is also used in other places in the code base, I'm not sure if this would cause other side effects.

ilovezfs commented 7 years ago

@zkat any chance of getting this upstreamed?

zkat commented 7 years ago

Could I get a test for this? There's been a lot of thrash around how paths are escaped on various platforms and I'd feel better if I had stuff that covered this corner case for next time it gets poked at.

chrmoritz commented 7 years ago

Unfortunately I'm not able to find a proper way to write a test for this, because the affecting string is directly read from process.argv[0].

Edit: Maybe it's possibly by manipulating process.argv[0] in the tests. But I'm not sure how error prone this is, because another test could call child.spawn and fail during the small time window between manipulation process.argv[0] and reverting it back.

ilovezfs commented 7 years ago

Thanks @chrmoritz and @zkat :heart: