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

vm.createContext creates more than described. #46976

Open
axkibe opened this issue Mar 6, 2023 · 9 comments
Open

vm.createContext creates more than described. #46976

axkibe opened this issue Mar 6, 2023 · 9 comments
Labels
doc Issues and PRs related to the documentations. v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.

Comments

@axkibe
Copy link
Contributor

axkibe commented Mar 6, 2023

Affected URL(s)

https://nodejs.org/api/vm.html#vmcreatecontextcontextobject-options

Description of the problem

See following test case:

const vm = require('node:vm');
const context = vm.createContext({ foo: console.log });

(async () => {
    const module = new vm.SourceTextModule(
    'foo( console ); console.log( "bar" );',
    { context: context, });
    await module.link(() => {});
    await module.evaluate();
})();

From basic understand as document in the API this should fail, as the "console" has never been handed down into the new context.. yet it is there. Albeit the log is not working.

I guess this is a documentation issue, as the standard javascript globals are created regardless? If I hand down the console object into the created context, console.log works, but if not IMO it should either throw a call to undefined function error, or as said, the docs should describe better what is actually happening on creating a new Context.

PS: I'm aware that nodes vm.SourceTextModule probably may not be properly sandboxed, in my case this doesn't matter, I just found it confusing, the "console" object exists at all in the new context.

@axkibe axkibe added the doc Issues and PRs related to the documentations. label Mar 6, 2023
@VoltrexKeyva VoltrexKeyva added the vm Issues and PRs related to the vm subsystem. label Mar 7, 2023
@bnoordhuis
Copy link
Member

console is a builtin/standard global object, it's just that the default implementation doesn't do anything.

Documentation pull request welcome although I'm not sure what it should say or where.

@axkibe
Copy link
Contributor Author

axkibe commented Mar 7, 2023

IMO the createContext() documentation should mention that ES standard global objects are implicitly created for the next context if not specified. And by default console.log doesn't do anything. Dunno if there are other caveats..

As I read the docs, only the keys to globalObject should be present.

I was just wondering for a while why nothing is printed, and when I realizied, I didn't hand down the console object, I wondered, why it didn't throw a console.log is an undefined call message.

@bnoordhuis
Copy link
Member

createContext() documentation should mention that ES standard global objects are implicitly created

That's kinda like saying "water is wet", though.

Perhaps a better improvement is to make the console methods actually print. (I believe they already do, but only to the DevTools console, not to the tty.)

It doesn't necessarily have to be node's fancy console.log(), just a basic one, otherwise you have to drag a lot of node's internal infrastructure into the new context.

One complication is the worker_threads module. Right now console.log() is implemented by messaging the main thread, which then prints on the worker_thread's behalf (but there's discussion of changing that.)

console.log() from a vm context should probably (?) behave the same way.

Pull request welcome.

@legendecas
Copy link
Member

legendecas commented Mar 22, 2023

It is true that console is not part of the ecma262. V8 has installed the inspector console to the context's extra binding: https://chromium-review.googlesource.com/c/v8/v8/+/3364786 and Node.js has migrated to use the console on the extra binding already (#43142). The next step for it would be that stop installing the inspector console to the freshly created context's globalThis by default.

@axkibe
Copy link
Contributor Author

axkibe commented Mar 28, 2023

I'm still not sure how to write it properly, but I'm thinking on it.

I just had the case that Sets were no longer "instanceof Set" for sets passed into the vm via the linker since createContext created another version of "Set". It was easy to fix tough:

const myContext = vm.createContext( { console: console, Set: Set } );

Might be needed for other stuff as well, but not needed in my case.

About console, I suggest an API on this basic level should be a perfect sandbox by default and any hole created on purpose than holes preinstalled and having to be plugged.

PS: What I kind of miss is having access to "thisContext" from node, from the current node environment, so I can load custom created sourceTextModules. Right now I'm gluing together the node Context with the context for sourceTextModule with synthetic modules.

@bnoordhuis
Copy link
Member

^-- is borderline off-topic and better suited for a discussion than continuing it here. That said... node users have a tendency to copy bad solutions from the bug tracker, so to set the record straight:

vm.createContext( { console: console, Set: Set } )

Bad solution to the instanceof problem. Better solution: get the Set from the context:

const cx = vm.createContext()
const {Set: TheirSet} = vm.runInContext("globalThis", cx)

I kind of miss is having access to "thisContext" from node

vm.runInThisContext()?

@axkibe
Copy link
Contributor Author

axkibe commented Mar 28, 2023

I'm coming from vm.runInThisContext before which I used so far, but I got the idea it is CJS only. no? Sorry if I'm misunderstanding. I thought there is no equivalent for ESM.

I might be better of using a custom loader instead tough, but yes off topic. Sorry.

@domenic
Copy link
Contributor

domenic commented May 11, 2023

It would help jsdom if there was no console created in vm.createContext(), and instead only the standard ES262 globals. We currently have to do some hacks to get around this.

@legendecas
Copy link
Member

@domenic: It would help jsdom if there was no console created in vm.createContext(), and instead only the standard ES262 globals. We currently have to do some hacks to get around this.

Submitted https://chromium-review.googlesource.com/c/v8/v8/+/4522545 to avoid installing the console to the global object by default.

@legendecas legendecas added the v8 engine Issues and PRs related to the V8 dependency. label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants