From 849788946fc9e33339f3e7461debd98b3050fc3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Mon, 4 Sep 2017 13:49:46 +0200 Subject: [PATCH 1/3] src: intercept property descriptors on sandbox Fixes: https://github.com/nodejs/node/issues/2734 Fixes: https://github.com/nodejs/node/issues/5350 Fixes: https://github.com/nodejs/node/issues/5679 --- src/node_contextify.cc | 111 ++++++++++++--- ...t-vm-attributes-property-not-on-sandbox.js | 25 ---- ...t-vm-attributes-property-not-on-sandbox.js | 16 +++ test/parallel/test-vm-context.js | 7 +- .../test-vm-data-property-writable.js | 0 .../test-vm-getters.js | 0 .../test-vm-global-define-property.js | 2 +- .../test-vm-global-property-interceptors.js | 130 ++++++++++++++++++ 8 files changed, 246 insertions(+), 45 deletions(-) delete mode 100644 test/known_issues/test-vm-attributes-property-not-on-sandbox.js create mode 100644 test/parallel/test-vm-attributes-property-not-on-sandbox.js rename test/{known_issues => parallel}/test-vm-data-property-writable.js (100%) rename test/{known_issues => parallel}/test-vm-getters.js (100%) create mode 100644 test/parallel/test-vm-global-property-interceptors.js 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/); From bd90db97a692e274ec5aa8390363e524774b236c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Mon, 4 Sep 2017 13:50:08 +0200 Subject: [PATCH 2/3] test: add known issue vm test for Reflect.ownKeys --- test/known_issues/test-vm-ownkeys.js | 39 ++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 test/known_issues/test-vm-ownkeys.js diff --git a/test/known_issues/test-vm-ownkeys.js b/test/known_issues/test-vm-ownkeys.js new file mode 100644 index 00000000000000..e8da389d679473 --- /dev/null +++ b/test/known_issues/test-vm-ownkeys.js @@ -0,0 +1,39 @@ +'use strict'; + +require('../common'); +const vm = require('vm'); +const assert = require('assert'); + +const sym1 = Symbol('1'); +const sym2 = Symbol('2'); +const sandbox = { + a: true, + [sym1]: true +}; +Object.defineProperty(sandbox, 'b', { value: true }); +Object.defineProperty(sandbox, sym2, { value: true }); + +const ctx = vm.createContext(sandbox); + +// sanity check +assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]); +assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']); +assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]); + +const nativeKeys = vm.runInNewContext('Reflect.ownKeys(this);'); +const ownKeys = vm.runInContext('Reflect.ownKeys(this);', ctx); +const restKeys = ownKeys.filter((key) => !nativeKeys.includes(key)); +//eslint-disable-next-line no-restricted-properties +assert.deepEqual(restKeys, ['a', 'b', sym1, sym2]); + +const nativeNames = vm.runInNewContext('Object.getOwnPropertyNames(this);'); +const ownNames = vm.runInContext('Object.getOwnPropertyNames(this);', ctx); +const restNames = ownNames.filter((name) => !nativeNames.includes(name)); +//eslint-disable-next-line no-restricted-properties +assert.deepEqual(restNames, ['a', 'b']); + +const nativeSym = vm.runInNewContext('Object.getOwnPropertySymbols(this);'); +const ownSym = vm.runInContext('Object.getOwnPropertySymbols(this);', ctx); +const restSym = ownSym.filter((sym) => !nativeSym.includes(sym)); +//eslint-disable-next-line no-restricted-properties +assert.deepEqual(restSym, [sym1, sym2]); From 5f2c4faf3e23fb4a5a65609fbfac1dadca70e948 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Tue, 5 Sep 2017 10:15:00 +0200 Subject: [PATCH 3/3] try ToDetailString --- src/node_contextify.cc | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 9015b74d4e19ed..df45ce82763663 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -30,6 +30,7 @@ #include "util-inl.h" #include "v8-debug.h" +#include #include namespace node { @@ -73,6 +74,11 @@ using v8::WeakCallbackInfo; namespace { +std::ostream& operator<<(std::ostream& os, Local value) { + String::Utf8Value utf8(value); + return os << *utf8; +} + class ContextifyContext { protected: // V8 reserves the first field in context objects for the debugger. We use the @@ -424,6 +430,8 @@ class ContextifyContext { if (ctx->context_.IsEmpty()) return; + Local context = ctx->context(); + auto attributes = PropertyAttribute::None; bool is_declared = ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(), @@ -435,12 +443,19 @@ class ContextifyContext { 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()); + std::ostringstream error_message; + error_message << "Cannot assign to read only property '"; + Local property_name = + property->ToDetailString(context).ToLocalChecked(); + error_message << property_name; + error_message << "' of "; + error_message << ctx->sandbox()->TypeOf(isolate); + error_message << " '"; + Local sandbox_name = + ctx->sandbox()->ToDetailString(context).ToLocalChecked(); + error_message << sandbox_name; + error_message << "'"; + env->ThrowTypeError(error_message.str().c_str()); } return; } @@ -537,6 +552,8 @@ class ContextifyContext { if (ctx->context_.IsEmpty()) return; + Local context = ctx->context(); + auto attributes = PropertyAttribute::None; bool is_declared = ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(), @@ -549,7 +566,8 @@ class ContextifyContext { if (is_declared && non_enumerable) { if (args.ShouldThrowOnError()) { std::string error_message("Cannot redefine property: "); - Local property_name = property->ToDetailString(); + Local property_name = + property->ToDetailString(context).ToLocalChecked(); String::Utf8Value utf8_name(property_name); error_message += *utf8_name; env->ThrowTypeError(error_message.c_str()); @@ -557,7 +575,6 @@ class ContextifyContext { return; } - Local context = ctx->context(); auto add_desc_copy_to_sandbox = [&] (PropertyDescriptor* desc_copy) { if (desc.has_enumerable()) {