vm: fix property queries for proxy sandboxes

Author: brianathere

src/node_contextify.cc

@@ -58,6 +58,7 @@ using v8::Int32;
 using v8::Integer;
 using v8::Intercepted;
 using v8::Isolate;
+using v8::Just;
 using v8::JustVoid;
 using v8::KeyCollectionMode;
 using v8::Local;
@@ -475,6 +476,60 @@ bool ContextifyContext::IsStillInitializing(const ContextifyContext* ctx) {
   return ctx == nullptr || ctx->context_.IsEmpty();
 }
 
+static Maybe<bool> GetOwnPropertyAttributes(Isolate* isolate,
+                                            Local<Context> context,
+                                            Local<Object> object,
+                                            Local<Name> property,
+                                            PropertyAttribute* attributes) {
+  Local<Value> desc_value;
+  if (!object->GetOwnPropertyDescriptor(context, property).ToLocal(&desc_value))
+    return Nothing<bool>();
+  if (desc_value->IsUndefined()) return Just(false);
+  if (!desc_value->IsObject()) return Nothing<bool>();
+
+  Local<Object> desc = desc_value.As<Object>();
+  int result = PropertyAttribute::None;
+
+  Local<String> enumerable_string =
+      FIXED_ONE_BYTE_STRING(isolate, "enumerable");
+  Local<Value> enumerable;
+  if (!desc->Get(context, enumerable_string).ToLocal(&enumerable)) {
+    return Nothing<bool>();
+  }
+  if (!enumerable->IsTrue()) {
+    result |= PropertyAttribute::DontEnum;
+  }
+
+  Local<String> configurable_string =
+      FIXED_ONE_BYTE_STRING(isolate, "configurable");
+  Local<Value> configurable;
+  if (!desc->Get(context, configurable_string).ToLocal(&configurable)) {
+    return Nothing<bool>();
+  }
+  if (!configurable->IsTrue()) {
+    result |= PropertyAttribute::DontDelete;
+  }
+
+  Local<String> writable_string = FIXED_ONE_BYTE_STRING(isolate, "writable");
+  Maybe<bool> maybe_has_writable =
+      desc->HasOwnProperty(context, writable_string);
+  if (maybe_has_writable.IsNothing()) {
+    return Nothing<bool>();
+  }
+  if (maybe_has_writable.FromJust()) {
+    Local<Value> writable;
+    if (!desc->Get(context, writable_string).ToLocal(&writable)) {
+      return Nothing<bool>();
+    }
+    if (!writable->IsTrue()) {
+      result |= PropertyAttribute::ReadOnly;
+    }
+  }
+
+  *attributes = static_cast<PropertyAttribute>(result);
+  return Just(true);
+}
+
 // static
 Intercepted ContextifyContext::PropertyQueryCallback(
     Local<Name> property, const PropertyCallbackInfo<Integer>& args) {
@@ -490,18 +545,15 @@ Intercepted ContextifyContext::PropertyQueryCallback(
 
   Local<Context> context = ctx->context();
   Local<Object> sandbox = ctx->sandbox();
+  Isolate* isolate = args.GetIsolate();
 
   PropertyAttribute attr;
 
-  Maybe<bool> maybe_has = sandbox->HasRealNamedProperty(context, property);
+  Maybe<bool> maybe_has =
+      GetOwnPropertyAttributes(isolate, context, sandbox, property, &attr);
   if (maybe_has.IsNothing()) {
-    return Intercepted::kNo;
+    return Intercepted::kYes;
   } 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 {

test/parallel/test-vm-proxy-sandbox-property-query.js

@@ -0,0 +1,71 @@
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+const sandbox = new Proxy({}, {});
+const ctx = vm.createContext(sandbox);
+
+vm.runInContext(`
+Object.defineProperty(this, 'foo', {
+  value: 42,
+  writable: true,
+  enumerable: true,
+  configurable: true,
+});
+Object.defineProperty(this, 'hidden', {
+  value: 1,
+  enumerable: false,
+  configurable: true,
+});
+Object.defineProperty(this, 'readonly', {
+  value: 2,
+  writable: false,
+  enumerable: true,
+  configurable: true,
+});
+`, ctx);
+
+const result = vm.runInContext(`({
+  value: foo,
+  descriptor: Object.getOwnPropertyDescriptor(globalThis, 'foo'),
+  ownNamesIncludes: Object.getOwnPropertyNames(globalThis).includes('foo'),
+  hasOwnProperty:
+    Object.prototype.hasOwnProperty.call(globalThis, 'foo'),
+  objectHasOwn: Object.hasOwn(globalThis, 'foo'),
+  inGlobalThis: 'foo' in globalThis,
+  reflectHas: Reflect.has(globalThis, 'foo'),
+  keysIncludesFoo: Object.keys(globalThis).includes('foo'),
+  keysIncludesHidden: Object.keys(globalThis).includes('hidden'),
+  hiddenIsEnumerable:
+    Object.prototype.propertyIsEnumerable.call(globalThis, 'hidden'),
+  readonlyDescriptor:
+    Object.getOwnPropertyDescriptor(globalThis, 'readonly'),
+  hasOwnToString: Object.hasOwn(globalThis, 'toString'),
+  toStringInGlobalThis: 'toString' in globalThis,
+})`, ctx);
+
+assert.strictEqual(result.value, 42);
+assert.deepStrictEqual({ ...result.descriptor }, {
+  value: 42,
+  writable: true,
+  enumerable: true,
+  configurable: true,
+});
+assert.strictEqual(result.ownNamesIncludes, true);
+assert.strictEqual(result.hasOwnProperty, true);
+assert.strictEqual(result.objectHasOwn, true);
+assert.strictEqual(result.inGlobalThis, true);
+assert.strictEqual(result.reflectHas, true);
+assert.strictEqual(result.keysIncludesFoo, true);
+assert.strictEqual(result.keysIncludesHidden, false);
+assert.strictEqual(result.hiddenIsEnumerable, false);
+assert.deepStrictEqual({ ...result.readonlyDescriptor }, {
+  value: 2,
+  writable: false,
+  enumerable: true,
+  configurable: true,
+});
+assert.strictEqual(result.hasOwnToString, false);
+assert.strictEqual(result.toStringInGlobalThis, true);