Skip to content

Commit fe5666f

Browse files
legendecasRafaelGSS
legendecas
authored and
RafaelGSS
committed
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. PR-URL: #54522 Refs: jsdom/jsdom#3688 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
1 parent 522d5a3 commit fe5666f

5 files changed

Lines changed: 88 additions & 24 deletions

File tree

‎src/node_contextify.cc‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -769,19 +769,25 @@ Intercepted ContextifyContext::PropertyDeleterCallback(
769769
// static
770770
void ContextifyContext::PropertyEnumeratorCallback(
771771
const PropertyCallbackInfo<Array>& args) {
772+
// Named enumerator will be invoked on Object.keys,
773+
// Object.getOwnPropertyNames, Object.getOwnPropertySymbols,
774+
// Object.getOwnPropertyDescriptors, for...in, etc. operations.
775+
// Named enumerator should return all own non-indices property names,
776+
// including string properties and symbol properties. V8 will filter the
777+
// result array to match the expected symbol-only, enumerable-only with
778+
// NamedPropertyQueryCallback.
772779
ContextifyContext* ctx = ContextifyContext::Get(args);
773780

774781
// Still initializing
775782
if (IsStillInitializing(ctx)) return;
776783

777784
Local<Array> properties;
778-
// Only get named properties, exclude symbols and indices.
785+
// Only get own named properties, exclude indices.
779786
if (!ctx->sandbox()
780787
->GetPropertyNames(
781788
ctx->context(),
782-
KeyCollectionMode::kIncludePrototypes,
783-
static_cast<PropertyFilter>(PropertyFilter::ONLY_ENUMERABLE |
784-
PropertyFilter::SKIP_SYMBOLS),
789+
KeyCollectionMode::kOwnOnly,
790+
static_cast<PropertyFilter>(PropertyFilter::ALL_PROPERTIES),
785791
IndexFilter::kSkipIndices)
786792
.ToLocal(&properties))
787793
return;
@@ -792,6 +798,12 @@ void ContextifyContext::PropertyEnumeratorCallback(
792798
// static
793799
void ContextifyContext::IndexedPropertyEnumeratorCallback(
794800
const PropertyCallbackInfo<Array>& args) {
801+
// Indexed enumerator will be invoked on Object.keys,
802+
// Object.getOwnPropertyNames, Object.getOwnPropertyDescriptors, for...in,
803+
// etc. operations. Indexed enumerator should return all own non-indices index
804+
// properties. V8 will filter the result array to match the expected
805+
// enumerable-only with IndexedPropertyQueryCallback.
806+
795807
Isolate* isolate = args.GetIsolate();
796808
HandleScope scope(isolate);
797809
ContextifyContext* ctx = ContextifyContext::Get(args);
@@ -802,9 +814,15 @@ void ContextifyContext::IndexedPropertyEnumeratorCallback(
802814

803815
Local<Array> properties;
804816

805-
// By default, GetPropertyNames returns string and number property names, and
806-
// doesn't convert the numbers to strings.
807-
if (!ctx->sandbox()->GetPropertyNames(context).ToLocal(&properties)) return;
817+
// Only get own index properties.
818+
if (!ctx->sandbox()
819+
->GetPropertyNames(
820+
context,
821+
KeyCollectionMode::kOwnOnly,
822+
static_cast<PropertyFilter>(PropertyFilter::SKIP_SYMBOLS),
823+
IndexFilter::kIncludeIndices)
824+
.ToLocal(&properties))
825+
return;
808826

809827
std::vector<v8::Global<Value>> properties_vec;
810828
if (FromV8Array(context, properties, &properties_vec).IsNothing()) {

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

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
'use strict';
22
require('../common');
3+
const globalNames = require('../common/globals');
34
const vm = require('vm');
45
const assert = require('assert');
56

@@ -39,11 +40,62 @@ const cases = [
3940
key: 'value',
4041
},
4142
},
43+
(() => {
44+
const obj = {
45+
__proto__: {
46+
[Symbol.toStringTag]: 'proto',
47+
},
48+
};
49+
Object.defineProperty(obj, '1', {
50+
value: 'value',
51+
enumerable: false,
52+
configurable: true,
53+
});
54+
Object.defineProperty(obj, 'key', {
55+
value: 'value',
56+
enumerable: false,
57+
configurable: true,
58+
});
59+
Object.defineProperty(obj, Symbol('symbol'), {
60+
value: 'value',
61+
enumerable: false,
62+
configurable: true,
63+
});
64+
Object.defineProperty(obj, Symbol('symbol-enumerable'), {
65+
value: 'value',
66+
enumerable: true,
67+
configurable: true,
68+
});
69+
return obj;
70+
})(),
4271
];
4372

4473
for (const [idx, obj] of cases.entries()) {
4574
const ctx = vm.createContext(obj);
4675
const globalObj = vm.runInContext('this', ctx);
47-
const keys = Object.keys(globalObj);
48-
assert.deepStrictEqual(keys, Object.keys(obj), `Case ${idx} failed`);
76+
assert.deepStrictEqual(Object.keys(globalObj), Object.keys(obj), `Case ${idx} failed: Object.keys`);
77+
78+
const ownPropertyNamesInner = difference(Object.getOwnPropertyNames(globalObj), globalNames.intrinsics);
79+
const ownPropertyNamesOuter = Object.getOwnPropertyNames(obj);
80+
assert.deepStrictEqual(
81+
ownPropertyNamesInner,
82+
ownPropertyNamesOuter,
83+
`Case ${idx} failed: Object.getOwnPropertyNames`
84+
);
85+
86+
// FIXME(legendecas): globalThis[@@toStringTag] is unconditionally
87+
// initialized to the sandbox's constructor name, even if it does not exist
88+
// on the sandbox object. This may incorrectly initialize the prototype
89+
// @@toStringTag on the globalThis as an own property, like
90+
// Window.prototype[@@toStringTag] should be a property on the prototype.
91+
assert.deepStrictEqual(
92+
Object.getOwnPropertySymbols(globalObj).filter((it) => it !== Symbol.toStringTag),
93+
Object.getOwnPropertySymbols(obj),
94+
`Case ${idx} failed: Object.getOwnPropertySymbols`
95+
);
4996
}
97+
98+
function difference(arrA, arrB) {
99+
const setB = new Set(arrB);
100+
return arrA.filter((x) => !setB.has(x));
101+
};

test/known_issues/test-vm-ownkeys.js renamed to test/parallel/test-vm-ownkeys.js

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,9 @@ Object.defineProperty(sandbox, sym2, { value: true });
1515

1616
const ctx = vm.createContext(sandbox);
1717

18-
// Sanity check
19-
// Please uncomment these when the test is no longer broken
20-
// assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]);
21-
// assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']);
22-
// assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]);
18+
assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]);
19+
assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']);
20+
assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]);
2321

2422
const nativeKeys = vm.runInNewContext('Reflect.ownKeys(this);');
2523
const ownKeys = vm.runInContext('Reflect.ownKeys(this);', ctx);

test/known_issues/test-vm-ownpropertynames.js renamed to test/parallel/test-vm-ownpropertynames.js

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,9 @@ Object.defineProperty(sandbox, sym2, { value: true });
1515

1616
const ctx = vm.createContext(sandbox);
1717

18-
// Sanity check
19-
// Please uncomment these when the test is no longer broken
20-
// assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]);
21-
// assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']);
22-
// assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]);
18+
assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]);
19+
assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']);
20+
assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]);
2321

2422
const nativeNames = vm.runInNewContext('Object.getOwnPropertyNames(this);');
2523
const ownNames = vm.runInContext('Object.getOwnPropertyNames(this);', ctx);

test/known_issues/test-vm-ownpropertysymbols.js renamed to test/parallel/test-vm-ownpropertysymbols.js

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,9 @@ Object.defineProperty(sandbox, sym2, { value: true });
1515

1616
const ctx = vm.createContext(sandbox);
1717

18-
// Sanity check
19-
// Please uncomment these when the test is no longer broken
20-
// assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]);
21-
// assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']);
22-
// assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]);
18+
assert.deepStrictEqual(Reflect.ownKeys(sandbox), ['a', 'b', sym1, sym2]);
19+
assert.deepStrictEqual(Object.getOwnPropertyNames(sandbox), ['a', 'b']);
20+
assert.deepStrictEqual(Object.getOwnPropertySymbols(sandbox), [sym1, sym2]);
2321

2422
const nativeSym = vm.runInNewContext('Object.getOwnPropertySymbols(this);');
2523
const ownSym = vm.runInContext('Object.getOwnPropertySymbols(this);', ctx);

0 commit comments

Comments
 (0)