src: fix vm enumerator callbacks

doc/api/vm.md

@@ -944,6 +944,48 @@ within which it can operate. The process of creating the V8 Context and
 associating it with the `sandbox` object is what this document refers to as
 "contextifying" the `sandbox`.
 
+## vm module and Proxy object
+
+Leveraging a `Proxy` object as the sandbox of a VM context could result in a
+very powerful runtime environment that intercepts all accesses to the global
+object. However, there are some restrictions in the JavaScript engine that one
+needs to be aware of to prevent unexpected results. In particular, providing a
+`Proxy` object with a `get` handler could disallow any access to the original
+global properties of the new VM context, as the `get` hook does not distinguish
+between the `undefined` value and "requested property is not present" –
+the latter of which would ordinarily trigger a lookup on the context global
+object.
+
+Included below is a sample for how to work around this restriction. It
+initializes the sandbox as a `Proxy` object without any hooks, only to add them
+after the relevant properties have been saved.
+
+```js
+'use strict';
+const { createContext, runInContext } = require('vm');
+
+function createProxySandbox(handlers) {
+  // Create a VM context with a Proxy object with no hooks specified.
+  const sandbox = {};
+  const proxyHandlers = {};
+  const contextifiedProxy = createContext(new Proxy(sandbox, proxyHandlers));
+
+  // Save the initial globals onto our sandbox object.
+  const contextThis = runInContext('this', contextifiedProxy);
+  for (const prop of Reflect.ownKeys(contextThis)) {
+    const descriptor = Object.getOwnPropertyDescriptor(contextThis, prop);
+    Object.defineProperty(sandbox, prop, descriptor);
+  }
+
+  // Now that `sandbox` contains all the initial global properties, assign the
+  // provided handlers to the handlers we used to create the Proxy.
+  Object.assign(proxyHandlers, handlers);
+
+  // Return the created contextified Proxy object.
+  return contextifiedProxy;
+}
+```
+
 [`Error`]: errors.html#errors_class_error
 [`URL`]: url.html#url_class_url
 [`eval()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval

src/node_contextify.cc

@@ -143,19 +143,21 @@ Local<Context> ContextifyContext::CreateV8Context(
 
   NamedPropertyHandlerConfiguration config(PropertyGetterCallback,
                                            PropertySetterCallback,
-                                           PropertyDescriptorCallback,
+                                           PropertyQueryCallback,
                                            PropertyDeleterCallback,
                                            PropertyEnumeratorCallback,
                                            PropertyDefinerCallback,
+                                           PropertyDescriptorCallback,
                                            CreateDataWrapper(env));
 
   IndexedPropertyHandlerConfiguration indexed_config(
       IndexedPropertyGetterCallback,
       IndexedPropertySetterCallback,
-      IndexedPropertyDescriptorCallback,
+      IndexedPropertyQueryCallback,
       IndexedPropertyDeleterCallback,
-      PropertyEnumeratorCallback,
+      IndexedPropertyEnumeratorCallback,
       IndexedPropertyDefinerCallback,
+      IndexedPropertyDescriptorCallback,
       CreateDataWrapper(env));
 
   object_template->SetHandler(config);
@@ -391,6 +393,28 @@ void ContextifyContext::PropertySetterCallback(
   ctx->sandbox()->Set(property, value);
 }
 
+// static
+void ContextifyContext::PropertyQueryCallback(
+    Local<Name> property,
+    const PropertyCallbackInfo<Integer>& args) {
+  ContextifyContext* ctx = ContextifyContext::Get(args);
+
+  // Still initializing
+  if (ctx->context_.IsEmpty())
+    return;
+
+  Local<Context> context = ctx->context();
+
+  Local<Object> sandbox = ctx->sandbox();
+
+  PropertyAttribute attributes;
+  if (sandbox->HasOwnProperty(context, property).FromMaybe(false) &&
+      sandbox->GetPropertyAttributes(context, property).To(&attributes)) {
+    args.GetReturnValue().Set(attributes);
+  }
+}
+
+
 // static
 void ContextifyContext::PropertyDescriptorCallback(
     Local<Name> property,
@@ -504,7 +528,16 @@ void ContextifyContext::PropertyEnumeratorCallback(
   if (ctx->context_.IsEmpty())
     return;
 
-  args.GetReturnValue().Set(ctx->sandbox()->GetPropertyNames());
+  Local<Array> ret;
+  if (!ctx->sandbox()->GetPropertyNames(
+          ctx->context(),
+          v8::KeyCollectionMode::kOwnOnly,
+          v8::SKIP_SYMBOLS,
+          v8::IndexFilter::kSkipIndices).ToLocal(&ret)) {
+    return;
+  }
+
+  args.GetReturnValue().Set(ret);
 }
 
 // static
@@ -536,6 +569,20 @@ void ContextifyContext::IndexedPropertySetterCallback(
       Uint32ToName(ctx->context(), index), value, args);
 }
 
+// static
+void ContextifyContext::IndexedPropertyQueryCallback(
+    uint32_t index,
+    const PropertyCallbackInfo<Integer>& args) {
+  ContextifyContext* ctx = ContextifyContext::Get(args);
+
+  // Still initializing
+  if (ctx->context_.IsEmpty())
+    return;
+
+  ContextifyContext::PropertyQueryCallback(
+      Uint32ToName(ctx->context(), index), args);
+}
+
 // static
 void ContextifyContext::IndexedPropertyDescriptorCallback(
     uint32_t index,
@@ -585,6 +632,39 @@ void ContextifyContext::IndexedPropertyDeleterCallback(
   args.GetReturnValue().Set(false);
 }
 
+// static
+void ContextifyContext::IndexedPropertyEnumeratorCallback(
+    const PropertyCallbackInfo<Array>& args) {
+  ContextifyContext* ctx = ContextifyContext::Get(args);
+
+  // Still initializing
+  if (ctx->context_.IsEmpty())
+    return;
+
+  Local<Array> all_keys;
+  if (!ctx->sandbox()->GetPropertyNames(
+          ctx->context(),
+          v8::KeyCollectionMode::kOwnOnly,
+          v8::SKIP_SYMBOLS,
+          v8::IndexFilter::kIncludeIndices).ToLocal(&all_keys)) {
+    return;
+  }
+  uint32_t all_keys_len = all_keys->Length();
+
+  Local<Array> ret = Array::New(ctx->env()->isolate());
+  for (uint32_t i = 0, j = 0; i < all_keys_len; ++i) {
+    Local<Value> entry;
+    if (!all_keys->Get(ctx->context(), i).ToLocal(&entry))
+      return;
+    if (!entry->IsUint32())
+      continue;
+    if (!ret->Set(ctx->context(), j++, entry).FromMaybe(false))
+      return;
+  }
+
+  args.GetReturnValue().Set(ret);
+}
+
 class ContextifyScript : public BaseObject {
  private:
   Persistent<UnboundScript> script_;

src/node_contextify.h

@@ -69,6 +69,9 @@ class ContextifyContext {
       v8::Local<v8::Name> property,
       v8::Local<v8::Value> value,
       const v8::PropertyCallbackInfo<v8::Value>& args);
+  static void PropertyQueryCallback(
+      v8::Local<v8::Name> property,
+      const v8::PropertyCallbackInfo<v8::Integer>& args);
   static void PropertyDescriptorCallback(
       v8::Local<v8::Name> property,
       const v8::PropertyCallbackInfo<v8::Value>& args);
@@ -88,6 +91,9 @@ class ContextifyContext {
       uint32_t index,
       v8::Local<v8::Value> value,
       const v8::PropertyCallbackInfo<v8::Value>& args);
+  static void IndexedPropertyQueryCallback(
+      uint32_t index,
+      const v8::PropertyCallbackInfo<v8::Integer>& args);
   static void IndexedPropertyDescriptorCallback(
       uint32_t index,
       const v8::PropertyCallbackInfo<v8::Value>& args);
@@ -98,6 +104,8 @@ class ContextifyContext {
   static void IndexedPropertyDeleterCallback(
       uint32_t index,
       const v8::PropertyCallbackInfo<v8::Boolean>& args);
+  static void IndexedPropertyEnumeratorCallback(
+      const v8::PropertyCallbackInfo<v8::Array>& args);
   Environment* const env_;
   Persistent<v8::Context> context_;
 };

test/parallel/test-vm-object-keys.js

@@ -0,0 +1,45 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+// Regression test for https://github.com/nodejs/node/issues/22723.
+
+const kFoo = Symbol('kFoo');
+
+const test = {
+  'not': 'empty',
+  '0': 0,
+  '0.5': 0.5,
+  '1': 1,
+  '-1': -1,
+  [kFoo]: kFoo
+};
+vm.createContext(test);
+const proxied = vm.runInContext('this', test);
+
+// TODO(addaleax): The .sort() calls should not be necessary; the property
+// order should be indices, then other properties, then symbols.
+assert.deepStrictEqual(
+  Object.keys(proxied).sort(),
+  ['0', '1', 'not', '0.5', '-1'].sort());
+assert.deepStrictEqual(
+  // Filter out names shared by all global objects, i.e. JS builtins.
+  Object.getOwnPropertyNames(proxied)
+      .filter((name) => !(name in global))
+      .sort(),
+  ['0', '1', 'not', '0.5', '-1'].sort());
+assert.deepStrictEqual(Object.getOwnPropertySymbols(proxied), []);
+
+Object.setPrototypeOf(test, { inherited: 'true', 17: 42 });
+
+assert.deepStrictEqual(
+  Object.keys(proxied).sort(),
+  ['0', '1', 'not', '0.5', '-1'].sort());
+assert.deepStrictEqual(
+  // Filter out names shared by all global objects, i.e. JS builtins.
+  Object.getOwnPropertyNames(proxied)
+      .filter((name) => !(name in global))
+          .sort(),
+  ['0', '1', 'not', '0.5', '-1'].sort());
+assert.deepStrictEqual(Object.getOwnPropertySymbols(proxied), []);

test/parallel/test-vm-proxy.js

@@ -0,0 +1,83 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+const sandbox = {};
+const proxyHandlers = {};
+const contextifiedProxy = vm.createContext(new Proxy(sandbox, proxyHandlers));
+
+// One must get the globals and manually assign it to our own global object, to
+// mitigate against https://github.com/nodejs/node/issues/17465.
+const contextThis = vm.runInContext('this', contextifiedProxy);
+for (const prop of Reflect.ownKeys(contextThis)) {
+  const descriptor = Object.getOwnPropertyDescriptor(contextThis, prop);
+  Object.defineProperty(sandbox, prop, descriptor);
+}
+
+// Finally, activate the proxy.
+const numCalled = {};
+for (const hook of Reflect.ownKeys(Reflect)) {
+  numCalled[hook] = 0;
+  proxyHandlers[hook] = (...args) => {
+    numCalled[hook]++;
+    return Reflect[hook](...args);
+  };
+}
+
+{
+  // Make sure the `in` operator only calls `getOwnPropertyDescriptor` and not
+  // `get`.
+  // Refs: https://github.com/nodejs/node/issues/17480
+  assert.strictEqual(vm.runInContext('"a" in this', contextifiedProxy), false);
+  assert.deepStrictEqual(numCalled, {
+    defineProperty: 0,
+    deleteProperty: 0,
+    apply: 0,
+    construct: 0,
+    get: 0,
+    getOwnPropertyDescriptor: 1,
+    getPrototypeOf: 0,
+    has: 0,
+    isExtensible: 0,
+    ownKeys: 0,
+    preventExtensions: 0,
+    set: 0,
+    setPrototypeOf: 0
+  });
+}
+
+{
+  // Make sure `Object.getOwnPropertyDescriptor` only calls
+  // `getOwnPropertyDescriptor` and not `get`.
+  // Refs: https://github.com/nodejs/node/issues/17481
+
+  // Get and store the function in a lexically scoped variable to avoid
+  // interfering with the actual test.
+  vm.runInContext(
+    'const { getOwnPropertyDescriptor } = Object;',
+    contextifiedProxy);
+
+  for (const p of Reflect.ownKeys(numCalled)) {
+    numCalled[p] = 0;
+  }
+
+  assert.strictEqual(
+    vm.runInContext('getOwnPropertyDescriptor(this, "a")', contextifiedProxy),
+    undefined);
+  assert.deepStrictEqual(numCalled, {
+    defineProperty: 0,
+    deleteProperty: 0,
+    apply: 0,
+    construct: 0,
+    get: 0,
+    getOwnPropertyDescriptor: 1,
+    getPrototypeOf: 0,
+    has: 0,
+    isExtensible: 0,
+    ownKeys: 0,
+    preventExtensions: 0,
+    set: 0,
+    setPrototypeOf: 0
+  });
+}