search

httprb / http

HTTP (The Gem! a.k.a. http.rb) - a fast Ruby HTTP client with a chainable API, streaming support, and timeouts
MIT License
3.01k stars 321 forks source link

HTTP::ConnectionError on HTTPS queries through HTTP proxies #685

Closed ClearlyClaire closed 3 years ago

ClearlyClaire commented 3 years ago

Hi, We are using the http gem in Mastodon, and I've been reported failures when using a HTTP proxy.

Upon investigation, it seems it occurs directly in the http gem and only fails with HTTPS URLs.

Indeed, the following works:

HTTP.via('127.0.0.1', 8118).get('http://httpbin.org/get')

But the following fails:

HTTP.via('127.0.0.1', 8118).get('https://httpbin.org/get')

It fails with the following error:

/home/mastodon/live/vendor/bundle/ruby/3.0.0/gems/http-5.0.1/lib/http/connection.rb:108:in `read_headers!': couldn't read response headers (HTTP::ConnectionError)
    from /home/mastodon/live/vendor/bundle/ruby/3.0.0/gems/http-5.0.1/lib/http/client.rb:76:in `perform'
    from /home/mastodon/live/vendor/bundle/ruby/3.0.0/gems/httplog-1.5.0/lib/httplog/adapters/http.rb:12:in `block (2 levels) in <class:Client>'
    from /usr/lib/ruby/3.0.0/benchmark.rb:308:in `realtime'
    from /home/mastodon/live/vendor/bundle/ruby/3.0.0/gems/httplog-1.5.0/lib/httplog/adapters/http.rb:11:in `block in <class:Client>'
    from /home/mastodon/live/vendor/bundle/ruby/3.0.0/gems/http-5.0.1/lib/http/client.rb:31:in `request'
    from /home/mastodon/live/vendor/bundle/ruby/3.0.0/gems/http-5.0.1/lib/http/chainable.rb:20:in `get'
    from (irb):1:in `<main>'

This also fails with http 5.0.0 but not with 4.4. Version 5.0.0pre3 seems to also work.

ClearlyClaire commented 3 years ago

git bisecting it points to 8f8682c9a052299eea6e2ea314b7dd652e3a32e3 introducing the issue

tarcieri commented 3 years ago

cc @bryanp

bryanp commented 3 years ago

I'm able to reproduce—investigating.

bryanp commented 3 years ago

Okay, I traced the issue all the way back to the llhttp library that the llhttp gem provides bindings to: https://github.com/nodejs/llhttp/issues/128

This is causing our handler to be in an incorrect state. I'll report back once I hear back from the llhttp folks.

bryanp commented 3 years ago

It looks to be an issue with the implementation in #651. We need to reset the parser rather than simply call finish.

I'll be releasing an update to llhttp-ffi today or tomorrow and will PR a corresponding change here.

bryanp commented 3 years ago

Well, it all happened faster than I expected: https://github.com/httprb/http/pull/686. I'll let @tarcieri make the call on when to cut a release.

tarcieri commented 3 years ago

@ClearlyClaire I've merged a prospective fix in #686. Can you confirm it fixes your problem?

ClearlyClaire commented 3 years ago

Yes, I can confirm this solves my issue!

tarcieri commented 3 years ago

Released in 5.0.2