Skip to content

Commit 9fd976b

Browse files
legendecastargos
authored and
aduh95
committed
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 <[email protected]> PR-URL: #53517 Fixes: #52720 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
1 parent 021e2cf commit 9fd976b

File tree

4 files changed

+255
-5
lines changed

4 files changed

+255
-5
lines changed

‎src/node_contextify.cc‎

Lines changed: 116 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,13 @@ using v8::FunctionCallbackInfo;
5050
using v8::FunctionTemplate;
5151
using v8::HandleScope;
5252
using v8::IndexedPropertyHandlerConfiguration;
53+
using v8::IndexFilter;
5354
using v8::Int32;
55+
using v8::Integer;
5456
using v8::Intercepted;
5557
using v8::Isolate;
5658
using v8::Just;
59+
using v8::KeyCollectionMode;
5760
using v8::Local;
5861
using v8::Maybe;
5962
using v8::MaybeLocal;
@@ -72,6 +75,7 @@ using v8::Promise;
7275
using v8::PropertyAttribute;
7376
using v8::PropertyCallbackInfo;
7477
using v8::PropertyDescriptor;
78+
using v8::PropertyFilter;
7579
using v8::PropertyHandlerFlags;
7680
using v8::Script;
7781
using v8::ScriptCompiler;
@@ -175,20 +179,22 @@ void ContextifyContext::InitializeGlobalTemplates(IsolateData* isolate_data) {
175179
NamedPropertyHandlerConfiguration config(
176180
PropertyGetterCallback,
177181
PropertySetterCallback,
178-
PropertyDescriptorCallback,
182+
PropertyQueryCallback,
179183
PropertyDeleterCallback,
180184
PropertyEnumeratorCallback,
181185
PropertyDefinerCallback,
186+
PropertyDescriptorCallback,
182187
{},
183188
PropertyHandlerFlags::kHasNoSideEffect);
184189

185190
IndexedPropertyHandlerConfiguration indexed_config(
186191
IndexedPropertyGetterCallback,
187192
IndexedPropertySetterCallback,
188-
IndexedPropertyDescriptorCallback,
193+
IndexedPropertyQueryCallback,
189194
IndexedPropertyDeleterCallback,
190-
PropertyEnumeratorCallback,
195+
IndexedPropertyEnumeratorCallback,
191196
IndexedPropertyDefinerCallback,
197+
IndexedPropertyDescriptorCallback,
192198
{},
193199
PropertyHandlerFlags::kHasNoSideEffect);
194200

@@ -353,17 +359,20 @@ void ContextifyContext::RegisterExternalReferences(
353359
ExternalReferenceRegistry* registry) {
354360
registry->Register(MakeContext);
355361
registry->Register(CompileFunction);
362+
registry->Register(PropertyQueryCallback);
356363
registry->Register(PropertyGetterCallback);
357364
registry->Register(PropertySetterCallback);
358365
registry->Register(PropertyDescriptorCallback);
359366
registry->Register(PropertyDeleterCallback);
360367
registry->Register(PropertyEnumeratorCallback);
361368
registry->Register(PropertyDefinerCallback);
369+
registry->Register(IndexedPropertyQueryCallback);
362370
registry->Register(IndexedPropertyGetterCallback);
363371
registry->Register(IndexedPropertySetterCallback);
364372
registry->Register(IndexedPropertyDescriptorCallback);
365373
registry->Register(IndexedPropertyDeleterCallback);
366374
registry->Register(IndexedPropertyDefinerCallback);
375+
registry->Register(IndexedPropertyEnumeratorCallback);
367376
}
368377

369378
// makeContext(sandbox, name, origin, strings, wasm);
@@ -451,6 +460,51 @@ bool ContextifyContext::IsStillInitializing(const ContextifyContext* ctx) {
451460
return ctx == nullptr || ctx->context_.IsEmpty();
452461
}
453462

463+
// static
464+
Intercepted ContextifyContext::PropertyQueryCallback(
465+
Local<Name> property, const PropertyCallbackInfo<Integer>& args) {
466+
ContextifyContext* ctx = ContextifyContext::Get(args);
467+
468+
// Still initializing
469+
if (IsStillInitializing(ctx)) {
470+
return Intercepted::kNo;
471+
}
472+
473+
Local<Context> context = ctx->context();
474+
Local<Object> sandbox = ctx->sandbox();
475+
476+
PropertyAttribute attr;
477+
478+
Maybe<bool> maybe_has = sandbox->HasRealNamedProperty(context, property);
479+
if (maybe_has.IsNothing()) {
480+
return Intercepted::kNo;
481+
} else if (maybe_has.FromJust()) {
482+
Maybe<PropertyAttribute> maybe_attr =
483+
sandbox->GetRealNamedPropertyAttributes(context, property);
484+
if (!maybe_attr.To(&attr)) {
485+
return Intercepted::kNo;
486+
}
487+
args.GetReturnValue().Set(attr);
488+
return Intercepted::kYes;
489+
} else {
490+
maybe_has = ctx->global_proxy()->HasRealNamedProperty(context, property);
491+
if (maybe_has.IsNothing()) {
492+
return Intercepted::kNo;
493+
} else if (maybe_has.FromJust()) {
494+
Maybe<PropertyAttribute> maybe_attr =
495+
ctx->global_proxy()->GetRealNamedPropertyAttributes(context,
496+
property);
497+
if (!maybe_attr.To(&attr)) {
498+
return Intercepted::kNo;
499+
}
500+
args.GetReturnValue().Set(attr);
501+
return Intercepted::kYes;
502+
}
503+
}
504+
505+
return Intercepted::kNo;
506+
}
507+
454508
// static
455509
Intercepted ContextifyContext::PropertyGetterCallback(
456510
Local<Name> property, const PropertyCallbackInfo<Value>& args) {
@@ -695,13 +749,70 @@ void ContextifyContext::PropertyEnumeratorCallback(
695749
if (IsStillInitializing(ctx)) return;
696750

697751
Local<Array> properties;
698-
699-
if (!ctx->sandbox()->GetPropertyNames(ctx->context()).ToLocal(&properties))
752+
// Only get named properties, exclude symbols and indices.
753+
if (!ctx->sandbox()
754+
->GetPropertyNames(
755+
ctx->context(),
756+
KeyCollectionMode::kIncludePrototypes,
757+
static_cast<PropertyFilter>(PropertyFilter::ONLY_ENUMERABLE |
758+
PropertyFilter::SKIP_SYMBOLS),
759+
IndexFilter::kSkipIndices)
760+
.ToLocal(&properties))
700761
return;
701762

702763
args.GetReturnValue().Set(properties);
703764
}
704765

766+
// static
767+
void ContextifyContext::IndexedPropertyEnumeratorCallback(
768+
const PropertyCallbackInfo<Array>& args) {
769+
Isolate* isolate = args.GetIsolate();
770+
HandleScope scope(isolate);
771+
ContextifyContext* ctx = ContextifyContext::Get(args);
772+
Local<Context> context = ctx->context();
773+
774+
// Still initializing
775+
if (IsStillInitializing(ctx)) return;
776+
777+
Local<Array> properties;
778+
779+
// By default, GetPropertyNames returns string and number property names, and
780+
// doesn't convert the numbers to strings.
781+
if (!ctx->sandbox()->GetPropertyNames(context).ToLocal(&properties)) return;
782+
783+
std::vector<v8::Global<Value>> properties_vec;
784+
if (FromV8Array(context, properties, &properties_vec).IsNothing()) {
785+
return;
786+
}
787+
788+
// Filter out non-number property names.
789+
std::vector<Local<Value>> indices;
790+
for (uint32_t i = 0; i < properties->Length(); i++) {
791+
Local<Value> prop = properties_vec[i].Get(isolate);
792+
if (!prop->IsNumber()) {
793+
continue;
794+
}
795+
indices.push_back(prop);
796+
}
797+
798+
args.GetReturnValue().Set(
799+
Array::New(args.GetIsolate(), indices.data(), indices.size()));
800+
}
801+
802+
// static
803+
Intercepted ContextifyContext::IndexedPropertyQueryCallback(
804+
uint32_t index, const PropertyCallbackInfo<Integer>& args) {
805+
ContextifyContext* ctx = ContextifyContext::Get(args);
806+
807+
// Still initializing
808+
if (IsStillInitializing(ctx)) {
809+
return Intercepted::kNo;
810+
}
811+
812+
return ContextifyContext::PropertyQueryCallback(
813+
Uint32ToName(ctx->context(), index), args);
814+
}
815+
705816
// static
706817
Intercepted ContextifyContext::IndexedPropertyGetterCallback(
707818
uint32_t index, const PropertyCallbackInfo<Value>& args) {

‎src/node_contextify.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,9 @@ class ContextifyContext : public BaseObject {
9494
bool produce_cached_data,
9595
v8::Local<v8::Symbol> id_symbol,
9696
const errors::TryCatchScope& try_catch);
97+
static v8::Intercepted PropertyQueryCallback(
98+
v8::Local<v8::Name> property,
99+
const v8::PropertyCallbackInfo<v8::Integer>& args);
97100
static v8::Intercepted PropertyGetterCallback(
98101
v8::Local<v8::Name> property,
99102
const v8::PropertyCallbackInfo<v8::Value>& args);
@@ -113,6 +116,8 @@ class ContextifyContext : public BaseObject {
113116
const v8::PropertyCallbackInfo<v8::Boolean>& args);
114117
static void PropertyEnumeratorCallback(
115118
const v8::PropertyCallbackInfo<v8::Array>& args);
119+
static v8::Intercepted IndexedPropertyQueryCallback(
120+
uint32_t index, const v8::PropertyCallbackInfo<v8::Integer>& args);
116121
static v8::Intercepted IndexedPropertyGetterCallback(
117122
uint32_t index, const v8::PropertyCallbackInfo<v8::Value>& args);
118123
static v8::Intercepted IndexedPropertySetterCallback(
@@ -127,6 +132,8 @@ class ContextifyContext : public BaseObject {
127132
const v8::PropertyCallbackInfo<void>& args);
128133
static v8::Intercepted IndexedPropertyDeleterCallback(
129134
uint32_t index, const v8::PropertyCallbackInfo<v8::Boolean>& args);
135+
static void IndexedPropertyEnumeratorCallback(
136+
const v8::PropertyCallbackInfo<v8::Array>& args);
130137

131138
v8::Global<v8::Context> context_;
132139
std::unique_ptr<v8::MicrotaskQueue> microtask_queue_;

‎test/parallel/test-vm-global-property-enumerator.js‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
'use strict';
2+
require('../common');
3+
const vm = require('vm');
4+
const assert = require('assert');
5+
6+
// Regression of https://github.com/nodejs/node/issues/53346
7+
8+
const cases = [
9+
{
10+
get key() {
11+
return 'value';
12+
},
13+
},
14+
{
15+
// Intentionally single setter.
16+
// eslint-disable-next-line accessor-pairs
17+
set key(value) {},
18+
},
19+
{},
20+
{
21+
key: 'value',
22+
},
23+
(new class GetterObject {
24+
get key() {
25+
return 'value';
26+
}
27+
}()),
28+
(new class SetterObject {
29+
// Intentionally single setter.
30+
// eslint-disable-next-line accessor-pairs
31+
set key(value) {
32+
// noop
33+
}
34+
}()),
35+
[],
36+
[['key', 'value']],
37+
{
38+
__proto__: {
39+
key: 'value',
40+
},
41+
},
42+
];
43+
44+
for (const [idx, obj] of cases.entries()) {
45+
const ctx = vm.createContext(obj);
46+
const globalObj = vm.runInContext('this', ctx);
47+
const keys = Object.keys(globalObj);
48+
assert.deepStrictEqual(keys, Object.keys(obj), `Case ${idx} failed`);
49+
}

‎test/parallel/test-vm-global-property-prototype.js‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const vm = require('vm');
5+
6+
const sandbox = {
7+
onSelf: 'onSelf',
8+
};
9+
10+
function onSelfGetter() {
11+
return 'onSelfGetter';
12+
}
13+
14+
Object.defineProperty(sandbox, 'onSelfGetter', {
15+
get: onSelfGetter,
16+
});
17+
18+
Object.defineProperty(sandbox, 1, {
19+
value: 'onSelfIndexed',
20+
writable: false,
21+
enumerable: false,
22+
configurable: true,
23+
});
24+
25+
const ctx = vm.createContext(sandbox);
26+
27+
const result = vm.runInContext(`
28+
Object.prototype.onProto = 'onProto';
29+
Object.defineProperty(Object.prototype, 'onProtoGetter', {
30+
get() {
31+
return 'onProtoGetter';
32+
},
33+
});
34+
Object.defineProperty(Object.prototype, 2, {
35+
value: 'onProtoIndexed',
36+
writable: false,
37+
enumerable: false,
38+
configurable: true,
39+
});
40+
41+
const resultHasOwn = {
42+
onSelf: Object.hasOwn(this, 'onSelf'),
43+
onSelfGetter: Object.hasOwn(this, 'onSelfGetter'),
44+
onSelfIndexed: Object.hasOwn(this, 1),
45+
onProto: Object.hasOwn(this, 'onProto'),
46+
onProtoGetter: Object.hasOwn(this, 'onProtoGetter'),
47+
onProtoIndexed: Object.hasOwn(this, 2),
48+
};
49+
50+
const getDesc = (prop) => Object.getOwnPropertyDescriptor(this, prop);
51+
const resultDesc = {
52+
onSelf: getDesc('onSelf'),
53+
onSelfGetter: getDesc('onSelfGetter'),
54+
onSelfIndexed: getDesc(1),
55+
onProto: getDesc('onProto'),
56+
onProtoGetter: getDesc('onProtoGetter'),
57+
onProtoIndexed: getDesc(2),
58+
};
59+
({
60+
resultHasOwn,
61+
resultDesc,
62+
});
63+
`, ctx);
64+
65+
// eslint-disable-next-line no-restricted-properties
66+
assert.deepEqual(result, {
67+
resultHasOwn: {
68+
onSelf: true,
69+
onSelfGetter: true,
70+
onSelfIndexed: true,
71+
onProto: false,
72+
onProtoGetter: false,
73+
onProtoIndexed: false,
74+
},
75+
resultDesc: {
76+
onSelf: { value: 'onSelf', writable: true, enumerable: true, configurable: true },
77+
onSelfGetter: { get: onSelfGetter, set: undefined, enumerable: false, configurable: false },
78+
onSelfIndexed: { value: 'onSelfIndexed', writable: false, enumerable: false, configurable: true },
79+
onProto: undefined,
80+
onProtoGetter: undefined,
81+
onProtoIndexed: undefined,
82+
},
83+
});

0 commit comments

Comments
 (0)