From ee69806a338add1ebfef7eaad92a13273826c98e Mon Sep 17 00:00:00 2001
From: Michael Ridgway <mcridgway@gmail.com>
Date: Mon, 29 Oct 2018 17:42:35 -0700
Subject: [PATCH] Fix: remove invalid triggering of uncaughtException handler
 (#1710)

The previous (prior to 7.x) functionality for uncaught exceptions only triggered the `uncaughtException` handler if it was truly an uncaught thrown error. Right now in Restify 7, if you turn on `handleUncaughtExceptions` and add a listener for the event, all errors from `next(err)` and internal errors like `MethodNotAllowed` will trigger the `uncaughtException` handler.
---
 lib/server.js       | 22 +++++++++++++++++-----
 test/server.test.js | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/lib/server.js b/lib/server.js
index de17a128e..e64bdd19e 100644
--- a/lib/server.js
+++ b/lib/server.js
@@ -816,7 +816,7 @@ Server.prototype._onRequest = function _onRequest(req, res) {
         handlerDomain.add(req);
         handlerDomain.add(res);
         handlerDomain.on('error', function onError(err) {
-            self._onHandlerError(err, req, res);
+            self._onHandlerError(err, req, res, true);
         });
         handlerDomain.run(function run() {
             self._runPre(req, res);
@@ -1065,9 +1065,15 @@ Server.prototype._onHandlerStop = function _onHandlerStop(req, res) {
  * @param  {Error|String|undefined} err - router handler error or route name
  * @param  {Request} req - request
  * @param  {Response} res - response
+ * @param  {boolean} isUncaught - whether the error is uncaught
  * @returns {undefined} no return value
  */
-Server.prototype._onHandlerError = function _onHandlerError(err, req, res) {
+Server.prototype._onHandlerError = function _onHandlerError(
+    err,
+    req,
+    res,
+    isUncaught
+) {
     var self = this;
 
     // Call route by name
@@ -1098,7 +1104,7 @@ Server.prototype._onHandlerError = function _onHandlerError(err, req, res) {
     res.err = res.err || err;
 
     // Error happened in router handlers
-    self._routeErrorResponse(req, res, err);
+    self._routeErrorResponse(req, res, err, isUncaught);
 };
 
 /**
@@ -1256,16 +1262,22 @@ Server.prototype._finishReqResCycle = function _finishReqResCycle(
  * @param    {Request}     req -    the request object
  * @param    {Response}    res -    the response object
  * @param    {Error}       err -    error
+ * @param    {boolean}     isUncaught - whether the error is uncaught
  * @returns  {undefined} no return value
  */
 Server.prototype._routeErrorResponse = function _routeErrorResponse(
     req,
     res,
-    err
+    err,
+    isUncaught
 ) {
     var self = this;
 
-    if (self.listenerCount('uncaughtException') > 1) {
+    if (
+        isUncaught &&
+        self.handleUncaughtExceptions &&
+        self.listenerCount('uncaughtException') > 1
+    ) {
         self.emit('uncaughtException', req, res, req.route, err);
         return;
     }
diff --git a/test/server.test.js b/test/server.test.js
index f5bb4b60f..f9b20d2ad 100644
--- a/test/server.test.js
+++ b/test/server.test.js
@@ -674,6 +674,44 @@ test('GH-77 uncaughtException (default behavior)', function(t) {
     });
 });
 
+// eslint-disable-next-line
+test('handleUncaughtExceptions should not call handler for internal errors', function(t) {
+    SERVER.get('/', function(req, res, next) {
+        // This route is not used for the test but at least one route needs to
+        // be registered to Restify in order for routing logic to be run
+        assert.fail('should not run');
+    });
+
+    SERVER.on('uncaughtException', function throwError(err) {
+        t.ifError(err);
+        t.end();
+    });
+
+    CLIENT.head('/', function(err, _, res) {
+        t.ok(err);
+        t.equal(res.statusCode, 405);
+        t.end();
+    });
+});
+
+// eslint-disable-next-line
+test('handleUncaughtExceptions should not call handler for next(new Error())', function(t) {
+    SERVER.get('/', function(req, res, next) {
+        next(new Error('I am not fatal'));
+    });
+
+    SERVER.on('uncaughtException', function throwError(err) {
+        t.ifError(err);
+        t.end();
+    });
+
+    CLIENT.get('/', function(err, _, res) {
+        t.ok(err);
+        t.equal(res.statusCode, 500);
+        t.end();
+    });
+});
+
 test('GH-77 uncaughtException (with custom handler)', function(t) {
     SERVER.on('uncaughtException', function(req, res, route, err) {
         res.send(204);