Built-in properties cannot be accessed using Reflect, when Proxy is passed as vm context

[psrok1]

• Version: v6.2.2
• Platform: 3.16.0-73-generic feature: Buffer.prototype.indexOf #95~14.04.1-Ubuntu SMP Thu Jun 9 08:00:04 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: vm

Associated with #6158
Empty Proxy object correctly handles this case, but unfortunately it's impossible to access 'built-in' from custom getter (using Reflect)

var assert = require('assert');
var vm = require("vm");

var code = "String.prototype.f = function(){}; ''.f()";

// Test 1
assert.strictEqual(
    typeof vm.runInNewContext("String", 
            new Proxy({},{})), 
    'function');

// Test 2
assert.strictEqual(
    typeof vm.runInNewContext("String", 
            new Proxy({},{
                get: function(target, property, receiver) {
                    return Reflect.get(target, property);
                }
            })), 
    'function');

Second test returns AssertionError: 'undefined' === 'function'

[fhinkel]

I'm not sure I'd expect that to work, since target is {}. I'd run

// Test 2
assert.strictEqual(
  typeof vm.runInNewContext("String",
    new Proxy({},{
      get: function(target, property, receiver) {
        return Reflect.get(global, property);
      }
    })),
  'function');

[RoboPhred]

The workaround does not make use of the new context, but simply forwards the current one. This breaks the sandboxing. For example:

assert.strictNotEqual(
  vm.runInNewContext("String.foo = 42",
    new Proxy({},{
      get: function(target, property, receiver) {
        return Reflect.get(global, property);
      }
    })),
  String.foo);

[doughsay]

Was hoping this might be fixed in Node 8.0.0, but looks like it's still an issue:

> vm.runInNewContext("String", {})
[Function: String]
> vm.runInNewContext("String", new Proxy({},{}))
[Function: String]
> vm.runInNewContext("String", new Proxy({},{get: function(target, property) { return target[property] }}))
undefined
> vm.runInNewContext("String", new Proxy({},{get: function(target, property) { return Reflect.get(target, property) }}))
undefined

Basically, contextifying a Proxy that has a handler that traps get makes it impossible to get to that contexts globals. I don't know exactly what I'd expect in this situation anyway, but this is rather unfortunate. Are there any clever workarounds we could try here?

[Trott]

This is still an issue in Node.js 10.0.0, right?

@nodejs/vm Does this seem like a confirmed bug? Or is it not clear that it's a bug? If it is a bug, should we add a known_issues test for it?

[devsnek]

i'm currently of the opinion that this is not a bug, for i submit to you the following:

> vm.runInNewContext("String", new Proxy({}, { has() { return false } }))
[Function: String]
>

the proxy said it had the property "String" and didn't allow property access to walk up to the next level

[bnoordhuis]

I'm of the same opinion: not a bug, just misuse of proxies.

Note how target in #7458 (comment) is also simply the empty object:

// passing {}
vm.runInNewContext("String", new Proxy({}, {get: function(target, property) {
  return target[property] // undefined, naturally
}}))

// passing {String}
vm.runInNewContext("String", new Proxy({String}, {get: function(target, property) {
  return target[property] // now it works
}}))

I'll close this out.

[doughsay]

ok, thanks, so to clarify my use-case: if you want to trap get when using a proxy object as context, explicitly add all the globals you may want to use to the target.

It seems a reasonable workaround I guess, but it still feels a bit weird to me that...

vm.runInNewContext("String", new Proxy({},{}))

...works (i.e. if you're NOT trapping get), but just the act of adding the trap causes the globals to no longer be accessible. It feels like there's a hidden default implementation of get that is impossible to extend/replicate.

[devsnek]

@doughsay yeah that stuff tripped me up too, you might want to take a look at proxy membranes