From b636069662e1be742bd90ec2c201db705d93aab3 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Sat, 11 Feb 2023 15:12:26 +0000 Subject: [PATCH 1/4] 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 | 22 +++- test/parallel/test-vm-global-setter.js | 147 +++++++++++++++++++++++-- test/parallel/test-vm-global-symbol.js | 11 ++ test/parallel/test-vm-not-strict.js | 39 +++++++ 4 files changed, 207 insertions(+), 12 deletions(-) create mode 100644 test/parallel/test-vm-not-strict.js diff --git a/src/node_contextify.cc b/src/node_contextify.cc index bd46a4a1e50e86..1a3dbc803b1cdf 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -526,10 +526,26 @@ void ContextifyContext::PropertySetterCallback( !is_function) return; - USE(ctx->sandbox()->Set(context, property, value)); - if (is_contextual_store || is_function) { - args.GetReturnValue().Set(value); + 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(); + Local get = String::NewFromUtf8(isolate, "get").ToLocalChecked(); + Local set = String::NewFromUtf8(isolate, "set").ToLocalChecked(); + is_get_set_property = + desc_obj->HasOwnProperty(context, get).FromMaybe(false) || + desc_obj->HasOwnProperty(context, set).FromMaybe(false); } + + USE(ctx->sandbox()->Set(context, property, value)); + + // 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..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..ceee45751af8a4 --- /dev/null +++ b/test/parallel/test-vm-not-strict.js @@ -0,0 +1,39 @@ +/* eslint-disable strict, no-var, no-delete-var, node-core/required-modules, node-core/require-common-first */ +// While common, should be used as first require of the test, using it causes errors: Unexpected global(s) found: data, b. +// Actually in this specific test case calling vm.runInThisContext exposes variables outside of the vm (behaviour existing since at least v14). +// The other rules (strict, no-var, no-delete-var) have been disabled to test this specific not strict case. +// 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']); + +// While the variables have been declared in the vm context, they are accessible in the global one too. +// This behaviour has been there at least from v14 of node, and still exist in 16 and 18. +assert.equal(typeof unusedB, 'number'); +assert.equal(typeof unusedC, 'number'); From af0f961f24d437bf373b7a429dbb5e6774f57e85 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Sun, 12 Feb 2023 20:48:01 +0000 Subject: [PATCH 2/4] Apply comments of 1st review --- src/env_properties.h | 2 ++ src/node_contextify.cc | 8 +++----- test/parallel/test-vm-not-strict.js | 14 ++++++-------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/env_properties.h b/src/env_properties.h index 52cd06ed92699e..d47d52cafdde20 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") \ @@ -271,6 +272,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 1a3dbc803b1cdf..1da75d4894ef79 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -533,13 +533,11 @@ void ContextifyContext::PropertySetterCallback( ->GetOwnPropertyDescriptor(context, property) .ToLocal(&desc) && desc->IsObject()) { + Environment* env = Environment::GetCurrent(context); Local desc_obj = desc.As(); - Isolate* isolate = context->GetIsolate(); - Local get = String::NewFromUtf8(isolate, "get").ToLocalChecked(); - Local set = String::NewFromUtf8(isolate, "set").ToLocalChecked(); is_get_set_property = - desc_obj->HasOwnProperty(context, get).FromMaybe(false) || - desc_obj->HasOwnProperty(context, set).FromMaybe(false); + desc_obj->HasOwnProperty(context, env->get_string()).FromMaybe(false) || + desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false); } USE(ctx->sandbox()->Set(context, property, value)); diff --git a/test/parallel/test-vm-not-strict.js b/test/parallel/test-vm-not-strict.js index ceee45751af8a4..8a72158a8a0fc0 100644 --- a/test/parallel/test-vm-not-strict.js +++ b/test/parallel/test-vm-not-strict.js @@ -1,7 +1,7 @@ -/* eslint-disable strict, no-var, no-delete-var, node-core/required-modules, node-core/require-common-first */ -// While common, should be used as first require of the test, using it causes errors: Unexpected global(s) found: data, b. -// Actually in this specific test case calling vm.runInThisContext exposes variables outside of the vm (behaviour existing since at least v14). -// The other rules (strict, no-var, no-delete-var) have been disabled to test this specific not strict case. +/* 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'); @@ -33,7 +33,5 @@ data.push(c); assert.deepStrictEqual(data, ['direct', 'this', 'new']); -// While the variables have been declared in the vm context, they are accessible in the global one too. -// This behaviour has been there at least from v14 of node, and still exist in 16 and 18. -assert.equal(typeof unusedB, 'number'); -assert.equal(typeof unusedC, 'number'); +assert.strictEqual(typeof unusedB, 'number'); // Declared within runInThisContext +assert.strictEqual(typeof unusedC, 'undefined'); // Declared within runInContext From 6572627f487f61f672ce6a4ab68bcc587630f947 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Wed, 8 Mar 2023 18:35:01 +0000 Subject: [PATCH 3/4] Apply comments of 2nd review --- src/node_contextify.cc | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 1da75d4894ef79..33779d7ba56cc3 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -526,24 +526,22 @@ void ContextifyContext::PropertySetterCallback( !is_function) return; + USE(ctx->sandbox()->Set(context, property, value)); + Local desc; - bool is_get_set_property = false; if (is_declared_on_sandbox && ctx->sandbox() ->GetOwnPropertyDescriptor(context, property) - .ToLocal(&desc) && - desc->IsObject()) { + .ToLocal(&desc)) { Environment* env = Environment::GetCurrent(context); Local desc_obj = desc.As(); - is_get_set_property = - desc_obj->HasOwnProperty(context, env->get_string()).FromMaybe(false) || - desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false); - } - USE(ctx->sandbox()->Set(context, property, value)); - - // We have to specify the return value for any contextual or get/set property - if (is_get_set_property) args.GetReturnValue().Set(value); + // We have to specify the return value for any contextual or get/set + // property + if (desc_obj->HasOwnProperty(context, env->get_string()).FromJust() || + desc_obj->HasOwnProperty(context, env->set_string()).FromJust()) + args.GetReturnValue().Set(value); + } } // static From 790fa93e216214253dada7f8298f24a0a2d26747 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Thu, 9 Mar 2023 11:27:33 +0000 Subject: [PATCH 4/4] Apply comments of 3rd review --- src/node_contextify.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 33779d7ba56cc3..9579d0d5b9a7a0 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -526,7 +526,7 @@ void ContextifyContext::PropertySetterCallback( !is_function) return; - USE(ctx->sandbox()->Set(context, property, value)); + if (ctx->sandbox()->Set(context, property, value).IsNothing()) return; Local desc; if (is_declared_on_sandbox && @@ -538,8 +538,8 @@ void ContextifyContext::PropertySetterCallback( // We have to specify the return value for any contextual or get/set // property - if (desc_obj->HasOwnProperty(context, env->get_string()).FromJust() || - desc_obj->HasOwnProperty(context, env->set_string()).FromJust()) + if (desc_obj->HasOwnProperty(context, env->get_string()).FromMaybe(false) || + desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false)) args.GetReturnValue().Set(value); } }