Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

vm: runInThisContext is missing require #9211

Closed
n-riesco opened this issue Feb 13, 2015 · 16 comments
Closed

vm: runInThisContext is missing require #9211

n-riesco opened this issue Feb 13, 2015 · 16 comments

Comments

@n-riesco
Copy link

runInThisContext is missing require only when running a javascript file. I think the following commands explain better what I mean:

$ cat test.js
var vm = require("vm");
console.log(vm.runInThisContext("require instanceof Function"));

$ node < test.js
true

$ node --eval "$(cat test.js)"
true

$ node test.js

/home/user/test.js:2
console.log(vm.runInThisContext("require instanceof Function"));
               ^
ReferenceError: require is not defined
    at evalmachine.<anonymous>:1:1
    at Object.<anonymous> (/home/user/test.js:2:16)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:902:3
make: *** [all] Error 8
$ node --version
v0.10.25

I see the same behaviour is also present in v0.11.15.

And I've just checked it is also present on the official docker images for v0.10 and v0.12:

$ docker run -it --rm --name my-running-script -v "$PWD":/usr/src/myapp -w /usr/src/myapp node:0.10 node test.js
Unable to find image 'node:0.10' locally
Pulling repository node
f60f6620117e: Download complete 
511136ea3c5a: Download complete 
8771fbfe935c: Download complete 
0e30e84e9513: Download complete 
c90a56bfe7dd: Download complete 
6b030fdd4748: Download complete 
787349ce806c: Download complete 
70a6b638a713: Download complete 
be7f2e01d4b1: Download complete 
2d3fb7a55e9c: Download complete 
d8b52c267f8b: Download complete 

/usr/src/myapp/test.js:2
console.log(vm.runInThisContext("require instanceof Function"));
               ^
ReferenceError: require is not defined
    at evalmachine.<anonymous>:1:1
    at Object.<anonymous> (/usr/src/myapp/test.js:2:16)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:929:3
$ docker run -it --rm --name my-running-script -v "$PWD":/usr/src/myapp -w /usr/src/myapp node:0.12 node test.js
Unable to find image 'node:0.12' locally
Pulling repository node
12f018678fb1: Download complete 
511136ea3c5a: Download complete 
8771fbfe935c: Download complete 
0e30e84e9513: Download complete 
c90a56bfe7dd: Download complete 
6b030fdd4748: Download complete 
787349ce806c: Download complete 
70a6b638a713: Download complete 
380e6bb3cafc: Download complete 
0b0b06183499: Download complete 
3dd8081876d6: Download complete 
evalmachine.<anonymous>:1
require instanceof Function
^
ReferenceError: require is not defined
    at evalmachine.<anonymous>:1:1
    at Object.exports.runInThisContext (vm.js:74:17)
    at Object.<anonymous> (/usr/src/myapp/test.js:2:16)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
    at startup (node.js:129:16)
    at node.js:814:3
@jasnell
Copy link
Member

jasnell commented Feb 13, 2015

See http://stackoverflow.com/questions/20899863/the-module-property-is-undefined-when-using-vm-runinthiscontext. Specifically, try something like this:

test.js -

var m = require('module');
var src = 'exports.check = require instanceof Function';
var res = require('vm').runInThisContext(m.wrap(src))(exports, require, module, __filename, __dirname);
console.log(module.exports);

If you then node test.js, you ought to see {check: true}

@n-riesco
Copy link
Author

The SO question doesn't apply to this issue.

The SO question deals with the difference in behaviour between require and runInThisContext.

The issue I'm reporting deals with the difference in behaviour of runInThisContext when a script is run as node test.js or node < test.js.

@jasnell
Copy link
Member

jasnell commented Feb 14, 2015

The SO answer points to the issue: when you specify node test.js, the code is being run in the same context but not in the same scope, so the globals are not available. You have to pass those in somehow as illustrated in the example. When you pass the script in using stdin, however, the code is being executed within the same context AND scope, so the globals are available.

@n-riesco
Copy link
Author

global is available in all my tests. It is require what is missing. See below:

$ cat test-global.js 
var vm = require("vm");
console.log("global:", vm.runInThisContext("global instanceof Object"));
console.log("require:", vm.runInThisContext("require instanceof Object"));

$ node --eval "$(cat test-global.js)"
global: true
require: true

$ node test-global.js 
global: true

/home/nriesco/Documents/sync/src/javascript/bug/eval-flag/test-global.js:3
console.log("require:", vm.runInThisContext("require instanceof Object"));
                           ^
ReferenceError: require is not defined
    at evalmachine.<anonymous>:1:1
    at Object.<anonymous> (/home/nriesco/Documents/sync/src/javascript/bug/eval-flag/test-global.js:3:28)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:902:3

@n-riesco n-riesco reopened this Feb 14, 2015
@n-riesco
Copy link
Author

One more piece of information to support that node script.js and node --eval "$(cat script.js)" are expected to show the same behaviour can be found in node's usage help:

$ node --help
Usage: node [options] [ -e script | script.js ] [arguments] 
       node debug script.js [arguments] 

Options:
  -v, --version        print node's version
  -e, --eval script    evaluate script
  -p, --print          evaluate script and print result
  -i, --interactive    always enter the REPL even if stdin
                       does not appear to be a terminal
  --no-deprecation     silence deprecation warnings
  --trace-deprecation  show stack traces on deprecations
  --v8-options         print v8 command line options
  --max-stack-size=val set max v8 stack size (bytes)

Environment variables:
NODE_PATH              ':'-separated list of directories
                       prefixed to the module search path.
NODE_MODULE_CONTEXTS   Set to 1 to load modules in their own
                       global contexts.
NODE_DISABLE_COLORS    Set to 1 to disable colors in the REPL

Documentation can be found at http://nodejs.org/

@trevnorris trevnorris added the vm label Feb 16, 2015
@jasnell
Copy link
Member

jasnell commented Jun 29, 2015

@trevnorris @chrisdickinson ... any further thoughts on this?

@trevnorris
Copy link

sorry. nope.

@jasnell
Copy link
Member

jasnell commented Jun 29, 2015

@n-riesco ... I definitely agree that it's irritating and confusing. A quick test shows the same behavior in io.js. Technically, it's not a bug but it's not optimal by any means. I'll keep this open but I'm going to tag this as a defer to the converged stream. Since it involves a change in existing implemented behavior, it likely would need to land in either io.js or the converged repo.

@joyent/node-tsc ... do any of you have any additional thoughts?

@n-riesco
Copy link
Author

@jasnell Thanks for taking care of this issue.

Whatever the final decision is, please, consider I'm making use of the current behaviour of node --eval "$(cat script.js)" and node < script.js in these projects:

@isaacs
Copy link

isaacs commented Aug 24, 2015

I would argue that this is working as designed.

runInThisContext runs in this context, but that is akin to new Function(code)() rather than eval(code). That is, it runs in this global environment, but not inline in the same closure scope where it is invoked.

require is a function parameter (aka a "free var"), not a global. (Along with __filename, __dirname, module, and exports.) This is designed in this way because each module has to have a different require function in order to behave properly. (Remember, require('../relative/path') is relative to the module, not the current working directory.)

If there was an easy way to put closure scope free vars and declared vars, then it would surely be a useful thing to have on the vm module. But it would need to be a different name, or else this would introduce a very subtle breaking change that is probably not worth the hazard.

I recommend closing this issue if the TSC doesn't decide it's worth putting on the roadmap.

@isaacs
Copy link

isaacs commented Aug 24, 2015

If there is a "bug" here, imo, it's that this does work in --eval and <stdin modes, not that it doesn't work in module mode. And that bug is fixable, we just have to wrap the code rather than making stuff global.

@n-riesco
Copy link
Author

@isaacs Just to be clear, you are arguing that a script using require within vm.runInThisContext() should fail, e.g.:

var vm = require("vm");
console.log("global:", vm.runInThisContext("global instanceof Object"));
console.log("require:", vm.runInThisContext("require instanceof Object"));

whereas a script using eval() would work:

console.log("global:", eval("global instanceof Object"));
console.log("require:", eval("require instanceof Object"));

An argument against removing require from vm is that it would make vm something other than a Node.js virtual machine (require is an integral part of Node.js).


My current thought is that runInThisContext() shouldn't be akin to new Function(), but more like:

function runInThisContext(code) {
    return eval(code);
}

@trevnorris
Copy link

In all honesty runInThisContext() would be better named as runInGlobalContext(). Because it doesn't obey this in any sense.

The two cases of this would be to run in the current execution scope (which @n-riesco is basically what you're asking for). Or to run with the same this of the caller. Neither are true. The only thing you are given is that this == global inside the vm code.

@domenic Is this how it's supposed to work?

@domenic
Copy link

domenic commented Aug 25, 2015

Yeah, it is. @isaacs's explanation is helpful.

It's actually really important for security that require not be exposed. If you want to expose it, then you do. But we can't suddenly give all that formerly-sandboxed code access to require("fs") and friends by changing runInThisContext.

If you want eval, use eval.

Anyway, I agree it's confusingly named. The word "context" here means "V8 context" not e.g. "ES execution context" (~= function scope).

@domenic
Copy link

domenic commented Aug 25, 2015

An argument against removing require from vm is that it would make vm something other than a Node.js virtual machine (require is an integral part of Node.js).

"Node.js virtual machine" is a nonsensical term. There is only one VM in your program: the V8 VM. That is what the vm module is representing.

@n-riesco
Copy link
Author

On 25/08/15 19:07, Domenic Denicola wrote:

It's actually really important for security that require not be exposed. If you want to expose it, then you do. But we can't suddenly give all that formerly-sandboxed code access to |require("fs")| and friends by changing |runInThisContext|.

One could argue that to prevent access to require("fs"), vm.runInNewContext() is more appropriate

Anyway, I agree it's confusingly named. The word "context" here means "V8 context" not e.g. "ES execution context" (~= function scope).

My experience is that this "confusion"/"expectation" is very common amongst users of vm.

jasnell pushed a commit to nodejs/node that referenced this issue Apr 22, 2016
The intention behind is to present the user a way to
execute code in a vm context. The current API doesn't
allow this out-of-the-box, since it is neither passing a require
function nor creating context with one.
The missing docs for this behaviour have produced a number of
Q&A items and have also been discussed in the node-archive repo.
In both cases there was no real canonical answer.

Refs: nodejs/node-v0.x-archive#9211, #4955
PR-URL: #5323
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue Apr 25, 2016
The intention behind is to present the user a way to
execute code in a vm context. The current API doesn't
allow this out-of-the-box, since it is neither passing a require
function nor creating context with one.
The missing docs for this behaviour have produced a number of
Q&A items and have also been discussed in the node-archive repo.
In both cases there was no real canonical answer.

Refs: nodejs/node-v0.x-archive#9211, nodejs#4955
PR-URL: nodejs#5323
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
jasnell pushed a commit to nodejs/node that referenced this issue Apr 26, 2016
The intention behind is to present the user a way to
execute code in a vm context. The current API doesn't
allow this out-of-the-box, since it is neither passing a require
function nor creating context with one.
The missing docs for this behaviour have produced a number of
Q&A items and have also been discussed in the node-archive repo.
In both cases there was no real canonical answer.

Refs: nodejs/node-v0.x-archive#9211, #4955
PR-URL: #5323
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jul 11, 2016
The intention behind is to present the user a way to
execute code in a vm context. The current API doesn't
allow this out-of-the-box, since it is neither passing a require
function nor creating context with one.
The missing docs for this behaviour have produced a number of
Q&A items and have also been discussed in the node-archive repo.
In both cases there was no real canonical answer.

Refs: nodejs/node-v0.x-archive#9211, #4955
PR-URL: #5323
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jul 11, 2016
The intention behind is to present the user a way to
execute code in a vm context. The current API doesn't
allow this out-of-the-box, since it is neither passing a require
function nor creating context with one.
The missing docs for this behaviour have produced a number of
Q&A items and have also been discussed in the node-archive repo.
In both cases there was no real canonical answer.

Refs: nodejs/node-v0.x-archive#9211, #4955
PR-URL: #5323
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jul 12, 2016
The intention behind is to present the user a way to
execute code in a vm context. The current API doesn't
allow this out-of-the-box, since it is neither passing a require
function nor creating context with one.
The missing docs for this behaviour have produced a number of
Q&A items and have also been discussed in the node-archive repo.
In both cases there was no real canonical answer.

Refs: nodejs/node-v0.x-archive#9211, #4955
PR-URL: #5323
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jul 14, 2016
The intention behind is to present the user a way to
execute code in a vm context. The current API doesn't
allow this out-of-the-box, since it is neither passing a require
function nor creating context with one.
The missing docs for this behaviour have produced a number of
Q&A items and have also been discussed in the node-archive repo.
In both cases there was no real canonical answer.

Refs: nodejs/node-v0.x-archive#9211, #4955
PR-URL: #5323
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jul 14, 2016
The intention behind is to present the user a way to
execute code in a vm context. The current API doesn't
allow this out-of-the-box, since it is neither passing a require
function nor creating context with one.
The missing docs for this behaviour have produced a number of
Q&A items and have also been discussed in the node-archive repo.
In both cases there was no real canonical answer.

Refs: nodejs/node-v0.x-archive#9211, #4955
PR-URL: #5323
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@Trott Trott closed this as completed Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants