Suppress error handler stack traces in production#564 by digitalsadhu · Pull Request #1502 · strongloop/loopback

digitalsadhu · 2015-07-05T17:42:01Z

Creates a new loopback.errorHandler function that wraps the existing express errorhandler module and essentially just unsets the stack trace when the app is in production.

Note, a modification to middleware.json file generation will need to occur for this approach to work.

"final:after": {
"errorhandler": {}
}

will need to change to:

"final:after": {
"loopback#errorHandler": {}
}

Fixes#564

@bajtos This is my attempt to solve this issue. Thoughts?
If you like the approach I can put some more effort into writing tests and documentation. If you would prefer another approach, please advise.

slnode · 2015-07-05T17:42:04Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

bajtos · 2015-07-07T16:08:38Z

Hi @digitalsadhu, thank you for the pull request.

I would prefer to expose a configuration option instead of hard-coding this behaviour. That way we can preserve backwards compatibility.

functionerrorHandler(options){if(options.includeStack!==false){// original error handler}else{// remove stack}}

The configuration - server/middleware.production.json:

{
"final:after": {
"loopback#errorHandler": {
"params": {
"includeStack": false
}
}
}

Thoughts?

digitalsadhu · 2015-07-07T19:09:05Z

Yes thats nicer, will make the change

digitalsadhu · 2015-07-08T17:26:59Z

will look into fixing tests shortly

digitalsadhu · 2015-07-09T21:06:23Z

Tests are passing locally, under iojs, 0.10 and 0.12 so not sure whats going on with Travis.

@bajtos How is this looking now?

bajtos · 2015-07-10T15:35:14Z

Tests are passing locally, under iojs, 0.10 and 0.12 so not sure whats going on with Travis.

Did you run npm test too? That will run the tests in PhantomJS too, that's the part failing on Travis.

bajtos · 2015-07-10T15:36:58Z

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.

bajtos · 2015-07-10T15:41:01Z

Thank you for the update @digitalsadhu. I am starting a long (3 weeks+) paternity leave on Monday, it's likely that I won't be able to continue reviewing this patch as timely as I'd like. Sorry for that.

bajtos · 2015-07-10T15:41:10Z

@slnode ok to test

digitalsadhu · 2015-07-12T19:27:51Z

Thanks @bajtos. I'm on paternity leave 3 days/week myself. Enjoy :)
Anyway, that buys me some time to make fixes.
Thanks for the feedback

bajtos · 2015-08-24T13:28:34Z

@digitalsadhu ping, what's the status of this pull request? Are you still keen to get it finished & landed?

Removes stack trace in production mode. Signed-off-by: Richard Walker <digitalsadhu@gmail.com>

Signed-off-by: Richard Walker <digitalsadhu@gmail.com>

This is now done in the new error handler middleware module Signed-off-by: Richard Walker <digitalsadhu@gmail.com>

Instead of removing stack when NODE_ENV=production, instead remove stack when options.includeStack === false Signed-off-by: Richard Walker <digitalsadhu@gmail.com>

Signed-off-by: Richard Walker <digitalsadhu@gmail.com>

digitalsadhu · 2015-09-02T10:15:50Z

@bajtos sorry for the delay, I have made fixes based on your comments, rebased loopback master onto my branch and pushed. How are we looking?

Also, question. Express is logging to the console during tests which is a bit ugly. Whats the best way to disable this?

bajtos · 2015-09-02T14:35:01Z

sorry for the delay, I have made fixes based on your comments, rebased loopback master onto my branch and pushed. How are we looking?

Thank you, the patch LGTM now.

Express is logging to the console during tests which is a bit ugly. Whats the best way to disable this?

I believe you can pass log:false in the options, could you try that out please? See https://github.com/expressjs/errorhandler#log

bajtos · 2015-09-02T14:37:11Z

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

bajtos · 2015-09-02T14:38:37Z

@digitalsadhu I am looking forward to land this patch! Please remove console logs from unit tests and fix the comment above first, or let me know whether I should make these two small changes myself.

It looks like I had missed this. I realised when I tried to disable console logging and found that the options I passed to the loopback error handler were not getting passed on to the errorHandler module

This test ensures that options passed to the loopback error handler get passed on to express error handler

digitalsadhu · 2015-09-02T17:51:59Z

@bajtos

moved the dep from dev deps to deps
attempted to disable logging and discovered I wasn't passing the options object through to express errorHandler. Have added a test for this case and fixed. Logging now disabled in console for tests

bajtos · 2015-09-03T06:36:01Z

discovered I wasn't passing the options object through to express errorHandler. Have added a test for this case and fixed.

Haha, good catch! I appreciate you added a unit-test for that too 👍

bajtos · 2015-09-03T06:41:28Z

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.

bajtos · 2015-09-03T06:42:57Z

Landed via 57e40cf, thank you for the contribution!

bajtos · 2015-09-03T07:22:19Z

Note: when includeStack is false, the stack is removed from the error messages printed to the console too.

When running in production, supress error stack traces from the error responses (pages). This change affects the global error handler used, it does not affect strong-remoting's REST handler. See strongloop/loopback#1502

altsang added the #community contribution label Jul 5, 2015

bajtos self-assigned this Jul 7, 2015

bajtos reviewed Jul 10, 2015
View reviewed changes

bajtos added the blocked label Aug 4, 2015

digitalsadhu added 8 commits September 2, 2015 12:08

Create wrapped express errorhandler
3baba47
Removes stack trace in production mode. Signed-off-by: Richard Walker <digitalsadhu@gmail.com>

Load new error handler module
791c7ac
Signed-off-by: Richard Walker <digitalsadhu@gmail.com>

Remove errorhandler title setting
32f758d
This is now done in the new error handler middleware module Signed-off-by: Richard Walker <digitalsadhu@gmail.com>

Replace env with options configuration
5bfed4d
Instead of removing stack when NODE_ENV=production, instead remove stack when options.includeStack === false Signed-off-by: Richard Walker <digitalsadhu@gmail.com>

Allow middleware to be called without param
7eba03c
Signed-off-by: Richard Walker <digitalsadhu@gmail.com>

Add test dependencies
2274705

Add basic tests for error handler
06cad82

Fix issues based on PR comments from@bajtos
07666d4

digitalsadhu force-pushed the suppress_stack_traces_in_production#564 branch from f5d8aea to 07666d4Compare September 2, 2015 10:09

bajtos reviewed Sep 2, 2015
View reviewed changes

bajtos removed the blocked label Sep 2, 2015

Move errorHandler dep from devDependencies
b700801

digitalsadhu added 3 commits September 2, 2015 19:33

Pass options to errorHandler module
826dc98
It looks like I had missed this. I realised when I tried to disable console logging and found that the options I passed to the loopback error handler were not getting passed on to the errorHandler module

Turn off console logging in tests
c60ff5e

Add additional test to cover options passing
8527f52
This test ensures that options passed to the loopback error handler get passed on to express error handler

bajtos reviewed Sep 3, 2015
View reviewed changes

bajtos closed this in 57e40cfSep 3, 2015

bajtos removed the #community contribution label Sep 3, 2015

bajtos added the needs-doc label Sep 3, 2015

bajtos mentioned this pull request Sep 3, 2015
Proposal: suppress stack traces in HTTP responses when running in production expressjs/errorhandler#13
Closed

This was referenced Sep 3, 2015
Suppress error stack traces in production strongloop/loopback-workspace#226
Merged
Error handling in production [EPIC PLACEHOLDER] #1650
Closed

Uh oh!

Conversation

digitalsadhu commented Jul 5, 2015

Uh oh!

slnode commented Jul 5, 2015

Uh oh!

bajtos commented Jul 7, 2015

Uh oh!

digitalsadhu commented Jul 7, 2015

Uh oh!

digitalsadhu commented Jul 8, 2015

Uh oh!

digitalsadhu commented Jul 9, 2015

Uh oh!

bajtos commented Jul 10, 2015

Uh oh!

bajtosJul 10, 2015

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jul 10, 2015

Uh oh!

bajtos commented Jul 10, 2015

Uh oh!

digitalsadhu commented Jul 12, 2015

Uh oh!

bajtos commented Aug 24, 2015

Uh oh!

digitalsadhu commented Sep 2, 2015

Uh oh!

bajtos commented Sep 2, 2015

Uh oh!

bajtosSep 2, 2015

Choose a reason for hiding this comment

Uh oh!

bajtos commented Sep 2, 2015

Uh oh!

digitalsadhu commented Sep 2, 2015

Uh oh!

bajtos commented Sep 3, 2015

Uh oh!

bajtosSep 3, 2015

Choose a reason for hiding this comment

Uh oh!

bajtos commented Sep 3, 2015

Uh oh!

bajtos commented Sep 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants