From bf4faf3ffc69e364428dde9bd70b0e8138a93a6e Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 4 Dec 2018 00:02:51 +0100 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/24831 Reviewed-By: Anna Henningsen Reviewed-By: Ujjwal Sharma --- lib/internal/util/comparisons.js | 51 +++++++++++++++++------- test/parallel/test-assert-deep.js | 64 +++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 13 deletions(-) diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js index 905946fdee3f44..1d959f843bcbd3 100644 --- a/lib/internal/util/comparisons.js +++ b/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); } diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index 0abad5788e7dd1..c1e8c2f246663b 100644 --- a/test/parallel/test-assert-deep.js +++ b/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); +}