diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 14534ab89650b4..f319420ae02f35 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -700,8 +700,7 @@ 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()) { @@ -709,23 +708,23 @@ Intercepted ContextifyContext::PropertyDefinerCallback( desc.has_get() ? desc.get() : Undefined(isolate).As(), desc.has_set() ? desc.set() : Undefined(isolate).As()); - 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 = desc.has_value() ? desc.value() : Undefined(isolate).As(); + Maybe 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 new file mode 100644 index 00000000000000..9e59d9ffd603e2 --- /dev/null +++ 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); +}