vm: properly handle defining props on any value

While it was supposed to fix most of the remaining issues,
nodejs#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.

src/node_contextify.cc

@@ -527,10 +527,22 @@ void ContextifyContext::PropertySetterCallback(
       !is_function)
     return;
 
+  Local<Value> desc;
+  bool is_get_set_property = false;
+  if (is_declared_on_sandbox &&
+      ctx->sandbox()->GetOwnPropertyDescriptor(context, property).ToLocal(&desc) &&
+      desc->IsObject()) {
+    Local<Object> desc_obj = desc.As<Object>();
+    Isolate* isolate = context->GetIsolate();
+    is_get_set_property = desc_obj->HasOwnProperty(context, String::NewFromUtf8(isolate, "get").ToLocalChecked()).FromMaybe(false) ||
+        desc_obj->HasOwnProperty(context, String::NewFromUtf8(isolate, "set").ToLocalChecked()).FromMaybe(false);
+  }
+
   USE(ctx->sandbox()->Set(context, property, value));
-  if (is_contextual_store || is_function) {
+
+  // We have to specify the return value for any contextual or get/set property
+  if (is_get_set_property)
     args.GetReturnValue().Set(value);
-  }
 }
 
 // static

test/parallel/test-vm-global-setter.js

@@ -3,27 +3,164 @@ const common = require('../common');
 const assert = require('assert');
 const vm = require('vm');
 
+const getSetSymbolReceivingFunction = Symbol('sym-1');
+const getSetSymbolReceivingNumber = Symbol('sym-2');
+const symbolReceivingNumber = Symbol('sym-3');
+const unknownSymbolReceivingNumber = Symbol('sym-4');
+
 const window = createWindow();
 
-const descriptor =
-  Object.getOwnPropertyDescriptor(window.globalProxy, 'onhashchange');
+const descriptor1 = Object.getOwnPropertyDescriptor(
+  window.globalProxy,
+  'getSetPropReceivingFunction'
+);
+assert.strictEqual(typeof descriptor1.get, 'function');
+assert.strictEqual(typeof descriptor1.set, 'function');
+assert.strictEqual(descriptor1.configurable, true);
+
+const descriptor2 = Object.getOwnPropertyDescriptor(
+  window.globalProxy,
+  'getSetPropReceivingNumber'
+);
+assert.strictEqual(typeof descriptor2.get, 'function');
+assert.strictEqual(typeof descriptor2.set, 'function');
+assert.strictEqual(descriptor2.configurable, true);
+
+const descriptor3 = Object.getOwnPropertyDescriptor(
+  window.globalProxy,
+  'propReceivingNumber'
+);
+assert.strictEqual(descriptor3.value, 44);
+
+const descriptor4 = Object.getOwnPropertyDescriptor(
+  window.globalProxy,
+  'unknownPropReceivingNumber'
+);
+assert.strictEqual(descriptor4, undefined);
+
+const descriptor5 = Object.getOwnPropertyDescriptor(
+  window.globalProxy,
+  getSetSymbolReceivingFunction
+);
+assert.strictEqual(typeof descriptor5.get, 'function');
+assert.strictEqual(typeof descriptor5.set, 'function');
+assert.strictEqual(descriptor5.configurable, true);
+
+const descriptor6 = Object.getOwnPropertyDescriptor(
+  window.globalProxy,
+  getSetSymbolReceivingNumber
+);
+assert.strictEqual(typeof descriptor6.get, 'function');
+assert.strictEqual(typeof descriptor6.set, 'function');
+assert.strictEqual(descriptor6.configurable, true);
+
+const descriptor7 = Object.getOwnPropertyDescriptor(
+  window.globalProxy,
+  symbolReceivingNumber
+);
+assert.strictEqual(descriptor7.value, 48);
 
-assert.strictEqual(typeof descriptor.get, 'function');
-assert.strictEqual(typeof descriptor.set, 'function');
-assert.strictEqual(descriptor.configurable, true);
+const descriptor8 = Object.getOwnPropertyDescriptor(
+  window.globalProxy,
+  unknownSymbolReceivingNumber
+);
+assert.strictEqual(descriptor8, undefined);
+
+const descriptor9 = Object.getOwnPropertyDescriptor(
+  window.globalProxy,
+  'getSetPropThrowing'
+);
+assert.strictEqual(typeof descriptor9.get, 'function');
+assert.strictEqual(typeof descriptor9.set, 'function');
+assert.strictEqual(descriptor9.configurable, true);
+
+const descriptor10 = Object.getOwnPropertyDescriptor(
+  window.globalProxy,
+  'nonWritableProp'
+);
+assert.strictEqual(descriptor10.value, 51);
+assert.strictEqual(descriptor10.writable, false);
 
 // Regression test for GH-42962. This assignment should not throw.
-window.globalProxy.onhashchange = () => {};
+window.globalProxy.getSetPropReceivingFunction = () => {};
+assert.strictEqual(window.globalProxy.getSetPropReceivingFunction, 42);
+
+window.globalProxy.getSetPropReceivingNumber = 143;
+assert.strictEqual(window.globalProxy.getSetPropReceivingNumber, 43);
+
+window.globalProxy.propReceivingNumber = 144;
+assert.strictEqual(window.globalProxy.propReceivingNumber, 144);
+
+window.globalProxy.unknownPropReceivingNumber = 145;
+assert.strictEqual(window.globalProxy.unknownPropReceivingNumber, 145);
+
+window.globalProxy[getSetSymbolReceivingFunction] = () => {};
+assert.strictEqual(window.globalProxy[getSetSymbolReceivingFunction], 46);
+
+window.globalProxy[getSetSymbolReceivingNumber] = 147;
+assert.strictEqual(window.globalProxy[getSetSymbolReceivingNumber], 47);
 
-assert.strictEqual(window.globalProxy.onhashchange, 42);
+window.globalProxy[symbolReceivingNumber] = 148;
+assert.strictEqual(window.globalProxy[symbolReceivingNumber], 148);
+
+window.globalProxy[unknownSymbolReceivingNumber] = 149;
+assert.strictEqual(window.globalProxy[unknownSymbolReceivingNumber], 149);
+
+let throwOnSet = false;
+try {
+  window.globalProxy.getSetPropThrowing = 150;
+} catch (err) {
+  throwOnSet = true;
+}
+assert.strictEqual(window.globalProxy.getSetPropThrowing, 50);
+assert.ok(throwOnSet);
+
+throwOnSet = false;
+try {
+  window.globalProxy.nonWritableProp = 151;
+} catch (err) {
+  throwOnSet = true;
+}
+assert.strictEqual(window.globalProxy.nonWritableProp, 51);
+assert.ok(throwOnSet);
 
 function createWindow() {
   const obj = {};
   vm.createContext(obj);
-  Object.defineProperty(obj, 'onhashchange', {
+  Object.defineProperty(obj, 'getSetPropReceivingFunction', {
     get: common.mustCall(() => 42),
     set: common.mustCall(),
-    configurable: true
+    configurable: true,
+  });
+  Object.defineProperty(obj, 'getSetPropReceivingNumber', {
+    get: common.mustCall(() => 43),
+    set: common.mustCall(),
+    configurable: true,
+  });
+  obj.propReceivingNumber = 44;
+  // no property for unknownPropReceivingNumber
+  Object.defineProperty(obj, getSetSymbolReceivingFunction, {
+    get: common.mustCall(() => 46),
+    set: common.mustCall(),
+    configurable: true,
+  });
+  Object.defineProperty(obj, getSetSymbolReceivingNumber, {
+    get: common.mustCall(() => 47),
+    set: common.mustCall(),
+    configurable: true,
+  });
+  obj[symbolReceivingNumber] = 48;
+  // no property for unknownSymbolReceivingNumber
+  Object.defineProperty(obj, 'getSetPropThrowing', {
+    get: common.mustCall(() => 50),
+    set: common.mustCall(() => {
+      throw new Error('setter called');
+    }),
+    configurable: true,
+  });
+  Object.defineProperty(obj, 'nonWritableProp', {
+    value: 51,
+    writable: false,
   });
 
   obj.globalProxy = vm.runInContext('this', obj);

test/parallel/test-vm-global-symbol.js

@@ -4,6 +4,7 @@ const assert = require('assert');
 const vm = require('vm');
 
 const global = vm.runInContext('this', vm.createContext());
+
 const totoSymbol = Symbol.for('toto');
 Object.defineProperty(global, totoSymbol, {
   enumerable: true,
@@ -13,3 +14,13 @@ Object.defineProperty(global, totoSymbol, {
 });
 assert.strictEqual(global[totoSymbol], 4);
 assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol));
+
+const totoKey = 'toto';
+Object.defineProperty(global, totoKey, {
+  enumerable: true,
+  writable: true,
+  value: 5,
+  configurable: true,
+});
+assert.strictEqual(global[totoKey], 5);
+assert.ok(Object.getOwnPropertyNames(global).includes(totoKey));