`assert.deepEqual` and `assert.deepStrictEqual` incorrectly compare Proxied arrays when using `ownKeys` trap

[itaylor]

Version

v17.4.0

Platform

Darwin macbook-pro-5.lan 21.2.0 Darwin Kernel Version 21.2.0: Sun Nov 28 20:28:54 PST 2021; root:xnu-8019.61.5~1/RELEASE_X86_64 x86_64

Subsystem

assert

What steps will reproduce the bug?

assert.deepEqual(new Proxy(['foo'], {}), ['foo']); // works properly
assert.deepEqual(new Proxy(['foo'], { ownKeys: (target) => Reflect.ownKeys(target) }), ['foo']) // throws

assert.deepStrictEqual(new Proxy(['foo'], {}), ['foo']); // works properly
assert.deepStrictEqual(new Proxy(['foo'], { ownKeys: (target) => Reflect.ownKeys(target) }), ['foo']) // throws

In the above, Reflect.ownKeys(target) should be the same as not setting an ownKeys trap, but it isn't. The trap works fine elsewhere

Indeed,

Reflect.ownKeys(['foo'])  // [ '0', 'length' ]
assert.deepEqual(new Proxy(['foo'], { ownKeys: (target) => ['0', 'length'] }), ['foo'])  // throws

How often does it reproduce? Is there a required condition?

Every time

What is the expected behavior?

Using a ownKeys trap shouldn't cause comparison errors.

The ownKeys trap seems to be working fine elsewhere. I traced the code to:

node/lib/internal/util/comparisons.js

Line 179 in dab8ab2

const keys1 = getOwnNonIndexProperties(val1, filter);

Where the GetOwnNonIndexProperties function is called. Perhaps it is incorrectly filtering or the filtering is broken for proxies somehow?

What do you see instead?

Comparison errors from assert.deepEquals and assert.deepStrictEquals when the Arrays compared have the same content, but are using Proxies with ownKeys traps.

Additional information

I tried this on node 14, 16, and 17, with the same results.

[Trott]

@nodejs/assert

[benjamingr]

I don't think we make any guarantees on equality of proxies to non-proxies of the same object in assert so this is more of a feature request than a bug I think?

[benjamingr]

Hmm

> require('internal/test/binding').internalBinding('util').getOwnNonIndexProperties(new Proxy(['foo'], { ownKeys: (target) => Reflect.ownKeys(target) }), 0)
[ 0, 'length' ]

So the issue is that 0 is considered an index property.

So the issue is that

object->GetPropertyNames(
        context, KeyCollectionMode::kOwnOnly,
        filter,
        IndexFilter::kSkipIndices)
          .ToLocal(&properties)

Does not ignore index properties with a proxy trap?

[ljharb]

Reflect.ownKeys doesn’t just take one argument, and when used in proxy traps, you’re supposed to use the other arguments. What happens if your handler is { ownKeys: Reflect.ownKeys }?

[targos]

@ljharb what are the other arguments?

[ljharb]

The last one is “target”, ie, the receiver. https://tc39.es/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-ownpropertykeys I’m on mobile rn so i haven’t checked to see if it makes a difference.

[benjamingr]

@ljharb it wouldn't matter in this case:

> require('internal/test/binding').internalBinding('util').getOwnNonIndexProperties(new Proxy(['foo'], { ownKeys: Reflect.ownKeys }), 0)
[ 0, 'length' ]
> (node:70102) internal/test/binding: These APIs are for internal testing only. Do not use them.
(Use `node --trace-warnings ...` to show where the warning was created)

This is a "bug" either in v8 or how we use v8 which returns index properties from the call above. (I saw bug because I don't believe we ever discussed what proxy behavior should be)

[itaylor]

@ljharb, The problem still occurs with passing {ownKeys: Reflect.ownKeys}

assert.deepEqual(new Proxy(['foo'], { ownKeys: Reflect.ownKeys }), ['foo']) // throws

I see in the spec that you're right, it should be passing a target argument, but that argument isn't listed in the MDN documentation of Reflect.ownKeys.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/ownKeys

[ljharb]

Proxies are an advanced feature only intended to be used in concert with a membranes implementation, so it’s not surprising that MDN’s docs are incomplete there.

[benjamingr]

A fix can be something like this:

diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js
index c126bd6346..6a529c100f 100644
--- a/lib/internal/util/comparisons.js
+++ b/lib/internal/util/comparisons.js
@@ -36,6 +36,7 @@ const {
   isRegExp,
   isSet,
   isNativeError,
+  isProxy,
   isBoxedPrimitive,
   isNumberObject,
   isStringObject,
@@ -68,6 +69,14 @@ function areSimilarRegExps(a, b) {
          a.lastIndex === b.lastIndex;
 }
 
+function getOwnNonIndexPropertiesHandleProxies(target, filter) {
+  const res = getOwnNonIndexProperties(target, filter);
+  if (isProxy(target)) {
+    return res.filter((x) => typeof x !== 'number');
+  }
+  return res;
+}
+
 function areSimilarFloatArrays(a, b) {
   if (a.byteLength !== b.byteLength) {
     return false;
@@ -176,8 +185,8 @@ function innerDeepEqual(val1, val2, strict, memos) {
       return false;
     }
     const filter = strict ? ONLY_ENUMERABLE : ONLY_ENUMERABLE | SKIP_SYMBOLS;
-    const keys1 = getOwnNonIndexProperties(val1, filter);
-    const keys2 = getOwnNonIndexProperties(val2, filter);
+    const keys1 = getOwnNonIndexPropertiesHandleProxies(val1, filter);
+    const keys2 = getOwnNonIndexPropertiesHandleProxies(val2, filter);
     if (keys1.length !== keys2.length) {
       return false;
     }
@@ -217,8 +226,8 @@ function innerDeepEqual(val1, val2, strict, memos) {
     // only contain numeric keys, we don't need to exam further than checking
     // the symbols.
     const filter = strict ? ONLY_ENUMERABLE : ONLY_ENUMERABLE | SKIP_SYMBOLS;
-    const keys1 = getOwnNonIndexProperties(val1, filter);
-    const keys2 = getOwnNonIndexProperties(val2, filter);
+    const keys1 = getOwnNonIndexPropertiesHandleProxies(val1, filter);
+    const keys2 = getOwnNonIndexPropertiesHandleProxies(val2, filter);
     if (keys1.length !== keys2.length) {
       return false;
     }

We should also probably file a v8 bug?

[ljharb]

Wouldn’t that fix potentially interfere with proxies of arrays, or proxies of objects with numeric string keys?

[benjamingr]

@ljharb

Wouldn’t that fix potentially interfere with proxies of arrays, or proxies of objects with numeric string keys?

It would as in today they don't work and following this patch they do and return the same keys as the non-proxy version

[devsnek]

0 is an index regardless of what kind of object it is pulled off of. If this is coming from IndexFilter::kSkipIndices I would suggest fixing it there.