Bugsnag client forces Net::HTTP to ignore http_proxy env variable

[dexhorthy]

Howdy -- wanna start by thanking the whole bugsnag team -- we are big bugsnag fans over here, keep up the great work! We've found one issue (this behavior may even be intentional) around handling of http proxies. Would be happy to work on a patch for this.

Expected behavior

Busnag respects http_proxy env var when set, and doesn't pass nil to Net::HTTP for the proxy address.

    uri = URI.parse("https://some-destination.example.com")
    ENV['http_proxy'] = "http://some-proxy:3128"

    http = Net::HTTP.new(uri.host, uri.port) 
    request = Net::HTTP::Get.new(uri.request_uri)
    response = http.request(request) # request will be routed through proxy from ENV

Observed behavior

Bugsnag always passes nil for the http proxy address unless it is explicitly set via config.proxy_host. This overrides the default Net::HTTP behavior of transparently using the proxy set in ENV['http_proxy']. This happens here:

bugsnag-ruby/lib/bugsnag/delivery/synchronous.rb

Line 27 in 42552a8

http = Net::HTTP.new(uri.host, uri.port, configuration.proxy_host, configuration.proxy_port, configuration.proxy_user, configuration.proxy_password)

    uri = URI.parse("https://some-destination.example.com")
    ENV['http_proxy'] = "http://some-proxy:3128"

    # if config.proxy_host, port, etc is set to nil, those will be passed in here
    http = Net::HTTP.new(uri.host, uri.port, nil, nil, nil, nil) # nil passed for proxy, wont use proxy from env
    request = Net::HTTP::Get.new(uri.request_uri)
    response = http.request(request) # not routed through proxy

Steps to reproduce

Set http_proxy env var, configure bugsnag without modifying proxy configuration. Requests to bugsnag API will be not be routed through proxy.

Version

bugsnag (~> 2.0)

Additional information

Notably, Net::HTTP doesn't transparently support proxies with authentication, so there are still many cases where users of this client would need to manually configure Bugsnag's http proxy settings. That should always be the source of truth if set, but I'm wondering if y'all would be open to allowing configuration via the http_proxy env var, and not passing in nil for the proxy address if those configuration options are unset.

This could technically be a breaking change if anyone was relying on the bugsnag client to explicity pass nil for the proxy address.

For what its worth, it looks like the httpparty folks ended up with a pretty minimal patch to fix this https://github.com/jnunemaker/httparty/pull/222/files#diff-cc7dd34b572c3fe580e64c5099d5fd2eR69
(here's the related issue jnunemaker/httparty#184)

Thanks again for all the work on bugsnag -- happy to clarify anything in here if need be!
Dex

[Cawllec]

Hi @dexhorthy, We're going to be releasing a new major version soon and this is definitely something we'll include with that. I'll link this to a PR when done, would be great to get your feedback.

[dexhorthy]

Hi @Cawllec -- thanks for the response and for taking a look at this issue!

I had a look at the PR #379, but I don't think that's going to do it.

I believe the convention for the http_proxy env var is to put the entire url w/ protocol and port in that variable, e.g.

http_proxy="http://my-cool-proxy.mycompany.com:3128"

but the bugsnag-ruby config expects these in different fields, and without the protocol, e.g.

      config.proxy_host = "my-cool-proxy.mycompany.com"
      config.proxy_port = 3128

You could parse each URI component out with URI.parse, but my advice would be to let Net::HTTP handle all of that for you.

As in the HTTParty patch I linked, I believe you could fix with the following patch: (happy to send as PR)

diff --git a/lib/bugsnag/delivery/synchronous.rb b/lib/bugsnag/delivery/synchronous.rb
index f8d990f..bd60b96 100644
--- a/lib/bugsnag/delivery/synchronous.rb
+++ b/lib/bugsnag/delivery/synchronous.rb
@@ -24,7 +24,13 @@ module Bugsnag
 
         def request(url, body, configuration)
           uri = URI.parse(url)
+
+          if configuration.proxy_host
             http = Net::HTTP.new(uri.host, uri.port, configuration.proxy_host, configuration.proxy_port, configuration.proxy_user, configuration.proxy_password)
+          else
+            http = Net::HTTP.new(uri.host, uri.port)
+          end
+
           http.read_timeout = configuration.timeout
           http.open_timeout = configuration.timeout

( didn't test this, but that's the general idea)