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

@@ -526,10 +526,26 @@ void ContextifyContext::PropertySetterCallback(
       !is_function)
     return;
 
-  USE(ctx->sandbox()->Set(context, property, value));
-  if (is_contextual_store || is_function) {
-    args.GetReturnValue().Set(value);
+  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();
+    Local<Name> get = String::NewFromUtf8(isolate, "get").ToLocalChecked();
+    Local<Name> set = String::NewFromUtf8(isolate, "set").ToLocalChecked();
+    is_get_set_property =
+        desc_obj->HasOwnProperty(context, get).FromMaybe(false) ||
+        desc_obj->HasOwnProperty(context, set).FromMaybe(false);
   }
+
+  USE(ctx->sandbox()->Set(context, property, value));
+
+  // 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,156 @@ 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);
 
-assert.strictEqual(window.globalProxy.onhashchange, 42);
+window.globalProxy[getSetSymbolReceivingNumber] = 147;
+assert.strictEqual(window.globalProxy[getSetSymbolReceivingNumber], 47);
+
+window.globalProxy[symbolReceivingNumber] = 148;
+assert.strictEqual(window.globalProxy[symbolReceivingNumber], 148);
+
+window.globalProxy[unknownSymbolReceivingNumber] = 149;
+assert.strictEqual(window.globalProxy[unknownSymbolReceivingNumber], 149);
+
+assert.throws(
+  () => (window.globalProxy.getSetPropThrowing = 150),
+  new Error('setter called')
+);
+assert.strictEqual(window.globalProxy.getSetPropThrowing, 50);
+
+assert.throws(
+  () => (window.globalProxy.nonWritableProp = 151),
+  new TypeError('Cannot redefine property: nonWritableProp')
+);
+assert.strictEqual(window.globalProxy.nonWritableProp, 51);
 
 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;
+  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;
+  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));

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

@@ -0,0 +1,39 @@
+/* eslint-disable strict, no-var, no-delete-var, node-core/required-modules, node-core/require-common-first */
+// While common, should be used as first require of the test, using it causes errors: Unexpected global(s) found: data, b.
+// Actually in this specific test case calling vm.runInThisContext exposes variables outside of the vm (behaviour existing since at least v14).
+// The other rules (strict, no-var, no-delete-var) have been disabled to test this specific not strict case.
+// Related to bug report: https://github.com/nodejs/node/issues/43129
+var assert = require('assert');
+var vm = require('vm');
+
+var data = [];
+var a = 'direct';
+delete a;
+data.push(a);
+
+var item2 = vm.runInThisContext(`
+var unusedB = 1;
+var data = [];
+var b = "this";
+delete b;
+data.push(b);
+data[0]
+`);
+data.push(item2);
+
+vm.runInContext(
+  `
+var unusedC = 1;
+var c = "new";
+delete c;
+data.push(c);
+`,
+  vm.createContext({ data: data })
+);
+
+assert.deepStrictEqual(data, ['direct', 'this', 'new']);
+
+// While the variables have been declared in the vm context, they are accessible in the global one too.
+// This behaviour has been there at least from v14 of node, and still exist in 16 and 18.
+assert.equal(typeof unusedB, 'number');
+assert.equal(typeof unusedC, 'number');