vm,src: add property query interceptors

Distinguish named property and indexed property when enumerating keys
and handle query interceptions.

Co-Authored-By: Michaël Zasso <targos@protonmail.com>
PR-URL: https://github.com/nodejs/node/pull/53517
Fixes: https://github.com/nodejs/node/issues/52720
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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<Name> property, const PropertyCallbackInfo<Integer>& args) {
+  ContextifyContext* ctx = ContextifyContext::Get(args);
+
+  // Still initializing
+  if (IsStillInitializing(ctx)) {
+    return Intercepted::kNo;
+  }
+
+  Local<Context> context = ctx->context();
+  Local<Object> sandbox = ctx->sandbox();
+
+  PropertyAttribute attr;
+
+  Maybe<bool> maybe_has = sandbox->HasRealNamedProperty(context, property);
+  if (maybe_has.IsNothing()) {
+    return Intercepted::kNo;
+  } else if (maybe_has.FromJust()) {
+    Maybe<PropertyAttribute> 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<PropertyAttribute> 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<Name> property, const PropertyCallbackInfo<Value>& args) {
@@ -695,13 +749,70 @@ void ContextifyContext::PropertyEnumeratorCallback(
   if (IsStillInitializing(ctx)) return;
 
   Local<Array> 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>(PropertyFilter::ONLY_ENUMERABLE |
+                                           PropertyFilter::SKIP_SYMBOLS),
+               IndexFilter::kSkipIndices)
+           .ToLocal(&properties))
     return;
 
   args.GetReturnValue().Set(properties);
 }
 
+// static
+void ContextifyContext::IndexedPropertyEnumeratorCallback(
+    const PropertyCallbackInfo<Array>& args) {
+  Isolate* isolate = args.GetIsolate();
+  HandleScope scope(isolate);
+  ContextifyContext* ctx = ContextifyContext::Get(args);
+  Local<Context> context = ctx->context();
+
+  // Still initializing
+  if (IsStillInitializing(ctx)) return;
+
+  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;
+
+  std::vector<v8::Global<Value>> properties_vec;
+  if (FromV8Array(context, properties, &properties_vec).IsNothing()) {
+    return;
+  }
+
+  // Filter out non-number property names.
+  std::vector<Local<Value>> indices;
+  for (uint32_t i = 0; i < properties->Length(); i++) {
+    Local<Value> 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<Integer>& 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<Value>& args) {

src/node_contextify.h

@@ -94,6 +94,9 @@ class ContextifyContext : public BaseObject {
       bool produce_cached_data,
       v8::Local<v8::Symbol> id_symbol,
       const errors::TryCatchScope& try_catch);
+  static v8::Intercepted PropertyQueryCallback(
+      v8::Local<v8::Name> property,
+      const v8::PropertyCallbackInfo<v8::Integer>& args);
   static v8::Intercepted PropertyGetterCallback(
       v8::Local<v8::Name> property,
       const v8::PropertyCallbackInfo<v8::Value>& args);
@@ -113,6 +116,8 @@ class ContextifyContext : public BaseObject {
       const v8::PropertyCallbackInfo<v8::Boolean>& args);
   static void PropertyEnumeratorCallback(
       const v8::PropertyCallbackInfo<v8::Array>& args);
+  static v8::Intercepted IndexedPropertyQueryCallback(
+      uint32_t index, const v8::PropertyCallbackInfo<v8::Integer>& args);
   static v8::Intercepted IndexedPropertyGetterCallback(
       uint32_t index, const v8::PropertyCallbackInfo<v8::Value>& args);
   static v8::Intercepted IndexedPropertySetterCallback(
@@ -127,6 +132,8 @@ class ContextifyContext : public BaseObject {
       const v8::PropertyCallbackInfo<void>& args);
   static v8::Intercepted IndexedPropertyDeleterCallback(
       uint32_t index, const v8::PropertyCallbackInfo<v8::Boolean>& args);
+  static void IndexedPropertyEnumeratorCallback(
+      const v8::PropertyCallbackInfo<v8::Array>& args);
 
   v8::Global<v8::Context> context_;
   std::unique_ptr<v8::MicrotaskQueue> microtask_queue_;

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

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