fix(exec): Fails to Execute Binaries Named After Shell Keywords

[13sfaith]

The best example of this bug is in the original issue (#8190). Basically, if an exec command has a shell keyword (i.e npx select) it will trigger a bash syntax error.

Things I did:

1. Added a test that fails to execute a binary named after a shell keyword (select)
2. Wrap all execution commands (after handling parsing for locations/downloads/etc) in quotes to guarantee a programs execution and not a bash parse

This is my first PR on this project so please let me know if I missed something! I'm happy to make whatever changes we need to get it fixed!

Thanks!

References

Fixes #8190

[wraithgar]

At a glance this appears to escape no matter the environment. Wouldn't this break osx/linux?

[13sfaith]

At a glance this appears to escape no matter the environment. Wouldn't this break osx/linux?

I tested this in both osx and Linux. I was, however, concerned with it breaking in windows cmd/powershell.

I have a windows machine as well, let me try and run it on there see if it breaks. If it does I'll add an environment check for that.

Thanks for taking a look. I'll update shortly.

[13sfaith]

Yeah, it did fail on windows/powershell. This check fixes that. All tests now pass.

Works on:

• OSX
• Linux (Ubuntu and WSL)
• Windows 11 (cmd and PowerShell)

[13sfaith]

Alright I had forgotten about the libnpmexec test coverage and eslint. My bad, my latest commit should resolve these issues.

[13sfaith]

so close... I did not realize my windows test wasn't passing arguments. It wasn't a problem on my mac because there are other tests that do pass arguments that go through exec.

I'm feeling good that it should work now.

[owlstronaut]

@13sfaith I think this works in the simple test case, but I think @wraithgar initial concern still stands. It's perfectly plausible that users are already passing quoted arguments, and it seems this will cause those to be double quoted. I set up a quick project locally to test your change:

# test-script.js
#!/usr/bin/env node
const fs = require('fs');
const args = process.argv.slice(2); // Get command-line arguments
fs.writeFileSync('args.log', JSON.stringify(args, null, 2)); // Write args to a log file

{
  "name": "npm-quote-test",
  "version": "1.0.0",
  "main": "test-script.js",
  "bin": {
    "test-script": "./test-script.js"
  }
}

npm link && npm exec test-script '"argument with spaces"' without this change:

[
  "\"argument with spaces\""
]

With the change

[
  "\"\"argument with spaces\"\""
]

I think this has the potential to break too many things as it is

[13sfaith]

Thank you @owlstronaut! Let me take a look and get a more comprehensive solution.

[13sfaith]

Thank you @owlstronaut your comment helped me realize I could simplify my approach.

The initial issue was that even with quotes the exec would fail because npm strips the quotes (no matter how many).

By adding the quotes around just the process path (i.e arg[0]) we can both solve the original issue and your concern!

[owlstronaut]

@13sfaith fyi I'm going to have a hard time getting over the mental hurdle of wrapping this in quotes as being the fix we want -- even when these tests pass. It feels like codifying a work-around. Something like direct execution by absolute path feels a lot more robust to me

[13sfaith]

I understand @owlstronaut let me look into options for that.

[13sfaith]

@owlstronaut I have looked a bit into it and I wonder if converting to the full path is the best solution.

The logical flow falls into this function call:

const p = promiseSpawn(spawnShell, spawnArgs, spawnOpts, {
    event,
    script: cmd,
    pkgid: pkg._id,
    path,
  })

At this point we have an environment (set in spawnOpts) that includes a list of all possible places the executing program can be. As an example for me when I run a simple program (the one listed in the original issue) I get the following environment:

'/Users/spencerfaith/programming/npx-shell-keyword-issue/node_modules/.bin:/Users/spencerfaith/programming/node_modules/.bin:/Users/spencerfaith/node_modules/.bin:/Users/node_modules/.bin:/node_modules/.bin:/Users/spencerfaith/programming/npm-cli/node_modules/@npmcli/run-script/lib/node-gyp-bin:<my default path>'

Once we create this environment we then pass it into a shell and have the shell find where the program is. If the program doesn't exist then it will handle that failed search.

In order to implement a solution which passes the full path we will essentially have to redo what the shell is already doing; searching through a list of directories to find the first matching program. This of course wouldn't be technically very difficult but it does leave a lot of space for things to go wrong. Many more tests will have to be written and more infrastructure built out to get the same result as the existing solution.

I, of course, am happy to oblige but wanted to bring to your attention the potential pitfalls of implementing a solution that performs this search and final path building.

[owlstronaut]

@13sfaith Thanks for doing some digging! Let me discuss with the team and get back to you on this

[owlstronaut]

@13sfaith Leaning towards accepting your contribution once the linting is cleaned up - and I'm going to play around with it a little more. Definitely agree we don't want to go down the full path solution, thank you for investigating that.

[13sfaith]

Awesome @owlstronaut. I'm happy I asked before building it. I have fixed the linting issue and ran the tests on my main machine (mac) I hope the checks should pass.

Please let me know if there is anything you'd like me to add or tweak. Obviously I don't want any contribution let alone my first to be hurting the integrity of the codebase.

[owlstronaut]

I've done a battery of manual testing on this and it seems to work well

[13sfaith]

Woohoo! Thank you @owlstronaut