From 807c108be8056defa40702bc4fc69c11656fec3b Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Sun, 18 Nov 2018 04:19:16 +0100
Subject: [PATCH] util: improve internal `isError()` validation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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: https://github.com/nodejs/node/pull/24746
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
---
 lib/internal/util.js                        |  8 ++++-
 lib/util.js                                 | 12 +++++--
 test/parallel/test-internal-util-helpers.js | 37 +++++++++++++++++++++
 3 files changed, 54 insertions(+), 3 deletions(-)
 create mode 100644 test/parallel/test-internal-util-helpers.js

diff --git a/lib/internal/util.js b/lib/internal/util.js
index 3524b9e1d62112..23c201da371099 100644
--- a/lib/internal/util.js
+++ b/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) {
diff --git a/lib/util.js b/lib/util.js
index 1e11440ff3fd42..cd96db044a834b 100644
--- a/lib/util.js
+++ b/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;
 
@@ -429,7 +435,9 @@ module.exports = exports = {
   isRegExp,
   isObject,
   isDate,
-  isError,
+  isError(e) {
+    return objectToString(e) === '[object Error]' || e instanceof Error;
+  },
   isFunction,
   isPrimitive,
   log,
diff --git a/test/parallel/test-internal-util-helpers.js b/test/parallel/test-internal-util-helpers.js
new file mode 100644
index 00000000000000..bf60cff9bda4be
--- /dev/null
+++ b/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));
+}