From cdb7d2f2ba6958b709f2dfbf97a9305385e56861 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sat, 18 Aug 2018 14:04:05 -0400 Subject: [PATCH] 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 + }); +}