diff --git a/src/node_contextify.cc b/src/node_contextify.cc index c73db420f18b4e..5239c411500431 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -38,6 +38,7 @@ using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; +using v8::IndexedPropertyHandlerConfiguration; using v8::Integer; using v8::Just; using v8::Local; @@ -58,6 +59,7 @@ using v8::ScriptOrigin; using v8::String; using v8::Symbol; using v8::TryCatch; +using v8::Uint32; using v8::Uint8Array; using v8::UnboundScript; using v8::Value; @@ -65,6 +67,11 @@ using v8::WeakCallbackInfo; namespace { +Local Uint32ToName(Local context, uint32_t index) { + return Uint32::New(context->GetIsolate(), index)->ToString(context) + .ToLocalChecked(); +} + class ContextifyContext { protected: // V8 reserves the first field in context objects for the debugger. We use the @@ -111,93 +118,6 @@ class ContextifyContext { return Local::Cast(context()->GetEmbedderData(kSandboxObjectIndex)); } - // XXX(isaacs): This function only exists because of a shortcoming of - // the V8 SetNamedPropertyHandler function. - // - // It does not provide a way to intercept Object.defineProperty(..) - // calls. As a result, these properties are not copied onto the - // contextified sandbox when a new global property is added via either - // a function declaration or a Object.defineProperty(global, ...) call. - // - // Note that any function declarations or Object.defineProperty() - // globals that are created asynchronously (in a setTimeout, callback, - // etc.) will happen AFTER the call to copy properties, and thus not be - // caught. - // - // The way to properly fix this is to add some sort of a - // Object::SetNamedDefinePropertyHandler() function that takes a callback, - // which receives the property name and property descriptor as arguments. - // - // Luckily, such situations are rare, and asynchronously-added globals - // weren't supported by Node's VM module until 0.12 anyway. But, this - // should be fixed properly in V8, and this copy function should be - // removed once there is a better way. - void CopyProperties() { - HandleScope scope(env()->isolate()); - - Local context = PersistentToLocal(env()->isolate(), context_); - Local global = - context->Global()->GetPrototype()->ToObject(env()->isolate()); - Local sandbox_obj = sandbox(); - - Local clone_property_method; - - Local names = global->GetOwnPropertyNames(); - int length = names->Length(); - for (int i = 0; i < length; i++) { - Local key = names->Get(i)->ToString(env()->isolate()); - Maybe has = sandbox_obj->HasOwnProperty(context, key); - - // Check for pending exceptions - if (has.IsNothing()) - return; - - if (!has.FromJust()) { - Local desc_vm_context = - global->GetOwnPropertyDescriptor(context, key) - .ToLocalChecked().As(); - - bool is_accessor = - desc_vm_context->Has(context, env()->get_string()).FromJust() || - desc_vm_context->Has(context, env()->set_string()).FromJust(); - - auto define_property_on_sandbox = [&] (PropertyDescriptor* desc) { - desc->set_configurable(desc_vm_context - ->Get(context, env()->configurable_string()).ToLocalChecked() - ->BooleanValue(context).FromJust()); - desc->set_enumerable(desc_vm_context - ->Get(context, env()->enumerable_string()).ToLocalChecked() - ->BooleanValue(context).FromJust()); - CHECK(sandbox_obj->DefineProperty(context, key, *desc).FromJust()); - }; - - if (is_accessor) { - Local get = - desc_vm_context->Get(context, env()->get_string()) - .ToLocalChecked().As(); - Local set = - desc_vm_context->Get(context, env()->set_string()) - .ToLocalChecked().As(); - - PropertyDescriptor desc(get, set); - define_property_on_sandbox(&desc); - } else { - Local value = - desc_vm_context->Get(context, env()->value_string()) - .ToLocalChecked(); - - bool writable = - desc_vm_context->Get(context, env()->writable_string()) - .ToLocalChecked()->BooleanValue(context).FromJust(); - - PropertyDescriptor desc(value, writable); - define_property_on_sandbox(&desc); - } - } - } -} - - // This is an object that just keeps an internal pointer to this // ContextifyContext. It's passed to the NamedPropertyHandler. If we // pass the main JavaScript context object we're embedded in, then the @@ -227,13 +147,25 @@ class ContextifyContext { Local object_template = function_template->InstanceTemplate(); - NamedPropertyHandlerConfiguration config(GlobalPropertyGetterCallback, - GlobalPropertySetterCallback, - GlobalPropertyQueryCallback, - GlobalPropertyDeleterCallback, - GlobalPropertyEnumeratorCallback, + NamedPropertyHandlerConfiguration config(PropertyGetterCallback, + PropertySetterCallback, + PropertyDescriptorCallback, + PropertyDeleterCallback, + PropertyEnumeratorCallback, + PropertyDefinerCallback, CreateDataWrapper(env)); + + IndexedPropertyHandlerConfiguration indexed_config( + IndexedPropertyGetterCallback, + IndexedPropertySetterCallback, + IndexedPropertyDescriptorCallback, + IndexedPropertyDeleterCallback, + PropertyEnumeratorCallback, + IndexedPropertyDefinerCallback, + CreateDataWrapper(env)); + object_template->SetHandler(config); + object_template->SetHandler(indexed_config); Local ctx = NewContext(env->isolate(), object_template); @@ -373,7 +305,7 @@ class ContextifyContext { } - static void GlobalPropertyGetterCallback( + static void PropertyGetterCallback( Local property, const PropertyCallbackInfo& args) { ContextifyContext* ctx; @@ -402,7 +334,7 @@ class ContextifyContext { } - static void GlobalPropertySetterCallback( + static void PropertySetterCallback( Local property, Local value, const PropertyCallbackInfo& args) { @@ -414,9 +346,8 @@ class ContextifyContext { return; auto attributes = PropertyAttribute::None; - bool is_declared = - ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(), - property) + bool is_declared = ctx->global_proxy() + ->GetRealNamedPropertyAttributes(ctx->context(), property) .To(&attributes); bool read_only = static_cast(attributes) & @@ -441,16 +372,16 @@ class ContextifyContext { bool is_function = value->IsFunction(); if (!is_declared && args.ShouldThrowOnError() && is_contextual_store && - !is_function) + !is_function) return; ctx->sandbox()->Set(property, value); } - static void GlobalPropertyQueryCallback( + static void PropertyDescriptorCallback( Local property, - const PropertyCallbackInfo& args) { + const PropertyCallbackInfo& args) { ContextifyContext* ctx; ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); @@ -459,23 +390,80 @@ class ContextifyContext { return; Local context = ctx->context(); - Maybe maybe_prop_attr = - ctx->sandbox()->GetRealNamedPropertyAttributes(context, property); - if (maybe_prop_attr.IsNothing()) { - maybe_prop_attr = - ctx->global_proxy()->GetRealNamedPropertyAttributes(context, - property); - } + Local sandbox = ctx->sandbox(); - if (maybe_prop_attr.IsJust()) { - PropertyAttribute prop_attr = maybe_prop_attr.FromJust(); - args.GetReturnValue().Set(prop_attr); + if (sandbox->HasOwnProperty(context, property).FromMaybe(false)) { + args.GetReturnValue().Set( + sandbox->GetOwnPropertyDescriptor(context, property) + .ToLocalChecked()); } } - static void GlobalPropertyDeleterCallback( + static void PropertyDefinerCallback( + Local property, + const PropertyDescriptor& desc, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + Local context = ctx->context(); + v8::Isolate* isolate = context->GetIsolate(); + + auto attributes = PropertyAttribute::None; + bool is_declared = + ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(), + property) + .To(&attributes); + bool read_only = + static_cast(attributes) & + static_cast(PropertyAttribute::ReadOnly); + + // If the property is set on the global as read_only, don't change it on + // the global or sandbox. + if (is_declared && read_only) + return; + + Local sandbox = ctx->sandbox(); + + auto define_prop_on_sandbox = + [&] (PropertyDescriptor* desc_for_sandbox) { + if (desc.has_enumerable()) { + desc_for_sandbox->set_enumerable(desc.enumerable()); + } + if (desc.has_configurable()) { + desc_for_sandbox->set_configurable(desc.configurable()); + } + // Set the property on the sandbox. + sandbox->DefineProperty(context, property, *desc_for_sandbox); + }; + + if (desc.has_get() || desc.has_set()) { + PropertyDescriptor desc_for_sandbox( + desc.has_get() ? desc.get() : v8::Undefined(isolate).As(), + desc.has_set() ? desc.set() : v8::Undefined(isolate).As()); + + define_prop_on_sandbox(&desc_for_sandbox); + } else { + Local value = + desc.has_value() ? desc.value() : v8::Undefined(isolate).As(); + + if (desc.has_writable()) { + PropertyDescriptor desc_for_sandbox(value, desc.writable()); + define_prop_on_sandbox(&desc_for_sandbox); + } else { + PropertyDescriptor desc_for_sandbox(value); + define_prop_on_sandbox(&desc_for_sandbox); + } + } + } + + static void PropertyDeleterCallback( Local property, const PropertyCallbackInfo& args) { ContextifyContext* ctx; @@ -496,7 +484,7 @@ class ContextifyContext { } - static void GlobalPropertyEnumeratorCallback( + static void PropertyEnumeratorCallback( const PropertyCallbackInfo& args) { ContextifyContext* ctx; ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); @@ -507,6 +495,83 @@ class ContextifyContext { args.GetReturnValue().Set(ctx->sandbox()->GetPropertyNames()); } + + static void IndexedPropertyGetterCallback( + uint32_t index, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + PropertyGetterCallback(Uint32ToName(ctx->context(), index), args); + } + + + static void IndexedPropertySetterCallback( + uint32_t index, + Local value, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + PropertySetterCallback(Uint32ToName(ctx->context(), index), value, args); + } + + + static void IndexedPropertyDescriptorCallback( + uint32_t index, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + PropertyDescriptorCallback(Uint32ToName(ctx->context(), index), args); + } + + + static void IndexedPropertyDefinerCallback( + uint32_t index, + const PropertyDescriptor& desc, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + PropertyDefinerCallback(Uint32ToName(ctx->context(), index), desc, args); + } + + static void IndexedPropertyDeleterCallback( + uint32_t index, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + Maybe success = ctx->sandbox()->Delete(ctx->context(), index); + + if (success.FromMaybe(false)) + return; + + // Delete failed on the sandbox, intercept and do not delete on + // the global object. + args.GetReturnValue().Set(false); + } }; class ContextifyScript : public BaseObject { @@ -702,14 +767,12 @@ class ContextifyScript : public BaseObject { TryCatch try_catch(env->isolate()); // Do the eval within the context Context::Scope context_scope(contextify_context->context()); - if (EvalMachine(contextify_context->env(), - timeout, - display_errors, - break_on_sigint, - args, - &try_catch)) { - contextify_context->CopyProperties(); - } + EvalMachine(contextify_context->env(), + timeout, + display_errors, + break_on_sigint, + args, + &try_catch); if (try_catch.HasCaught()) { try_catch.ReThrow(); diff --git a/test/known_issues/test-vm-attributes-property-not-on-sandbox.js b/test/known_issues/test-vm-attributes-property-not-on-sandbox.js deleted file mode 100644 index d9534c3d4393a9..00000000000000 --- a/test/known_issues/test-vm-attributes-property-not-on-sandbox.js +++ /dev/null @@ -1,25 +0,0 @@ -'use strict'; -require('../common'); -const assert = require('assert'); -const vm = require('vm'); - -// The QueryCallback explicitly calls GetRealNamedPropertyAttributes -// on the global proxy if the property is not found on the sandbox. -// -// foo is not defined on the sandbox until we call CopyProperties(). -// In the QueryCallback, we do not find the property on the sandbox -// and look up its PropertyAttributes on the global_proxy(). -// PropertyAttributes are always flattened to a value -// descriptor. -const sandbox = {}; -vm.createContext(sandbox); -const code = `Object.defineProperty( - this, - 'foo', - { get: function() {return 17} } - ); - var desc = Object.getOwnPropertyDescriptor(this, 'foo');`; - -vm.runInContext(code, sandbox); -// The descriptor is flattened. We wrongly have typeof desc.value = 'number'. -assert.strictEqual(typeof sandbox.desc.get, 'function'); diff --git a/test/parallel/test-vm-attributes-property-not-on-sandbox.js b/test/parallel/test-vm-attributes-property-not-on-sandbox.js new file mode 100644 index 00000000000000..313dd71e47d860 --- /dev/null +++ b/test/parallel/test-vm-attributes-property-not-on-sandbox.js @@ -0,0 +1,18 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +// Assert that accessor descriptors are not flattened on the sandbox. +// Issue: https://github.com/nodejs/node/issues/2734 +const sandbox = {}; +vm.createContext(sandbox); +const code = `Object.defineProperty( + this, + 'foo', + { get: function() {return 17} } + ); + var desc = Object.getOwnPropertyDescriptor(this, 'foo');`; + +vm.runInContext(code, sandbox); +assert.strictEqual(typeof sandbox.desc.get, 'function'); diff --git a/test/parallel/test-vm-context.js b/test/parallel/test-vm-context.js index 29e3a86fab57a3..3c976648579952 100644 --- a/test/parallel/test-vm-context.js +++ b/test/parallel/test-vm-context.js @@ -106,16 +106,3 @@ assert.throws(() => { // https://github.com/nodejs/node/issues/6158 ctx = new Proxy({}, {}); assert.strictEqual(typeof vm.runInNewContext('String', ctx), 'function'); - -// https://github.com/nodejs/node/issues/10223 -ctx = vm.createContext(); -vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx); -assert.strictEqual(ctx.x, 42); -assert.strictEqual(vm.runInContext('x', ctx), 42); - -vm.runInContext('x = 0', ctx); // Does not throw but x... -assert.strictEqual(vm.runInContext('x', ctx), 42); // ...should be unaltered. - -assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx), - /Cannot assign to read only property 'x'/); -assert.strictEqual(vm.runInContext('x', ctx), 42); diff --git a/test/known_issues/test-vm-data-property-writable.js b/test/parallel/test-vm-data-property-writable.js similarity index 51% rename from test/known_issues/test-vm-data-property-writable.js rename to test/parallel/test-vm-data-property-writable.js index f29052a73a7b6b..00937ae412edda 100644 --- a/test/known_issues/test-vm-data-property-writable.js +++ b/test/parallel/test-vm-data-property-writable.js @@ -7,11 +7,22 @@ const assert = require('assert'); const context = vm.createContext({}); -const code = ` +let code = ` Object.defineProperty(this, 'foo', {value: 5}); Object.getOwnPropertyDescriptor(this, 'foo'); `; -const desc = vm.runInContext(code, context); +let desc = vm.runInContext(code, context); assert.strictEqual(desc.writable, false); + +// Check that interceptors work for symbols. +code = ` + const bar = Symbol('bar'); + Object.defineProperty(this, bar, {value: 6}); + Object.getOwnPropertyDescriptor(this, bar); +`; + +desc = vm.runInContext(code, context); + +assert.strictEqual(desc.value, 6); diff --git a/test/known_issues/test-vm-getters.js b/test/parallel/test-vm-getters.js similarity index 100% rename from test/known_issues/test-vm-getters.js rename to test/parallel/test-vm-getters.js diff --git a/test/parallel/test-vm-global-define-property.js b/test/parallel/test-vm-global-define-property.js index 30709fccaab453..00bd21052884d8 100644 --- a/test/parallel/test-vm-global-define-property.js +++ b/test/parallel/test-vm-global-define-property.js @@ -44,4 +44,4 @@ assert(res); assert.strictEqual(typeof res, 'object'); assert.strictEqual(res, x); assert.strictEqual(o.f, res); -assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'g', 'f']); +assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'f', 'g']); diff --git a/test/known_issues/test-vm-global-non-writable-properties.js b/test/parallel/test-vm-global-non-writable-properties.js similarity index 89% rename from test/known_issues/test-vm-global-non-writable-properties.js rename to test/parallel/test-vm-global-non-writable-properties.js index 8d7dfce55e3d81..2a2f70ff7ed5aa 100644 --- a/test/known_issues/test-vm-global-non-writable-properties.js +++ b/test/parallel/test-vm-global-non-writable-properties.js @@ -7,7 +7,6 @@ const vm = require('vm'); const ctx = vm.createContext(); vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx); -assert.strictEqual(ctx.x, undefined); // Not copied out by cloneProperty(). assert.strictEqual(vm.runInContext('x', ctx), 42); vm.runInContext('x = 0', ctx); // Does not throw but x... assert.strictEqual(vm.runInContext('x', ctx), 42); // ...should be unaltered. diff --git a/test/known_issues/test-vm-proxy-failure-CP.js b/test/parallel/test-vm-proxy-failure-CP.js similarity index 85% rename from test/known_issues/test-vm-proxy-failure-CP.js rename to test/parallel/test-vm-proxy-failure-CP.js index cb636f2ae2bb80..343948ac4c6377 100644 --- a/test/known_issues/test-vm-proxy-failure-CP.js +++ b/test/parallel/test-vm-proxy-failure-CP.js @@ -1,13 +1,10 @@ 'use strict'; - -// Sandbox throws in CopyProperties() despite no code being run -// Issue: https://github.com/nodejs/node/issues/11902 - - require('../common'); const assert = require('assert'); const vm = require('vm'); +// Check that we do not accidentally query attributes. +// Issue: https://github.com/nodejs/node/issues/11902 const handler = { getOwnPropertyDescriptor: (target, prop) => { throw new Error('whoops'); @@ -16,5 +13,4 @@ const handler = { const sandbox = new Proxy({ foo: 'bar' }, handler); const context = vm.createContext(sandbox); - assert.doesNotThrow(() => vm.runInContext('', context)); diff --git a/test/parallel/test-vm-strict-assign.js b/test/parallel/test-vm-strict-assign.js new file mode 100644 index 00000000000000..a61db82c639a23 --- /dev/null +++ b/test/parallel/test-vm-strict-assign.js @@ -0,0 +1,20 @@ +'use strict'; +require('../common'); +const assert = require('assert'); + +const vm = require('vm'); + +// https://github.com/nodejs/node/issues/10223 +const ctx = vm.createContext(); + +// Define x with writable = false. +vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx); +assert.strictEqual(ctx.x, 42); +assert.strictEqual(vm.runInContext('x', ctx), 42); + +vm.runInContext('x = 0', ctx); // Does not throw but x... +assert.strictEqual(vm.runInContext('x', ctx), 42); // ...should be unaltered. + +assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx), + /Cannot assign to read only property 'x'/); +assert.strictEqual(vm.runInContext('x', ctx), 42); diff --git a/test/parallel/test-vm-strict-mode.js b/test/parallel/test-vm-strict-mode.js deleted file mode 100644 index f3820d8aead6af..00000000000000 --- a/test/parallel/test-vm-strict-mode.js +++ /dev/null @@ -1,27 +0,0 @@ -'use strict'; -require('../common'); -const assert = require('assert'); -const vm = require('vm'); -const ctx = vm.createContext(); - -// Test strict mode inside a vm script, i.e., using an undefined variable -// throws a ReferenceError. Also check that variables -// that are not successfully set in the vm, must not be set -// on the sandboxed context. - -vm.runInContext('w = 1;', ctx); -assert.strictEqual(1, ctx.w); - -assert.throws(function() { vm.runInContext('"use strict"; x = 1;', ctx); }, - /ReferenceError: x is not defined/); -assert.strictEqual(undefined, ctx.x); - -vm.runInContext('"use strict"; var y = 1;', ctx); -assert.strictEqual(1, ctx.y); - -vm.runInContext('"use strict"; this.z = 1;', ctx); -assert.strictEqual(1, ctx.z); - -// w has been defined -vm.runInContext('"use strict"; w = 2;', ctx); -assert.strictEqual(2, ctx.w);