assert,util: harden comparison

The former algorithm used checks which were unsafe. Most of these
have been replaced with alternatives that can not be manipulated or
fooled that easily.

PR-URL: nodejs#24831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

lib/internal/util/comparisons.js

@@ -7,7 +7,14 @@ const {
   isDate,
   isMap,
   isRegExp,
-  isSet
+  isSet,
+  isNativeError,
+  isBoxedPrimitive,
+  isNumberObject,
+  isStringObject,
+  isBooleanObject,
+  isBigIntObject,
+  isSymbolObject
 } = internalBinding('types');
 const {
   getOwnNonIndexProperties,
@@ -33,6 +40,13 @@ const kIsMap = 3;
 const objectToString = uncurryThis(Object.prototype.toString);
 const hasOwnProperty = uncurryThis(Object.prototype.hasOwnProperty);
 const propertyIsEnumerable = uncurryThis(Object.prototype.propertyIsEnumerable);
+const dateGetTime = uncurryThis(Date.prototype.getTime);
+
+const bigIntValueOf = uncurryThis(BigInt.prototype.valueOf);
+const booleanValueOf = uncurryThis(Boolean.prototype.valueOf);
+const numberValueOf = uncurryThis(Number.prototype.valueOf);
+const symbolValueOf = uncurryThis(Symbol.prototype.valueOf);
+const stringValueOf = uncurryThis(String.prototype.valueOf);
 
 const objectKeys = Object.keys;
 const getPrototypeOf = Object.getPrototypeOf;
@@ -82,6 +96,24 @@ function isObjectOrArrayTag(tag) {
   return tag === '[object Array]' || tag === '[object Object]';
 }
 
+function isEqualBoxedPrimitive(val1, val2) {
+  if (isNumberObject(val1)) {
+    return isNumberObject(val2) &&
+           objectIs(numberValueOf(val1), numberValueOf(val2));
+  }
+  if (isStringObject(val1)) {
+    return isStringObject(val2) && stringValueOf(val1) === stringValueOf(val2);
+  }
+  if (isBooleanObject(val1)) {
+    return isBooleanObject(val2) &&
+           booleanValueOf(val1) === booleanValueOf(val2);
+  }
+  if (isBigIntObject(val1)) {
+    return isBigIntObject(val2) && bigIntValueOf(val1) === bigIntValueOf(val2);
+  }
+  return isSymbolObject(val2) && symbolValueOf(val1) === symbolValueOf(val2);
+}
+
 // Notes: Type tags are historical [[Class]] properties that can be set by
 // FunctionTemplate::SetClassName() in C++ or Symbol.toStringTag in JS
 // and retrieved using Object.prototype.toString.call(obj) in JS
@@ -117,7 +149,7 @@ function strictDeepEqual(val1, val2, memos) {
   if (getPrototypeOf(val1) !== getPrototypeOf(val2)) {
     return false;
   }
-  if (val1Tag === '[object Array]') {
+  if (Array.isArray(val1)) {
     // Check for sparse arrays and general fast path
     if (val1.length !== val2.length) {
       return false;
@@ -133,15 +165,14 @@ function strictDeepEqual(val1, val2, memos) {
     return keyCheck(val1, val2, kStrict, memos, kNoIterator);
   }
   if (isDate(val1)) {
-    // TODO: Make these safe.
-    if (val1.getTime() !== val2.getTime()) {
+    if (dateGetTime(val1) !== dateGetTime(val2)) {
       return false;
     }
   } else if (isRegExp(val1)) {
     if (!areSimilarRegExps(val1, val2)) {
       return false;
     }
-  } else if (val1Tag === '[object Error]') {
+  } else if (isNativeError(val1) || val1 instanceof Error) {
     // Do not compare the stack as it might differ even though the error itself
     // is otherwise identical. The non-enumerable name should be identical as
     // the prototype is also identical. Otherwise this is caught later on.
@@ -175,14 +206,8 @@ function strictDeepEqual(val1, val2, memos) {
     if (!areEqualArrayBuffers(val1, val2)) {
       return false;
     }
-  // TODO: Make the valueOf checks safe.
-  } else if (typeof val1.valueOf === 'function') {
-    const val1Value = val1.valueOf();
-    if (val1Value !== val1 &&
-        (typeof val2.valueOf !== 'function' ||
-          !innerDeepEqual(val1Value, val2.valueOf(), kStrict))) {
-      return false;
-    }
+  } else if (isBoxedPrimitive(val1) && !isEqualBoxedPrimitive(val1, val2)) {
+    return false;
   }
   return keyCheck(val1, val2, kStrict, memos, kNoIterator);
 }

test/parallel/test-assert-deep.js

@@ -985,3 +985,67 @@ assert.throws(
             '  ]'
   }
 );
+
+// Verify that manipulating the `getTime()` function has no impact on the time
+// verification.
+{
+  const a = new Date('2000');
+  const b = new Date('2000');
+  Object.defineProperty(a, 'getTime', {
+    value: () => 5
+  });
+  assert.deepStrictEqual(a, b);
+}
+
+// Verify that extra keys will be tested for when using fake arrays.
+{
+  const a = {
+    0: 1,
+    1: 1,
+    2: 'broken'
+  };
+  Object.setPrototypeOf(a, Object.getPrototypeOf([]));
+  Object.defineProperty(a, Symbol.toStringTag, {
+    value: 'Array',
+  });
+  Object.defineProperty(a, 'length', {
+    value: 2
+  });
+  assert.notDeepStrictEqual(a, [1, 1]);
+}
+
+// Verify that changed tags will still check for the error message.
+{
+  const err = new Error('foo');
+  err[Symbol.toStringTag] = 'Foobar';
+  const err2 = new Error('bar');
+  err2[Symbol.toStringTag] = 'Foobar';
+  assertNotDeepOrStrict(err, err2, AssertionError);
+}
+
+// Check for non-native errors.
+{
+  const source = new Error('abc');
+  const err = Object.create(
+    Object.getPrototypeOf(source), Object.getOwnPropertyDescriptors(source));
+  Object.defineProperty(err, 'message', { value: 'foo' });
+  const err2 = Object.create(
+    Object.getPrototypeOf(source), Object.getOwnPropertyDescriptors(source));
+  Object.defineProperty(err2, 'message', { value: 'bar' });
+  err[Symbol.toStringTag] = 'Foo';
+  err2[Symbol.toStringTag] = 'Foo';
+  assert.notDeepStrictEqual(err, err2);
+}
+
+// Verify that `valueOf` is not called for boxed primitives.
+{
+  const a = new Number(5);
+  const b = new Number(5);
+  Object.defineProperty(a, 'valueOf', {
+    value: () => { throw new Error('failed'); }
+  });
+  Object.defineProperty(b, 'valueOf', {
+    value: () => { throw new Error('failed'); }
+  });
+  assertDeepAndStrictEqual(a, b);
+}