Skip to content

Commit

Permalink
util: improve internal isError() validation
Browse files Browse the repository at this point in the history
The current internal isError function checked the toString value
instead of using the more precise `util.types.isNativeError()` check.
The `instanceof` check is not removed due to possible errors that
are not native but still an instance of Error.

PR-URL: #24746
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
  • Loading branch information
BridgeAR committed Dec 5, 2018
1 parent e13571c commit 807c108
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 3 deletions.
8 changes: 7 additions & 1 deletion lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ const {
arrow_message_private_symbol: kArrowMessagePrivateSymbolIndex,
decorated_private_symbol: kDecoratedPrivateSymbolIndex
} = internalBinding('util');
const {
isNativeError
} = internalBinding('types');

const { errmap } = internalBinding('uv');

Expand All @@ -26,7 +29,10 @@ function removeColors(str) {
}

function isError(e) {
return objectToString(e) === '[object Error]' || e instanceof Error;
// An error could be an instance of Error while not being a native error
// or could be from a different realm and not be instance of Error but still
// be a native error.
return isNativeError(e) || e instanceof Error;
}

function objectToString(o) {
Expand Down
12 changes: 10 additions & 2 deletions lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,16 @@ const {
const {
deprecate,
getSystemErrorName: internalErrorName,
isError,
promisify,
} = require('internal/util');

const ReflectApply = Reflect.apply;

function uncurryThis(func) {
return (thisArg, ...args) => ReflectApply(func, thisArg, args);
}
const objectToString = uncurryThis(Object.prototype.toString);

let CIRCULAR_ERROR_MESSAGE;
let internalDeepEqual;

Expand Down Expand Up @@ -429,7 +435,9 @@ module.exports = exports = {
isRegExp,
isObject,
isDate,
isError,
isError(e) {
return objectToString(e) === '[object Error]' || e instanceof Error;
},
isFunction,
isPrimitive,
log,
Expand Down
37 changes: 37 additions & 0 deletions test/parallel/test-internal-util-helpers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Flags: --expose-internals
'use strict';

require('../common');
const assert = require('assert');
const { types } = require('util');
const { isError } = require('internal/util');
const vm = require('vm');

// Special cased errors. Test the internal function which is used in
// `util.inspect()`, the `repl` and maybe more. This verifies that errors from
// different realms, and non native instances of error are properly detected as
// error while definitely false ones are not detected. This is different than
// the public `util.isError()` function which falsy detects the fake errors as
// actual errors.
{
const fake = { [Symbol.toStringTag]: 'Error' };
assert(!types.isNativeError(fake));
assert(!(fake instanceof Error));
assert(!isError(fake));

const err = new Error('test');
const newErr = Object.create(
Object.getPrototypeOf(err),
Object.getOwnPropertyDescriptors(err));
Object.defineProperty(err, 'message', { value: err.message });
assert(types.isNativeError(err));
assert(!types.isNativeError(newErr));
assert(newErr instanceof Error);
assert(isError(newErr));

const context = vm.createContext({});
const differentRealmErr = vm.runInContext('new Error()', context);
assert(types.isNativeError(differentRealmErr));
assert(!(differentRealmErr instanceof Error));
assert(isError(differentRealmErr));
}

0 comments on commit 807c108

Please sign in to comment.