[FEATURE] escape user provided arguments appropriately for the platform

[nlf]

What / Why

Currently we use JSON.stringify() on each provided user argument as an attempt to escape them, this causes a significant number of issues however. Notably passing literal bash variable names (i.e. npm run foo -- 'echo $1') does not work correctly, we also fail to correctly escape sequences like \n.

In order to resolve this, we plan to reintroduce puka.

These changes are inspired by the work of @lydell, who has kindly offered their continued collaboration on this problem, in this pull request, which was an excellent start but identified some problems with how puka does this escaping currently. Due to the lack of shebang support in Windows we place batch files (foo.cmd) for bins installed by npm rather than symlinks as we do for Linux/OSX. This shim requires an extra level of escaping to ensure the correct string is received by the node package being run. While puka currently does this double escaping correctly, it causes a problem when the command portion of a script is not a batch file (i.e. node.exe) where we only want one level of escaping.

The author of puka, @rhendric, has very kindly offered to help us with a solution for this by attempting to determine if the command being run is a batch script or not and adjusting the levels of escaping appropriately for Windows users.

The posix shell escaping appears to be sufficient in puka as it is today and does not appear to need any modification.

This issue will serve as the focal point for communication between the npm team, @rhendric and @lydell while they collaborate with us to get this implemented.

Implementation

As is implemented and discussed in the original run-script pull request, we will apply this escaping only to the user provided additional arguments. We will not attempt to coerce a script declared in a package.json to run on a platform other than the one it was designed for.

We have identified a condition matrix that we believe will give us a starting point:

• shell type:
  • posix sh-like (any differences between bash vs sh vs zsh?)
  • windows cmd (regular old binary)
  • windows cmd (batch file)
  • windows powershell? (aside: maybe we could fix some stuff by using powershell instead of bash? or maybe that'd make things worse? idk, but it is ubiquitous now.)
• command being run:
  • npx some-package some args
  • npm exec some-package some args
  • npx --package some-package some command
  • npm exec --package some-package some-command
  • npx --package some-package -c 'some-command'
  • npm exec --package some-package -c 'some-command'
  • npm run foo (where scripts.foo is some-command some-args)
  • npm run foo -- some-args
• funkiness of args (all of these are the unquoted actual string value in <code> block, so if you see ', then it has ' chars in it)
  • $1 (ie: get literal $1 as the arg)
  • "$1" (arg has double-quotes)
  • '$1' (arg has single-quotes)
  • \$1
  • --arg="$1"
  • --arg=npm exec -c "$1"
  • --arg=npm exec -c '$1'
  • '--arg=npm exec -c "$1"'

For each item in this matrix, we should identify the expected result, the current result, and any breakage that may be caused by replacing the current result with the expected one. Ideally this same matrix will be used to build a suite of tests that will help us ensure that we are doing the correct thing and we do not introduce regressions.

[rhendric]

You might want to add to your condition matrix:

• Running under WSL, as distinct from running normally under Windows with a POSIXy shell like Git/Cygwin bash (in theory, I think WSL should want and get the same escaping that the POSIX OSes get, but if OS detection is handled in some nonstandard way, I could imagine bugs here).
• Some more Windows-specific funky arguments—in particular, the characters ^ and % need Windows-specific attention, and %CD% or %PATH% is also a good thing to test (cmd.exe will do things like ignore a plain %, ignore %VAR% if VAR is not a defined environment variable, but replace %VAR% if it is). PowerShell might have its own set of special characters, if that ends up being included in the support matrix. (Puka's big list of Windows-specific unit tests is here. You don't necessarily need to copy all of that over into npm's test suite, but it gives you an idea of what I considered to be most fraught.)
• Don't forget to test some non-ASCII characters. Does cmd.exe support emojis yet?

[nlf]

You might want to add to your condition matrix:

great points! thanks for the tips! i'll have to figure out how to get WSL into our CI matrix if we need it, opened #8 to track that

i've got a first pass at a very very naive initial commit up and am starting to implement these feature issues and submitting pull requests as i go. my plan for this issue is to implement puka essentially in the same way that was done in the run-script pull request, and then start writing out tests covering the above matrix.

i'll do my best to identify the desired escaping in the tests such that they'll pass when puka works as we would like it to, i'm hopeful that will give you an advantage when tweaking puka itself @rhendric. i'll request a review from you on the pull request when i open it since i feel like you likely have a better grasp on expectations of escaping in Windows than anyone else i know.

[nlf]

👋 i've pushed a branch (see #12) that lays the ground work for integration and testing. i'm wondering if you folks might be willing to join the node-tooling slack so we can discuss next steps in a bit higher bandwidth. i'm happy to push forward with filling out the full test matrix described above, but i think it's pretty clear that you both understand the nuances involved a lot better than i do so i'd love to have a conversation about it first.

if you're willing, you can get yourself an invite to the slack here: https://devtoolscommunity.herokuapp.com
i'm in the #npm channel there or you can shoot me a dm