From 90d4c93efabff1f522764d1f4af5994fb677dd84 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Sat, 11 Feb 2023 15:12:26 +0000 Subject: [PATCH] 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`. --- src/node_contextify.cc | 16 ++- test/parallel/test-vm-global-setter.js | 155 +++++++++++++++++++++++-- test/parallel/test-vm-global-symbol.js | 11 ++ 3 files changed, 171 insertions(+), 11 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index d618dafcb5f14d..79fea1cdd6ed9c 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -527,10 +527,22 @@ void ContextifyContext::PropertySetterCallback( !is_function) return; + Local desc; + bool is_get_set_property = false; + if (is_declared_on_sandbox && + ctx->sandbox()->GetOwnPropertyDescriptor(context, property).ToLocal(&desc) && + desc->IsObject()) { + Local desc_obj = desc.As(); + Isolate* isolate = context->GetIsolate(); + is_get_set_property = desc_obj->HasOwnProperty(context, String::NewFromUtf8(isolate, "get").ToLocalChecked()).FromMaybe(false) || + desc_obj->HasOwnProperty(context, String::NewFromUtf8(isolate, "set").ToLocalChecked()).FromMaybe(false); + } + USE(ctx->sandbox()->Set(context, property, value)); - if (is_contextual_store || is_function) { + + // We have to specify the return value for any contextual or get/set property + if (is_get_set_property) args.GetReturnValue().Set(value); - } } // static diff --git a/test/parallel/test-vm-global-setter.js b/test/parallel/test-vm-global-setter.js index 878937f612ad64..9cfd6c2b44d734 100644 --- a/test/parallel/test-vm-global-setter.js +++ b/test/parallel/test-vm-global-setter.js @@ -3,27 +3,164 @@ 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); + +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); -assert.strictEqual(typeof descriptor.get, 'function'); -assert.strictEqual(typeof descriptor.set, 'function'); -assert.strictEqual(descriptor.configurable, true); +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); + +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); -assert.strictEqual(window.globalProxy.onhashchange, 42); +window.globalProxy[symbolReceivingNumber] = 148; +assert.strictEqual(window.globalProxy[symbolReceivingNumber], 148); + +window.globalProxy[unknownSymbolReceivingNumber] = 149; +assert.strictEqual(window.globalProxy[unknownSymbolReceivingNumber], 149); + +let throwOnSet = false; +try { + window.globalProxy.getSetPropThrowing = 150; +} catch (err) { + throwOnSet = true; +} +assert.strictEqual(window.globalProxy.getSetPropThrowing, 50); +assert.ok(throwOnSet); + +throwOnSet = false; +try { + window.globalProxy.nonWritableProp = 151; +} catch (err) { + throwOnSet = true; +} +assert.strictEqual(window.globalProxy.nonWritableProp, 51); +assert.ok(throwOnSet); 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; + // no property for unknownPropReceivingNumber + 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; + // no property for unknownSymbolReceivingNumber + 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); diff --git a/test/parallel/test-vm-global-symbol.js b/test/parallel/test-vm-global-symbol.js index a0dfbac7b8b10b..92038d9bfcf02d 100644 --- a/test/parallel/test-vm-global-symbol.js +++ b/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));