search

elixir-plug / plug_cowboy

Plug adapter for the Cowboy web server
Other
243 stars 48 forks source link

Logging the conn logs sensitive information #89

Closed jared-mackey closed 1 year ago

jared-mackey commented 1 year ago

Hi,

It looks like the exception logger from the plug here adds the entire conn to the logger metadata. The conn can contain sensitive information such as headers with authorization keys, cookies with sensitive values, or maybe even request/response bodies with sensitive data in them that should not be logged. In cases where metadata: :all is used this leads to those sensitive values being logged out which may be great for debugging, is not great for production environments where those values need to be scrubbed and or other serious implications.

Should this be added to the metadata by default given the high sensitivity of those values and the somewhat common practice to allow all metadata?

Additionally, I do not see any mention of this in the docs for the plug. Maybe it's somewhere in the phoenix or plug docs though and I missed it.

Last but not least, thank you for the awesome project, we really appreciate all the hard work put into this community <3

josevalim commented 1 year ago

Hi @jared-mackey! How are you logging it? The default backend only logs metadata that can be converted to strings, which conn cannot.

jared-mackey commented 1 year ago

@josevalim Oh, that's a good bit of info to know. I am using the logger_json library to log out all logs as json. Here's my config which is mostly taken from their documentation page linked there. They do mention that some parameters can be logged from plug/phoenix but I thought I had disabled them. 

config :logger_json, :backend, metadata: :all, formatter: LoggerJSON.Formatters.DatadogLogger
config :logger, backends: [LoggerJSON]

filter_parameters = [
  ... all of my sensitive keys
]

config :phoenix, filter_parameters: filter_parameters

I had tested this a lot locally but I guess I didn't test an exception raising. :)

josevalim commented 1 year ago

Yeah, I think you will need a custom formatted because I am almost sure we added the conn for people to extract metadata.

we could also accept a config for removing it.

josevalim commented 1 year ago

A PR for the second one is welcome :)