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: nodejs#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));
+}