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

doc: vm, run http server in vm by requiring #5323

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@eljefedelrodeodeljefe
Copy link
Contributor

commented Feb 19, 2016

As user in order to run node code in a vm, you would need to apply some objects to it. This is not quite well documented and may or may not have been reason for a series of issues and SO questions.

This PR would add an example to explain this. It is to some extent controversial and was discussed in a code PR #4955. Issue would have been nodejs/node-v0.x-archive#9211 and this link to SO.

An easier solution would be to use eval(), which I regard as an anti-pattern.
Also it uses require('module').wrap(code), which I believe to be a "private" API. I have taken and modified this example from the referenced issues.

/cc @nodejs/documentation

@vkurchatkin

View changes

doc/api/vm.markdown Outdated
console.log('Server running at http://127.0.0.1:8124/');
`

let wrap = require('module').wrap(code)

This comment has been minimized.

Copy link
@vkurchatkin

vkurchatkin Feb 19, 2016

Member

module is private

This comment has been minimized.

Copy link
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Feb 19, 2016

Author Contributor

Please make valuable comments that bring the discussion forth not back. Any other way then than this and eval?

This comment has been minimized.

Copy link
@vkurchatkin

vkurchatkin Feb 19, 2016

Member
vm.runInThisContext(`
(function(require) {
  require('http')
})
`)(require)
@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2016

Force pushed this. Doesn't look pretty but solves the problem.

@mscdex mscdex added doc vm labels Feb 19, 2016

@lordKnighton

This comment has been minimized.

Copy link

commented Feb 19, 2016

@jasnell

View changes

doc/api/vm.markdown Outdated
const vm = require('vm')

let code = `
(function(require) {

This comment has been minimized.

Copy link
@jasnell

jasnell Feb 19, 2016

Member

minor nit... having the let code = on a separate line with no indenting on the next line makes it a bit confusing.

This comment has been minimized.

Copy link
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Feb 19, 2016

Author Contributor

agree. fixed and force pushed. Thx.

@jasnell

This comment has been minimized.

Copy link
Member

commented Feb 19, 2016

LGTM

@silverwind

View changes

doc/api/vm.markdown Outdated
@@ -301,6 +301,34 @@ e.g. `(0,eval)('code')`. However, it also has the following additional options:
- `timeout`: a number of milliseconds to execute `code` before terminating
execution. If execution is terminated, an [`Error`][] will be thrown.

## Example: Run a Server within a VM

This comment has been minimized.

Copy link
@silverwind

silverwind Mar 3, 2016

Contributor

I think this should either not be a heading, or a 3rd level heading.

This comment has been minimized.

Copy link
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Mar 4, 2016

Author Contributor

I think we are using the heading style when it is an example, outside of a method, but rather a guide or such. E.g. here https://github.com/nodejs/node/blob/master/doc/api/readline.markdown#example-tiny-cli . If you'd rather want it belonging to .runInThisContext(), I can do it w/o pounds.

Having a look at for example streams, it would be 3rd level, but because it's having 2nd level topics like Writable etc. We are a little inconsistent about this I fear. :(

No strong opinions though.

@jasnell

View changes

doc/api/vm.markdown Outdated
will need to apply additional objects to it. Say, you want to run a simple
web server based on the `http` module, you can achieve this by doing something
like this:

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 7, 2016

Member

Looking at this again, I'd generally prefer staying away from "you" language... this can be reworded as:

The context of `.runInThisContext()` refers to the v8 context. The code passed 
to this VM context will have it's own isolated scope. To run a simple web server 
using the `http` module, for instance, the code passed to the context must either 
call `require('http')` on it's own, or have a reference to the `http` module passed 
to it. For instance:
@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2016

@jasnell agree. Changed it to what you provided.

Also added this issue, for potential harmonization of pronouns nodejs/docs#87

@Qard

This comment has been minimized.

Copy link
Member

commented Mar 7, 2016

Not sure how I feel about documenting this in this way. Normally the vm functions are meant to provide a sandboxed context and this is doing the opposite. The problem I see here is that it's not immediately clear that require is not just a stateless function. Using require will stuff modules into the cache globally, and you could do some nefarious things, like mess with the contents of require.cache. If we do want to document this, I think we need to be very clear about the risks involved.

As a side note: I'd only really consider eval() to be an anti-pattern when used on user-supplied code. In a controlled environment, I'd consider it at worst to be a possible deoptimization trigger. It's not actually that bad, if you are careful.

@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2016

I'd wish for the API to be a little less confusing, but it is what is and at least it allows for some configuration. Also I wouldn't use eval() because you can't directly return from it. The use case I found for it is in a child_process - possibly - on a remote system. The layer of security then would be to share explicitly share resources between parent and child or not.

Documenting dangers would be fine of course. Maybe also hinting to the use case above.

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 21, 2016

@Qard @eljefedelrodeodeljefe ... any further thoughts on this one?

@Qard

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

It worries me a little, but I think I'm 👍 overall. LGTM.

Might be worth discussing more in a docs meeting, but I think we can add to it later rather than blocking.

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

@eljefedelrodeodeljefe I was just about to land this but noticed the commit message is not so informative. Would you be able to update that? LGTM otherwise

@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2016

I have updated the commit message to reflect intention. Also I have added @Qard's remarks concerning the statefulness of require in this case in a note-section below the example.

Also happy in case we need broader discussion

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

LGTM

@thefourtheye

View changes

doc/api/vm.markdown Outdated
@@ -301,6 +301,38 @@ e.g. `(0,eval)('code')`. However, it also has the following additional options:
- `timeout`: a number of milliseconds to execute `code` before terminating
execution. If execution is terminated, an [`Error`][] will be thrown.

## Example: Run a Server within a VM

The context of `.runInThisContext()` refers to the v8 context. The code passed

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 2, 2016

Contributor

Minot nit. I think it is V8.

@thefourtheye

View changes

doc/api/vm.markdown Outdated
## Example: Run a Server within a VM

The context of `.runInThisContext()` refers to the v8 context. The code passed
to this VM context will have it's own isolated scope. To run a simple web server

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 2, 2016

Contributor

Isn't it its?

@thefourtheye

View changes

doc/api/vm.markdown Outdated
to it. For instance:

```js
'use strict'

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 2, 2016

Contributor

Missing semi-colons

This comment has been minimized.

Copy link
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 2, 2016

Author Contributor

Which ones? After 'use strict' is not necessary, since it works and activates strict mode.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 2, 2016

Contributor

In our repo every piece of code will have semicolons to mark the end of line. This example doesn't conform to that practice

@thefourtheye

View changes

doc/api/vm.markdown Outdated
```

_Note: `require()` in the above case shares the state with context it is passed
from. This might introduce risks when unknown code is executed._

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 2, 2016

Contributor

Can we expand this a little bit? Personally, I would love to know the potential problems, if there is risk involved. Unknown code here means untrusted code or user supplied code?

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 2, 2016

Member

I think it means untrusted code. But +1 on describing the potential problems. (The vm docs are a little vague for my taste with respect to that aspect in general…)

This comment has been minimized.

Copy link
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 2, 2016

Author Contributor

As mentioned above, you likely need to run vm anyhow in separate process, that has non critical rights and contexts. vm is not constructed out-of-box to provide a infinitely safe sandbox, imo. I will add the process part and will add a subordinate clause that: you can mess with the calling threads objects and abuse its context.

@addaleax

View changes

doc/api/vm.markdown Outdated
})
`

vm.runInThisContext(code)(require)

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 2, 2016

Member

Shouldn’t this be vm.runInNewContext? Otherwise it seems meaningless to me to point out security issues, because the whole set of current globals is available anyway.

This comment has been minimized.

Copy link
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 2, 2016

Author Contributor

This will not work, because then you'd likely need to pass all objects you want to use including console. Also goal for this example is not security, it's running node code. For more security you'd want to run this in a separate process.

evalmachine.<anonymous>:11
    console.log('Server running at http://127.0.0.1:8124/');
    ^
ReferenceError: console is not defined
    at evalmachine.<anonymous>:11:5
    at Object.<anonymous> (/Users/jefe/repos/node_rename_2/runinnewcontext.js:18:25)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:447:10)
    at startup (node.js:146:18)
    at node.js:404:3
@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2016

Addressed latest comments and force pushed. @thefourtheye now with semi-colons, thx.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

doc: add vm example, be able to require modules
From squashed: remove you reference, @jasnell

Also:
The intention behind is to present the user a way he/she can execute the
node 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. Proposed API changes, haven't
moved forward.

ref: nodejs/node-v0.x-archive#9211, #4955
@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2016

rebased. Also tried to contact @thefourtheye concerning sign-off on slack. But I think he is just incredibly busy.

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2016

Really sorry about the delay. LGTM.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

Awesome, ok, one final ping to @nodejs/documentation as a last call for comments.

@benjamingr

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

LGTM

1 similar comment
@Qard

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

LGTM

jasnell added a commit that referenced this pull request Apr 22, 2016

doc: add vm example, be able to require modules
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>
@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

Landed in 6815a3b. Fixed up the commit log a bit

@jasnell jasnell closed this Apr 22, 2016

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

doc: add vm example, be able to require modules
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 added a commit that referenced this pull request Apr 26, 2016

doc: add vm example, be able to require modules
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

This comment has been minimized.

Copy link
Member

commented Jun 1, 2016

@jasnell / @eljefedelrodeodeljefe is this something we want for lts?

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 1, 2016

MylesBorins added a commit that referenced this pull request Jul 11, 2016

doc: add vm example, be able to require modules
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 added a commit that referenced this pull request Jul 11, 2016

doc: add vm example, be able to require modules
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 added a commit that referenced this pull request Jul 12, 2016

doc: add vm example, be able to require modules
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 MylesBorins referenced this pull request Jul 12, 2016

Merged

v4.5.0 proposal #7688

MylesBorins added a commit that referenced this pull request Jul 14, 2016

doc: add vm example, be able to require modules
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 added a commit that referenced this pull request Jul 14, 2016

doc: add vm example, be able to require modules
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.