bithavoc / express-winston

express.js middleware for winstonjs
https://www.npmjs.com/package/express-winston
MIT License
797 stars 187 forks source link

Log original req.body object #140

Closed ahnkee closed 7 years ago

ahnkee commented 7 years ago

I ran into an issue with mutated data being logged, as opposed to the actual original request object. Some middlewares, such as https://github.com/ctavan/express-validator, mutate req.body -- in this case, when sanitizers are run. By the time errorLogger or logger record, in such a scenario, an incorrect representation of the original request object is logged.

When a request is initiated, I make a deep copy of req.body and assign it to req._originalBody. When end is invoked on logger or when errorLogger is triggered, req._originalBody is reassigned back to req.body and the key for _.originalBody is deleted.

rosston commented 7 years ago

Sorry, I'm getting to this late.

To be honest, I want to keep this functionality out of the module. Mutating req.body seems like a bad thing that other code shouldn't be doing, and I don't really want to add custom workarounds in express-winston for such behavior.

For what it's worth, you can still use a similar approach in your own application to work around express-validator. You can set up an express middleware that simply does req._originalBody = _.cloneDeep(req.body); and calls next. And then if you'd prefer req._originalBody to be in req.body's place in the log, I'm pretty sure you can use dynamicMeta to accomplish that. For example, this code might do the trick:

dynamicMeta: function(req, res, err) {
  var reqWithOriginalBody = _.assign({}, req, {
    body: req._originalBody
  });
  delete reqWithOriginalBody._originalBody;
  return {
    req: reqWithOriginalBody
  }
}
ahnkee commented 7 years ago

Sorry for my super late response to your response.

Appreciate the feedback and suggestion. For now I'm using a fork but I may just add an early middleware, per your suggestion.

I see your point and get it. However, I do think this is an issue that many users may face (and may even be unaware of, as I was for a while). Mutating req.body is probably a bad idea but it is being done by some fairly popular and widely used plugins.

Especially for something like an info log, I think it's very important that whatever is logged, accurately represents what was initially submitted. The approach I PRed does add some overhead but it doesn't break any other functionality and I personally think it's a fair price to pay for having accurate logs. Just my two cents!

If anyone else comes here looking because they have mutated data due to other plugins, at least they'll have a workaround.