src: fix vm enumerator callbacks

Some of the choices here are odd, including that symbols are missing.
However, that matches previous behaviour.

What had to be changed was that inherited properties are no longer
included; the alternative would be to also refactor the descriptor
callbacks to provide data for inherited properties.

Fixes: nodejs#22723
Refs: nodejs#22390

Author: addaleax

src/node_contextify.cc

@@ -155,7 +155,7 @@ Local<Context> ContextifyContext::CreateV8Context(
       IndexedPropertySetterCallback,
       IndexedPropertyQueryCallback,
       IndexedPropertyDeleterCallback,
-      PropertyEnumeratorCallback,
+      IndexedPropertyEnumeratorCallback,
       IndexedPropertyDefinerCallback,
       IndexedPropertyDescriptorCallback,
       CreateDataWrapper(env));
@@ -528,7 +528,16 @@ void ContextifyContext::PropertyEnumeratorCallback(
   if (ctx->context_.IsEmpty())
     return;
 
-  args.GetReturnValue().Set(ctx->sandbox()->GetPropertyNames());
+  Local<Array> ret;
+  if (!ctx->sandbox()->GetPropertyNames(
+          ctx->context(),
+          v8::KeyCollectionMode::kOwnOnly,
+          v8::SKIP_SYMBOLS,
+          v8::IndexFilter::kSkipIndices).ToLocal(&ret)) {
+    return;
+  }
+
+  args.GetReturnValue().Set(ret);
 }
 
 // static
@@ -623,6 +632,39 @@ void ContextifyContext::IndexedPropertyDeleterCallback(
   args.GetReturnValue().Set(false);
 }
 
+// static
+void ContextifyContext::IndexedPropertyEnumeratorCallback(
+    const PropertyCallbackInfo<Array>& args) {
+  ContextifyContext* ctx = ContextifyContext::Get(args);
+
+  // Still initializing
+  if (ctx->context_.IsEmpty())
+    return;
+
+  Local<Array> all_keys;
+  if (!ctx->sandbox()->GetPropertyNames(
+          ctx->context(),
+          v8::KeyCollectionMode::kOwnOnly,
+          v8::SKIP_SYMBOLS,
+          v8::IndexFilter::kIncludeIndices).ToLocal(&all_keys)) {
+    return;
+  }
+  uint32_t all_keys_len = all_keys->Length();
+
+  Local<Array> ret = Array::New(ctx->env()->isolate());
+  for (uint32_t i = 0, j = 0; i < all_keys_len; ++i) {
+    Local<Value> entry;
+    if (!all_keys->Get(ctx->context(), i).ToLocal(&entry))
+      return;
+    if (!entry->IsUint32())
+      continue;
+    if (!ret->Set(ctx->context(), j++, entry).FromMaybe(false))
+      return;
+  }
+
+  args.GetReturnValue().Set(ret);
+}
+
 class ContextifyScript : public BaseObject {
  private:
   Persistent<UnboundScript> script_;

src/node_contextify.h

@@ -104,6 +104,8 @@ class ContextifyContext {
   static void IndexedPropertyDeleterCallback(
       uint32_t index,
       const v8::PropertyCallbackInfo<v8::Boolean>& args);
+  static void IndexedPropertyEnumeratorCallback(
+      const v8::PropertyCallbackInfo<v8::Array>& args);
   Environment* const env_;
   Persistent<v8::Context> context_;
 };

test/parallel/test-vm-object-keys.js

@@ -0,0 +1,45 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+// Regression test for https://github.com/nodejs/node/issues/22723.
+
+const kFoo = Symbol('kFoo');
+
+const test = {
+  'not': 'empty',
+  '0': 0,
+  '0.5': 0.5,
+  '1': 1,
+  '-1': -1,
+  [kFoo]: kFoo
+};
+vm.createContext(test);
+const proxied = vm.runInContext('this', test);
+
+// TODO(addaleax): The .sort() calls should not be necessary; the property
+// order should be indices, then other properties, then symbols.
+assert.deepStrictEqual(
+  Object.keys(proxied).sort(),
+  ['0', '1', 'not', '0.5', '-1'].sort());
+assert.deepStrictEqual(
+  // Filter out names shared by all global objects, i.e. JS builtins.
+  Object.getOwnPropertyNames(proxied)
+      .filter((name) => !(name in global))
+      .sort(),
+  ['0', '1', 'not', '0.5', '-1'].sort());
+assert.deepStrictEqual(Object.getOwnPropertySymbols(proxied), []);
+
+Object.setPrototypeOf(test, { inherited: 'true', 17: 42 });
+
+assert.deepStrictEqual(
+  Object.keys(proxied).sort(),
+  ['0', '1', 'not', '0.5', '-1'].sort());
+assert.deepStrictEqual(
+  // Filter out names shared by all global objects, i.e. JS builtins.
+  Object.getOwnPropertyNames(proxied)
+      .filter((name) => !(name in global))
+          .sort(),
+  ['0', '1', 'not', '0.5', '-1'].sort());
+assert.deepStrictEqual(Object.getOwnPropertySymbols(proxied), []);