From cdb7d2f2ba6958b709f2dfbf97a9305385e56861 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sat, 18 Aug 2018 14:04:05 -0400 Subject: [PATCH 1/2] src: implement query callbacks for vm This is a re-land of a commit landed as part of https://github.com/nodejs/node/pull/22390. --- This allows using a Proxy object as the sandbox for a VM context. Refs: https://github.com/nodejs/node/pull/22390 Fixes: https://github.com/nodejs/node/issues/17480 Fixes: https://github.com/nodejs/node/issues/17481 Reviewed-By: Anna Henningsen Reviewed-By: Gus Caplan Reviewed-By: James M Snell --- doc/api/vm.md | 42 +++++++++++++++++ src/node_contextify.cc | 42 ++++++++++++++++- src/node_contextify.h | 6 +++ test/parallel/test-vm-proxy.js | 83 ++++++++++++++++++++++++++++++++++ 4 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-vm-proxy.js 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..92e2ed9429d59a 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, 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, @@ -536,6 +560,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, diff --git a/src/node_contextify.h b/src/node_contextify.h index d2b8387f214109..965303a79bbdb1 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); 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 + }); +} From 276ad5ed49180f6376e94ad2264ec20b9bcfa7b2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 13 Sep 2018 10:22:06 +0200 Subject: [PATCH 2/2] src: fix vm enumerator callbacks Some of the choices here are odd, including that symbols are missing. However, that matches previous behaviour. What had to be changed was that inherited properties are no longer included; the alternative would be to also refactor the descriptor callbacks to provide data for inherited properties. Fixes: https://github.com/nodejs/node/issues/22723 Refs: https://github.com/nodejs/node/pull/22390 --- src/node_contextify.cc | 46 ++++++++++++++++++- src/node_contextify.h | 2 + test/parallel/test-vm-object-keys.js | 45 ++++++++++++++++++ .../test-vm-ownpropertynames.js | 0 4 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-vm-object-keys.js rename test/{known_issues => parallel}/test-vm-ownpropertynames.js (100%) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 92e2ed9429d59a..e20705d003c2f0 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -155,7 +155,7 @@ Local ContextifyContext::CreateV8Context( IndexedPropertySetterCallback, IndexedPropertyQueryCallback, IndexedPropertyDeleterCallback, - PropertyEnumeratorCallback, + IndexedPropertyEnumeratorCallback, IndexedPropertyDefinerCallback, IndexedPropertyDescriptorCallback, CreateDataWrapper(env)); @@ -528,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 @@ -623,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 965303a79bbdb1..67fc55240cc065 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -104,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