vm: return all own names and symbols in property enumerator interceptor

Property enumerator methods like `Object.getOwnPropertyNames`,
`Object.getOwnPropertySymbols`, and `Object.keys` all invokes the
named property enumerator interceptor. V8 will filter the result based
on the invoked enumerator variant. Fix the enumerator interceptor to
return all potential properties.

src/node_contextify.cc

@@ -290,18 +290,6 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
   Local<Object> wrapper;
   {
     Context::Scope context_scope(v8_context);
-    Local<String> ctor_name = sandbox_obj->GetConstructorName();
-    if (!ctor_name->Equals(v8_context, env->object_string()).FromMaybe(false) &&
-        new_context_global
-            ->DefineOwnProperty(
-                v8_context,
-                v8::Symbol::GetToStringTag(env->isolate()),
-                ctor_name,
-                static_cast<v8::PropertyAttribute>(v8::DontEnum))
-            .IsNothing()) {
-      return BaseObjectPtr<ContextifyContext>();
-    }
-
     // Assign host_defined_options_id to the global object so that in the
     // callback of ImportModuleDynamically, we can get the
     // host_defined_options_id from the v8::Context without accessing the
@@ -742,19 +730,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 +759,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 +775,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,57 @@ 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`
+  );
+
+  assert.deepStrictEqual(
+    Object.getOwnPropertySymbols(globalObj),
+    Object.getOwnPropertySymbols(obj),
+    `Case ${idx} failed: Object.getOwnPropertySymbols`
+  );
 }
+
+function difference(arrA, arrB) {
+  const setB = new Set(arrB);
+  return arrA.filter((x) => !setB.has(x));
+};