Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1219 Net::HTTP still uses env proxy #1221

Merged
merged 3 commits into from Dec 23, 2020

Conversation

iMacTia
Copy link
Member

@iMacTia iMacTia commented Dec 23, 2020

Description

Ensures that the proxy_address is passed as nil to Net::HTTP to avoid that it picks the one from the environment.
Also, since Net::HTTP::Proxy is obsolete (https://ruby-doc.org/stdlib-2.3.2/libdoc/net/http/rdoc/Net/HTTP.html#method-c-Proxy), it has now been replaced with a different call to .new.
Fixes #1219

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving this!

conn_options[:proxy] = 'http://google.co.uk'
context 'when a proxy is provided as option' do
before do
conn_options[:proxy] = 'http://google.co.uk'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this value is a bit not-self-documenting? "Does Google act as a proxy? How?" the reader (me) might think.


it 'handles requests with proxy' do
res = conn.public_send(http_method, '/')
expect(res.status).to eq(200)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, which I woudl like: Separate the "acts" from the "assert" with a newline


it 'handles requests with proxy' do
res = conn.public_send(http_method, '/')
expect(res.status).to eq(200)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(A newline, please. Thanks!)

shared_examples 'proxy examples' do
it 'handles requests with proxy' do
res = conn.public_send(http_method, '/')
expect(res.status).to eq(200)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A newline, yeah?

@@ -1,5 +1,18 @@
# frozen_string_literal: true

shared_examples 'proxy examples' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really, I like this shared one.

@olleolleolle olleolleolle changed the title Fix/#1219 net http still uses env proxy Fix #1219 Net::HTTP still uses env proxy Dec 23, 2020
@iMacTia iMacTia merged commit 97a3bc2 into master Dec 23, 2020
@olleolleolle olleolleolle deleted the fix/#1219-net-http-still-uses-env-proxy branch December 23, 2020 14:03
iMacTia added a commit to lostisland/faraday-net_http that referenced this pull request Dec 23, 2020
@walterking
Copy link

faraday is a transitive dependency for us, is the expectation now that faraday will ignore env variables always? we were depending on that behavior

@iMacTia
Copy link
Member Author

iMacTia commented Dec 23, 2020

@walterking Not at all! The problem was that using Net::HTTP adapter and setting Faraday.ignore_env_proxy = true would still result in the request being proxied.

If you don't set that config (which defaults to false), Faraday will continue to automatically pick the proxy from the environment for you

@walterking
Copy link

walterking commented Dec 23, 2020

hm, we dont set that variable that i can see. we updated to 1.2 and started to get connection refused errors, which we usually see when the proxy isnt being used, and this pr seemed like the most likely to cause that change

We expect the error since i dont have a proxy actually running locally:

~ gem install faraday:1.1.0
Fetching faraday-1.1.0.gem
Successfully installed faraday-1.1.0
1 gem installed
~ HTTP_PROXY=http://127.0.0.1:1234 irb
2.6.6 :001 > require 'faraday'
 => true
2.6.6 :002 > Faraday.get('https://google.com')
/Users/king/.rvm/rubies/ruby-2.6.6/lib/ruby/2.6.0/net/http.rb:1109: warning: The environment variable HTTP_PROXY is discouraged.  Use http_proxy.
Traceback (most recent call last):
       16: from /Users/king/.rvm/gems/ruby-2.6.6/gems/faraday-1.1.0/lib/faraday/connection.rb:492:in `run_request'
       15: from /Users/king/.rvm/gems/ruby-2.6.6/gems/faraday-1.1.0/lib/faraday/rack_builder.rb:154:in `build_response'
       14: from /Users/king/.rvm/gems/ruby-2.6.6/gems/faraday-1.1.0/lib/faraday/request/url_encoded.rb:25:in `call'
       13: from /Users/king/.rvm/gems/ruby-2.6.6/gems/faraday-1.1.0/lib/faraday/adapter/net_http.rb:68:in `call'
       12: from /Users/king/.rvm/gems/ruby-2.6.6/gems/faraday-1.1.0/lib/faraday/adapter.rb:61:in `connection'
       11: from /Users/king/.rvm/gems/ruby-2.6.6/gems/faraday-1.1.0/lib/faraday/adapter/net_http.rb:70:in `block in call'
       10: from /Users/king/.rvm/gems/ruby-2.6.6/gems/faraday-1.1.0/lib/faraday/adapter/net_http.rb:128:in `perform_request'
        9: from /Users/king/.rvm/gems/ruby-2.6.6/gems/faraday-1.1.0/lib/faraday/adapter/net_http.rb:135:in `request_with_wrapped_block'
        8: from /Users/king/.rvm/gems/ruby-2.6.6/gems/faraday-1.1.0/lib/faraday/adapter/net_http.rb:144:in `request_via_get_method'
        7: from /Users/king/.rvm/rubies/ruby-2.6.6/lib/ruby/2.6.0/net/http.rb:919:in `start'
        6: from /Users/king/.rvm/rubies/ruby-2.6.6/lib/ruby/2.6.0/net/http.rb:930:in `do_start'
        5: from /Users/king/.rvm/rubies/ruby-2.6.6/lib/ruby/2.6.0/net/http.rb:945:in `connect'
        4: from /Users/king/.rvm/rubies/ruby-2.6.6/lib/ruby/2.6.0/timeout.rb:103:in `timeout'
        3: from /Users/king/.rvm/rubies/ruby-2.6.6/lib/ruby/2.6.0/timeout.rb:93:in `block in timeout'
        2: from /Users/king/.rvm/rubies/ruby-2.6.6/lib/ruby/2.6.0/net/http.rb:946:in `block in connect'
        1: from /Users/king/.rvm/rubies/ruby-2.6.6/lib/ruby/2.6.0/net/http.rb:949:in `rescue in block in connect'
Faraday::ConnectionFailed (Failed to open TCP connection to 127.0.0.1:1234 (Connection refused - connect(2) for "127.0.0.1" port 1234))
2.6.6 :003 >
~ gem install faraday:1.2.0
Fetching faraday-1.2.0.gem
Successfully installed faraday-1.2.0
1 gem installed
~ HTTP_PROXY=http://127.0.0.1:1234 irb
2.6.6 :001 > require 'faraday'
 => true
2.6.6 :002 > Faraday.get('https://google.com')
 => #<Faraday::Response:0x00007f87d28ab2c0 @on_complete_callbacks=[], @env=#<Faraday::Env @method=:get @url=#<URI::HTTPS https://google.com> @request=#<Faraday::RequestOptions (empty)> @request_headers={"User-Agent"=>"Faraday v1.2.0"} @ssl=#<Faraday::SSLOptions verify=true> @response=#<Faraday::Response:0x00007f87d28ab2c0 ...> @response_headers={"location"=>"https://www.google.com/", "content-type"=>"text/html; charset=UTF-8", "date"=>"Wed, 23 Dec 2020 21:30:51 GMT", "expires"=>"Fri, 22 Jan 2021 21:30:51 GMT", "cache-control"=>"public, max-age=2592000", "server"=>"gws", "content-length"=>"220", "x-xss-protection"=>"0", "x-frame-options"=>"SAMEORIGIN", "alt-svc"=>"h3-29=\":443\"; ma=2592000,h3-T051=\":443\"; ma=2592000,h3-Q050=\":443\"; ma=2592000,h3-Q046=\":443\"; ma=2592000,h3-Q043=\":443\"; ma=2592000,quic=\":443\"; ma=2592000; v=\"46,43\""} @status=301 @reason_phrase="Moved Permanently" @response_body="<HTML><HEAD><meta http-equiv=\"content-type\" content=\"text/html;charset=utf-8\">\n<TITLE>301 Moved</TITLE></HEAD><BODY>\n<H1>301 Moved</H1>\nThe document has moved\n<A HREF=\"https://www.google.com/\">here</A>.\r\n</BODY></HTML>\r\n">>
2.6.6 :003 >

@iMacTia
Copy link
Member Author

iMacTia commented Dec 23, 2020

Mmmh thanks for the example, I'll have a closer look and try to understand what's wrong, but yes it seems like 1.2 did change something...

That said, http_proxy should be used for http requests, while https_proxy is the one for https, so I'll try a few combinations. Right now my guess is that Net::HTTP might have used them incorrectly 🤔

@walterking
Copy link

walterking commented Dec 23, 2020

interesting. in our app we set both http and https, but when i switch my local to https it seems to do the right thing

@iMacTia
Copy link
Member Author

iMacTia commented Dec 24, 2020

So, I did some extra testing and I believe the issue is with Net::HTTP.

# ~ HTTP_PROXY=http://127.0.0.1:1234 irb
> require 'net/http'
> Net::HTTP.new('http://www.google.com').get('/')
=> Errno::ECONNREFUSED (Failed to open TCP connection to 127.0.0.1:1234 (Connection refused - connect(2) for "127.0.0.1" port 1234))
> Net::HTTP.new('https://www.google.com').get('/')
=> Errno::ECONNREFUSED (Failed to open TCP connection to 127.0.0.1:1234 (Connection refused - connect(2) for "127.0.0.1" port 1234))

# ~ HTTPS_PROXY=http://127.0.0.1:1234 irb
> require 'net/http'
> Net::HTTP.new('http://www.google.com').get('/')
=> SocketError (Failed to open TCP connection to http://www.google.com:80 (getaddrinfo: nodename nor servname provided, or not known))
> Net::HTTP.new('https://www.google.com').get('/')
=> Errno::ECONNREFUSED (Failed to open TCP connection to 127.0.0.1:1234 (Connection refused - connect(2) for "127.0.0.1" port 1234))

So as you can see HTTPS_PROXY is correctly ignored for http calls, however HTTP_PROXY is used for both http and https, which doesn't really makes sense... Faraday relies on URI#find_proxy instead (https://ruby-doc.org/stdlib-2.6.3/libdoc/uri/rdoc/URI/Generic.html#method-i-find_proxy) which behaves correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Net::HTTP Adapter still doesn't ignore env proxy
3 participants