Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit fe5666f

Browse filesBrowse 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
Copy full SHA for fe5666f

File tree

Expand file treeCollapse file tree

5 files changed

+88
-24
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+88
-24
lines changed
Open diff view settings
Collapse file

‎src/node_contextify.cc‎

Copy file name to clipboardExpand all lines: src/node_contextify.cc
+25-7Lines 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()) {
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-vm-global-property-enumerator.js
+54-2Lines 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+
};
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-vm-ownkeys.js
+3-5Lines 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);
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-vm-ownpropertynames.js
+3-5Lines 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);
Collapse file

‎…own_issues/test-vm-ownpropertysymbols.js‎ ‎…t/parallel/test-vm-ownpropertysymbols.js‎test/known_issues/test-vm-ownpropertysymbols.js renamed to test/parallel/test-vm-ownpropertysymbols.js test/known_issues/test-vm-ownpropertysymbols.js renamed to test/parallel/test-vm-ownpropertysymbols.js

Copy file name to clipboardExpand all lines: test/parallel/test-vm-ownpropertysymbols.js
+3-5Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.