From 953767251180eb63a61ddfb154d6f7468501a57b Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Sat, 18 Mar 2023 10:19:19 +0100 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`. PR-URL: https://github.com/nodejs/node/pull/46615 Reviewed-By: Vladimir de Turckheim Reviewed-By: Anna Henningsen --- src/env_properties.h | 2 + src/node_contextify.cc | 18 ++- test/parallel/test-vm-global-setter.js | 147 +++++++++++++++++++++++-- test/parallel/test-vm-global-symbol.js | 11 ++ test/parallel/test-vm-not-strict.js | 37 +++++++ 5 files changed, 203 insertions(+), 12 deletions(-) create mode 100644 test/parallel/test-vm-not-strict.js diff --git a/src/env_properties.h b/src/env_properties.h index 65ab65de3ac3fd..9133b8efbd5b74 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -136,6 +136,7 @@ V(frames_received_string, "framesReceived") \ V(frames_sent_string, "framesSent") \ V(function_string, "function") \ + V(get_string, "get") \ V(get_data_clone_error_string, "_getDataCloneError") \ V(get_shared_array_buffer_id_string, "_getSharedArrayBufferId") \ V(gid_string, "gid") \ @@ -269,6 +270,7 @@ V(servername_string, "servername") \ V(service_string, "service") \ V(session_id_string, "sessionId") \ + V(set_string, "set") \ V(shell_string, "shell") \ V(signal_string, "signal") \ V(sink_string, "sink") \ diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 5aadefd84d6af7..a21acf06a32781 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -527,9 +527,21 @@ void ContextifyContext::PropertySetterCallback( !is_function) return; - USE(ctx->sandbox()->Set(context, property, value)); - if (is_contextual_store || is_function) { - args.GetReturnValue().Set(value); + if (ctx->sandbox()->Set(context, property, value).IsNothing()) return; + + Local desc; + if (is_declared_on_sandbox && + ctx->sandbox() + ->GetOwnPropertyDescriptor(context, property) + .ToLocal(&desc)) { + Environment* env = Environment::GetCurrent(context); + Local desc_obj = desc.As(); + + // We have to specify the return value for any contextual or get/set + // property + if (desc_obj->HasOwnProperty(context, env->get_string()).FromMaybe(false) || + desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false)) + args.GetReturnValue().Set(value); } } diff --git a/test/parallel/test-vm-global-setter.js b/test/parallel/test-vm-global-setter.js index 878937f612ad64..36aab52e44e166 100644 --- a/test/parallel/test-vm-global-setter.js +++ b/test/parallel/test-vm-global-setter.js @@ -3,27 +3,156 @@ 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); -assert.strictEqual(window.globalProxy.onhashchange, 42); +window.globalProxy[getSetSymbolReceivingNumber] = 147; +assert.strictEqual(window.globalProxy[getSetSymbolReceivingNumber], 47); + +window.globalProxy[symbolReceivingNumber] = 148; +assert.strictEqual(window.globalProxy[symbolReceivingNumber], 148); + +window.globalProxy[unknownSymbolReceivingNumber] = 149; +assert.strictEqual(window.globalProxy[unknownSymbolReceivingNumber], 149); + +assert.throws( + () => (window.globalProxy.getSetPropThrowing = 150), + new Error('setter called') +); +assert.strictEqual(window.globalProxy.getSetPropThrowing, 50); + +assert.throws( + () => (window.globalProxy.nonWritableProp = 151), + new TypeError('Cannot redefine property: nonWritableProp') +); +assert.strictEqual(window.globalProxy.nonWritableProp, 51); 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; + 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; + 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)); diff --git a/test/parallel/test-vm-not-strict.js b/test/parallel/test-vm-not-strict.js new file mode 100644 index 00000000000000..8a72158a8a0fc0 --- /dev/null +++ b/test/parallel/test-vm-not-strict.js @@ -0,0 +1,37 @@ +/* eslint-disable strict, no-var, no-delete-var, no-undef, node-core/required-modules, node-core/require-common-first */ +// Importing common would break the execution. Indeed running `vm.runInThisContext` alters the global context +// when declaring new variables with `var`. The other rules (strict, no-var, no-delete-var) have been disabled +// in order to be able to test this specific not-strict case playing with `var` and `delete`. +// Related to bug report: https://github.com/nodejs/node/issues/43129 +var assert = require('assert'); +var vm = require('vm'); + +var data = []; +var a = 'direct'; +delete a; +data.push(a); + +var item2 = vm.runInThisContext(` +var unusedB = 1; +var data = []; +var b = "this"; +delete b; +data.push(b); +data[0] +`); +data.push(item2); + +vm.runInContext( + ` +var unusedC = 1; +var c = "new"; +delete c; +data.push(c); +`, + vm.createContext({ data: data }) +); + +assert.deepStrictEqual(data, ['direct', 'this', 'new']); + +assert.strictEqual(typeof unusedB, 'number'); // Declared within runInThisContext +assert.strictEqual(typeof unusedC, 'undefined'); // Declared within runInContext