Gracefully handle formatters throwing restify errors. (#1807)

Avoid uncaughtExceptions for expected & recoverable failures scenarios within formatters.
Add to JSON formatter failed serialization recovery.

Author: Adam Crabtree

lib/formatters/json.js

@@ -2,6 +2,8 @@
 
 'use strict';
 
+var errors = require('restify-errors');
+
 ///--- Exports
 
 /**
@@ -16,7 +18,18 @@
  * @returns  {String} data
  */
 function formatJSON(req, res, body) {
-    var data = body ? JSON.stringify(body) : 'null';
+    var data = 'null';
+    if (body) {
+        try {
+            data = JSON.stringify(body);
+        } catch (e) {
+            throw new errors.InternalServerError(
+                { cause: e, info: { formatter: 'json' } },
+                'could not format response body'
+            );
+        }
+    }
+
     // Setting the content-length header is not a formatting feature and should
     // be separated into another module
     res.setHeader('Content-Length', Buffer.byteLength(data));

lib/response.js

@@ -494,6 +494,8 @@ function patch(Response) {
                 );
             }
 
+            var formatterType = type;
+
             if (self._charSet) {
                 type = type + '; charset=' + self._charSet;
             }
@@ -506,7 +508,26 @@ function patch(Response) {
                 // Finally, invoke the formatter and flush the request with it's
                 // results
 
-                return flush(self, formatter(self.req, self, body));
+                var formattedBody;
+                try {
+                    formattedBody = formatter(self.req, self, body);
+                } catch (e) {
+                    if (
+                        e instanceof errors.RestError ||
+                        e instanceof errors.HttpError
+                    ) {
+                        var res = formatterError(
+                            self,
+                            e,
+                            'error in formatter (' +
+                                formatterType +
+                                ') formatting response body'
+                        );
+                        return res;
+                    }
+                    throw e;
+                }
+                return flush(self, formattedBody);
             }
         }
 
@@ -873,22 +894,27 @@ function flush(res, body) {
  * @function formatterError
  * @param {Response} res - response
  * @param {Error} err - error
+ * @param {String} [msg] - custom log message
  * @returns {Response} response
  */
-function formatterError(res, err) {
+function formatterError(res, err, msg) {
     // If the user provided a non-success error code, we don't want to
     // mess with it since their error is probably more important than
     // our inability to format their message.
     if (res.statusCode >= 200 && res.statusCode < 300) {
         res.statusCode = err.statusCode;
     }
 
+    if (typeof msg !== 'string') {
+        msg = 'error retrieving formatter';
+    }
+
     res.log.warn(
         {
             req: res.req,
             err: err
         },
-        'error retrieving formatter'
+        msg
     );
 
     return flush(res);

package.json

@@ -141,6 +141,7 @@
     "proxyquire": "^1.8.0",
     "restify-clients": "^2.6.6",
     "rimraf": "^2.6.3",
+    "sinon": "^7.5.0",
     "validator": "^7.2.0",
     "watershed": "^0.4.0"
   },

test/formatter.test.js

@@ -4,8 +4,11 @@
 /* eslint-disable func-names */
 
 var restifyClients = require('restify-clients');
+var errors = require('restify-errors');
+var sinon = require('sinon');
 
 var restify = require('../lib');
+var jsonFormatter = require('../lib/formatters/json');
 
 if (require.cache[__dirname + '/lib/helper.js']) {
     delete require.cache[__dirname + '/lib/helper.js'];
@@ -36,6 +39,9 @@ before(function(callback) {
                     // this is a bad formatter, on purpose.
                     return x.toString(); // eslint-disable-line no-undef
                 },
+                'text/syncerror_expected': function(req, res, body) {
+                    throw new errors.InternalServerError('Errors happen');
+                },
                 'application/foo; q=0.9': function(req, res, body) {
                     return 'foo!';
                 },
@@ -108,7 +114,7 @@ test('GH-845: sync formatter', function(t) {
 });
 
 test('GH-845: sync formatter should blow up', function(t) {
-    SERVER.on('uncaughtException', function(req, res, route, err) {
+    SERVER.once('uncaughtException', function(req, res, route, err) {
         t.ok(err);
         t.equal(err.name, 'ReferenceError');
         t.equal(err.message, 'x is not defined');
@@ -130,6 +136,29 @@ test('GH-845: sync formatter should blow up', function(t) {
     );
 });
 
+test('sync formatter should handle expected errors gracefully', function(t) {
+    SERVER.once('uncaughtException', function(req, res, route, err) {
+        throw new Error('Should not reach');
+    });
+
+    CLIENT.get(
+        {
+            path: '/sync',
+            headers: {
+                accept: 'text/syncerror_expected'
+            }
+        },
+        function(err, req, res, data) {
+            t.ok(err);
+            t.ok(req);
+            t.ok(res);
+            t.equal(res.statusCode, 500);
+            SERVER.removeAllListeners('uncaughtException');
+            t.end();
+        }
+    );
+});
+
 test('q-val priority', function(t) {
     var opts = {
         path: '/sync',
@@ -228,3 +257,43 @@ test('default jsonp formatter should escape line and paragraph separators', func
         t.end();
     });
 });
+
+// eslint-disable-next-line
+test('default json formatter should wrap & throw InternalServer error on unserializable bodies', function(t) {
+    t.expect(2);
+
+    sinon.spy(JSON, 'stringify');
+
+    SERVER.once('uncaughtException', function(req, res, route, err) {
+        console.log(err.stack); // For convenience
+        throw new Error('Should not reach');
+    });
+
+    var opts = {
+        path: '/badJSON',
+        name: 'badJSON'
+    };
+
+    SERVER.get(opts, function(req, res, next) {
+        var body = {};
+        // Add unserializable circular reference
+        body.body = body;
+
+        try {
+            jsonFormatter(req, res, body);
+            throw new Error('Should not reach');
+        } catch (e) {
+            t.ok(e instanceof errors.InternalServerError);
+            t.ok(JSON.stringify.threw(e.cause()));
+        }
+
+        res.send();
+    });
+
+    CLIENT.get('/badJSON', function(err, req, res, data) {
+        SERVER.rm('badJSON');
+        SERVER.removeAllListeners('uncaughtException');
+        JSON.stringify.restore();
+        t.end();
+    });
+});