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

structuredClone() uses wrong context in vm.runInContext() #55554

Open
kanongil opened this issue Oct 26, 2024 · 5 comments
Open

structuredClone() uses wrong context in vm.runInContext() #55554

kanongil opened this issue Oct 26, 2024 · 5 comments
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@kanongil
Copy link
Contributor

Version

v22.10.0

Platform

Darwin Silmaril.home 23.6.0 Darwin Kernel Version 23.6.0: Wed Jul 31 20:49:46 PDT 2024; root:xnu-10063.141.1.700.5~1/RELEASE_ARM64_T8103 arm64

Subsystem

vm

What steps will reproduce the bug?

const vm = require('node:vm');

const context = vm.createContext({ structuredClone });
const result = vm.runInContext('structuredClone(new Error()) instanceof Error', context);

console.log('match:', result);

How often does it reproduce? Is there a required condition?

100%

What is the expected behavior? Why is that the expected behavior?

match: true because structuredClone should use the context of the VM for the prototype of the produced Error instance.

What do you see instead?

match: false

Additional investigation shows that the produced Error uses the prototype of the main runtime. Ie. a context escape, which would be a security error, if not for the disclaimer not to run untrusted code.

Additional information

This issue also applies to all other cloneable types like Map. Even Object fails instanceof:

structuredClone({}) instanceof Object       // <= equals false inside VM

FYI, this causes problems when running tests in jest, which use the VM to run tests.

@avivkeller
Copy link
Member

avivkeller commented Oct 26, 2024

This behavior (to me) makes sense based on what does on behind-the-scenes.

Behind the scenes, the structuredClone is using the error from it's own context (the global process) in order to clone the object. So, it's not an instanceof the error in the context given. If you slightly modify the code:

const vm = require('node:vm');

const context = vm.createContext({ structuredClone, Error });
const result = vm.runInContext('structuredClone(new Error()) instanceof Error', context);

console.log('match:', result);

You'll see that it is true.

Take the following example,

const vm = require('node:vm');

const context = vm.createContext({ func: (input) => input instanceof Object });
const result = vm.runInContext('func({})', context);

console.log('match:', result);

The callback (func) is using data from it's context, as when it was defined, it can't access that of the VM.

@avivkeller avivkeller added the vm Issues and PRs related to the vm subsystem. label Oct 26, 2024
@kanongil
Copy link
Contributor Author

Thanks, but I'm already aware of how it happens. Using the parent prototypes in the VM doesn't help, as it allows the code easy access to modify the prototypes of the parent.

The question is why? And is it intentional? Maybe an API to bind a function to the context of the VM is needed?

As it is, it would be cumbersome to work around to restore the correct prototypes.

@aduh95
Copy link
Contributor

aduh95 commented Oct 27, 2024

Reading the spec, the serialization and deserialization steps for an error object do not include sending the prototype – which makes sense IMO, if you call postMessage with an error object, you wouldn't expect the prototype to be sent to the other context.

IMO the current behavior is correct per spec.

@joyeecheung
Copy link
Member

Looks like a duplicate of #46558 - the vm contexts do not have their own Node.js builtins. They only have V8 ones. The structuredClone you pass into the vm context API belongs to the outer context and will use the outer context, the support of creating a new set of builtins in the vm contexts has been a long standing TODO (#46558)

@kanongil
Copy link
Contributor Author

#46558 relates to exposing node APIs in the vm context, so it is only tangentially related to this issue, which concerns a web API exposed on the global object. Though I guess it is possible that it will be needed to be exposed to support the node APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants