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 3424afa12c3967..70f1d0c9378140 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 5e69e17515dfbc..900c4151842ea7 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 99ffb46783b8cb..dca1dfa50a1619 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 66851fa5ea7405..002fa5572d3662 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 900c4151842ea7..5cf056dca44d2f 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 002fa5572d3662..7dafecceed06e0 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');