diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 895f7b9d0961..f3c1cf3bad99 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -50,13 +50,10 @@ using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; using v8::IndexedPropertyHandlerConfiguration; -using v8::IndexFilter; using v8::Int32; -using v8::Integer; using v8::Intercepted; using v8::Isolate; using v8::Just; -using v8::KeyCollectionMode; using v8::Local; using v8::Maybe; using v8::MaybeLocal; @@ -75,7 +72,6 @@ using v8::Promise; using v8::PropertyAttribute; using v8::PropertyCallbackInfo; using v8::PropertyDescriptor; -using v8::PropertyFilter; using v8::PropertyHandlerFlags; using v8::Script; using v8::ScriptCompiler; @@ -179,22 +175,20 @@ void ContextifyContext::InitializeGlobalTemplates(IsolateData* isolate_data) { NamedPropertyHandlerConfiguration config( PropertyGetterCallback, PropertySetterCallback, - PropertyQueryCallback, + PropertyDescriptorCallback, PropertyDeleterCallback, PropertyEnumeratorCallback, PropertyDefinerCallback, - PropertyDescriptorCallback, {}, PropertyHandlerFlags::kHasNoSideEffect); IndexedPropertyHandlerConfiguration indexed_config( IndexedPropertyGetterCallback, IndexedPropertySetterCallback, - IndexedPropertyQueryCallback, + IndexedPropertyDescriptorCallback, IndexedPropertyDeleterCallback, - IndexedPropertyEnumeratorCallback, + PropertyEnumeratorCallback, IndexedPropertyDefinerCallback, - IndexedPropertyDescriptorCallback, {}, PropertyHandlerFlags::kHasNoSideEffect); @@ -359,20 +353,17 @@ void ContextifyContext::RegisterExternalReferences( ExternalReferenceRegistry* registry) { registry->Register(MakeContext); registry->Register(CompileFunction); - registry->Register(PropertyQueryCallback); registry->Register(PropertyGetterCallback); registry->Register(PropertySetterCallback); registry->Register(PropertyDescriptorCallback); registry->Register(PropertyDeleterCallback); registry->Register(PropertyEnumeratorCallback); registry->Register(PropertyDefinerCallback); - registry->Register(IndexedPropertyQueryCallback); registry->Register(IndexedPropertyGetterCallback); registry->Register(IndexedPropertySetterCallback); registry->Register(IndexedPropertyDescriptorCallback); registry->Register(IndexedPropertyDeleterCallback); registry->Register(IndexedPropertyDefinerCallback); - registry->Register(IndexedPropertyEnumeratorCallback); } // makeContext(sandbox, name, origin, strings, wasm); @@ -460,51 +451,6 @@ bool ContextifyContext::IsStillInitializing(const ContextifyContext* ctx) { return ctx == nullptr || ctx->context_.IsEmpty(); } -// static -Intercepted ContextifyContext::PropertyQueryCallback( - Local property, const PropertyCallbackInfo& args) { - ContextifyContext* ctx = ContextifyContext::Get(args); - - // Still initializing - if (IsStillInitializing(ctx)) { - return Intercepted::kNo; - } - - Local context = ctx->context(); - Local sandbox = ctx->sandbox(); - - PropertyAttribute attr; - - Maybe maybe_has = sandbox->HasRealNamedProperty(context, property); - if (maybe_has.IsNothing()) { - return Intercepted::kNo; - } else if (maybe_has.FromJust()) { - Maybe maybe_attr = - sandbox->GetRealNamedPropertyAttributes(context, property); - if (!maybe_attr.To(&attr)) { - return Intercepted::kNo; - } - args.GetReturnValue().Set(attr); - return Intercepted::kYes; - } else { - maybe_has = ctx->global_proxy()->HasRealNamedProperty(context, property); - if (maybe_has.IsNothing()) { - return Intercepted::kNo; - } else if (maybe_has.FromJust()) { - Maybe maybe_attr = - ctx->global_proxy()->GetRealNamedPropertyAttributes(context, - property); - if (!maybe_attr.To(&attr)) { - return Intercepted::kNo; - } - args.GetReturnValue().Set(attr); - return Intercepted::kYes; - } - } - - return Intercepted::kNo; -} - // static Intercepted ContextifyContext::PropertyGetterCallback( Local property, const PropertyCallbackInfo& args) { @@ -748,68 +694,11 @@ void ContextifyContext::PropertyEnumeratorCallback( if (IsStillInitializing(ctx)) return; Local properties; - // Only get named properties, exclude symbols and indices. - if (!ctx->sandbox() - ->GetPropertyNames( - ctx->context(), - KeyCollectionMode::kIncludePrototypes, - static_cast(PropertyFilter::ONLY_ENUMERABLE | - PropertyFilter::SKIP_SYMBOLS), - IndexFilter::kSkipIndices) - .ToLocal(&properties)) - return; - args.GetReturnValue().Set(properties); -} - -// static -void ContextifyContext::IndexedPropertyEnumeratorCallback( - const PropertyCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); - HandleScope scope(isolate); - ContextifyContext* ctx = ContextifyContext::Get(args); - Local context = ctx->context(); - - // Still initializing - if (IsStillInitializing(ctx)) return; - - Local properties; - - // By default, GetPropertyNames returns string and number property names, and - // doesn't convert the numbers to strings. - if (!ctx->sandbox()->GetPropertyNames(context).ToLocal(&properties)) return; - - std::vector> properties_vec; - if (FromV8Array(context, properties, &properties_vec).IsNothing()) { + if (!ctx->sandbox()->GetPropertyNames(ctx->context()).ToLocal(&properties)) return; - } - // Filter out non-number property names. - std::vector> indices; - for (uint32_t i = 0; i < properties->Length(); i++) { - Local prop = properties_vec[i].Get(isolate); - if (!prop->IsNumber()) { - continue; - } - indices.push_back(prop); - } - - args.GetReturnValue().Set( - Array::New(args.GetIsolate(), indices.data(), indices.size())); -} - -// static -Intercepted ContextifyContext::IndexedPropertyQueryCallback( - uint32_t index, const PropertyCallbackInfo& args) { - ContextifyContext* ctx = ContextifyContext::Get(args); - - // Still initializing - if (IsStillInitializing(ctx)) { - return Intercepted::kNo; - } - - return ContextifyContext::PropertyQueryCallback( - Uint32ToName(ctx->context(), index), args); + args.GetReturnValue().Set(properties); } // static diff --git a/src/node_contextify.h b/src/node_contextify.h index b9e846f70bad..023cdc5d3d8c 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -94,9 +94,6 @@ class ContextifyContext : public BaseObject { bool produce_cached_data, v8::Local id_symbol, const errors::TryCatchScope& try_catch); - static v8::Intercepted PropertyQueryCallback( - v8::Local property, - const v8::PropertyCallbackInfo& args); static v8::Intercepted PropertyGetterCallback( v8::Local property, const v8::PropertyCallbackInfo& args); @@ -116,8 +113,6 @@ class ContextifyContext : public BaseObject { const v8::PropertyCallbackInfo& args); static void PropertyEnumeratorCallback( const v8::PropertyCallbackInfo& args); - static v8::Intercepted IndexedPropertyQueryCallback( - uint32_t index, const v8::PropertyCallbackInfo& args); static v8::Intercepted IndexedPropertyGetterCallback( uint32_t index, const v8::PropertyCallbackInfo& args); static v8::Intercepted IndexedPropertySetterCallback( @@ -132,8 +127,6 @@ class ContextifyContext : public BaseObject { const v8::PropertyCallbackInfo& args); static v8::Intercepted IndexedPropertyDeleterCallback( uint32_t index, const v8::PropertyCallbackInfo& args); - static void IndexedPropertyEnumeratorCallback( - const v8::PropertyCallbackInfo& args); v8::Global context_; std::unique_ptr microtask_queue_; diff --git a/test/parallel/test-vm-global-property-enumerator.js b/test/parallel/test-vm-global-property-enumerator.js index 7b37c2af4109..be6b8352c4a6 100644 --- a/test/parallel/test-vm-global-property-enumerator.js +++ b/test/parallel/test-vm-global-property-enumerator.js @@ -7,6 +7,9 @@ const assert = require('assert'); const cases = [ { + get 1() { + return 'value'; + }, get key() { return 'value'; }, @@ -15,15 +18,21 @@ const cases = [ // Intentionally single setter. // eslint-disable-next-line accessor-pairs set key(value) {}, + // eslint-disable-next-line accessor-pairs + set 1(value) {}, }, {}, { key: 'value', + 1: 'value', }, (new class GetterObject { get key() { return 'value'; } + get 1() { + return 'value'; + } }()), (new class SetterObject { // Intentionally single setter. @@ -31,14 +40,13 @@ const cases = [ set key(value) { // noop } + // eslint-disable-next-line accessor-pairs + set 1(value) { + // noop + } }()), [], [['key', 'value']], - { - __proto__: { - key: 'value', - }, - }, ]; for (const [idx, obj] of cases.entries()) { @@ -47,3 +55,23 @@ for (const [idx, obj] of cases.entries()) { const keys = Object.keys(globalObj); assert.deepStrictEqual(keys, Object.keys(obj), `Case ${idx} failed`); } + +const specialCases = [ + [ + // Prototype named and index properties should not be enumerated. + // FIXME(legendecas): https://github.com/nodejs/node/issues/54436 + { + __proto__: { + key: 'value', + 1: 'value', + }, + }, + ['1', 'key'], + ], +]; +for (const [idx, [obj, expectedKeys]] of specialCases.entries()) { + const ctx = vm.createContext(obj); + const globalObj = vm.runInContext('this', ctx); + const keys = Object.keys(globalObj); + assert.deepStrictEqual(keys, expectedKeys, `Special case ${idx} failed`); +} diff --git a/test/parallel/test-vm-global-property-prototype.js b/test/parallel/test-vm-global-property-prototype.js index fe8abc8be45e..4ffef470c464 100644 --- a/test/parallel/test-vm-global-property-prototype.js +++ b/test/parallel/test-vm-global-property-prototype.js @@ -3,48 +3,107 @@ require('../common'); const assert = require('assert'); const vm = require('vm'); +const outerProto = { + onOuterProto: 'onOuterProto', + bothProto: 'onOuterProto', +}; +function onOuterProtoGetter() { + return 'onOuterProtoGetter'; +} +Object.defineProperties(outerProto, { + onOuterProtoGetter: { + get: onOuterProtoGetter, + }, + bothProtoGetter: { + get: onOuterProtoGetter, + }, + 0: { + value: 'onOuterProtoIndexed', + writable: false, + enumerable: false, + configurable: true, + }, + 3: { + value: 'onOuterProtoIndexed', + writable: false, + enumerable: false, + configurable: true, + }, +}); + +// Creating a new intermediate proto to mimic the +// window -> Window.prototype -> EventTarget.prototype chain in JSDom. +const sandboxProto = { + __proto__: outerProto, +}; + const sandbox = { + __proto__: sandboxProto, onSelf: 'onSelf', }; function onSelfGetter() { return 'onSelfGetter'; } - -Object.defineProperty(sandbox, 'onSelfGetter', { - get: onSelfGetter, -}); - -Object.defineProperty(sandbox, 1, { - value: 'onSelfIndexed', - writable: false, - enumerable: false, - configurable: true, +Object.defineProperties(sandbox, { + onSelfGetter: { + get: onSelfGetter, + }, + 1: { + value: 'onSelfIndexed', + writable: false, + enumerable: false, + configurable: true, + } }); const ctx = vm.createContext(sandbox); const result = vm.runInContext(` -Object.prototype.onProto = 'onProto'; -Object.defineProperty(Object.prototype, 'onProtoGetter', { - get() { - return 'onProtoGetter'; +Object.prototype.onInnerProto = 'onInnerProto'; +Object.defineProperties(Object.prototype, { + onInnerProtoGetter: { + get() { + return 'onInnerProtoGetter'; + }, + }, + 2: { + value: 'onInnerProtoIndexed', + writable: false, + enumerable: false, + configurable: true, }, }); -Object.defineProperty(Object.prototype, 2, { - value: 'onProtoIndexed', - writable: false, - enumerable: false, - configurable: true, + +// Override outer proto properties +Object.prototype.bothProto = 'onInnerProto'; +Object.defineProperties(Object.prototype, { + bothProtoGetter: { + get() { + return 'onInnerProtoGetter'; + }, + }, + 3: { + value: 'onInnerProtoIndexed', + writable: false, + enumerable: false, + configurable: true, + }, }); const resultHasOwn = { onSelf: Object.hasOwn(this, 'onSelf'), onSelfGetter: Object.hasOwn(this, 'onSelfGetter'), onSelfIndexed: Object.hasOwn(this, 1), - onProto: Object.hasOwn(this, 'onProto'), - onProtoGetter: Object.hasOwn(this, 'onProtoGetter'), - onProtoIndexed: Object.hasOwn(this, 2), + onOuterProto: Object.hasOwn(this, 'onOuterProto'), + onOuterProtoGetter: Object.hasOwn(this, 'onOuterProtoGetter'), + onOuterProtoIndexed: Object.hasOwn(this, 0), + onInnerProto: Object.hasOwn(this, 'onInnerProto'), + onInnerProtoGetter: Object.hasOwn(this, 'onInnerProtoGetter'), + onInnerProtoIndexed: Object.hasOwn(this, 2), + bothProto: Object.hasOwn(this, 'bothProto'), + bothProtoGetter: Object.hasOwn(this, 'bothProtoGetter'), + bothProtoIndexed: Object.hasOwn(this, 3), }; const getDesc = (prop) => Object.getOwnPropertyDescriptor(this, prop); @@ -52,13 +111,49 @@ const resultDesc = { onSelf: getDesc('onSelf'), onSelfGetter: getDesc('onSelfGetter'), onSelfIndexed: getDesc(1), - onProto: getDesc('onProto'), - onProtoGetter: getDesc('onProtoGetter'), - onProtoIndexed: getDesc(2), + onOuterProto: getDesc('onOuterProto'), + onOuterProtoGetter: getDesc('onOuterProtoGetter'), + onOuterProtoIndexed: getDesc(0), + onInnerProto: getDesc('onInnerProto'), + onInnerProtoGetter: getDesc('onInnerProtoGetter'), + onInnerProtoIndexed: getDesc(2), + bothProto: getDesc('bothProto'), + bothProtoGetter: getDesc('bothProtoGetter'), + bothProtoIndexed: getDesc(3), +}; +const resultIn = { + onSelf: 'onSelf' in this, + onSelfGetter: 'onSelfGetter' in this, + onSelfIndexed: 1 in this, + onOuterProto: 'onOuterProto' in this, + onOuterProtoGetter: 'onOuterProtoGetter' in this, + onOuterProtoIndexed: 0 in this, + onInnerProto: 'onInnerProto' in this, + onInnerProtoGetter: 'onInnerProtoGetter' in this, + onInnerProtoIndexed: 2 in this, + bothProto: 'bothProto' in this, + bothProtoGetter: 'bothProtoGetter' in this, + bothProtoIndexed: 3 in this, +}; +const resultValue = { + onSelf: this.onSelf, + onSelfGetter: this.onSelfGetter, + onSelfIndexed: this[1], + onOuterProto: this.onOuterProto, + onOuterProtoGetter: this.onOuterProtoGetter, + onOuterProtoIndexed: this[0], + onInnerProto: this.onInnerProto, + onInnerProtoGetter: this.onInnerProtoGetter, + onInnerProtoIndexed: this[2], + bothProto: this.bothProto, + bothProtoGetter: this.bothProtoGetter, + bothProtoIndexed: this[3], }; ({ resultHasOwn, resultDesc, + resultIn, + resultValue, }); `, ctx); @@ -68,16 +163,62 @@ assert.deepEqual(result, { onSelf: true, onSelfGetter: true, onSelfIndexed: true, - onProto: false, - onProtoGetter: false, - onProtoIndexed: false, + + // The following results should be false in terms of "normal" JavaScript + // objects. However, `in` operator only looks up properties on the inner + // prototype chain, the interceptor has to lookup the outer prototype chain + // to maintain compatibility. + // FIXME(legendecas): https://github.com/nodejs/node/issues/54436 + onOuterProto: true, + onOuterProtoGetter: true, + onOuterProtoIndexed: true, + onInnerProto: true, + onInnerProtoGetter: true, + onInnerProtoIndexed: true, + bothProto: true, + bothProtoGetter: true, + bothProtoIndexed: true, }, resultDesc: { onSelf: { value: 'onSelf', writable: true, enumerable: true, configurable: true }, onSelfGetter: { get: onSelfGetter, set: undefined, enumerable: false, configurable: false }, onSelfIndexed: { value: 'onSelfIndexed', writable: false, enumerable: false, configurable: true }, - onProto: undefined, - onProtoGetter: undefined, - onProtoIndexed: undefined, + onOuterProto: undefined, + onOuterProtoGetter: undefined, + onOuterProtoIndexed: undefined, + onInnerProto: undefined, + onInnerProtoGetter: undefined, + onInnerProtoIndexed: undefined, + bothProto: undefined, + bothProtoGetter: undefined, + bothProtoIndexed: undefined, + }, + resultIn: { + onSelf: true, + onSelfGetter: true, + onSelfIndexed: true, + onOuterProto: true, + onOuterProtoGetter: true, + onOuterProtoIndexed: true, + onInnerProto: true, + onInnerProtoGetter: true, + onInnerProtoIndexed: true, + bothProto: true, + bothProtoGetter: true, + bothProtoIndexed: true, }, + resultValue: { + onSelf: 'onSelf', + onSelfGetter: 'onSelfGetter', + onSelfIndexed: 'onSelfIndexed', + onOuterProto: 'onOuterProto', + onOuterProtoGetter: 'onOuterProtoGetter', + onOuterProtoIndexed: 'onOuterProtoIndexed', + onInnerProto: 'onInnerProto', + onInnerProtoGetter: 'onInnerProtoGetter', + onInnerProtoIndexed: 'onInnerProtoIndexed', + bothProto: 'onOuterProto', + bothProtoGetter: 'onOuterProtoGetter', + bothProtoIndexed: 'onOuterProtoIndexed', + } });