Skip to content
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.

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

`typeof` and `reformat` should not be enumerable #215

Closed
soulchild opened this issue Feb 21, 2019 · 2 comments
Assignees
Milestone

Comments

@soulchild
Copy link

@soulchild soulchild commented Feb 21, 2019

I'm unsure whether this is the right place for this issue (or if it's rather a Winston thing), but when logging Boom objects with Winston (e.g. Internal Server Errors), every error contains Boom's reformat function, making the log output somewhat confusing.

Here's an example:

error:  message=kaputt, stack=Error: kaputt
at Object.<anonymous> (auth.ts:27:16)
    at Generator.next (<anonymous>)
    at auth.ts:7:71
    at new Promise (<anonymous>)
    ...
at Layer.handle [as handle_request] (node_modules/express/lib/router/layer.js:95:5), data=null, isBoom=true, isServer=true, statusCode=500, statusCode=500, error=Internal Server Error, message=An internal server error occurred, , reformat=function (debug = false) {

    this.output.payload.statusCode = this.output.statusCode;
    this.output.payload.error = internals.codes.get(this.output.statusCode) || 'Unknown';

    if (this.output.statusCode === 500 && debug !== true) {
        this.output.payload.message = 'An internal server error occurred';              // Hide actual error from user
    }
    else if (this.message) {
        this.output.payload.message = this.message;
    }
}, typeof=internal(message, data, statusCode = 500) {

        return internals.serverError(message, data, statusCode, internals.Boom.internal);
    }

The code producing this is basically just the following Express error handler middleware:

export default () => (err, req, res, next) => {
  Boom.boomify(err);
  if (err.isServer) {
    logger.error(err);
  }
  ...
}

Any ideas? I'm using winston@2 and the boom@7.3.0

@soulchild soulchild changed the title Logging Boom error with Winston includes `reformat()` function Logging Boom error with Winston includes reformat() function Feb 21, 2019
@hueniverse hueniverse self-assigned this Sep 17, 2019
@hueniverse hueniverse added the bug label Sep 19, 2019
@hueniverse hueniverse added this to the 7.4.5 milestone Sep 19, 2019
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Sep 19, 2019

I am not sure what Winston is doing, but typeof and reformat should not be enumerable keys which might be the reason they are currently included.

@hueniverse hueniverse changed the title Logging Boom error with Winston includes reformat() function `typeof` and `reformat` should not be enumerable Sep 19, 2019
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Sep 19, 2019

Adding breaking change as this can have side effects if the properties are being manipulated elsewhere unexpectedly. They shouldn't, so this is still a patch bug fix, but the label should help people debug if this breaks something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.