src: fix ContextifyContext property definer interception result

legendecas · aduh95 · commit 860f9d8d4b7e · 2026-05-30T19:26:25.000+02:00
Signed-off-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: #63549 Fixes: #52634 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -700,32 +700,31 @@ Intercepted ContextifyContext::PropertyDefinerCallback(
         if (desc.has_configurable()) {
           desc_for_sandbox->set_configurable(desc.configurable());
         }
-        // Set the property on the sandbox.
-        USE(sandbox->DefineProperty(context, property, *desc_for_sandbox));
+        return sandbox->DefineProperty(context, property, *desc_for_sandbox);
       };
 
   if (desc.has_get() || desc.has_set()) {
     PropertyDescriptor desc_for_sandbox(
         desc.has_get() ? desc.get() : Undefined(isolate).As<Value>(),
         desc.has_set() ? desc.set() : Undefined(isolate).As<Value>());
 
-    define_prop_on_sandbox(&desc_for_sandbox);
-    // TODO(https://github.com/nodejs/node/issues/52634): this should return
-    // kYes to behave according to the expected semantics.
+    if (define_prop_on_sandbox(&desc_for_sandbox).FromMaybe(false))
+      return Intercepted::kYes;
     return Intercepted::kNo;
   } else {
     Local<Value> value =
         desc.has_value() ? desc.value() : Undefined(isolate).As<Value>();
 
+    Maybe<bool> result;
     if (desc.has_writable()) {
       PropertyDescriptor desc_for_sandbox(value, desc.writable());
-      define_prop_on_sandbox(&desc_for_sandbox);
+      result = define_prop_on_sandbox(&desc_for_sandbox);
     } else {
       PropertyDescriptor desc_for_sandbox(value);
-      define_prop_on_sandbox(&desc_for_sandbox);
+      result = define_prop_on_sandbox(&desc_for_sandbox);
     }
-    // TODO(https://github.com/nodejs/node/issues/52634): this should return
-    // kYes to behave according to the expected semantics.
+
+    if (result.FromMaybe(false)) return Intercepted::kYes;
     return Intercepted::kNo;
   }
 }
diff --git a/test/parallel/test-vm-property-definer-interception.js b/test/parallel/test-vm-property-definer-interception.js
@@ -0,0 +1,24 @@
+'use strict';
+
+require('../common');
+const vm = require('vm');
+const assert = require('assert');
+
+// Each [[DefineOwnProperty]] intercepted by the definer should invoke the
+// sandbox's [[DefineOwnProperty]] exactly once.
+{
+  let count = 0;
+  const sandbox = new Proxy({}, {
+    defineProperty(target, key, desc) {
+      count++;
+      return Reflect.defineProperty(target, key, desc);
+    },
+  });
+  const ctx = vm.createContext(sandbox);
+  vm.runInContext(`
+    Object.defineProperty(this, 'a', { value: 1 });
+    Object.defineProperty(this, 'b', { value: 2, writable: true });
+    Object.defineProperty(this, 'c', { get() { return 3; } });
+  `, ctx);
+  assert.strictEqual(count, 3);
+}
