From b95399746c1954506dc2b2f621b2d43cf1cdf1a7 Mon Sep 17 00:00:00 2001 From: Alex Liu Date: Tue, 12 Mar 2019 20:08:20 -0700 Subject: [PATCH 1/3] fix: properly handle non-errors thrown in domains --- lib/server.js | 12 +++++++++--- test/server.test.js | 6 ++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/server.js b/lib/server.js index b3b1f32a7..43653c81a 100644 --- a/lib/server.js +++ b/lib/server.js @@ -1309,13 +1309,19 @@ Server.prototype._routeErrorResponse = function _routeErrorResponse( return; } - // Expose only knonw errors - if (_.isNumber(err.statusCode)) { + // only automatically send errors that are known (e.g., restify-errors) + if (err && _.isNumber(err.statusCode)) { res.send(err); return; } - res.send(new errors.InternalError(emitErr || err)); + var finalErr = emitErr || err; + if (!(finalErr instanceof Error)) { + // if user land domain threw a string or number or any other + // non-error object, handle it here as best we can. + finalErr = (finalErr && finalErr.toString()) || ''; + } + res.send(new errors.InternalError(finalErr)); }); }; diff --git a/test/server.test.js b/test/server.test.js index 4ddba6356..883a39e24 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -2505,9 +2505,10 @@ test('uncaughtException should handle thrown undefined literal', function(t) { } ); - CLIENT.get('/foo', function(err, _, res) { + CLIENT.get('/foo', function(err, _, res, data) { t.ok(err); t.equal(res.statusCode, 500); + t.equal(data.message, ''); t.end(); }); }); @@ -2535,8 +2536,9 @@ test('uncaughtException should handle thrown Number', function(t) { } ); - CLIENT.get('/foo', function(err, _, res) { + CLIENT.get('/foo', function(err, _, res, data) { t.ok(err); + t.equal(data.message, '1'); t.equal(res.statusCode, 500); t.end(); }); From 714a1b64254beed181ad8015fa176e8d07a0c434 Mon Sep 17 00:00:00 2001 From: Alex Liu Date: Fri, 15 Mar 2019 11:17:50 -0700 Subject: [PATCH 2/3] fix: pr comments --- lib/server.js | 17 +++--- test/server.test.js | 128 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 134 insertions(+), 11 deletions(-) diff --git a/lib/server.js b/lib/server.js index 43653c81a..c790d133c 100644 --- a/lib/server.js +++ b/lib/server.js @@ -1303,25 +1303,24 @@ Server.prototype._routeErrorResponse = function _routeErrorResponse( return; } - self._emitErrorEvents(req, res, null, err, function emitError(emitErr) { + self._emitErrorEvents(req, res, null, err, function emitError() { // Prevent double handling if (res._sent) { return; } // only automatically send errors that are known (e.g., restify-errors) - if (err && _.isNumber(err.statusCode)) { + if (err instanceof Error && _.isNumber(err.statusCode)) { res.send(err); return; } - var finalErr = emitErr || err; - if (!(finalErr instanceof Error)) { - // if user land domain threw a string or number or any other - // non-error object, handle it here as best we can. - finalErr = (finalErr && finalErr.toString()) || ''; - } - res.send(new errors.InternalError(finalErr)); + // if the thrown exception is not really an Error object, e.g., + // "throw 'foo';" + // try to do best effort here to pass on that value by casting it to a + // string. This should work even for falsy values like 0, false, null, + // or undefined. + res.send(new errors.InternalError(String(err))); }); }; diff --git a/test/server.test.js b/test/server.test.js index 883a39e24..970b0a6db 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -2482,6 +2482,37 @@ test('uncaughtException should not trigger named routeHandler', function(t) { }); }); +test('uncaughtException should handle thrown null', function(t) { + SERVER.get( + { + name: 'foo', + path: '/foo' + }, + function(req, res, next) { + throw null; //eslint-disable-line no-throw-literal + } + ); + + SERVER.get( + { + name: 'bar', + path: '/bar' + }, + function(req, res, next) { + // This code should not run, but we can test against the status code + res.send(200); + next(); + } + ); + + CLIENT.get('/foo', function(err, _, res, data) { + t.ok(err); + t.equal(res.statusCode, 500); + t.equal(data.message, 'null'); + t.end(); + }); +}); + test('uncaughtException should handle thrown undefined literal', function(t) { SERVER.get( { @@ -2508,12 +2539,43 @@ test('uncaughtException should handle thrown undefined literal', function(t) { CLIENT.get('/foo', function(err, _, res, data) { t.ok(err); t.equal(res.statusCode, 500); - t.equal(data.message, ''); + t.equal(data.message, 'undefined'); + t.end(); + }); +}); + +test('uncaughtException should handle thrown falsy number', function(t) { + SERVER.get( + { + name: 'foo', + path: '/foo' + }, + function(req, res, next) { + throw 0; //eslint-disable-line no-throw-literal + } + ); + + SERVER.get( + { + name: 'bar', + path: '/bar' + }, + function(req, res, next) { + // This code should not run, but we can test against the status code + res.send(200); + next(); + } + ); + + CLIENT.get('/foo', function(err, _, res, data) { + t.ok(err); + t.equal(data.message, '0'); + t.equal(res.statusCode, 500); t.end(); }); }); -test('uncaughtException should handle thrown Number', function(t) { +test('uncaughtException should handle thrown non falsy number', function(t) { SERVER.get( { name: 'foo', @@ -2544,6 +2606,68 @@ test('uncaughtException should handle thrown Number', function(t) { }); }); +test('uncaughtException should handle thrown boolean', function(t) { + SERVER.get( + { + name: 'foo', + path: '/foo' + }, + function(req, res, next) { + throw true; //eslint-disable-line no-throw-literal + } + ); + + SERVER.get( + { + name: 'bar', + path: '/bar' + }, + function(req, res, next) { + // This code should not run, but we can test against the status code + res.send(200); + next(); + } + ); + + CLIENT.get('/foo', function(err, _, res, data) { + t.ok(err); + t.equal(data.message, 'true'); + t.equal(res.statusCode, 500); + t.end(); + }); +}); + +test('uncaughtException should handle thrown falsy boolean', function(t) { + SERVER.get( + { + name: 'foo', + path: '/foo' + }, + function(req, res, next) { + throw false; //eslint-disable-line no-throw-literal + } + ); + + SERVER.get( + { + name: 'bar', + path: '/bar' + }, + function(req, res, next) { + // This code should not run, but we can test against the status code + res.send(200); + next(); + } + ); + + CLIENT.get('/foo', function(err, _, res, data) { + t.ok(err); + t.equal(data.message, 'false'); + t.equal(res.statusCode, 500); + t.end(); + }); +}); + test('should have proxy event handlers as instance', function(t) { var server = restify.createServer({ handleUpgrades: false From 83900d7a43afa963f7cd82d74b1b1408aa404c9c Mon Sep 17 00:00:00 2001 From: Alex Liu Date: Fri, 15 Mar 2019 18:21:58 -0700 Subject: [PATCH 3/3] fix: pr comments --- lib/server.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/server.js b/lib/server.js index c790d133c..c89dc790a 100644 --- a/lib/server.js +++ b/lib/server.js @@ -1390,10 +1390,9 @@ Server.prototype._emitErrorEvents = function _emitErrorEvents( } }, // eslint-disable-next-line handle-callback-err - function onResult(nullErr, results) { - // vasync will never return error here. callback with the original - // error to pass it on. - return cb(err); + function onResult(__, results) { + // vasync will never return error here since we throw them away. + return cb(); } ); };