src: use new V8 API in vm module

Remove the CopyProperties() hack in the vm module, i.e., Contextify.
Use different V8 API methods, that allow interception of
DefineProperty() and do not flatten accessor descriptors to
property descriptors.

Move many known issues to test cases. Factor out the last test in
test-vm-context.js for
https://github.com/nodejs/node/issues/10223
into its own file, test-vm-strict-assign.js.

Part of this CL is taken from a stalled PR by
https://github.com/AnnaMag
https://github.com/nodejs/node/pull/13265

This PR requires a backport of
37a3a15c3e

PR-URL: https://github.com/nodejs/node/pull/16293
Fixes: https://github.com/nodejs/node/issues/2734
Fixes: https://github.com/nodejs/node/issues/10223
Fixes: https://github.com/nodejs/node/issues/11803
Fixes: https://github.com/nodejs/node/issues/11902
Ref: https://github.com/nodejs/node/issues/6283
Ref: https://github.com/nodejs/node/pull/15114
Ref: https://github.com/nodejs/node/pull/13265
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

src/node_contextify.cc

@@ -38,6 +38,7 @@ using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
 using v8::HandleScope;
+using v8::IndexedPropertyHandlerConfiguration;
 using v8::Integer;
 using v8::Just;
 using v8::Local;
@@ -58,6 +59,7 @@ using v8::ScriptOrigin;
 using v8::String;
 using v8::Symbol;
 using v8::TryCatch;
+using v8::Uint32;
 using v8::Uint8Array;
 using v8::UnboundScript;
 using v8::Value;
@@ -65,6 +67,11 @@ using v8::WeakCallbackInfo;
 
 namespace {
 
+Local<Name> Uint32ToName(Local<Context> context, uint32_t index) {
+  return Uint32::New(context->GetIsolate(), index)->ToString(context)
+      .ToLocalChecked();
+}
+
 class ContextifyContext {
  protected:
   // V8 reserves the first field in context objects for the debugger. We use the
@@ -111,93 +118,6 @@ class ContextifyContext {
     return Local<Object>::Cast(context()->GetEmbedderData(kSandboxObjectIndex));
   }
 
-  // XXX(isaacs): This function only exists because of a shortcoming of
-  // the V8 SetNamedPropertyHandler function.
-  //
-  // It does not provide a way to intercept Object.defineProperty(..)
-  // calls.  As a result, these properties are not copied onto the
-  // contextified sandbox when a new global property is added via either
-  // a function declaration or a Object.defineProperty(global, ...) call.
-  //
-  // Note that any function declarations or Object.defineProperty()
-  // globals that are created asynchronously (in a setTimeout, callback,
-  // etc.) will happen AFTER the call to copy properties, and thus not be
-  // caught.
-  //
-  // The way to properly fix this is to add some sort of a
-  // Object::SetNamedDefinePropertyHandler() function that takes a callback,
-  // which receives the property name and property descriptor as arguments.
-  //
-  // Luckily, such situations are rare, and asynchronously-added globals
-  // weren't supported by Node's VM module until 0.12 anyway.  But, this
-  // should be fixed properly in V8, and this copy function should be
-  // removed once there is a better way.
-  void CopyProperties() {
-    HandleScope scope(env()->isolate());
-
-    Local<Context> context = PersistentToLocal(env()->isolate(), context_);
-    Local<Object> global =
-        context->Global()->GetPrototype()->ToObject(env()->isolate());
-    Local<Object> sandbox_obj = sandbox();
-
-    Local<Function> clone_property_method;
-
-    Local<Array> names = global->GetOwnPropertyNames();
-    int length = names->Length();
-    for (int i = 0; i < length; i++) {
-      Local<String> key = names->Get(i)->ToString(env()->isolate());
-      Maybe<bool> has = sandbox_obj->HasOwnProperty(context, key);
-
-      // Check for pending exceptions
-      if (has.IsNothing())
-        return;
-
-      if (!has.FromJust()) {
-        Local<Object> desc_vm_context =
-            global->GetOwnPropertyDescriptor(context, key)
-            .ToLocalChecked().As<Object>();
-
-        bool is_accessor =
-            desc_vm_context->Has(context, env()->get_string()).FromJust() ||
-            desc_vm_context->Has(context, env()->set_string()).FromJust();
-
-        auto define_property_on_sandbox = [&] (PropertyDescriptor* desc) {
-            desc->set_configurable(desc_vm_context
-                ->Get(context, env()->configurable_string()).ToLocalChecked()
-                ->BooleanValue(context).FromJust());
-            desc->set_enumerable(desc_vm_context
-                ->Get(context, env()->enumerable_string()).ToLocalChecked()
-                ->BooleanValue(context).FromJust());
-            CHECK(sandbox_obj->DefineProperty(context, key, *desc).FromJust());
-        };
-
-        if (is_accessor) {
-          Local<Function> get =
-              desc_vm_context->Get(context, env()->get_string())
-              .ToLocalChecked().As<Function>();
-          Local<Function> set =
-              desc_vm_context->Get(context, env()->set_string())
-              .ToLocalChecked().As<Function>();
-
-          PropertyDescriptor desc(get, set);
-          define_property_on_sandbox(&desc);
-        } else {
-          Local<Value> value =
-              desc_vm_context->Get(context, env()->value_string())
-              .ToLocalChecked();
-
-          bool writable =
-              desc_vm_context->Get(context, env()->writable_string())
-              .ToLocalChecked()->BooleanValue(context).FromJust();
-
-          PropertyDescriptor desc(value, writable);
-          define_property_on_sandbox(&desc);
-        }
-    }
-  }
-}
-
-
   // This is an object that just keeps an internal pointer to this
   // ContextifyContext.  It's passed to the NamedPropertyHandler.  If we
   // pass the main JavaScript context object we're embedded in, then the
@@ -227,13 +147,25 @@ class ContextifyContext {
     Local<ObjectTemplate> object_template =
         function_template->InstanceTemplate();
 
-    NamedPropertyHandlerConfiguration config(GlobalPropertyGetterCallback,
-                                             GlobalPropertySetterCallback,
-                                             GlobalPropertyQueryCallback,
-                                             GlobalPropertyDeleterCallback,
-                                             GlobalPropertyEnumeratorCallback,
+    NamedPropertyHandlerConfiguration config(PropertyGetterCallback,
+                                             PropertySetterCallback,
+                                             PropertyDescriptorCallback,
+                                             PropertyDeleterCallback,
+                                             PropertyEnumeratorCallback,
+                                             PropertyDefinerCallback,
                                              CreateDataWrapper(env));
+
+    IndexedPropertyHandlerConfiguration indexed_config(
+        IndexedPropertyGetterCallback,
+        IndexedPropertySetterCallback,
+        IndexedPropertyDescriptorCallback,
+        IndexedPropertyDeleterCallback,
+        PropertyEnumeratorCallback,
+        IndexedPropertyDefinerCallback,
+        CreateDataWrapper(env));
+
     object_template->SetHandler(config);
+    object_template->SetHandler(indexed_config);
 
     Local<Context> ctx = NewContext(env->isolate(), object_template);
 
@@ -373,7 +305,7 @@ class ContextifyContext {
   }
 
 
-  static void GlobalPropertyGetterCallback(
+  static void PropertyGetterCallback(
       Local<Name> property,
       const PropertyCallbackInfo<Value>& args) {
     ContextifyContext* ctx;
@@ -402,7 +334,7 @@ class ContextifyContext {
   }
 
 
-  static void GlobalPropertySetterCallback(
+  static void PropertySetterCallback(
       Local<Name> property,
       Local<Value> value,
       const PropertyCallbackInfo<Value>& args) {
@@ -414,9 +346,8 @@ class ContextifyContext {
       return;
 
     auto attributes = PropertyAttribute::None;
-    bool is_declared =
-        ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(),
-                                                            property)
+    bool is_declared = ctx->global_proxy()
+        ->GetRealNamedPropertyAttributes(ctx->context(), property)
         .To(&attributes);
     bool read_only =
         static_cast<int>(attributes) &
@@ -441,16 +372,16 @@ class ContextifyContext {
     bool is_function = value->IsFunction();
 
     if (!is_declared && args.ShouldThrowOnError() && is_contextual_store &&
-    !is_function)
+        !is_function)
       return;
 
     ctx->sandbox()->Set(property, value);
   }
 
 
-  static void GlobalPropertyQueryCallback(
+  static void PropertyDescriptorCallback(
       Local<Name> property,
-      const PropertyCallbackInfo<Integer>& args) {
+      const PropertyCallbackInfo<Value>& args) {
     ContextifyContext* ctx;
     ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
 
@@ -459,23 +390,80 @@ class ContextifyContext {
       return;
 
     Local<Context> context = ctx->context();
-    Maybe<PropertyAttribute> maybe_prop_attr =
-        ctx->sandbox()->GetRealNamedPropertyAttributes(context, property);
 
-    if (maybe_prop_attr.IsNothing()) {
-      maybe_prop_attr =
-          ctx->global_proxy()->GetRealNamedPropertyAttributes(context,
-                                                              property);
-    }
+    Local<Object> sandbox = ctx->sandbox();
 
-    if (maybe_prop_attr.IsJust()) {
-      PropertyAttribute prop_attr = maybe_prop_attr.FromJust();
-      args.GetReturnValue().Set(prop_attr);
+    if (sandbox->HasOwnProperty(context, property).FromMaybe(false)) {
+      args.GetReturnValue().Set(
+          sandbox->GetOwnPropertyDescriptor(context, property)
+              .ToLocalChecked());
     }
   }
 
 
-  static void GlobalPropertyDeleterCallback(
+  static void PropertyDefinerCallback(
+      Local<Name> property,
+      const PropertyDescriptor& desc,
+      const PropertyCallbackInfo<Value>& args) {
+    ContextifyContext* ctx;
+    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+
+    // Still initializing
+    if (ctx->context_.IsEmpty())
+      return;
+
+    Local<Context> context = ctx->context();
+    v8::Isolate* isolate = context->GetIsolate();
+
+    auto attributes = PropertyAttribute::None;
+    bool is_declared =
+        ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(),
+                                                            property)
+            .To(&attributes);
+    bool read_only =
+        static_cast<int>(attributes) &
+            static_cast<int>(PropertyAttribute::ReadOnly);
+
+    // If the property is set on the global as read_only, don't change it on
+    // the global or sandbox.
+    if (is_declared && read_only)
+      return;
+
+    Local<Object> sandbox = ctx->sandbox();
+
+    auto define_prop_on_sandbox =
+        [&] (PropertyDescriptor* desc_for_sandbox) {
+          if (desc.has_enumerable()) {
+            desc_for_sandbox->set_enumerable(desc.enumerable());
+          }
+          if (desc.has_configurable()) {
+            desc_for_sandbox->set_configurable(desc.configurable());
+          }
+          // Set the property on the sandbox.
+          sandbox->DefineProperty(context, property, *desc_for_sandbox);
+        };
+
+    if (desc.has_get() || desc.has_set()) {
+      PropertyDescriptor desc_for_sandbox(
+          desc.has_get() ? desc.get() : v8::Undefined(isolate).As<Value>(),
+          desc.has_set() ? desc.set() : v8::Undefined(isolate).As<Value>());
+
+      define_prop_on_sandbox(&desc_for_sandbox);
+    } else {
+      Local<Value> value =
+          desc.has_value() ? desc.value() : v8::Undefined(isolate).As<Value>();
+
+      if (desc.has_writable()) {
+        PropertyDescriptor desc_for_sandbox(value, desc.writable());
+        define_prop_on_sandbox(&desc_for_sandbox);
+      } else {
+        PropertyDescriptor desc_for_sandbox(value);
+        define_prop_on_sandbox(&desc_for_sandbox);
+      }
+    }
+  }
+
+  static void PropertyDeleterCallback(
       Local<Name> property,
       const PropertyCallbackInfo<Boolean>& args) {
     ContextifyContext* ctx;
@@ -496,7 +484,7 @@ class ContextifyContext {
   }
 
 
-  static void GlobalPropertyEnumeratorCallback(
+  static void PropertyEnumeratorCallback(
       const PropertyCallbackInfo<Array>& args) {
     ContextifyContext* ctx;
     ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
@@ -507,6 +495,83 @@ class ContextifyContext {
 
     args.GetReturnValue().Set(ctx->sandbox()->GetPropertyNames());
   }
+
+  static void IndexedPropertyGetterCallback(
+      uint32_t index,
+      const PropertyCallbackInfo<Value>& args) {
+    ContextifyContext* ctx;
+    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+
+    // Still initializing
+    if (ctx->context_.IsEmpty())
+      return;
+
+    PropertyGetterCallback(Uint32ToName(ctx->context(), index), args);
+  }
+
+
+  static void IndexedPropertySetterCallback(
+      uint32_t index,
+      Local<Value> value,
+      const PropertyCallbackInfo<Value>& args) {
+    ContextifyContext* ctx;
+    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+
+    // Still initializing
+    if (ctx->context_.IsEmpty())
+      return;
+
+    PropertySetterCallback(Uint32ToName(ctx->context(), index), value, args);
+  }
+
+
+  static void IndexedPropertyDescriptorCallback(
+      uint32_t index,
+      const PropertyCallbackInfo<Value>& args) {
+    ContextifyContext* ctx;
+    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+
+    // Still initializing
+    if (ctx->context_.IsEmpty())
+      return;
+
+    PropertyDescriptorCallback(Uint32ToName(ctx->context(), index), args);
+  }
+
+
+  static void IndexedPropertyDefinerCallback(
+      uint32_t index,
+      const PropertyDescriptor& desc,
+      const PropertyCallbackInfo<Value>& args) {
+    ContextifyContext* ctx;
+    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+
+    // Still initializing
+    if (ctx->context_.IsEmpty())
+      return;
+
+    PropertyDefinerCallback(Uint32ToName(ctx->context(), index), desc, args);
+  }
+
+  static void IndexedPropertyDeleterCallback(
+      uint32_t index,
+      const PropertyCallbackInfo<Boolean>& args) {
+    ContextifyContext* ctx;
+    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+
+    // Still initializing
+    if (ctx->context_.IsEmpty())
+      return;
+
+    Maybe<bool> success = ctx->sandbox()->Delete(ctx->context(), index);
+
+    if (success.FromMaybe(false))
+      return;
+
+    // Delete failed on the sandbox, intercept and do not delete on
+    // the global object.
+    args.GetReturnValue().Set(false);
+  }
 };
 
 class ContextifyScript : public BaseObject {
@@ -702,14 +767,12 @@ class ContextifyScript : public BaseObject {
       TryCatch try_catch(env->isolate());
       // Do the eval within the context
       Context::Scope context_scope(contextify_context->context());
-      if (EvalMachine(contextify_context->env(),
-                      timeout,
-                      display_errors,
-                      break_on_sigint,
-                      args,
-                      &try_catch)) {
-        contextify_context->CopyProperties();
-      }
+      EvalMachine(contextify_context->env(),
+                  timeout,
+                  display_errors,
+                  break_on_sigint,
+                  args,
+                  &try_catch);
 
       if (try_catch.HasCaught()) {
         try_catch.ReThrow();

test/known_issues/test-vm-attributes-property-not-on-sandbox.js

@@ -1,25 +0,0 @@
-'use strict';
-require('../common');
-const assert = require('assert');
-const vm = require('vm');
-
-// The QueryCallback explicitly calls GetRealNamedPropertyAttributes
-// on the global proxy if the property is not found on the sandbox.
-//
-// foo is not defined on the sandbox until we call CopyProperties().
-// In the QueryCallback, we do not find the property on the sandbox
-// and look up its PropertyAttributes on the global_proxy().
-// PropertyAttributes are always flattened to a value
-// descriptor.
-const sandbox = {};
-vm.createContext(sandbox);
-const code = `Object.defineProperty(
-               this,
-               'foo',
-               { get: function() {return 17} }
-             );
-             var desc = Object.getOwnPropertyDescriptor(this, 'foo');`;
-
-vm.runInContext(code, sandbox);
-// The descriptor is flattened. We wrongly have typeof desc.value = 'number'.
-assert.strictEqual(typeof sandbox.desc.get, 'function');

test/parallel/test-vm-attributes-property-not-on-sandbox.js

@@ -0,0 +1,18 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+// Assert that accessor descriptors are not flattened on the sandbox.
+// Issue: https://github.com/nodejs/node/issues/2734
+const sandbox = {};
+vm.createContext(sandbox);
+const code = `Object.defineProperty(
+               this,
+               'foo',
+               { get: function() {return 17} }
+             );
+             var desc = Object.getOwnPropertyDescriptor(this, 'foo');`;
+
+vm.runInContext(code, sandbox);
+assert.strictEqual(typeof sandbox.desc.get, 'function');

test/parallel/test-vm-context.js

@@ -106,16 +106,3 @@ assert.throws(() => {
 // https://github.com/nodejs/node/issues/6158
 ctx = new Proxy({}, {});
 assert.strictEqual(typeof vm.runInNewContext('String', ctx), 'function');
-
-// https://github.com/nodejs/node/issues/10223
-ctx = vm.createContext();
-vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
-assert.strictEqual(ctx.x, 42);
-assert.strictEqual(vm.runInContext('x', ctx), 42);
-
-vm.runInContext('x = 0', ctx);                      // Does not throw but x...
-assert.strictEqual(vm.runInContext('x', ctx), 42);  // ...should be unaltered.
-
-assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
-              /Cannot assign to read only property 'x'/);
-assert.strictEqual(vm.runInContext('x', ctx), 42);

test/parallel/test-vm-data-property-writable.js

@@ -7,11 +7,22 @@ const assert = require('assert');
 
 const context = vm.createContext({});
 
-const code = `
+let code = `
    Object.defineProperty(this, 'foo', {value: 5});
    Object.getOwnPropertyDescriptor(this, 'foo');
 `;
 
-const desc = vm.runInContext(code, context);
+let desc = vm.runInContext(code, context);
 
 assert.strictEqual(desc.writable, false);
+
+// Check that interceptors work for symbols.
+code = `
+   const bar = Symbol('bar');
+   Object.defineProperty(this, bar, {value: 6});
+   Object.getOwnPropertyDescriptor(this, bar);
+`;
+
+desc = vm.runInContext(code, context);
+
+assert.strictEqual(desc.value, 6);

test/parallel/test-vm-global-define-property.js

@@ -44,4 +44,4 @@ assert(res);
 assert.strictEqual(typeof res, 'object');
 assert.strictEqual(res, x);
 assert.strictEqual(o.f, res);
-assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'g', 'f']);
+assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'f', 'g']);

test/parallel/test-vm-global-non-writable-properties.js

@@ -7,7 +7,6 @@ const vm = require('vm');
 
 const ctx = vm.createContext();
 vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
-assert.strictEqual(ctx.x, undefined);  // Not copied out by cloneProperty().
 assert.strictEqual(vm.runInContext('x', ctx), 42);
 vm.runInContext('x = 0', ctx);                      // Does not throw but x...
 assert.strictEqual(vm.runInContext('x', ctx), 42);  // ...should be unaltered.

test/parallel/test-vm-proxy-failure-CP.js

@@ -1,13 +1,10 @@
 'use strict';
-
-// Sandbox throws in CopyProperties() despite no code being run
-// Issue: https://github.com/nodejs/node/issues/11902
-
-
 require('../common');
 const assert = require('assert');
 const vm = require('vm');
 
+// Check that we do not accidentally query attributes.
+// Issue: https://github.com/nodejs/node/issues/11902
 const handler = {
   getOwnPropertyDescriptor: (target, prop) => {
     throw new Error('whoops');
@@ -16,5 +13,4 @@ const handler = {
 const sandbox = new Proxy({ foo: 'bar' }, handler);
 const context = vm.createContext(sandbox);
 
-
 assert.doesNotThrow(() => vm.runInContext('', context));

test/parallel/test-vm-strict-assign.js

@@ -0,0 +1,20 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+
+const vm = require('vm');
+
+// https://github.com/nodejs/node/issues/10223
+const ctx = vm.createContext();
+
+// Define x with writable = false.
+vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
+assert.strictEqual(ctx.x, 42);
+assert.strictEqual(vm.runInContext('x', ctx), 42);
+
+vm.runInContext('x = 0', ctx);                      // Does not throw but x...
+assert.strictEqual(vm.runInContext('x', ctx), 42);  // ...should be unaltered.
+
+assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
+              /Cannot assign to read only property 'x'/);
+assert.strictEqual(vm.runInContext('x', ctx), 42);

test/parallel/test-vm-strict-mode.js

@@ -1,27 +0,0 @@
-'use strict';
-require('../common');
-const assert = require('assert');
-const vm = require('vm');
-const ctx = vm.createContext();
-
-// Test strict mode inside a vm script, i.e., using an undefined variable
-// throws a ReferenceError. Also check that variables
-// that are not successfully set in the vm, must not be set
-// on the sandboxed context.
-
-vm.runInContext('w = 1;', ctx);
-assert.strictEqual(1, ctx.w);
-
-assert.throws(function() { vm.runInContext('"use strict"; x = 1;', ctx); },
-              /ReferenceError: x is not defined/);
-assert.strictEqual(undefined, ctx.x);
-
-vm.runInContext('"use strict"; var y = 1;', ctx);
-assert.strictEqual(1, ctx.y);
-
-vm.runInContext('"use strict"; this.z = 1;', ctx);
-assert.strictEqual(1, ctx.z);
-
-// w has been defined
-vm.runInContext('"use strict"; w = 2;', ctx);
-assert.strictEqual(2, ctx.w);