Properly escape extra args

[lydell]

Before you think “puka all over again? Been there, done that” and close the issue – no, this is not the same thing again 😄

npm run whatever -- some --more 'args, yeah!'
                    ^^^^ ^^^^^^ ^^^^^^^^^^^^^
                    what this PR is about

This PR:

• Fixes a regression in npx@7 and makes it intuitive again (fixes [BUG] npx doesn't pass $1 properly to executed package on Linux/Darwin cli#3067 and [BUG] npx@7 regression: command injection / impossible to pass a bactick as argument cli#3306)
• Fixes [BUG] JSON.stringify should not be used for shell escaping #14 – for real this time
• Fixes [BUG] Using -- to pass additional arguments incorrectly wraps those arguments in quotes cli#2720 – for real this time
• Is not the same as da14d3b (removed in remove puka #22). This only touches extra args passed after --, not the scripts in package.json themselves.
• Does things the same way as Yarn: fix(run): improve escaping for script arguments yarnpkg/yarn#4135
• …which means escaping extra args using puka instead of JSON.stringify *

* Note: We don’t have to use puka. Point is, the JSON.stringify needs to replaced by something.

Let me explain in more detail:

Fixes a regression in npx@7 and makes it intuitive again

Let’s say you have a create-blog-post CLI installed globally. You run it like so:

create-blog-post --title 'Cool `ls` tricks'

One day you install create-blog-post locally instead. Then how do you run it? Well, you could just slap npx at the start, right? Wrong! The following does not do what you expect:

npx create-blog-post --title 'Cool `ls` tricks'

Let me show why. I’m using node -p 'process.argv[2]' -- instead of create-blog-post to show that the implementation of that tool wouldn’t matter:

❯ node -p 'process.argv[2]' -- --title 'Cool `ls` tricks'
Cool `ls` tricks

With npx in front:

❯ npx node -p 'process.argv[2]' -- --title 'Cool `ls` tricks'
Cool LICENSE
README.md
lib
map.js
node_modules
package-lock.json
package.json
test tricks

Oops! The argument was treated as shell script, executed ls and put the result in my string (backticks means command interpolation)!

npx@6 got it right:

❯ npx --version
6.14.12

❯ npx node -p 'process.argv[2]' -- --title 'Cool `ls` tricks'
Cool `ls` tricks

The worst thing is that I don’t even know how to workaround this issue in npx@7. Trying to add backslashes does not help. I just can’t figure out a way to pass literal backticks as an argument.

(The reason npx broke in v7 is because now it’s just an alias for npm exec, which is basically just npm run under the hood: npx foo --bar baz ➡️ npm run npx -- --bar baz with an automatic "npx": "foo" in package.json.)

Fixes #14 – for real this time

#14 mentions that it’s wrong to use JSON.stringify for escaping shell script. Which is correct – shell script has different escaping rules than JSON, and need special ones on Windows.

The issue links to this line:

run-script/lib/run-script-pkg.js

Line 30 in 477a99c

cmd = pkg.scripts[event] + args.map(a => ` ${JSON.stringify(a)}`).join('')

This line uses JSON.stringify too:

run-script/lib/run-script-pkg.js

Line 40 in 477a99c

cmd = 'node server.js' + args.map(a => ` ${JSON.stringify(a)}`).join('')

These were the only occurrences of JSON.stringify at that time.

The issue was closed by da14d3b – but that commit changes neither of those lines! Instead it does something else, which is slightly related topic wise (shell script escape stuff) but for a completely different part of this package.

The commit links to #3, which in turn links to npm/cli#52 which is a PR (by the author of puka; closed because the code moved to this repo) that does the exact same thing as this PR. This gives my PR some precedent, and shows that some confusion were going on when issues were closed.

The two ocurrances of JSON.stringify exist today as well:

run-script/lib/run-script-pkg.js

Lines 34 to 44 in 47a4d53

	cmd = pkg.scripts[event] + args.map(a => ` ${JSON.stringify(a)}`).join('')
	else if ( // If there is no preinstall or install script, default to rebuilding node-gyp packages.
	event === 'install' &&
	!scripts.install &&
	!scripts.preinstall &&
	gypfile !== false &&
	await isNodeGypPackage(path)
	)
	cmd = defaultGypInstallScript
	else if (event === 'start' && await isServerPackage(path))
	cmd = 'node server.js' + args.map(a => ` ${JSON.stringify(a)}`).join('')

This PR finally gets rid of them. Hopefully this will be the end of the confusion!

Fixes npm/cli#2720 – for real this time

npm/cli#2720 is about double quotes being added around extra args passed to npm run with unexpected escaping, due to the JSON.stringify.

A comment in that issue claims that the issue was fixed by #22 – that’s not true, since that pull request doesn’t touch the JSON.stringify lines.

---

Pinging @rhendric (the author of puka, npm/cli#52 and yarnpkg/yarn#4135), in case you’re interested (if not, sorry for the disturbance!).

Thanks for reading! I hope we can get this fixed once and for all 🙏 Let me know what you think!

[lydell]

puka comes with a sh export (defined as const sh = (...args) => ShellString.sh(...args).toString() basically), but we’re making our own so we can test the Windows version more easily.

https://gitlab.com/rhendric/puka#shellstring

---

process.env.__FAKE_TESTING_PLATFORM__ || process.platform is copied from is-windows.js – let me know if you’d like to share that, and if so, how.

[lydell]

This is how Yarn does it:

https://github.com/yarnpkg/yarn/blob/3119382885ea373d3c13d6a846de743eca8c914b/src/cli/commands/run.js#L136

And how the original PR by the author of puka did it:

https://github.com/npm/cli/pull/52/files#diff-5fe49e56d3dc3f4bb6cead3ec153dbee2b55cc91f22bf991cac6546f6aa42623R172

[lydell]

This is inspired by Yarn’s tests: https://github.com/yarnpkg/yarn/blob/3119382885ea373d3c13d6a846de743eca8c914b/__tests__/integration.js#L424

[rhendric]

See also conversation in #17.

The original use of Puka as introduced in #17/commit da14d3b was, ah, not how I expected it to be used. The trouble is, on Windows, when executing commands that invoke (one or more!) batch files, Puka's argument escaping is best in class (I can't say ‘perfect’ because there's a known bug with escaping line breaks, but I don't think that's solvable); but this same escaping strategy is incorrect when invoking binaries (like, notably, node.exe). A less aggressive escaping strategy, which seems to be what da14d3b hacked into existence, works with node.exe but fails some edge cases when invoking batch files. But the less aggressive strategy fails on fewer cases when invoking batch files than Puka's native (more aggressive) strategy fails on when invoking binaries.

The best solution probably involves searching %PATH% for each invokee and determining its nature. I'm quite open to adding functionality like that to Puka if it would be used. But all of my past interactions with the npm team on this issue have been glacially slow, which frankly makes me less inclined to do a lot of speculative work like that ahead of getting some buy-in.

Less-best alternatives that I can think of include doing some special-casey logic looking specifically for node[.exe] in commands, or simply telling people to put their binary-invoking commands in separate script files if they want argument escaping to work.

[rhendric]

This could still have issues if node resolves to node.exe (and not some node.cmd or node.bat alias). server.js will probably see extra escaping sigils if args involves anything that needs escaping.

[lydell]

👍 I’ll test this on a Windows machine soon

[lydell]

I tested this. You are right: If node points to node.exe, the escaping is a problem.

I created server.js like this:

console.log(process.argv)

And ran this from the Node.js REPL:

> require("./lib/run-script-pkg")({event:"start",path:process.cwd(),pkg:{},args:['"foo"']}).then(r=>console.log(r.stdout.toString()), console.error)
Promise { <pending> }
> [
  'C:\\Program Files\\nodejs\\node.exe',
  'C:\\Users\\Simon\\stuff\\run-script\\server.js',
  '^\\^foo\\^^'
]

As one can see, server.js didn’t receive "foo" as expected but ^\\^foo\\^^.

However, if I install Node.js using npm i node, then server.js does receive "foo" since we go through npm’s batch script wrapper in ./node_modules/.bin/node.cmd.

I fixed this by shipping with a tiny node.cmd wrapper that we can use always. With it, I get this in the REPL, both with node.exe and after npm i node:

> require("./lib/run-script-pkg")({event:"start",path:process.cwd(),pkg:{},args:['"foo"']}).then(r=>console.log(r.stdout.toString()), console.error)
Promise { <pending> }
> [
  'C:\\Program Files\\nodejs\\node.exe',
  'C:\\Users\\Simon\\stuff\\run-script\\server.js',
  '"foo"'
]

Here’s the fix: d1c4c26 (#31)

Do you see any potential problems with this approach?

[rhendric]

Should work, although just to be safe maybe rename the wrapper to something like nodewrapper.cmd, or really anything other than node.cmd? If the directory containing the wrapper happened to be the working directory, or if it ever ended up in %PATH% somehow, you'd have an infinite loop on your hands.

[rhendric]

You could also do what cross-spawn does, which is to take a dependency on the which package, use that to find node, and check to see if it's an .exe or something else; and then use Puka's quoteForWin32 or quoteForCmd respectively to escape the arguments. Or just use cross-spawn for the whole thing, assuming its escaping is as complete as Puka's (I have no reason to believe it is or isn't).

[lydell]

Also, in theory npm can’t do any escaping at all on the args since it allows configuring which shell to use. For example, if someone configures to use fish – it has different escaping rules than sh, and npm can’t know which. But it would feel bad to put the burden on 99% (made up number) of the users who don’t configure the shell to “double escape” their args:

npx node -p 'process.argv[2]' -- --title \''Cool `ls` tricks'\'

---

Another way of solving most of this problem is to:

• Keep the half-sucky escaping for npm run extra args
• Don’t implement npm exec and npx in terms of npm run. Then we can use cross-spawn instead and avoid having to create strings of shell script code to execute.

[lydell]

Note: This means that npm start with no "start" in package.json but with a server.js will print something like this on Windows:

> C:\absolue\path\to\npm\node_modules\run-script\lib\nodewrapper.cmd server.js

instead of:

> node server.js

Let me know if you’d rather want to print > node server.js on Windows too (even though that’s not what we’re actually executing).

[nlf]

does this comment mean that if a user has a script that runs a command that is not a batch file, we will be escaping it wrong?

if i have

{
  "scripts": {
    "go": "node server.js"
  }
}

and i run npm run go -- some args, then we won't be using the batch file wrapper, so does that mean that we're now producing a command that doesn't work correctly? what if the first command is a shell builtin, like cd?

[lydell]

You’re right, this won’t be using the batch file wrapper. node is most likely an executable, so the escaping would be wrong (except in cases where people have installed node using npm i node). Dang, Windows escaping sucks :( See this comment for more details: #31 (comment)

Running node in a script like in your example feels like a very common use case, so it would be nice if that worked good.

Ideas:

• Research if yarn has bug reports about this (since they use puka)
  EDIT: There seems to be 2 open Yarn issues about puka + Windows + Git Bash:

  • Windows & Git Bash: yarn run fails due to missing slashes yarnpkg/yarn#7732
  • Git Bash windows arguments are incorrectly escaped yarnpkg/yarn#8000

  Those are worth considering, but it’s not really about the node executable, though.

• Try to come up with more clever Windows solutions

• Only use puka for non-Windows, and come up with some other thing for Windows or keep the current behavior

• Don’t implement npx and npm exec in terms of npm run: Then we can use cross-spawn instead and avoid having to create strings of shell script code to execute.

In summary: This is super easy to do right on macOS and Linux, and a minefield on Windows.

[lydell]

The more I think about this, the more I realize something:

• I don’t use -- extra args with npm run very much, so I don’t care super much about that. It’s a bit of an impossible task to append extra args to an unknown shell string.

• I do use npx a lot and would like it to be intuitive. I think implementing it in terms of npm run feels pretty hacky – it’s like creating a string and using eval() in JavaScript for something that can be done just fine in the language.

[nlf]

we've been doing a lot of discussing about this one, and i think we have a game plan for how to move forward. i'll share more as we further flesh out the plan, but it's going to involve some refactoring of both this module and promise-spawn and creating a net new module that will be used as the "run things how npm runs them" engine, including doing things like appropriately escaping commands and arguments.

The best solution probably involves searching %PATH% for each invokee and determining its nature. I'm quite open to adding functionality like that to Puka if it would be used. But all of my past interactions with the npm team on this issue have been glacially slow, which frankly makes me less inclined to do a lot of speculative work like that ahead of getting some buy-in.

once we have a plan drawn up, i would very much like to work with you on this @rhendric. i think you're right that this is likely to be the best solution, but i can also appreciate your comment about our team being extremely slow in the past. i'm going to write up exactly what we're hoping to do with puka as part of speccing out the new module and then hopefully you're willing to collaborate with me on making it reality.

i'll follow up here when i have something written up. thank you for taking the time to put so much thought, research and time into this issue @lydell and @rhendric!

[rhendric]

hopefully you're willing to collaborate with me on making it reality.

Looking forward to it!

[lydell]

Thanks!

For those following along, there’s a bit of a plan outlined by isaacs here: npm/cli#3067 (comment)

I’m happy to collaborate on better solution and eventually close this PR – it’ll serve as research and documentation in the mean time.

[nlf]

i've created a new repo at https://github.com/npm/exec and created some issues to track this work. npm/exec#2 contains the specifics of solving this problem for the new implementation.

[scscgit]

Can you please confirm if this is also related to one different issue of escaping, which makes npx impossible to use (and so a CLI tool, which is needed as a workaround for different issues, cannot be used whatsoever without first installing it)?

package.json ->

"scripts": {
    "a": "npx --package cross-env@latest cross-env-shell echo \\\"a|b\\\"",
    "b": "cross-env-shell echo \\\"a|b\\\"",
}

npm --version: 6.14.13, Windows 10

npm run a
> npx --package cross-env@latest cross-env-shell echo \"a|b\"

npx: installed 7 in 3.415s
'b' is not recognized as an internal or external command,
operable program or batch file.
npm ERR! code ELIFECYCLE
npm ERR! errno 255
npm ERR! ... a: `npx --package cross-env@latest cross-env-shell echo \"a|b\"`npm ERR! Exit status 255

npm run b 
> cross-env-shell echo \"a|b\"

"a|b"

npm --version: 7.20.0, Windows 10

npm run a
> npx --package cross-env@latest cross-env-shell echo \"a|b\"

'b\""' is not recognized as an internal or external command,
operable program or batch file.

npm run b     
> cross-env-shell echo \"a|b\"

"a|b"

[rhendric]

@scscgit, I would expect that resolving this issue will mean that those two specific scripts have the same behavior.

[scscgit]

@rhendric should I open this as a separate npx issue then, or do you mean same in the sense that it'll be fixed? And by any chance if you worked on improving the escaping, do you have some clues about what could be the root cause of this issue / if there even is any specification based on which this shouldn't happen? (Considering that npm 6 is no longer being developed, npm 7 has different issues, and there is no way to run npx 7 from npm 6, this makes quite a troublesome situation even if it's resolved in the future.) Thanks!

[rhendric]

I mean that the two scripts should work the same as each other, instead of exhibiting different behavior from each other as you described.

I hesitate to say ‘fixed’ only because, depending on the shell being used, the escaping strategy you're using to prevent the | character from being interpreted as a shell pipe (before it gets to npx) might fail. But if it does, it should (after the change we've discussed is implemented) fail in both scripts.

As to the root cause, it's pretty obvious: someone used JSON.stringify to escape arguments, which is a mistake on the order of using subtraction instead of division just because 4 / 2 = 4 − 2. Everyone involved knows it's wrong. It's harder to implement the right thing (primarily because Windows is such a mess), but once it's released, the ‘specification’ should be simple: what npm run-script or npm exec or npx receives as an argument, it repeats verbatim to whatever script it's running.

[hufftheweevil]

Is this PR still being considered/worked on?

Let's say I have a simple script:

{
  "scripts": {
    "open": "node"
  }
}

Using npm run open -- --h executes as node "-h" and thus does not execute properly. I would expect it to execute as node -h
Is that the same bug that this PR is trying to fix?
I'm using npm@8.1.2 and Windows 10

[CAndRyan]

I am experiencing the same issue as @hufftheweevil, although I'm using npm@6.13.4 (node@12.16.1).
yarn@1.22.0 has no issue running the exact same script defined in package.json.

e.g. package.json

{
    "scripts": {
        "lint": "eslint ./src --ext .jsx --ext .js",
        "lint:fix": "npm run lint -- --fix"
    }
}

Calling npm run lint:fix attempts to execute eslint ./src --ext .jsx --ext .js "--fix".
Calling yarn lint:fix successfully executes eslint ./src --ext .jsx --ext .js --fix

[connectdotz]

Adding another example to this issue (jest-community/vscode-jest#838): in our extension, we pass a regex string for a path dynamically, for example "c:\\src\\test\.tsx". In npm 6.x and yarn, this string is passed along without any manipulation and everything worked. However, in npm 7/8, the escape char got double escaped: "c:\\\\src\\\\test\\.tsx", which failed the run script.

I see quite a few issues relating to escaping/translating special characters in the run arguments. I know this is a hairy issue, especially considering cross-platforms. However, I wonder if we can simply restore -- to be the "pass along without any modification" indicator, as stated in npm 16.x:

The special option -- is used by getopt to delimit the end of the options. npm will pass all the arguments after the -- directly to your script.

This should be an easy fix as it did not involve any complicated cross-platform argument manipulation logic and should help resolve many related regression issues... thoughts?