vm: known issue with CopyProperties throw on empty code

[jasnell]

CopyProperties() causes sandboxed Proxy to throw error
despite no code being run. The CopyProperties() function
will be removed shortly with the updates to the V8 API.

Refs: #11671

'use strict';

//Sandbox throws in CopyProperties() despite no code being run

require('../common');
const assert = require('assert');
const vm = require('vm');

const handler = {
    getOwnPropertyDescriptor: (target, prop) => {
      throw new Error('whoops');
    }
};
const sandbox = new Proxy({foo: 'bar'}, handler);
const context = vm.createContext(sandbox);


assert.doesNotThrow(() => vm.runInContext('', context));

[ace-n]

Not sure if this is related (or even a problem), but the example below throws an error on v8.0.0-pre:

'use strict';

const handler = {
    getOwnPropertyDescriptor: function(target, prop) {
      throw new Error('whoops');
    }
};
const sandbox = new Proxy({foo: 'bar'}, handler);
console.log(sandbox);

/Users/ace/Desktop/node/test.js:5
      throw new Error('whoops');
      ^

Error: whoops
    at Object.get (/Users/ace/Desktop/node/test.js:5:13)
    at formatValue (util.js:311:38)
    at inspect (util.js:152:10)
    at exports.format (util.js:34:24)
    at Console.log (console.js:85:24)
    at Object.<anonymous> (/Users/ace/Desktop/node/test.js:9:9)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)

AFAICT, console.log(sandbox) uses the getOwnPropertyDescriptor method as defined in the sandbox (i.e. in handler).

Since the console.log is running in the 'global' context, I'm guessing that it should be using the global getOwnPropertyDescriptor version (and not that of the sandbox).

Happy to investigate this further if this is indeed the/an issue and I'm on the right track.

[TimothyGu]

Since the console.log is running in the 'global' context, I'm guessing that it should be using the global getOwnPropertyDescriptor version (and not that of the sandbox).

Can you elaborate? What did you mean by "'global' context"? If you mean Object.getOwnPropertyDescriptors(), that function uses the getOwnPropertyDescriptor trap of the proxy per spec.

[ace-n]

@TimothyGu yeah - that is what I meant by the global context. What I'm not sure of is whether that trap is supposed to occur.

The error itself is triggered by calling Object.keys() on the sandbox object here.

It seems like it is, given that running the following example in Chrome's version of v8 gives this pattern:

/* sandbox code from previous comment */

console.log(sandbox); // no error
console.log(Object.keys(sandbox)); // raises error

To tie this back to the original issue: it seems like CopyProperties() calls Object.getOwnPropertyDescriptor() - but I don't know whether it should call the proxy's version or the default one.

[addaleax]

@ace-n I’m not sure how CopyProperties() relates to the code that you are using, though? Unless you’re using the vm module in some way, I would say it’s unrelated, so I’d suggest opening a new issue.

[ace-n]

@addaleax thanks - I've filed a new issue here and will continue the discussion there. Sorry for any confusion!

[Trott]

Should this stay open?

[fhinkel]

Yes, it's still an issue.