vm: properly handle defining props on any value

While it was supposed to fix most of the remaining issues,
https://github.com/nodejs/node/pull/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`.

PR-URL: https://github.com/nodejs/node/pull/46615
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

src/env_properties.h

@@ -135,6 +135,7 @@
   V(frames_received_string, "framesReceived")                                  \
   V(frames_sent_string, "framesSent")                                          \
   V(function_string, "function")                                               \
+  V(get_string, "get")                                                         \
   V(get_data_clone_error_string, "_getDataCloneError")                         \
   V(get_shared_array_buffer_id_string, "_getSharedArrayBufferId")              \
   V(gid_string, "gid")                                                         \
@@ -270,6 +271,7 @@
   V(servername_string, "servername")                                           \
   V(service_string, "service")                                                 \
   V(session_id_string, "sessionId")                                            \
+  V(set_string, "set")                                                         \
   V(shell_string, "shell")                                                     \
   V(signal_string, "signal")                                                   \
   V(sink_string, "sink")                                                       \

src/node_contextify.cc

@@ -527,9 +527,21 @@ void ContextifyContext::PropertySetterCallback(
       !is_function)
     return;
 
-  USE(ctx->sandbox()->Set(context, property, value));
-  if (is_contextual_store || is_function) {
-    args.GetReturnValue().Set(value);
+  if (ctx->sandbox()->Set(context, property, value).IsNothing()) return;
+
+  Local<Value> desc;
+  if (is_declared_on_sandbox &&
+      ctx->sandbox()
+          ->GetOwnPropertyDescriptor(context, property)
+          .ToLocal(&desc)) {
+    Environment* env = Environment::GetCurrent(context);
+    Local<Object> desc_obj = desc.As<Object>();
+
+    // We have to specify the return value for any contextual or get/set
+    // property
+    if (desc_obj->HasOwnProperty(context, env->get_string()).FromMaybe(false) ||
+        desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false))
+      args.GetReturnValue().Set(value);
   }
 }
 

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);
 
-assert.strictEqual(typeof descriptor.get, 'function');
-assert.strictEqual(typeof descriptor.set, 'function');
-assert.strictEqual(descriptor.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);
+
+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);
 
-assert.strictEqual(window.globalProxy.onhashchange, 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);
+
+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,37 @@
+/* eslint-disable strict, no-var, no-delete-var, no-undef, node-core/required-modules, node-core/require-common-first */
+// Importing common would break the execution. Indeed running `vm.runInThisContext` alters the global context
+// when declaring new variables with `var`. The other rules (strict, no-var, no-delete-var) have been disabled
+// in order to be able to test this specific not-strict case playing with `var` and `delete`.
+// 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']);
+
+assert.strictEqual(typeof unusedB, 'number'); // Declared within runInThisContext
+assert.strictEqual(typeof unusedC, 'undefined'); // Declared within runInContext