From a007ec8838f188108bb225c1c3d6fcae8be7abde Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 17 Sep 2018 20:13:59 +0200 Subject: [PATCH] Revert "src: implement query callbacks for vm" This reverts commit 85c356c10eec14f96eaf92ffc9a8481b591e3652 from PR https://github.com/nodejs/node/pull/22390. See the discussion in the (proposed) fix at https://github.com/nodejs/node/pull/22836. Refs: https://github.com/nodejs/node/pull/22836 Refs: https://github.com/nodejs/node/pull/22390 Fixes: https://github.com/nodejs/node/issues/22723 --- 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, 2 insertions(+), 171 deletions(-) delete mode 100644 test/parallel/test-vm-proxy.js diff --git a/doc/api/vm.md b/doc/api/vm.md index 94aace19fb3891..80a4760b6dd947 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -944,48 +944,6 @@ 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 92e2ed9429d59a..8b9fef548061c6 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -143,21 +143,19 @@ Local ContextifyContext::CreateV8Context( NamedPropertyHandlerConfiguration config(PropertyGetterCallback, PropertySetterCallback, - PropertyQueryCallback, + PropertyDescriptorCallback, PropertyDeleterCallback, PropertyEnumeratorCallback, PropertyDefinerCallback, - PropertyDescriptorCallback, CreateDataWrapper(env)); IndexedPropertyHandlerConfiguration indexed_config( IndexedPropertyGetterCallback, IndexedPropertySetterCallback, - IndexedPropertyQueryCallback, + IndexedPropertyDescriptorCallback, IndexedPropertyDeleterCallback, PropertyEnumeratorCallback, IndexedPropertyDefinerCallback, - IndexedPropertyDescriptorCallback, CreateDataWrapper(env)); object_template->SetHandler(config); @@ -393,28 +391,6 @@ 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, @@ -560,20 +536,6 @@ 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 965303a79bbdb1..d2b8387f214109 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -69,9 +69,6 @@ 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); @@ -91,9 +88,6 @@ 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 deleted file mode 100644 index e83ba0dee95aed..00000000000000 --- a/test/parallel/test-vm-proxy.js +++ /dev/null @@ -1,83 +0,0 @@ -'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 - }); -}