diff --git a/doc/api/vm.md b/doc/api/vm.md index 80a4760b6dd947..94aace19fb3891 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -944,6 +944,48 @@ within which it can operate. The process of creating the V8 Context and associating it with the `sandbox` object is what this document refers to as "contextifying" the `sandbox`. +## vm module and Proxy object + +Leveraging a `Proxy` object as the sandbox of a VM context could result in a +very powerful runtime environment that intercepts all accesses to the global +object. However, there are some restrictions in the JavaScript engine that one +needs to be aware of to prevent unexpected results. In particular, providing a +`Proxy` object with a `get` handler could disallow any access to the original +global properties of the new VM context, as the `get` hook does not distinguish +between the `undefined` value and "requested property is not present" – +the latter of which would ordinarily trigger a lookup on the context global +object. + +Included below is a sample for how to work around this restriction. It +initializes the sandbox as a `Proxy` object without any hooks, only to add them +after the relevant properties have been saved. + +```js +'use strict'; +const { createContext, runInContext } = require('vm'); + +function createProxySandbox(handlers) { + // Create a VM context with a Proxy object with no hooks specified. + const sandbox = {}; + const proxyHandlers = {}; + const contextifiedProxy = createContext(new Proxy(sandbox, proxyHandlers)); + + // Save the initial globals onto our sandbox object. + const contextThis = runInContext('this', contextifiedProxy); + for (const prop of Reflect.ownKeys(contextThis)) { + const descriptor = Object.getOwnPropertyDescriptor(contextThis, prop); + Object.defineProperty(sandbox, prop, descriptor); + } + + // Now that `sandbox` contains all the initial global properties, assign the + // provided handlers to the handlers we used to create the Proxy. + Object.assign(proxyHandlers, handlers); + + // Return the created contextified Proxy object. + return contextifiedProxy; +} +``` + [`Error`]: errors.html#errors_class_error [`URL`]: url.html#url_class_url [`eval()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 8b9fef548061c6..e20705d003c2f0 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -143,19 +143,21 @@ Local ContextifyContext::CreateV8Context( NamedPropertyHandlerConfiguration config(PropertyGetterCallback, PropertySetterCallback, - PropertyDescriptorCallback, + PropertyQueryCallback, PropertyDeleterCallback, PropertyEnumeratorCallback, PropertyDefinerCallback, + PropertyDescriptorCallback, CreateDataWrapper(env)); IndexedPropertyHandlerConfiguration indexed_config( IndexedPropertyGetterCallback, IndexedPropertySetterCallback, - IndexedPropertyDescriptorCallback, + IndexedPropertyQueryCallback, IndexedPropertyDeleterCallback, - PropertyEnumeratorCallback, + IndexedPropertyEnumeratorCallback, IndexedPropertyDefinerCallback, + IndexedPropertyDescriptorCallback, CreateDataWrapper(env)); object_template->SetHandler(config); @@ -391,6 +393,28 @@ void ContextifyContext::PropertySetterCallback( ctx->sandbox()->Set(property, value); } +// static +void ContextifyContext::PropertyQueryCallback( + Local property, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx = ContextifyContext::Get(args); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + Local context = ctx->context(); + + Local sandbox = ctx->sandbox(); + + PropertyAttribute attributes; + if (sandbox->HasOwnProperty(context, property).FromMaybe(false) && + sandbox->GetPropertyAttributes(context, property).To(&attributes)) { + args.GetReturnValue().Set(attributes); + } +} + + // static void ContextifyContext::PropertyDescriptorCallback( Local property, @@ -504,7 +528,16 @@ void ContextifyContext::PropertyEnumeratorCallback( if (ctx->context_.IsEmpty()) return; - args.GetReturnValue().Set(ctx->sandbox()->GetPropertyNames()); + Local ret; + if (!ctx->sandbox()->GetPropertyNames( + ctx->context(), + v8::KeyCollectionMode::kOwnOnly, + v8::SKIP_SYMBOLS, + v8::IndexFilter::kSkipIndices).ToLocal(&ret)) { + return; + } + + args.GetReturnValue().Set(ret); } // static @@ -536,6 +569,20 @@ void ContextifyContext::IndexedPropertySetterCallback( Uint32ToName(ctx->context(), index), value, args); } +// static +void ContextifyContext::IndexedPropertyQueryCallback( + uint32_t index, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx = ContextifyContext::Get(args); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + ContextifyContext::PropertyQueryCallback( + Uint32ToName(ctx->context(), index), args); +} + // static void ContextifyContext::IndexedPropertyDescriptorCallback( uint32_t index, @@ -585,6 +632,39 @@ void ContextifyContext::IndexedPropertyDeleterCallback( args.GetReturnValue().Set(false); } +// static +void ContextifyContext::IndexedPropertyEnumeratorCallback( + const PropertyCallbackInfo& args) { + ContextifyContext* ctx = ContextifyContext::Get(args); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + Local all_keys; + if (!ctx->sandbox()->GetPropertyNames( + ctx->context(), + v8::KeyCollectionMode::kOwnOnly, + v8::SKIP_SYMBOLS, + v8::IndexFilter::kIncludeIndices).ToLocal(&all_keys)) { + return; + } + uint32_t all_keys_len = all_keys->Length(); + + Local ret = Array::New(ctx->env()->isolate()); + for (uint32_t i = 0, j = 0; i < all_keys_len; ++i) { + Local entry; + if (!all_keys->Get(ctx->context(), i).ToLocal(&entry)) + return; + if (!entry->IsUint32()) + continue; + if (!ret->Set(ctx->context(), j++, entry).FromMaybe(false)) + return; + } + + args.GetReturnValue().Set(ret); +} + class ContextifyScript : public BaseObject { private: Persistent script_; diff --git a/src/node_contextify.h b/src/node_contextify.h index d2b8387f214109..67fc55240cc065 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -69,6 +69,9 @@ class ContextifyContext { v8::Local property, v8::Local value, const v8::PropertyCallbackInfo& args); + static void PropertyQueryCallback( + v8::Local property, + const v8::PropertyCallbackInfo& args); static void PropertyDescriptorCallback( v8::Local property, const v8::PropertyCallbackInfo& args); @@ -88,6 +91,9 @@ class ContextifyContext { uint32_t index, v8::Local value, const v8::PropertyCallbackInfo& args); + static void IndexedPropertyQueryCallback( + uint32_t index, + const v8::PropertyCallbackInfo& args); static void IndexedPropertyDescriptorCallback( uint32_t index, const v8::PropertyCallbackInfo& args); @@ -98,6 +104,8 @@ class ContextifyContext { static void IndexedPropertyDeleterCallback( uint32_t index, const v8::PropertyCallbackInfo& args); + static void IndexedPropertyEnumeratorCallback( + const v8::PropertyCallbackInfo& args); Environment* const env_; Persistent context_; }; diff --git a/test/parallel/test-vm-object-keys.js b/test/parallel/test-vm-object-keys.js new file mode 100644 index 00000000000000..efccad767db5c1 --- /dev/null +++ b/test/parallel/test-vm-object-keys.js @@ -0,0 +1,45 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +// Regression test for https://github.com/nodejs/node/issues/22723. + +const kFoo = Symbol('kFoo'); + +const test = { + 'not': 'empty', + '0': 0, + '0.5': 0.5, + '1': 1, + '-1': -1, + [kFoo]: kFoo +}; +vm.createContext(test); +const proxied = vm.runInContext('this', test); + +// TODO(addaleax): The .sort() calls should not be necessary; the property +// order should be indices, then other properties, then symbols. +assert.deepStrictEqual( + Object.keys(proxied).sort(), + ['0', '1', 'not', '0.5', '-1'].sort()); +assert.deepStrictEqual( + // Filter out names shared by all global objects, i.e. JS builtins. + Object.getOwnPropertyNames(proxied) + .filter((name) => !(name in global)) + .sort(), + ['0', '1', 'not', '0.5', '-1'].sort()); +assert.deepStrictEqual(Object.getOwnPropertySymbols(proxied), []); + +Object.setPrototypeOf(test, { inherited: 'true', 17: 42 }); + +assert.deepStrictEqual( + Object.keys(proxied).sort(), + ['0', '1', 'not', '0.5', '-1'].sort()); +assert.deepStrictEqual( + // Filter out names shared by all global objects, i.e. JS builtins. + Object.getOwnPropertyNames(proxied) + .filter((name) => !(name in global)) + .sort(), + ['0', '1', 'not', '0.5', '-1'].sort()); +assert.deepStrictEqual(Object.getOwnPropertySymbols(proxied), []); diff --git a/test/known_issues/test-vm-ownpropertynames.js b/test/parallel/test-vm-ownpropertynames.js similarity index 100% rename from test/known_issues/test-vm-ownpropertynames.js rename to test/parallel/test-vm-ownpropertynames.js diff --git a/test/parallel/test-vm-proxy.js b/test/parallel/test-vm-proxy.js new file mode 100644 index 00000000000000..e83ba0dee95aed --- /dev/null +++ b/test/parallel/test-vm-proxy.js @@ -0,0 +1,83 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +const sandbox = {}; +const proxyHandlers = {}; +const contextifiedProxy = vm.createContext(new Proxy(sandbox, proxyHandlers)); + +// One must get the globals and manually assign it to our own global object, to +// mitigate against https://github.com/nodejs/node/issues/17465. +const contextThis = vm.runInContext('this', contextifiedProxy); +for (const prop of Reflect.ownKeys(contextThis)) { + const descriptor = Object.getOwnPropertyDescriptor(contextThis, prop); + Object.defineProperty(sandbox, prop, descriptor); +} + +// Finally, activate the proxy. +const numCalled = {}; +for (const hook of Reflect.ownKeys(Reflect)) { + numCalled[hook] = 0; + proxyHandlers[hook] = (...args) => { + numCalled[hook]++; + return Reflect[hook](...args); + }; +} + +{ + // Make sure the `in` operator only calls `getOwnPropertyDescriptor` and not + // `get`. + // Refs: https://github.com/nodejs/node/issues/17480 + assert.strictEqual(vm.runInContext('"a" in this', contextifiedProxy), false); + assert.deepStrictEqual(numCalled, { + defineProperty: 0, + deleteProperty: 0, + apply: 0, + construct: 0, + get: 0, + getOwnPropertyDescriptor: 1, + getPrototypeOf: 0, + has: 0, + isExtensible: 0, + ownKeys: 0, + preventExtensions: 0, + set: 0, + setPrototypeOf: 0 + }); +} + +{ + // Make sure `Object.getOwnPropertyDescriptor` only calls + // `getOwnPropertyDescriptor` and not `get`. + // Refs: https://github.com/nodejs/node/issues/17481 + + // Get and store the function in a lexically scoped variable to avoid + // interfering with the actual test. + vm.runInContext( + 'const { getOwnPropertyDescriptor } = Object;', + contextifiedProxy); + + for (const p of Reflect.ownKeys(numCalled)) { + numCalled[p] = 0; + } + + assert.strictEqual( + vm.runInContext('getOwnPropertyDescriptor(this, "a")', contextifiedProxy), + undefined); + assert.deepStrictEqual(numCalled, { + defineProperty: 0, + deleteProperty: 0, + apply: 0, + construct: 0, + get: 0, + getOwnPropertyDescriptor: 1, + getPrototypeOf: 0, + has: 0, + isExtensible: 0, + ownKeys: 0, + preventExtensions: 0, + set: 0, + setPrototypeOf: 0 + }); +}