src: implement query callbacks for vm

This allows using a Proxy object as the sandbox for a VM context.

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>

doc/api/vm.md

@@ -916,6 +916,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,
       IndexedPropertyDefinerCallback,
+      IndexedPropertyDescriptorCallback,
       CreateDataWrapper(env));
 
   object_template->SetHandler(config);
@@ -390,6 +392,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,
@@ -535,6 +559,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,

src/node_contextify.h

@@ -65,6 +65,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);
@@ -84,6 +87,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);

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
+  });
+}