Unable to run globally installed node modules via this module

[tejashah88]

I'm not sure if this is a bug or intentional, but I've noticed that adding double quotes around any globally installed node modules (e.g. "npm") causes node to try and access it from your working directory. This actually causes this module to break since all the arguments are quoted. I tested this with the latest LTS release of node, which as of now is v8.10.0. It would be nice if there was either an alternate way to run these modules with this exec proxy or make a quick patch to allow this functionality with a small sacrifice to security.

[freitagbr]

Hi @tejashah88, can you provide a small example of some code that has this issue? Thanks.

[tejashah88]

const shell = require('shelljs-exec-proxy');
shell.npm.install(); // throws an error here

This happens for pretty much any npm command, or as said before, any globally installed node modules.

[nfischer]

I tried the same code, I have no problem. Which OS? Are you changing your prefix (npm config get prefix)?

Double-quoting should not be a problem on unix. Double quoting essentially just means "interpret this literally, don't try any aliases, don't glob expand, don't expand things that look like variables." I'm also pretty sure double-quoting is OK on Windows: we rely on this for shell.exec() in shelljs.

[tejashah88]

I am on windows, which might be why it's causing the problem.

Running npm config get prefix returns the following: C:\Users\<username>\AppData\Roaming\npm, which I haven't changed myself.

The error message indicates that it's trying to access the global module as if it was installed as a local dependency.

Error: Cannot find module 'C:\Users\<root_directory_of_project>\node_modules\npm\bin\npm-cli.js'

Sure enough, if I install npm locally (via npm install npm), and run the above code, then it runs like normal.

[nfischer]

@tejashah88 what's your PATH variable? Does it include the location of where you globally installed npm?

cmd> echo %PATH%

---

I think we use the same mechanism to find the locally-installed npm as we would to find the globally-installed npm: we just defer to PATH. npm run adds locally-installed modules to the PATH for you.

[tejashah88]

Yes, it does include the location of where npm was globally installed.

[nfischer]

My windows machine isn't set up for development currently. @tejashah88 could you perhaps help debugging this?

What is the output of running npm test in this repo on your Windows machine? If anything fails, I would be interested to know. I'm also interested in seeing which tests output something like "skipping test" (such as in this line).

If you feel up for it, could you send a pull request which adds a test to execute a globally installed command? If you can repro with a builtin command, that would be great.

[tejashah88]

So I ran the test scripts and interestingly, 5 tests failed, including all 3 that test for security.

Note that <root-dir> refers to the root directory of this module (when it was cloned from git).

  proxy
    √ appropriately handles inspect() and valueOf()
    √ returns appropriate keys
    √ does not override existing commands
    √ does not mess up non-command properties
    √ does not allow overriding reserved attributes
    √ does not allow deleting reserved attributes
    √ doesn't claim to have properties that don't exist in target
    √ allows adding new attributes
    commands
      - runs tr
      √ runs whoami (1265ms)
skipping test
      √ runs wc
skipping test
      √ runs du
skipping test
      √ runs rmdir
skipping test
      √ runs true
skipping test
      √ runs printf
      √ runs garbage commands (621ms)
      1) handles ShellStrings as arguments
    subcommands
      √ can use subcommands (673ms)
      √ can use subcommands with options (638ms)
      2) runs very long subcommand chains
    security
      3) handles unsafe filenames
      4) avoids globs
      5) escapes quotes


  17 passing (9s)
  1 pending
  5 failing

  1) proxy commands handles ShellStrings as arguments:

      AssertionError: expected true to be false
      + expected - actual

      -true
      +false

      at Assertion.fail (C:\<root-dir>\node_modules\should\cjs\should.js:326:17)
      at Assertion.value (C:\<root-dir>\node_modules\should\cjs\should.js:398:19)
      at Context.it (C:\<root-dir>\test\test.js:196:40)
      at callFnAsync (C:\<root-dir>\node_modules\mocha\lib\runnable.js:349:8)
      at Test.Runnable.run (C:\<root-dir>\node_modules\mocha\lib\runnable.js:301:7)
      at Runner.runTest (C:\<root-dir>\node_modules\mocha\lib\runner.js:422:10)
      at C:\<root-dir>\node_modules\mocha\lib\runner.js:528:12
      at next (C:\<root-dir>\node_modules\mocha\lib\runner.js:342:14)
      at C:\<root-dir>\node_modules\mocha\lib\runner.js:352:7
      at next (C:\<root-dir>\node_modules\mocha\lib\runner.js:284:14)
      at Immediate.<anonymous> (C:\<root-dir>\node_modules\mocha\lib\runner.js:320:5)

  2) proxy subcommands runs very long subcommand chains:

      AssertionError: expected '' to be 'one two three four five six seven\n'
      + expected - actual

      +one two three four five six seven

      at Assertion.fail (C:\<root-dir>\node_modules\should\cjs\should.js:326:17)
      at Assertion.value (C:\<root-dir>\node_modules\should\cjs\should.js:398:19)
      at Context.it (C:\<root-dir>\test\test.js:223:25)
      at callFnAsync (C:\<root-dir>\node_modules\mocha\lib\runnable.js:349:8)
      at Test.Runnable.run (C:\<root-dir>\node_modules\mocha\lib\runnable.js:301:7)
      at Runner.runTest (C:\<root-dir>\node_modules\mocha\lib\runner.js:422:10)
      at C:\<root-dir>\node_modules\mocha\lib\runner.js:528:12
      at next (C:\<root-dir>\node_modules\mocha\lib\runner.js:342:14)
      at C:\<root-dir>\node_modules\mocha\lib\runner.js:352:7
      at next (C:\<root-dir>\node_modules\mocha\lib\runner.js:284:14)
      at Immediate.<anonymous> (C:\<root-dir>\node_modules\mocha\lib\runner.js:320:5)

  3) proxy security handles unsafe filenames:

      AssertionError: expected true to be false
      + expected - actual

      -true
      +false

      at Assertion.fail (C:\<root-dir>\node_modules\should\cjs\should.js:326:17)
      at Assertion.value (C:\<root-dir>\node_modules\should\cjs\should.js:398:19)
      at Context.it (C:\<root-dir>\test\test.js:240:35)
      at callFnAsync (C:\<root-dir>\node_modules\mocha\lib\runnable.js:349:8)
      at Test.Runnable.run (C:\<root-dir>\node_modules\mocha\lib\runnable.js:301:7)
      at Runner.runTest (C:\<root-dir>\node_modules\mocha\lib\runner.js:422:10)
      at C:\<root-dir>\node_modules\mocha\lib\runner.js:528:12
      at next (C:\<root-dir>\node_modules\mocha\lib\runner.js:342:14)
      at C:\<root-dir>\node_modules\mocha\lib\runner.js:352:7
      at next (C:\<root-dir>\node_modules\mocha\lib\runner.js:284:14)
      at Immediate.<anonymous> (C:\<root-dir>\node_modules\mocha\lib\runner.js:320:5)

  4) proxy security avoids globs:

      AssertionError: expected 'hello world\r\n' to be 'hello world\n'
      + expected - actual

      -hello world
      +hello world

      at Assertion.fail (C:\<root-dir>\node_modules\should\cjs\should.js:326:17)
      at Assertion.value (C:\<root-dir>\node_modules\should\cjs\should.js:398:19)
      at Context.it (C:\<root-dir>\test\test.js:257:39)
      at callFnAsync (C:\<root-dir>\node_modules\mocha\lib\runnable.js:349:8)
      at Test.Runnable.run (C:\<root-dir>\node_modules\mocha\lib\runnable.js:301:7)
      at Runner.runTest (C:\<root-dir>\node_modules\mocha\lib\runner.js:422:10)
      at C:\<root-dir>\node_modules\mocha\lib\runner.js:528:12
      at next (C:\<root-dir>\node_modules\mocha\lib\runner.js:342:14)
      at C:\<root-dir>\node_modules\mocha\lib\runner.js:352:7
      at next (C:\<root-dir>\node_modules\mocha\lib\runner.js:284:14)
      at Immediate.<anonymous> (C:\<root-dir>\node_modules\mocha\lib\runner.js:320:5)

  5) proxy security escapes quotes:

      AssertionError: expected false to be true
      + expected - actual

      -false
      +true

      at Assertion.fail (C:\<root-dir>\node_modules\should\cjs\should.js:326:17)
      at Assertion.value (C:\<root-dir>\node_modules\should\cjs\should.js:398:19)
      at Context.it (C:\<root-dir>\test\test.js:267:36)
      at callFnAsync (C:\<root-dir>\node_modules\mocha\lib\runnable.js:349:8)
      at Test.Runnable.run (C:\<root-dir>\node_modules\mocha\lib\runnable.js:301:7)
      at Runner.runTest (C:\<root-dir>\node_modules\mocha\lib\runner.js:422:10)
      at C:\<root-dir>\node_modules\mocha\lib\runner.js:528:12
      at next (C:\<root-dir>\node_modules\mocha\lib\runner.js:342:14)
      at C:\<root-dir>\node_modules\mocha\lib\runner.js:352:7
      at next (C:\<root-dir>\node_modules\mocha\lib\runner.js:284:14)
      at Immediate.<anonymous> (C:\<root-dir>\node_modules\mocha\lib\runner.js:320:5)



---------------------|----------|----------|----------|----------|----------------|
File                 |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
---------------------|----------|----------|----------|----------|----------------|
 shelljs-exec-proxy\ |      100 |      100 |      100 |      100 |                |
  common.js          |      100 |      100 |      100 |      100 |                |
  index.js           |      100 |      100 |      100 |      100 |                |
---------------------|----------|----------|----------|----------|----------------|
All files            |      100 |      100 |      100 |      100 |                |
---------------------|----------|----------|----------|----------|----------------|

npm ERR! Test failed.  See above for more details.

[tejashah88]

I am busy for now, but eventually, I'd love to submit a PR for testing this peculiarity.

[nfischer]

@tejashah88 thanks for the info!

Some of the tests look to have simple errors (one fails because Windows uses a different newline). I'll fix that test, look at the others, and try to set up appveyor to run CI on windows.