hapijs / good

hapi process monitoring
Other
525 stars 160 forks source link

Add auth as a valid value for request in includes option #558

Closed nick-xie closed 7 years ago

nick-xie commented 7 years ago

See #556.

The includes option for requests currently has valid values of ['headers', 'payload'].

It would be helpful if it could also take 'auth' as a value since it could allow logs to include user account(s) attached to the request.

arb commented 7 years ago

I don't think this is a great idea... this could expose sensitive information unless the user was extremely careful.

nick-xie commented 7 years ago

I don't think it's particularly dangerous, doesn't it follow the same principle as the other options where if you don't want to expose info, don't use it? As well, I think it could be argued that the payload option could also contain sensitive info (e.g. passwords). Let me know what you think

arb commented 7 years ago

I'm going to have to say "no" to this at this time. It's a security risk and since we don't know the shape of the auth object, how it's serialized to JSON (or if it's even possible to do so) isn't predictable. Another open PR includes headers, maybe that'll give you enough to achieve what you're looking to do.

csrl commented 6 years ago

I'm not sure I follow what the concern over this was. Providing the auth object for the response event is harmless. Regardless of the format it might be in, it can be consumed by something that has knowledge of it, just like the payload. The payload itself may not be anything that can be serialized to JSON, and as nick-xie pointed out, the payload as well as the headers can contain sensitive authentication data. It is up to each implementation to redact its logs as appropriate.

Exposing the auth enables it to be optionally consumed and used to format the log.

Making the logging system have to parse the headers or payload to get the appropriate auth information, in order to associated the request with eg a user is duplication of code, and more error prone.

Please reconsider, as we also would find this useful.

fabiob commented 6 years ago

I would also ask this PR to be reconsidered.

When logging errors, we usually want to know which user was logged in when the error occurred. This information is located on request.auth.

manugb commented 6 years ago

+1

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.