From 566f6dd22947ae928156002e1fcb4b11013617b1 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Sat, 7 Apr 2018 14:22:29 +0200
Subject: [PATCH 1/2] assert: detect faulty throws usage

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.
---
 doc/api/errors.md            |  8 ++++++++
 lib/assert.js                | 14 ++++++++++++++
 lib/internal/errors.js       |  1 +
 test/parallel/test-assert.js | 29 +++++++++++++++++++++++++++--
 4 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/doc/api/errors.md b/doc/api/errors.md
index 3424afa12c39..70f1d0c93781 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 5e69e17515df..900c4151842e 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') {
+      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 99ffb46783b8..dca1dfa50a16 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 66851fa5ea74..002fa5572d36 100644
--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -833,13 +833,38 @@ common.expectsError(
 
   // eslint-disable-next-line no-throw-literal
   assert.throws(() => { throw undefined; }, /undefined/);
-  common.expectsError(
+  assert.throws(
     // eslint-disable-next-line no-throw-literal
     () => a.doesNotThrow(() => { throw undefined; }),
     {
-      type: assert.AssertionError,
+      name: 'AssertionError [ERR_ASSERTION]',
       code: 'ERR_ASSERTION',
       message: 'Got unwanted exception.\nundefined'
     }
   );
 }
+
+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.'
+  }
+);

From 0f0b79e4deb4f72bab046aa463bd2f1140da2303 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Tue, 10 Apr 2018 18:30:12 +0200
Subject: [PATCH 2/2] fixup: fix edge case

---
 lib/assert.js                | 2 +-
 test/parallel/test-assert.js | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/assert.js b/lib/assert.js
index 900c4151842e..5cf056dca44d 100644
--- a/lib/assert.js
+++ b/lib/assert.js
@@ -471,7 +471,7 @@ function expectsError(stackStartFn, actual, error, message) {
                                      ['Object', 'Error', 'Function', 'RegExp'],
                                      error);
     }
-    if (typeof actual === 'object') {
+    if (typeof actual === 'object' && actual !== null) {
       if (actual.message === error) {
         throw new ERR_AMBIGUOUS_ARGUMENT(
           'error/message',
diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
index 002fa5572d36..7dafecceed06 100644
--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -868,3 +868,7 @@ assert.throws(
              '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');