vm: properly handle defining symbol props

This PR is a follow-up of #46615. When bumping V8 for node 18, various bugs appeared around vm. Some have been fixed along the flow, but there is at least one remaining and described at: jestjs/jest#13338. The current fix does nothing on node 20: the new bump of v8 done for node 20 seems to fix it. But it would fix the problem in both node 18 and node 19. I confirmed it would fix node 19 by cherry-picking the commit on v19.x-staging and launching `make -j4 jstest` on it.
nodejs-github-bot · May 26, 2023 · Apr 15, 2023 · Apr 19, 2023 · Apr 19, 2023 · Apr 19, 2023
commit d164cd4d1e1df5d507ea719cf45607ab6d520137
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -527,6 +527,7 @@ void ContextifyContext::PropertySetterCallback(
       !is_function)
     return;
 
+  if (!is_declared && property->IsSymbol()) return;
   if (ctx->sandbox()->Set(context, property, value).IsNothing()) return;
 
   Local<Value> desc;

diff --git a/test/parallel/test-vm-global-get-own.js b/test/parallel/test-vm-global-get-own.js
@@ -0,0 +1,80 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+// These assertions check that we can set new keys to the global context,
+// get them back and also list them via getOwnProperty*.
+//
+// Related to:
+// - https://github.com/nodejs/node/issues/45983
+
+const global = vm.runInContext('this', vm.createContext());
+
+const totoSymbol = Symbol.for('toto');
+Object.defineProperty(global, totoSymbol, {
+  enumerable: true,
+  writable: true,
+  value: 4,
+  configurable: true,
+});
+assert.strictEqual(global[totoSymbol], 4);
+assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol));
+Object.defineProperty(global, totoSymbol, {
+  enumerable: true,
+  writable: true,
+  value: 44,
+  configurable: true,
+});
+assert.strictEqual(global[totoSymbol], 44);
+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));
+Object.defineProperty(global, totoKey, {
+  enumerable: true,
+  writable: true,
+  value: 55,
+  configurable: true,
+});
+assert.strictEqual(global[totoKey], 55);
+assert.ok(Object.getOwnPropertyNames(global).includes(totoKey));
+
+const titiSymbol = Symbol.for('titi');
+global[titiSymbol] = 6;
+assert.strictEqual(global[titiSymbol], 6);
+assert.ok(Object.getOwnPropertySymbols(global).includes(titiSymbol));
+global[titiSymbol] = 66;
+assert.strictEqual(global[titiSymbol], 66);
+assert.ok(Object.getOwnPropertySymbols(global).includes(titiSymbol));
+
+const titiKey = 'titi';
+global[titiKey] = 7;
+assert.strictEqual(global[titiKey], 7);
+assert.ok(Object.getOwnPropertyNames(global).includes(titiKey));
+global[titiKey] = 77;
+assert.strictEqual(global[titiKey], 77);
+assert.ok(Object.getOwnPropertyNames(global).includes(titiKey));
+
+const tutuSymbol = Symbol.for('tutu');
+global[tutuSymbol] = () => {};
+assert.strictEqual(typeof global[tutuSymbol], 'function');
+assert.ok(Object.getOwnPropertySymbols(global).includes(tutuSymbol));
+global[tutuSymbol] = () => {};
+assert.strictEqual(typeof global[tutuSymbol], 'function');
+assert.ok(Object.getOwnPropertySymbols(global).includes(tutuSymbol));
+
+const tutuKey = 'tutu';
+global[tutuKey] = () => {};
+assert.strictEqual(typeof global[tutuKey], 'function');
+assert.ok(Object.getOwnPropertyNames(global).includes(tutuKey));
+global[tutuKey] = () => {};
+assert.strictEqual(typeof global[tutuKey], 'function');
+assert.ok(Object.getOwnPropertyNames(global).includes(tutuKey));
diff --git a/test/parallel/test-vm-global-symbol.js b/test/parallel/test-vm-global-symbol.js