diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 06722624ad6d90..cc3109ad2111ce 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -50,10 +50,13 @@ 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; @@ -72,6 +75,7 @@ using v8::Promise; using v8::PropertyAttribute; using v8::PropertyCallbackInfo; using v8::PropertyDescriptor; +using v8::PropertyFilter; using v8::PropertyHandlerFlags; using v8::Script; using v8::ScriptCompiler; @@ -175,20 +179,22 @@ void ContextifyContext::InitializeGlobalTemplates(IsolateData* isolate_data) { NamedPropertyHandlerConfiguration config( PropertyGetterCallback, PropertySetterCallback, - PropertyDescriptorCallback, + PropertyQueryCallback, PropertyDeleterCallback, PropertyEnumeratorCallback, PropertyDefinerCallback, + PropertyDescriptorCallback, {}, PropertyHandlerFlags::kHasNoSideEffect); IndexedPropertyHandlerConfiguration indexed_config( IndexedPropertyGetterCallback, IndexedPropertySetterCallback, - IndexedPropertyDescriptorCallback, + IndexedPropertyQueryCallback, IndexedPropertyDeleterCallback, - PropertyEnumeratorCallback, + IndexedPropertyEnumeratorCallback, IndexedPropertyDefinerCallback, + IndexedPropertyDescriptorCallback, {}, PropertyHandlerFlags::kHasNoSideEffect); @@ -353,17 +359,20 @@ 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); @@ -451,6 +460,51 @@ 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) { @@ -695,13 +749,70 @@ void ContextifyContext::PropertyEnumeratorCallback( if (IsStillInitializing(ctx)) return; Local properties; - - if (!ctx->sandbox()->GetPropertyNames(ctx->context()).ToLocal(&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()) { + 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); +} + // static Intercepted ContextifyContext::IndexedPropertyGetterCallback( uint32_t index, const PropertyCallbackInfo& args) { diff --git a/src/node_contextify.h b/src/node_contextify.h index 023cdc5d3d8c00..b9e846f70bad4f 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -94,6 +94,9 @@ 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); @@ -113,6 +116,8 @@ 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( @@ -127,6 +132,8 @@ 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 new file mode 100644 index 00000000000000..7b37c2af41094d --- /dev/null +++ b/test/parallel/test-vm-global-property-enumerator.js @@ -0,0 +1,49 @@ +'use strict'; +require('../common'); +const vm = require('vm'); +const assert = require('assert'); + +// Regression of https://github.com/nodejs/node/issues/53346 + +const cases = [ + { + get key() { + return 'value'; + }, + }, + { + // Intentionally single setter. + // eslint-disable-next-line accessor-pairs + set key(value) {}, + }, + {}, + { + key: 'value', + }, + (new class GetterObject { + get key() { + return 'value'; + } + }()), + (new class SetterObject { + // Intentionally single setter. + // eslint-disable-next-line accessor-pairs + set key(value) { + // noop + } + }()), + [], + [['key', 'value']], + { + __proto__: { + key: 'value', + }, + }, +]; + +for (const [idx, obj] of cases.entries()) { + const ctx = vm.createContext(obj); + const globalObj = vm.runInContext('this', ctx); + const keys = Object.keys(globalObj); + assert.deepStrictEqual(keys, Object.keys(obj), `Case ${idx} failed`); +} diff --git a/test/parallel/test-vm-global-property-prototype.js b/test/parallel/test-vm-global-property-prototype.js new file mode 100644 index 00000000000000..fe8abc8be45ee2 --- /dev/null +++ b/test/parallel/test-vm-global-property-prototype.js @@ -0,0 +1,83 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +const sandbox = { + onSelf: 'onSelf', +}; + +function onSelfGetter() { + return 'onSelfGetter'; +} + +Object.defineProperty(sandbox, 'onSelfGetter', { + get: onSelfGetter, +}); + +Object.defineProperty(sandbox, 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.defineProperty(Object.prototype, 2, { + value: 'onProtoIndexed', + 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), +}; + +const getDesc = (prop) => Object.getOwnPropertyDescriptor(this, prop); +const resultDesc = { + onSelf: getDesc('onSelf'), + onSelfGetter: getDesc('onSelfGetter'), + onSelfIndexed: getDesc(1), + onProto: getDesc('onProto'), + onProtoGetter: getDesc('onProtoGetter'), + onProtoIndexed: getDesc(2), +}; +({ + resultHasOwn, + resultDesc, +}); +`, ctx); + +// eslint-disable-next-line no-restricted-properties +assert.deepEqual(result, { + resultHasOwn: { + onSelf: true, + onSelfGetter: true, + onSelfIndexed: true, + onProto: false, + onProtoGetter: false, + onProtoIndexed: false, + }, + 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, + }, +});