zkat / npx

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

Windows: npx is not escaping paths correctly #84

Closed zkat closed 7 years ago

zkat commented 7 years ago

Ref: https://github.com/zkat/npx/issues/62#issuecomment-315564704

I'm confused 'cause this should totally be the right way to escape things for child_process.exec. Note that C:\"Program Files"\... is there, which should be the way to escape paths on Windows.

I'm thinking it's possible that this sort of escaping only applies when you're dealing with the binary path itself, and arguments don't get processed the same way and thus need to be escaped as args?

katemihalikova commented 7 years ago

Right. What if npx just quotes whole args and all of them? "C:\Program Files\nodejs\node.exe" "C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js" "-v" works just fine in cmd.exe.

zkat commented 7 years ago

I think that's what needs to be done, yeah.

simonua commented 7 years ago

Thanks a lot for keeping on this! Pointing out the obvious but just so you know for certain what my setup looks like:

Node is located at C:\Program Files\nodejs\node.exe (spaces in path) npm is located at C:\Users\[userid]\AppData\Roaming\npm\npm.cmd (no spaces in path)

zkat commented 7 years ago

Let's see if https://github.com/zkat/npx/pull/87 works now?

katemihalikova commented 7 years ago

It's working fine for me. Works in both cmd.exe and git bash.

Since my npm path doesn't contain spaces, I've tried with npx --npm="C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js" cowsay hi and it's also working fine.

simonua commented 7 years ago

Nice! =) Thank you kindly, @zkat and @katemihalikova!

image

simonua commented 7 years ago

@zkat, sorry, quick note for the back of your mind: Looks like it should say "installed 5 packages in 21.205s.", yes?

zkat commented 7 years ago

lol probably, but adding a single word to that string involves updating 17 other translations, many to languages I don't even know how to touch 😂

simonua commented 7 years ago

And pluralization. =P

scambier commented 6 years ago

@aivo0 for information, I solved it by upgrading to node v8.12.0