Revert "src: implement query callbacks for vm"

This reverts commit 85c356c10eec14f96eaf92ffc9a8481b591e3652
from PR https://github.com/nodejs/node/pull/22390.

See the discussion in the (proposed) fix at
https://github.com/nodejs/node/pull/22836.

Refs: https://github.com/nodejs/node/pull/22836
Refs: https://github.com/nodejs/node/pull/22390
Fixes: https://github.com/nodejs/node/issues/22723

PR-URL: https://github.com/nodejs/node/pull/22911
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

doc/api/vm.md

@@ -944,48 +944,6 @@ 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,21 +143,19 @@ Local<Context> ContextifyContext::CreateV8Context(
 
   NamedPropertyHandlerConfiguration config(PropertyGetterCallback,
                                            PropertySetterCallback,
-                                           PropertyQueryCallback,
+                                           PropertyDescriptorCallback,
                                            PropertyDeleterCallback,
                                            PropertyEnumeratorCallback,
                                            PropertyDefinerCallback,
-                                           PropertyDescriptorCallback,
                                            CreateDataWrapper(env));
 
   IndexedPropertyHandlerConfiguration indexed_config(
       IndexedPropertyGetterCallback,
       IndexedPropertySetterCallback,
-      IndexedPropertyQueryCallback,
+      IndexedPropertyDescriptorCallback,
       IndexedPropertyDeleterCallback,
       PropertyEnumeratorCallback,
       IndexedPropertyDefinerCallback,
-      IndexedPropertyDescriptorCallback,
       CreateDataWrapper(env));
 
   object_template->SetHandler(config);
@@ -393,28 +391,6 @@ 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,
@@ -560,20 +536,6 @@ 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

@@ -69,9 +69,6 @@ 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);
@@ -91,9 +88,6 @@ 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

@@ -1,83 +0,0 @@
-'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
-  });
-}