diff --git a/src/node_contextify.cc b/src/node_contextify.cc index bc90501da0ffde..fc137486f5e6ba 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -769,19 +769,25 @@ Intercepted ContextifyContext::PropertyDeleterCallback( // static void ContextifyContext::PropertyEnumeratorCallback( const PropertyCallbackInfo& args) { + // Named enumerator will be invoked on Object.keys, + // Object.getOwnPropertyNames, Object.getOwnPropertySymbols, + // Object.getOwnPropertyDescriptors, for...in, etc. operations. + // Named enumerator should return all own non-indices property names, + // including string properties and symbol properties. V8 will filter the + // result array to match the expected symbol-only, enumerable-only with + // NamedPropertyQueryCallback. ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing if (IsStillInitializing(ctx)) return; Local properties; - // Only get named properties, exclude symbols and indices. + // Only get own named properties, exclude indices. if (!ctx->sandbox() ->GetPropertyNames( ctx->context(), - KeyCollectionMode::kIncludePrototypes, - static_cast(PropertyFilter::ONLY_ENUMERABLE | - PropertyFilter::SKIP_SYMBOLS), + KeyCollectionMode::kOwnOnly, + static_cast(PropertyFilter::ALL_PROPERTIES), IndexFilter::kSkipIndices) .ToLocal(&properties)) return; @@ -792,6 +798,12 @@ void ContextifyContext::PropertyEnumeratorCallback( // static void ContextifyContext::IndexedPropertyEnumeratorCallback( const PropertyCallbackInfo& args) { + // Indexed enumerator will be invoked on Object.keys, + // Object.getOwnPropertyNames, Object.getOwnPropertyDescriptors, for...in, + // etc. operations. Indexed enumerator should return all own non-indices index + // properties. V8 will filter the result array to match the expected + // enumerable-only with IndexedPropertyQueryCallback. + Isolate* isolate = args.GetIsolate(); HandleScope scope(isolate); ContextifyContext* ctx = ContextifyContext::Get(args); @@ -802,9 +814,15 @@ void ContextifyContext::IndexedPropertyEnumeratorCallback( 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; + // Only get own index properties. + if (!ctx->sandbox() + ->GetPropertyNames( + context, + KeyCollectionMode::kOwnOnly, + static_cast(PropertyFilter::SKIP_SYMBOLS), + IndexFilter::kIncludeIndices) + .ToLocal(&properties)) + return; std::vector> properties_vec; if (FromV8Array(context, properties, &properties_vec).IsNothing()) { diff --git a/test/parallel/test-vm-global-property-enumerator.js b/test/parallel/test-vm-global-property-enumerator.js index 7b37c2af41094d..7443c2c27d6c89 100644 --- a/test/parallel/test-vm-global-property-enumerator.js +++ b/test/parallel/test-vm-global-property-enumerator.js @@ -1,5 +1,6 @@ 'use strict'; require('../common'); +const globalNames = require('../common/globals'); const vm = require('vm'); const assert = require('assert'); @@ -39,11 +40,62 @@ const cases = [ key: 'value', }, }, + (() => { + const obj = { + __proto__: { + [Symbol.toStringTag]: 'proto', + }, + }; + Object.defineProperty(obj, '1', { + value: 'value', + enumerable: false, + configurable: true, + }); + Object.defineProperty(obj, 'key', { + value: 'value', + enumerable: false, + configurable: true, + }); + Object.defineProperty(obj, Symbol('symbol'), { + value: 'value', + enumerable: false, + configurable: true, + }); + Object.defineProperty(obj, Symbol('symbol-enumerable'), { + value: 'value', + enumerable: true, + configurable: true, + }); + return obj; + })(), ]; 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`); + assert.deepStrictEqual(Object.keys(globalObj), Object.keys(obj), `Case ${idx} failed: Object.keys`); + + const ownPropertyNamesInner = difference(Object.getOwnPropertyNames(globalObj), globalNames.intrinsics); + const ownPropertyNamesOuter = Object.getOwnPropertyNames(obj); + assert.deepStrictEqual( + ownPropertyNamesInner, + ownPropertyNamesOuter, + `Case ${idx} failed: Object.getOwnPropertyNames` + ); + + // FIXME(legendecas): globalThis[@@toStringTag] is unconditionally + // initialized to the sandbox's constructor name, even if it does not exist + // on the sandbox object. This may incorrectly initialize the prototype + // @@toStringTag on the globalThis as an own property, like + // Window.prototype[@@toStringTag] should be a property on the prototype. + assert.deepStrictEqual( + Object.getOwnPropertySymbols(globalObj).filter((it) => it !== Symbol.toStringTag), + Object.getOwnPropertySymbols(obj), + `Case ${idx} failed: Object.getOwnPropertySymbols` + ); } + +function difference(arrA, arrB) { + const setB = new Set(arrB); + return arrA.filter((x) => !setB.has(x)); +}; diff --git a/test/known_issues/test-vm-ownkeys.js b/test/parallel/test-vm-ownkeys.js similarity index 68% rename from test/known_issues/test-vm-ownkeys.js rename to test/parallel/test-vm-ownkeys.js index 9d1bae72b50c28..47938a176bbf3b 100644 --- a/test/known_issues/test-vm-ownkeys.js +++ b/test/parallel/test-vm-ownkeys.js @@ -15,11 +15,9 @@ Object.defineProperty(sandbox, sym2, { value: true }); const ctx = vm.createContext(sandbox); -// Sanity check -// Please uncomment these when the test is no longer broken -// assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]); -// assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']); -// assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]); +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); diff --git a/test/known_issues/test-vm-ownpropertynames.js b/test/parallel/test-vm-ownpropertynames.js similarity index 69% rename from test/known_issues/test-vm-ownpropertynames.js rename to test/parallel/test-vm-ownpropertynames.js index 41f6822ee1a47f..f076195257536f 100644 --- a/test/known_issues/test-vm-ownpropertynames.js +++ b/test/parallel/test-vm-ownpropertynames.js @@ -15,11 +15,9 @@ Object.defineProperty(sandbox, sym2, { value: true }); const ctx = vm.createContext(sandbox); -// Sanity check -// Please uncomment these when the test is no longer broken -// assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]); -// assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']); -// assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]); +assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]); +assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']); +assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]); const nativeNames = vm.runInNewContext('Object.getOwnPropertyNames(this);'); const ownNames = vm.runInContext('Object.getOwnPropertyNames(this);', ctx); diff --git a/test/known_issues/test-vm-ownpropertysymbols.js b/test/parallel/test-vm-ownpropertysymbols.js similarity index 69% rename from test/known_issues/test-vm-ownpropertysymbols.js rename to test/parallel/test-vm-ownpropertysymbols.js index 676133cc9ecaea..f943b8604949cd 100644 --- a/test/known_issues/test-vm-ownpropertysymbols.js +++ b/test/parallel/test-vm-ownpropertysymbols.js @@ -15,11 +15,9 @@ Object.defineProperty(sandbox, sym2, { value: true }); const ctx = vm.createContext(sandbox); -// Sanity check -// Please uncomment these when the test is no longer broken -// assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]); -// assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']); -// assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]); +assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]); +assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']); +assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]); const nativeSym = vm.runInNewContext('Object.getOwnPropertySymbols(this);'); const ownSym = vm.runInContext('Object.getOwnPropertySymbols(this);', ctx);