src: intercept property descriptors on sandbox

Fixes: nodejs#2734
Fixes: nodejs#5350
Fixes: nodejs#5679

src/node_contextify.cc

@@ -30,6 +30,8 @@
 #include "util-inl.h"
 #include "v8-debug.h"
 
+#include <string>
+
 namespace node {
 
 using v8::Array;
@@ -44,6 +46,7 @@ using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
 using v8::HandleScope;
 using v8::Integer;
+using v8::Isolate;
 using v8::Just;
 using v8::Local;
 using v8::Maybe;
@@ -234,9 +237,10 @@ class ContextifyContext {
 
     NamedPropertyHandlerConfiguration config(GlobalPropertyGetterCallback,
                                              GlobalPropertySetterCallback,
-                                             GlobalPropertyQueryCallback,
+                                             GlobalPropertyDescriptorCallback,
                                              GlobalPropertyDeleterCallback,
                                              GlobalPropertyEnumeratorCallback,
+                                             GlobalPropertyDefinerCallback,
                                              CreateDataWrapper(env));
     object_template->SetHandler(config);
 
@@ -413,22 +417,33 @@ class ContextifyContext {
       const PropertyCallbackInfo<Value>& args) {
     ContextifyContext* ctx;
     ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+    Environment* env = ctx->env();
+    Isolate* isolate = env->isolate();
 
     // Still initializing
     if (ctx->context_.IsEmpty())
       return;
 
     auto attributes = PropertyAttribute::None;
     bool is_declared =
-        ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(),
-                                                            property)
+        ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(),
+                                                       property)
         .To(&attributes);
     bool read_only =
         static_cast<int>(attributes) &
         static_cast<int>(PropertyAttribute::ReadOnly);
 
-    if (is_declared && read_only)
+    if (is_declared && read_only) {
+      if (args.ShouldThrowOnError()) {
+        std::string error_message("Cannot assign to read only property '");
+        Local<String> property_name = property->ToDetailString();
+        String::Utf8Value utf8_name(property_name);
+        error_message += *utf8_name;
+        error_message +=  "' of object '#<Object>'";
+        env->ThrowTypeError(error_message.c_str());
+      }
       return;
+    }
 
     // true for x = 5
     // false for this.x = 5
@@ -453,9 +468,9 @@ class ContextifyContext {
   }
 
 
-  static void GlobalPropertyQueryCallback(
+  static void GlobalPropertyDescriptorCallback(
       Local<Name> property,
-      const PropertyCallbackInfo<Integer>& args) {
+      const PropertyCallbackInfo<Value>& args) {
     ContextifyContext* ctx;
     ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
 
@@ -464,18 +479,14 @@ class ContextifyContext {
       return;
 
     Local<Context> context = ctx->context();
-    Maybe<PropertyAttribute> maybe_prop_attr =
-        ctx->sandbox()->GetRealNamedPropertyAttributes(context, property);
+    MaybeLocal<Value> maybe_prop_desc =
+        ctx->sandbox()->GetOwnPropertyDescriptor(context, property);
 
-    if (maybe_prop_attr.IsNothing()) {
-      maybe_prop_attr =
-          ctx->global_proxy()->GetRealNamedPropertyAttributes(context,
-                                                              property);
-    }
-
-    if (maybe_prop_attr.IsJust()) {
-      PropertyAttribute prop_attr = maybe_prop_attr.FromJust();
-      args.GetReturnValue().Set(prop_attr);
+    if (!maybe_prop_desc.IsEmpty()) {
+      Local<Value> prop_desc = maybe_prop_desc.ToLocalChecked();
+      if (!prop_desc->IsUndefined()) {
+        args.GetReturnValue().Set(prop_desc);
+      }
     }
   }
 
@@ -512,6 +523,72 @@ class ContextifyContext {
 
     args.GetReturnValue().Set(ctx->sandbox()->GetPropertyNames());
   }
+
+
+  static void GlobalPropertyDefinerCallback(
+      Local<Name> property,
+      const PropertyDescriptor& desc,
+      const PropertyCallbackInfo<Value>& args) {
+    ContextifyContext* ctx;
+    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+    Environment* env = ctx->env();
+
+    // Still initializing
+    if (ctx->context_.IsEmpty())
+      return;
+
+    auto attributes = PropertyAttribute::None;
+    bool is_declared =
+        ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(),
+                                                       property)
+        .To(&attributes);
+    bool non_enumerable =
+        static_cast<int>(attributes) &
+        static_cast<int>(PropertyAttribute::DontDelete);
+
+    if (is_declared && non_enumerable) {
+      if (args.ShouldThrowOnError()) {
+        std::string error_message("Cannot redefine property: ");
+        Local<String> property_name = property->ToDetailString();
+        String::Utf8Value utf8_name(property_name);
+        error_message += *utf8_name;
+        env->ThrowTypeError(error_message.c_str());
+      }
+      return;
+    }
+
+    Local<Context> context = ctx->context();
+    auto add_desc_copy_to_sandbox =
+        [&] (PropertyDescriptor* desc_copy) {
+          if (desc.has_enumerable()) {
+            desc_copy->set_enumerable(desc.enumerable());
+          }
+          if (desc.has_configurable()) {
+            desc_copy->set_configurable(desc.configurable());
+          }
+          Maybe<bool> result =
+              ctx->sandbox()->DefineProperty(context, property, *desc_copy);
+          if (result.IsJust()) {
+            args.GetReturnValue().Set(result.FromJust());
+          }
+        };
+
+    Isolate* isolate = context->GetIsolate();
+    if (desc.has_get() || desc.has_set()) {
+      Local<Value> get =
+          desc.has_get() ? desc.get() : Undefined(isolate).As<Value>();
+      Local<Value> set =
+          desc.has_set() ? desc.set() : Undefined(isolate).As<Value>();
+      PropertyDescriptor desc_copy(get, set);
+      add_desc_copy_to_sandbox(&desc_copy);
+    } else {
+      bool writable = desc.has_writable() ? desc.writable() : false;
+      Local<Value> value =
+          desc.has_value() ? desc.value() :  Undefined(isolate).As<Value>();
+      PropertyDescriptor desc_copy(value, writable);
+      add_desc_copy_to_sandbox(&desc_copy);
+    }
+  }
 };
 
 class ContextifyScript : public BaseObject {

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

@@ -0,0 +1,16 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+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

@@ -115,6 +115,9 @@ 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'/);
+const error = new RegExp(
+  'TypeError: Cannot assign to read only property \'x\' of ' +
+  'object \'#<Object>\''
+);
+assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx), error);
 assert.strictEqual(vm.runInContext('x', ctx), 42);

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-property-interceptors.js

@@ -0,0 +1,130 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+const dSymbol = Symbol('d');
+const sandbox = {
+  a: 'a',
+  dSymbol
+};
+
+Object.defineProperties(sandbox, {
+  b: {
+    value: 'b'
+  },
+  c: {
+    value: 'c',
+    writable: true,
+    enumerable: true
+  },
+  [dSymbol]: {
+    value: 'd'
+  },
+  e: {
+    value: 'e',
+    configurable: true
+  },
+  f: {}
+});
+
+const ctx = vm.createContext(sandbox);
+
+const result = vm.runInContext(`
+const getDesc = (prop) => Object.getOwnPropertyDescriptor(this, prop);
+const result = {
+  a: getDesc('a'),
+  b: getDesc('b'),
+  c: getDesc('c'),
+  d: getDesc(dSymbol),
+  e: getDesc('e'),
+  f: getDesc('f'),
+  g: getDesc('g')
+};
+result;
+`, ctx);
+
+//eslint-disable-next-line no-restricted-properties
+assert.deepEqual(result, {
+  a: { value: 'a', writable: true, enumerable: true, configurable: true },
+  b: { value: 'b', writable: false, enumerable: false, configurable: false },
+  c: { value: 'c', writable: true, enumerable: true, configurable: false },
+  d: { value: 'd', writable: false, enumerable: false, configurable: false },
+  e: { value: 'e', writable: false, enumerable: false, configurable: true },
+  f: {
+    value: undefined,
+    writable: false,
+    enumerable: false,
+    configurable: false
+  },
+  g: undefined
+});
+
+// define new properties
+vm.runInContext(`
+Object.defineProperty(this, 'h', {value: 'h'});
+Object.defineProperty(this, 'i', {});
+Object.defineProperty(this, 'j', {
+  get() { return 'j'; }
+});
+
+let kValue = 0;
+Object.defineProperty(this, 'k', {
+  get() { return kValue; },
+  set(value) { kValue = value }
+});
+`, ctx);
+
+assert.deepStrictEqual(Object.getOwnPropertyDescriptor(ctx, 'h'), {
+  value: 'h',
+  writable: false,
+  enumerable: false,
+  configurable: false
+});
+
+assert.deepStrictEqual(Object.getOwnPropertyDescriptor(ctx, 'i'), {
+  value: undefined,
+  writable: false,
+  enumerable: false,
+  configurable: false
+});
+
+const jDesc = Object.getOwnPropertyDescriptor(ctx, 'j');
+assert.strictEqual(typeof jDesc.get, 'function');
+assert.strictEqual(typeof jDesc.set, 'undefined');
+assert.strictEqual(jDesc.enumerable, false);
+assert.strictEqual(jDesc.configurable, false);
+
+const kDesc = Object.getOwnPropertyDescriptor(ctx, 'k');
+assert.strictEqual(typeof kDesc.get, 'function');
+assert.strictEqual(typeof kDesc.set, 'function');
+assert.strictEqual(kDesc.enumerable, false);
+assert.strictEqual(kDesc.configurable, false);
+
+assert.strictEqual(ctx.k, 0);
+ctx.k = 1;
+assert.strictEqual(ctx.k, 1);
+assert.strictEqual(vm.runInContext('k;', ctx), 1);
+vm.runInContext('k = 2;', ctx);
+assert.strictEqual(ctx.k, 2);
+assert.strictEqual(vm.runInContext('k;', ctx), 2);
+
+// redefine properties on the global object
+assert.strictEqual(typeof vm.runInContext('encodeURI;', ctx), 'function');
+assert.strictEqual(ctx.encodeURI, undefined);
+vm.runInContext(`
+Object.defineProperty(this, 'encodeURI', { value: 42 });
+`, ctx);
+assert.strictEqual(vm.runInContext('encodeURI;', ctx), 42);
+assert.strictEqual(ctx.encodeURI, 42);
+
+// redefine properties on the sandbox
+vm.runInContext(`
+Object.defineProperty(this, 'e', { value: 'newE' });
+`, ctx);
+assert.strictEqual(ctx.e, 'newE');
+
+assert.throws(() => vm.runInContext(`
+'use strict';
+Object.defineProperty(this, 'f', { value: 'newF' });
+`, ctx), /TypeError: Cannot redefine property: f/);