vm: return all own names and symbols in property enumerator

src/node_contextify.cc

@@ -742,19 +742,25 @@ Intercepted ContextifyContext::PropertyDeleterCallback(
 // static
 void ContextifyContext::PropertyEnumeratorCallback(
     const PropertyCallbackInfo<Array>& 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<Array> 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>(PropertyFilter::ONLY_ENUMERABLE |
-                                           PropertyFilter::SKIP_SYMBOLS),
+               KeyCollectionMode::kOwnOnly,
+               static_cast<PropertyFilter>(PropertyFilter::ALL_PROPERTIES),
                IndexFilter::kSkipIndices)
            .ToLocal(&properties))
     return;
@@ -765,6 +771,12 @@ void ContextifyContext::PropertyEnumeratorCallback(
 // static
 void ContextifyContext::IndexedPropertyEnumeratorCallback(
     const PropertyCallbackInfo<Array>& 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);
@@ -775,9 +787,15 @@ void ContextifyContext::IndexedPropertyEnumeratorCallback(
 
   Local<Array> 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>(PropertyFilter::SKIP_SYMBOLS),
+               IndexFilter::kIncludeIndices)
+           .ToLocal(&properties))
+    return;
 
   std::vector<v8::Global<Value>> properties_vec;
   if (FromV8Array(context, properties, &properties_vec).IsNothing()) {

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));
+};

test/known_issues/test-vm-ownkeys.js → 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);

test/known_issues/test-vm-ownpropertynames.js → 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);

test/known_issues/test-vm-ownpropertysymbols.js → 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);