From a25f56730e815794ff645a2a050da22343fa7d89 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Sat, 7 Apr 2018 14:22:29 +0200
Subject: [PATCH] assert: detect faulty throws usage
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

One of the biggest downsides to the `assert.throws` API is that it
does not check for the error message in case that is used as second
argument. It will instead be used in case no error is thrown.

This improves the situation by checking the actual error message
against the provided one and throws an error in case they are
identical. It is very unlikely that the user wants to use that error
message as information instead of checking against that message.

PR-URL: https://github.com/nodejs/node/pull/19867
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 doc/api/errors.md            |  8 ++++++++
 lib/assert.js                | 14 ++++++++++++++
 lib/internal/errors.js       |  1 +
 test/parallel/test-assert.js | 29 +++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+)

diff --git a/doc/api/errors.md b/doc/api/errors.md
index 3923780defa..49834414dec 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -586,6 +586,14 @@ found [here][online].
 <a id="nodejs-error-codes"></a>
 ## Node.js Error Codes
 
+<a id="ERR_AMBIGUOUS_ARGUMENT"></a>
+### ERR_AMBIGUOUS_ARGUMENT
+
+This is triggered by the `assert` module in case e.g.,
+`assert.throws(fn, message)` is used in a way that the message is the thrown
+error message. This is ambiguous because the message is not verifying the error
+message and will only be thrown in case no error is thrown.
+
 <a id="ERR_ARG_NOT_ITERABLE"></a>
 ### ERR_ARG_NOT_ITERABLE
 
diff --git a/lib/assert.js b/lib/assert.js
index 385ec332a6f..0a505d553a4 100644
--- a/lib/assert.js
+++ b/lib/assert.js
@@ -29,6 +29,7 @@ const {
   AssertionError,
   errorCache,
   codes: {
+    ERR_AMBIGUOUS_ARGUMENT,
     ERR_INVALID_ARG_TYPE
   }
 } = require('internal/errors');
@@ -470,6 +471,19 @@ function expectsError(stackStartFn, actual, error, message) {
                                      ['Object', 'Error', 'Function', 'RegExp'],
                                      error);
     }
+    if (typeof actual === 'object' && actual !== null) {
+      if (actual.message === error) {
+        throw new ERR_AMBIGUOUS_ARGUMENT(
+          'error/message',
+          `The error message "${actual.message}" is identical to the message.`
+        );
+      }
+    } else if (actual === error) {
+      throw new ERR_AMBIGUOUS_ARGUMENT(
+        'error/message',
+        `The error "${actual}" is identical to the message.`
+      );
+    }
     message = error;
     error = null;
   }
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index b190ae32bf6..c8be91796ec 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -640,6 +640,7 @@ module.exports = exports = {
 //
 // Note: Node.js specific errors must begin with the prefix ERR_
 
+E('ERR_AMBIGUOUS_ARGUMENT', 'The "%s" argument is ambiguous. %s', TypeError);
 E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError);
 E('ERR_ASSERTION', '%s', Error);
 E('ERR_ASYNC_CALLBACK', '%s must be a function', TypeError);
diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
index 4115167d4b4..c0615516049 100644
--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -836,3 +836,32 @@ common.expectsError(
     }
   );
 }
+
+assert.throws(
+  () => a.throws(
+    // eslint-disable-next-line no-throw-literal
+    () => { throw 'foo'; },
+    'foo'
+  ),
+  {
+    code: 'ERR_AMBIGUOUS_ARGUMENT',
+    message: 'The "error/message" argument is ambiguous. ' +
+             'The error "foo" is identical to the message.'
+  }
+);
+
+assert.throws(
+  () => a.throws(
+    () => { throw new TypeError('foo'); },
+    'foo'
+  ),
+  {
+    code: 'ERR_AMBIGUOUS_ARGUMENT',
+    message: 'The "error/message" argument is ambiguous. ' +
+             'The error message "foo" is identical to the message.'
+  }
+);
+
+// Should not throw.
+// eslint-disable-next-line no-restricted-syntax, no-throw-literal
+assert.throws(() => { throw null; }, 'foo');