Skip to content

Suppress error handler stack traces in production#564#1502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
digitalsadhu wants to merge 12 commits into from
Closed

Suppress error handler stack traces in production#564 #1502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3baba47
Create wrapped express errorhandler
digitalsadhu Jul 5, 2015
791c7ac
Load new error handler module
digitalsadhu Jul 5, 2015
32f758d
Remove errorhandler title setting
digitalsadhu Jul 5, 2015
5bfed4d
Replace env with options configuration
digitalsadhu Jul 7, 2015
7eba03c
Allow middleware to be called without param
digitalsadhu Jul 8, 2015
2274705
Add test dependencies
digitalsadhu Jul 9, 2015
06cad82
Add basic tests for error handler
digitalsadhu Jul 9, 2015
07666d4
Fix issues based on PR comments from @bajtos
digitalsadhu Sep 2, 2015
b700801
Move errorHandler dep from devDependencies
digitalsadhu Sep 2, 2015
826dc98
Pass options to errorHandler module
digitalsadhu Sep 2, 2015
c60ff5e
Turn off console logging in tests
digitalsadhu Sep 2, 2015
8527f52
Add additional test to cover options passing
digitalsadhu Sep 2, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions lib/loopback.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -159,6 +159,9 @@ if (loopback.isServer) {

loopback.urlNotFound = loopback['url-not-found'];
delete loopback['url-not-found'];

loopback.errorHandler = loopback['error-handler'];
delete loopback['error-handler'];

@bajtosbajtosJul 10, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: group by property name, not by operations.

loopback.urlNotFound = loopback['url-not-found'];
delete loopback['url-not-found'];
loopback.errorHandler = loopback['error-handler'];
delete loopback['error-handler'];

In this particular case, I'd personally keep loopback[error-handler] so that it can be loaded in middleware.json via loopback#error-handler. Not a big deal though.

}

/*
Expand All@@ -180,12 +183,6 @@ if (loopback.isServer) {
loopback.faviconFile = path.resolve(__dirname, '../favicon.ico');
}

/*!
* Error handler title
*/

loopback.errorHandler.title = 'Loopback';

/**
* Add a remote method to a model.
* @param {Function} fn
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -40,6 +40,7 @@
"debug": "^2.1.2",
"depd": "^1.0.0",
"ejs": "^2.3.1",
"errorhandler": "^1.3.4",
"express": "^4.12.2",
"inflection": "^1.6.0",
"loopback-connector-remote": "^1.0.3",
Expand All@@ -60,7 +61,6 @@
"browserify": "^10.0.0",
"chai": "^2.1.1",
"cookie-parser": "^1.3.4",
"errorhandler": "^1.3.4",
"es5-shim": "^4.1.0",
"grunt": "^0.4.5",
"grunt-browserify": "^3.5.0",
Expand Down
15 changes: 15 additions & 0 deletions server/middleware/error-handler.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
var expressErrorHandler = require('errorhandler');

@bajtosbajtosSep 2, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing to address. errorhandler is a devDependency, thus this line will thrown when loopback is installed in a project that does not have errorhandler in project's dependencies.

Please make errorhandler a regular dependency in loopback's package.json

expressErrorHandler.title = 'Loopback';

module.exports = errorHandler;

function errorHandler(options) {
if (!options || options.includeStack !== false) {
return expressErrorHandler(options);
} else {
return function errorHandler(err, req, res, next) {
delete err.stack;
return expressErrorHandler(options)(err, req, res, next);

@bajtosbajtosSep 3, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a new expressErrorHandler(options) for each request is suboptimal, it's better to call it only once. I'll fix this myself to speed things up.

};
}
}
55 changes: 55 additions & 0 deletions test/error-handler.test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
var loopback = require('../');
var app;
var assert = require('assert');

@bajtosbajtosJul 10, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't mock req/res objects, express maintainers are discouraging that. Setup a full loopback app and make a real request using supertest instead.

varapp=loopback();app.use(loopback.urlNotFound());app.use(loopback.errorHandler(/* opts */));request(app).get('/url-does-not-exist')// verify that the response does or does not contain stack

var request = require('supertest');

describe('loopback.errorHandler(options)', function() {

it('should return default middleware when options object is not present', function(done) {

//arrange
var app = loopback();
app.use(loopback.urlNotFound());
app.use(loopback.errorHandler({ log: false }));

//act/assert
request(app)
.get('/url-does-not-exist')
.end(function(err, res) {
assert.ok(res.error.text.match(/<ul id="stacktrace"><li> &nbsp; &nbsp;at raiseUrlNotFoundError/));
done();
});
});

it('should delete stack when options.includeStack is false', function(done) {

//arrange
var app = loopback();
app.use(loopback.urlNotFound());
app.use(loopback.errorHandler({ includeStack: false, log: false }));

//act/assert
request(app)
.get('/url-does-not-exist')
.end(function(err, res) {
assert.ok(res.error.text.match(/<ul id="stacktrace"><\/ul>/));
done();
});
});

it('should pass options on to error handler module', function(done) {
//arrange
var app = loopback();
app.use(loopback.urlNotFound());
app.use(loopback.errorHandler({ includeStack: false, log: customLogger }));

//act
request(app).get('/url-does-not-exist').end();

//assert
function customLogger(err, str, req) {
assert.ok(err.message === 'Cannot GET /url-does-not-exist');
done();
}
});
});