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

[RoboPhred]

So I accept that this isn't a bug as-stated, but that still leaves us without any means of proxying a context global while forwarding requests to the original global.

The proposed workaround does not work, as the String being passed is now the outer context's string, not the new context String. To adapt the previous example, the following will fail:

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

[bnoordhuis]

Use vm.runInContext() to obtain a reference to this (the global object) first, then call vm.runInContext() again with the code you want to evaluate. You can then proxy to the context's global object.

[TimothyGu]

Note other issues currently associated with using Proxy as the global: #17465 #17480 #17481