From 17b3ee33c26a870e1989d3f70ca9b54d59ab8767 Mon Sep 17 00:00:00 2001
From: Nicolas DUBIEN <github@dubien.org>
Date: Sat, 4 Feb 2023 11:32:31 +0100
Subject: [PATCH] vm: properly support symbols on globals
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A regression has been introduced in node 18.2.0,
it makes the following snippet fails while it used to work in the past:

```
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,
  writable: true,
  value: 4,
  configurable: true,
});
assert(Object.getOwnPropertySymbols(global).includes(totoSymbol));
```

Regression introduced by: https://github.com/nodejs/node/pull/42963.
So I basically attempted to start understanding what it changed to make
it fix the initial issue while not breaking the symbol related one.

Fixes: https://github.com/nodejs/node/issues/45983
PR-URL: https://github.com/nodejs/node/pull/46458
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
---
 src/node_contextify.cc                 |  4 +++-
 test/parallel/test-vm-global-symbol.js | 15 +++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 test/parallel/test-vm-global-symbol.js

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 28b3266f20b070..d618dafcb5f14d 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -528,7 +528,9 @@ void ContextifyContext::PropertySetterCallback(
     return;
 
   USE(ctx->sandbox()->Set(context, property, value));
-  args.GetReturnValue().Set(value);
+  if (is_contextual_store || is_function) {
+    args.GetReturnValue().Set(value);
+  }
 }
 
 // static
diff --git a/test/parallel/test-vm-global-symbol.js b/test/parallel/test-vm-global-symbol.js
new file mode 100644
index 00000000000000..a0dfbac7b8b10b
--- /dev/null
+++ b/test/parallel/test-vm-global-symbol.js
@@ -0,0 +1,15 @@
+'use strict';
+require('../common');
+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,
+  writable: true,
+  value: 4,
+  configurable: true,
+});
+assert.strictEqual(global[totoSymbol], 4);
+assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol));