diff --git a/src/node_contextify.cc b/src/node_contextify.cc index c2037c4cbe7354..9015b74d4e19ed 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -30,6 +30,8 @@ #include "util-inl.h" #include "v8-debug.h" +#include + namespace node { using v8::Array; @@ -44,6 +46,7 @@ using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; using v8::Integer; +using v8::Isolate; using v8::Just; using v8::Local; using v8::Maybe; @@ -234,9 +237,10 @@ class ContextifyContext { NamedPropertyHandlerConfiguration config(GlobalPropertyGetterCallback, GlobalPropertySetterCallback, - GlobalPropertyQueryCallback, + GlobalPropertyDescriptorCallback, GlobalPropertyDeleterCallback, GlobalPropertyEnumeratorCallback, + GlobalPropertyDefinerCallback, CreateDataWrapper(env)); object_template->SetHandler(config); @@ -413,6 +417,8 @@ class ContextifyContext { const PropertyCallbackInfo& args) { ContextifyContext* ctx; ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); + Environment* env = ctx->env(); + Isolate* isolate = env->isolate(); // Still initializing if (ctx->context_.IsEmpty()) @@ -420,15 +426,24 @@ class ContextifyContext { auto attributes = PropertyAttribute::None; bool is_declared = - ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(), - property) + ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(), + property) .To(&attributes); bool read_only = static_cast(attributes) & static_cast(PropertyAttribute::ReadOnly); - if (is_declared && read_only) + if (is_declared && read_only) { + if (args.ShouldThrowOnError()) { + std::string error_message("Cannot assign to read only property '"); + Local property_name = property->ToDetailString(); + String::Utf8Value utf8_name(property_name); + error_message += *utf8_name; + error_message += "' of object '#'"; + env->ThrowTypeError(error_message.c_str()); + } return; + } // true for x = 5 // false for this.x = 5 @@ -453,9 +468,9 @@ class ContextifyContext { } - static void GlobalPropertyQueryCallback( + static void GlobalPropertyDescriptorCallback( Local property, - const PropertyCallbackInfo& args) { + const PropertyCallbackInfo& args) { ContextifyContext* ctx; ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); @@ -464,18 +479,14 @@ class ContextifyContext { return; Local context = ctx->context(); - Maybe maybe_prop_attr = - ctx->sandbox()->GetRealNamedPropertyAttributes(context, property); + MaybeLocal maybe_prop_desc = + ctx->sandbox()->GetOwnPropertyDescriptor(context, property); - if (maybe_prop_attr.IsNothing()) { - maybe_prop_attr = - ctx->global_proxy()->GetRealNamedPropertyAttributes(context, - property); - } - - if (maybe_prop_attr.IsJust()) { - PropertyAttribute prop_attr = maybe_prop_attr.FromJust(); - args.GetReturnValue().Set(prop_attr); + if (!maybe_prop_desc.IsEmpty()) { + Local prop_desc = maybe_prop_desc.ToLocalChecked(); + if (!prop_desc->IsUndefined()) { + args.GetReturnValue().Set(prop_desc); + } } } @@ -512,6 +523,72 @@ class ContextifyContext { args.GetReturnValue().Set(ctx->sandbox()->GetPropertyNames()); } + + + static void GlobalPropertyDefinerCallback( + Local property, + const PropertyDescriptor& desc, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); + Environment* env = ctx->env(); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + auto attributes = PropertyAttribute::None; + bool is_declared = + ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(), + property) + .To(&attributes); + bool non_enumerable = + static_cast(attributes) & + static_cast(PropertyAttribute::DontDelete); + + if (is_declared && non_enumerable) { + if (args.ShouldThrowOnError()) { + std::string error_message("Cannot redefine property: "); + Local property_name = property->ToDetailString(); + String::Utf8Value utf8_name(property_name); + error_message += *utf8_name; + env->ThrowTypeError(error_message.c_str()); + } + return; + } + + Local context = ctx->context(); + auto add_desc_copy_to_sandbox = + [&] (PropertyDescriptor* desc_copy) { + if (desc.has_enumerable()) { + desc_copy->set_enumerable(desc.enumerable()); + } + if (desc.has_configurable()) { + desc_copy->set_configurable(desc.configurable()); + } + Maybe result = + ctx->sandbox()->DefineProperty(context, property, *desc_copy); + if (result.IsJust()) { + args.GetReturnValue().Set(result.FromJust()); + } + }; + + Isolate* isolate = context->GetIsolate(); + if (desc.has_get() || desc.has_set()) { + Local get = + desc.has_get() ? desc.get() : Undefined(isolate).As(); + Local set = + desc.has_set() ? desc.set() : Undefined(isolate).As(); + PropertyDescriptor desc_copy(get, set); + add_desc_copy_to_sandbox(&desc_copy); + } else { + bool writable = desc.has_writable() ? desc.writable() : false; + Local value = + desc.has_value() ? desc.value() : Undefined(isolate).As(); + PropertyDescriptor desc_copy(value, writable); + add_desc_copy_to_sandbox(&desc_copy); + } + } }; class ContextifyScript : public BaseObject { 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..873d22e82a6140 --- /dev/null +++ b/test/parallel/test-vm-attributes-property-not-on-sandbox.js @@ -0,0 +1,16 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +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 7e5404796e7533..e425755e6d4ef0 100644 --- a/test/parallel/test-vm-context.js +++ b/test/parallel/test-vm-context.js @@ -115,6 +115,9 @@ 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'/); +const error = new RegExp( + 'TypeError: Cannot assign to read only property \'x\' of ' + + 'object \'#\'' +); +assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx), error); 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 100% rename from test/known_issues/test-vm-data-property-writable.js rename to test/parallel/test-vm-data-property-writable.js 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/parallel/test-vm-global-property-interceptors.js b/test/parallel/test-vm-global-property-interceptors.js new file mode 100644 index 00000000000000..dd3d4e5c792949 --- /dev/null +++ b/test/parallel/test-vm-global-property-interceptors.js @@ -0,0 +1,130 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +const dSymbol = Symbol('d'); +const sandbox = { + a: 'a', + dSymbol +}; + +Object.defineProperties(sandbox, { + b: { + value: 'b' + }, + c: { + value: 'c', + writable: true, + enumerable: true + }, + [dSymbol]: { + value: 'd' + }, + e: { + value: 'e', + configurable: true + }, + f: {} +}); + +const ctx = vm.createContext(sandbox); + +const result = vm.runInContext(` +const getDesc = (prop) => Object.getOwnPropertyDescriptor(this, prop); +const result = { + a: getDesc('a'), + b: getDesc('b'), + c: getDesc('c'), + d: getDesc(dSymbol), + e: getDesc('e'), + f: getDesc('f'), + g: getDesc('g') +}; +result; +`, ctx); + +//eslint-disable-next-line no-restricted-properties +assert.deepEqual(result, { + a: { value: 'a', writable: true, enumerable: true, configurable: true }, + b: { value: 'b', writable: false, enumerable: false, configurable: false }, + c: { value: 'c', writable: true, enumerable: true, configurable: false }, + d: { value: 'd', writable: false, enumerable: false, configurable: false }, + e: { value: 'e', writable: false, enumerable: false, configurable: true }, + f: { + value: undefined, + writable: false, + enumerable: false, + configurable: false + }, + g: undefined +}); + +// define new properties +vm.runInContext(` +Object.defineProperty(this, 'h', {value: 'h'}); +Object.defineProperty(this, 'i', {}); +Object.defineProperty(this, 'j', { + get() { return 'j'; } +}); + +let kValue = 0; +Object.defineProperty(this, 'k', { + get() { return kValue; }, + set(value) { kValue = value } +}); +`, ctx); + +assert.deepStrictEqual(Object.getOwnPropertyDescriptor(ctx, 'h'), { + value: 'h', + writable: false, + enumerable: false, + configurable: false +}); + +assert.deepStrictEqual(Object.getOwnPropertyDescriptor(ctx, 'i'), { + value: undefined, + writable: false, + enumerable: false, + configurable: false +}); + +const jDesc = Object.getOwnPropertyDescriptor(ctx, 'j'); +assert.strictEqual(typeof jDesc.get, 'function'); +assert.strictEqual(typeof jDesc.set, 'undefined'); +assert.strictEqual(jDesc.enumerable, false); +assert.strictEqual(jDesc.configurable, false); + +const kDesc = Object.getOwnPropertyDescriptor(ctx, 'k'); +assert.strictEqual(typeof kDesc.get, 'function'); +assert.strictEqual(typeof kDesc.set, 'function'); +assert.strictEqual(kDesc.enumerable, false); +assert.strictEqual(kDesc.configurable, false); + +assert.strictEqual(ctx.k, 0); +ctx.k = 1; +assert.strictEqual(ctx.k, 1); +assert.strictEqual(vm.runInContext('k;', ctx), 1); +vm.runInContext('k = 2;', ctx); +assert.strictEqual(ctx.k, 2); +assert.strictEqual(vm.runInContext('k;', ctx), 2); + +// redefine properties on the global object +assert.strictEqual(typeof vm.runInContext('encodeURI;', ctx), 'function'); +assert.strictEqual(ctx.encodeURI, undefined); +vm.runInContext(` +Object.defineProperty(this, 'encodeURI', { value: 42 }); +`, ctx); +assert.strictEqual(vm.runInContext('encodeURI;', ctx), 42); +assert.strictEqual(ctx.encodeURI, 42); + +// redefine properties on the sandbox +vm.runInContext(` +Object.defineProperty(this, 'e', { value: 'newE' }); +`, ctx); +assert.strictEqual(ctx.e, 'newE'); + +assert.throws(() => vm.runInContext(` +'use strict'; +Object.defineProperty(this, 'f', { value: 'newF' }); +`, ctx), /TypeError: Cannot redefine property: f/);