util: improve internal isError() validation

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>

lib/internal/util.js

@@ -12,6 +12,9 @@ const {
  arrow_message_private_symbol: kArrowMessagePrivateSymbolIndex,
  decorated_private_symbol: kDecoratedPrivateSymbolIndex
 } = internalBinding('util');
+const {
+ isNativeError
+} = internalBinding('types');
 
 const { errmap } = internalBinding('uv');
 
@@ -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) {

lib/util.js

@@ -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;
 
@@ -443,7 +449,9 @@ module.exports = exports = {
  isRegExp,
  isObject,
  isDate,
- isError,
+ isError(e) {
+ return objectToString(e) === '[object Error]' || e instanceof Error;
+ },
  isFunction,
  isPrimitive,
  log,

test/parallel/test-internal-util-helpers.js

@@ -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));
+}