Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] npx@7 regression: command injection / impossible to pass a bactick as argument #3306

Closed
1 task done
lydell opened this issue May 25, 2021 · 4 comments
Closed
1 task done
Labels
Bug thing that needs fixing cmd:run-script related to `npm run-script` Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release Release 8.x work is associated with a specific npm 8 release

Comments

@lydell
Copy link

lydell commented May 25, 2021

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

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)!

Expected Behavior

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.

Steps To Reproduce

  1. macOS or Linux (Windows have different issues)
  2. With this config...
  3. Run npx node -p 'process.argv[1]' '`' (tested in sh, bash, zsh, fish)
  4. See error: sh: -c: line 0: unexpected EOF while looking for matching ``'

Environment

  • OS: macOS Big Sur (also happens on any Linux)
  • Node: 16.1.0
  • npm: 7.11.2

Fix

npm/run-script#31

I’m posting an issue here as well so the PR has something to close 😄 And also to help people who have encountered the same problem can more easily find this.

@lydell lydell added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels May 25, 2021
@balazs4
Copy link

balazs4 commented Dec 26, 2021

another usecase, same problem

antonmedv/fx#176

I'm looking forward to npm/exec#2

@ruyadorno ruyadorno added cmd:run-script related to `npm run-script` Priority 2 secondary priority issue Release 8.x work is associated with a specific npm 8 release and removed Needs Triage needs review for next steps labels Mar 2, 2022
@ruyadorno
Copy link
Contributor

cc @nlf who has been following up with npm/run-script#31

@nlf
Copy link
Contributor

nlf commented May 5, 2022

I'm unable to reproduce this in npm@8.9.0. I'm going to close this issue, but please do open a new one if you see this happen again.

@nlf nlf closed this as not planned Won't fix, can't repro, duplicate, stale May 5, 2022
@lydell
Copy link
Author

lydell commented May 5, 2022

@nlf Can you reopen this one, please? This is still an issue in 8.9.0.

❯ docker run --rm -it node:18 bash
Unable to find image 'node:18' locally
18: Pulling from library/node
6aefca2dc61d: Already exists
967757d56527: Already exists
c357e2c68cb3: Already exists
c766e27afb21: Already exists
32a180f5cf85: Already exists
3507b5066a40: Already exists
3c68557c340d: Pull complete
925b4ef5803f: Pull complete
3c263125b4e4: Pull complete
Digest: sha256:71c779ea8a157e6efadcafdae79f00fb39b7f3fdb2819ebef3984315c281540f
Status: Downloaded newer image for node:18
root@c99362005ff4:/# npm -v
8.8.0
root@c99362005ff4:/# npx node -p 'process.argv[2]' -- --title 'Cool `ls` tricks'
Cool bin
boot
dev
etc
home
lib
lib64
media
mnt
opt
proc
root
run
sbin
srv
sys
tmp
usr
var tricks
root@c99362005ff4:/# npm i -g npm

changed 15 packages, and audited 202 packages in 7s

11 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
root@c99362005ff4:/# npm -v
8.9.0
root@c99362005ff4:/# npx node -p 'process.argv[2]' -- --title 'Cool `ls` tricks'
Cool bin
boot
dev
etc
home
lib
lib64
media
mnt
opt
proc
root
run
sbin
srv
sys
tmp
usr
var tricks
root@c99362005ff4:/# node -p 'process.argv[2]' -- --title 'Cool `ls` tricks'
Cool `ls` tricks

And when you do fix it, please link the commit that fixes it. Bonus points for a regression test!

Edit: Opened a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing cmd:run-script related to `npm run-script` Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release Release 8.x work is associated with a specific npm 8 release
Projects
None yet
Development

No branches or pull requests

4 participants