Added flag to disable realm binding

[danzlarkin]

Hey @hankjacobs

I've added a small bit of functionality which disables the VM bindings to the context realm which are used for instanceof and other methods as were added in your latest release.

Adding the flag VM_DEFAULT_REALM like so to the bindings will disable binding the VM realm to the parent node realm which instantiates the VM:

const bindings = { VM_DEFAULT_REALM: true }
const cw = new Cloudworker(`const a = new Object()`, { bindings })

When using the flag instanceof between the VM object a and the global Object protoype will equal false intentionally as the Object realms remain different.

I believe this flag is a crucial component which should be able to be used in my opinion as a number of packages including math.js and others break now when running within the latest version of Cloudworker.

I think we should not be modifying the VM context unless we give users the option to restore this context

I've added tests which you can see in this in my pull request.

I also added one to validate no-eval to reflect the state of Cloudflare Workers as I noticed this was also changed on the VM.

I'd appreciate it if you can merge this and release this ASAP as we're really needing this for Restt and our other projects within my team at work.

Thanks again for all your hard work keeping things up to date :)

(p.s. I won't be available to reply to any questions for the next few days as I'll be on holiday but would appreciate it if this is merged before then)

[hankjacobs]

Hi @larkin-nz,

The goal of cloudworker is to match the behavior of Cloudflare Workers as best as possible. In the example you cited, a instanceof Object would return true within CF Workers. cloudworker, as it's currently implemented, matches this behavior. The flag you added would break that behavior.

I am curious what specifically breaks with math.js. Maybe there is another solution rather than disabling realm bindings and breaking CF worker compliant behavior.

I'm also hesitant to expose the concept of realms to developers. CF Workers don't surface this concept and I'd argue it's not relevant to the majority of users.

That being said, I do appreciate the test for no-eval :).

[danzlarkin]

Hey @hankjacobs,

Cloudflare Worker bindings are currently only able to be Wasm or KV as it it.

When we're doing things with bindings in Cloudworker we naturally allow there to be another custom bindings which goes away from the spec but provides the right flexabilty for a stable development example.

For example, within Restt-CLI we use the bindings to bind a flag as to whether is running from Restt-CLI or from the production server, and this includes a variety of function hooks for debugging also which is very useful.

In the tests which we have currently in Cloudworker, we test whether an Object within a binding function (which has a scope different to the VM) in an instanceof Object which is within the global scope.

The prototype of say Object in the VM (running in the Cloudworker) is not actually the same as that of the Object of that outside the VM (the globally scoped process which started our Cloudworker) as our VM in naturally sandboxed.

I don`t believe that this actually has any real relevance to Cloudflare Workers as such because there are no functions as I understand within the current bindings which will change between realms as above and I see the purpose of those enforced bindings as moving away from spec.

Take the case where we don't bind this.Object = Object within the class Context we can prove these checks using the following:

// Create a context for testing
const context = new runtime.Context(() => {}, Symbol('factory'), {
  checkA (a) { return a instanceof Object },
  checkB (a, b) { return a instanceof b },
})

// The result will be `false` as `new Object()` is from a different realm to `Object`
const resultA = vm.runInNewContext('checkA(new Object())', context)

// The result will be `true` as `new Object()` is from the same realm `Object`
const resultB = vm.runInNewContext('checkB(new Object(), Object)', context)

// The result will be `true` as `new Object()` is from the same realm `Object`
const resultC = vm.runInNewContext('new Object() instanceof Object', context)

I feel that these bindings should not be made in Context as were made in the release prior as they aren't technically correct.

My other solution to this would be to have a key called unset which would enable the developer to unset a binding from the context which would also correct the problems in our case.

I spent a whole bunch of time working through the stack with math.js and it seemed to be a problem which would occur due to how the package typed-function works (https://www.npmjs.com/package/typed-function)

The package is pretty major and receives over 100k downloads a week so this must naturally cause problems for a number of packages outside math.js.

Let me know when you've had a think about it, but I think the best course of action if you don't want either of my solutions mentioned would be rolling back those bindings added in 0.1.0.

Cheers

[hankjacobs]

This seems very specific to your use of cloudworker. The reason I added the code to handle objects coming from the runtime realm is that without it using instanceof against objects returned from any of the various methods that interact with the runtime (e.g. fetch) can fail (because the runtime code originated from a different realm). That goes against the behavior of CF Workers and is just a generally confusing failure because the fact there are different realms is purely an implementation detail.

That being said, have you considered running the Cloudworker portion of your package in a VM? That would run in it’s own realm and would allow you to do comparisons like you desire (assuming that it’s even possible — I haven’t tried myself).

I’ll give some thought as to how cloudworker could more generally solve this issue but I will not be reverting those changes as of now.

Now as far as math.js andtyped-function go, what specifically is the error. Could you please share an example and the failure in another issue? I’ll happily look into what it would take to fix that.

[danzlarkin]

I see, that makes sense, have you considered just having these types passed into the scoped functions like how my example B worked above?

If you bound a property to the Context which stored a reference to all of the prototypes in the global realm such as Object and used these within the Fetch package I think this is a better option.

I'm just about to head out for a hike for 5 days so I'm not able to access my laptop to spin up a simple test case with math.js for you, but simply webpacking a script which only includes the statement import * as mathjs from 'mathjs'should throw the error when run in Cloudworker.

I'll think more about a custom VM but I'd prefer to stay with Cloudworker as much as possible.

[danzlarkin]

Hey @hankjacobs, have you put any more thought into this?

[iameli]

@larkin-nz Turning this on made fetch() inside the container stop working for me.

TypeError: First argument must be a string, Buffer, ArrayBuffer, Array, or array-like object.

I do agree with your concerns about realm binding, though. I think we will want to find a different solution for external IO though fetch() and whatnot without actually binding all the types and objects from outside of the VM, perhaps facilitating external IO though a socket or some such. That would address this issue permanently without breaking other functionality. I'm going to be doing some work on this in the next week, I'll tag you if I come up with any designs.