Should the environment nuked by default?

[bklang]

In adhearsion-reporter v2.0 we did not specify an environment. I had (incorrectly) thought this meant that no environment was included with every report. Per the guidelines at https://github.com/toolmantim/toadhopper/blob/58a34756eb59c745b4ddb9f3d0a283f63c944c80/lib/toadhopper.rb#L99, the environment really should be sanitized before sending, as it very likely will include secret information like database passwords and API keys.

In version 2.1 I started manually creating an "environment" that was sent along with reports, which currently only contains the version of Adhearsion and the local hostname.

In looking at old bug reports, I see that the environment was actually included by default, making the 2.1 change actually a bit of a regression.

So this issue is an open question to users of adhearsion-reporter: should the old behavior of including the full environment be included in error reports? We can't fully sanitize the environment before sending because most sensitive settings are app-specific. I actually would like to include as much of the environment as possible - it adds a lot of context. I also want to provide a default setting for ENV['HOSTNAME'] if it is not already set, using the new functionality in adhearsion-reporter 2.1.

Thoughts? How best can we sanitize the environment? Allow the app to declare a list of blacklisted env vars?

[benlangfeld]

I think a blacklist which defaults to ["#{Loquacious.env_prefix}_PUNCHBLOCK_PASSWORD"] would be a good idea.

[benlangfeld]

Toadhopper already provides filters. We should probably use that.

[bklang]

Nice, I didn't know that existed. So I need to find a way to expose this through the adhearsion-reporter config, and document it with a big warning. Any thoughts on a default list of filters? The Punchblock password comes to mind.

[bklang]

@benlangfeld And then I saw your suggested default filter comment. I'm also going to go through our production apps and see if any other common strings should be filtered.

[JustinAiken]

AHN_XMPP_PASSWORD will also be common... anything with _PASSWORD really...

[benlangfeld]

Toadhopper already filters anything with /password/ by default.

[JustinAiken]

Are you sure? I don't see anything in toadhopper that sets default filters, and looking at airbrakes sent with adhearsion-reporter, I see punchblock and xmpp passwords...

[sfgeorge]

In looking at old bug reports, I see that the environment was actually included by default, making the 2.1 change actually a bit of a regression.

Yep, I can confirm that that was the behavior that I observed prior to adhearsion-reporter 2.1, sounds like Justin can as well. I'm really glad you're raising this up anew, as I feel that the old/assumed behavior was bad.

Including the environment

So this issue is an open question to users of adhearsion-reporter: should the old behavior of including the full environment be included in error reports?

I vote no for 2 reasons:

1. We don't provide users of adhearsion-reporter with an easy** way to specify filters to send into the Toadhopper instance used from adhearsion-reporter.
2. Both adhearsion-reporter and toadhopper default the API domain to the unencrypted* http://**airbrake.io. This raises the stakes even higher if we miss filtering something.

Here's an alternative suggestion to sending the full environment by default:

1. Provide a config.reporter.environment = [:compact, :full] option, with :compact as default.
2. Provide a convenient means of applying filters.

Regarding filters

** It's extremely cool that adhearsion-reporter allows you to override the notifier class it will instantiate by way of the config.reporter.notifier/ENV['AHN_REPORTER_NOTIFIER'] config setting. Using this, one could technically interact with @notifier.filters in order to specify desired filters. But I would argue that needing filters as a use case is probably common enough that a more convenient approach should be available.

Additionally, I'd like to note that it's extremely difficult to cover all of the bases by setting default filters. I think that toadhopper (not adhearsion-reporter) should use some defaults, which I agree with Justin on - it does not currently.

Here are a number of sensitive environment variables that I've seen in open source ruby projects. I think this shows how hard it is to try to anticipate them all with default filters:

• AWS_ACCESS_KEY_ID
• AWS_SECRET_ACCESS_KEY
• AWS_SECURITY_TOKEN
• S3_KEY
• S3_SECRET
• RAILS_DB_PWD (Particularly easy to collide with standard PWD variable)
• AHN_REPORTER_API_KEY (Ok, I admit that if you are reviewing messages within Airbrake, you probably already know this :smile: )
• DATABASE_URL
• GH_BASIC_SECRET_ID
• SECRET_TOKEN

Regarding HTTP

*** By default, users of adhearsion-reporter and/or toadhopper will have their messages posted over http://airbrake.io on an insecure channel. Scary!

I personally believe that this is best solved by...

1. Updating toadhopper to default to https
2. adhearsion-reporter should not set a default that is redundant of toadhopper. Instead, it should default to nil, and allow toadhopper's default to take over. (This will not break the experience/change the URI on users unless they upgrade Toadhopper). Only if user sets config.reporter.url/ENV['AHN_REPORTER_URL'] should this be overridden. Besides... adhearsion-reporter also supports ErrBit and now New Relic. Seems silly that the default API URL prefers Airbrake.

This HTTP issue is clearly out of scope of the topic of hand -- I only bring it up as it is related. But I admit it should be discussed further as a separate Issue.

Thanks for listening! Thoughts?