search

socketry / async-http

MIT License
316 stars 45 forks source link

`Async::HTTP::Client` behaves weird if `Async::HTTP::Endpoint.for(...)` is given `:https` instead of `"https"` #165

Closed postmodern closed 2 months ago

postmodern commented 2 months ago

I noticed that that Async::HTTP::Client behaves oddly (ex: doesn't initialize a SSL connection and sends HTTP requests anyways) if Async::HTTP::Endpoint.for(...) is given a Symbol URI scheme name instead of a String. I think there should be a .to_s call on the given scheme or passed through String() to coerce it into a String value.

Steps To Reproduce

require 'async/http'

endpoint = Async::HTTP::Endpoint.for(:https,'example.com', port: 443)
client = Async::HTTP::Client.new(endpoint)
Async do
  p client.get('/')
end
  0.1s     warn: Async::Task [oid=0xeb0] [ec=0xec4] [pid=34116] [2024-07-12 19:19:18 -0700]
               | Task may have ended with unhandled exception.
               |   Errno::ECONNRESET: Connection reset by peer
               |   → /opt/rubies/ruby-3.1.6/lib/ruby/3.1.0/socket.rb:452 in `__read_nonblock'
               |     /opt/rubies/ruby-3.1.6/lib/ruby/3.1.0/socket.rb:452 in `read_nonblock'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/io-stream-0.4.0/lib/io/stream/buffered.rb:103 in `sysread'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/io-stream-0.4.0/lib/io/stream/generic.rb:268 in `fill_read_buffer'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/io-stream-0.4.0/lib/io/stream/generic.rb:101 in `read_until'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/async-http-0.68.0/lib/async/http/protocol/http1/connection.rb:50 in `read_line'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/protocol-http1-0.19.1/lib/protocol/http1/connection.rb:212 in `read_response_line'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/protocol-http1-0.19.1/lib/protocol/http1/connection.rb:220 in `read_response'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/async-http-0.68.0/lib/async/http/protocol/http1/response.rb:15 in `read'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/async-http-0.68.0/lib/async/http/protocol/http1/client.rb:62 in `call'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/protocol-http-0.26.5/lib/protocol/http/request.rb:54 in `call'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/async-http-0.68.0/lib/async/http/client.rb:183 in `make_response'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/async-http-0.68.0/lib/async/http/client.rb:105 in `call'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/protocol-http-0.26.5/lib/protocol/http/methods.rb:74 in `block (2 levels) in <class:Methods>'
               |     test_async_http_endpoint.rb:7 in `block in <main>'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/async-2.12.1/lib/async/task.rb:164 in `block in run'
               |     /data/home/postmodern/code/ronin-rb/vendor/bundle/ruby/3.1.0/gems/async-2.12.1/lib/async/task.rb:377 in `block in schedule'

Versions

ioquatix commented 2 months ago

We are using schemes as per:

irb(main):002> URI.scheme_list
=> {"WS"=>URI::WS, "WSS"=>URI::WSS, "HTTP"=>URI::HTTP, "HTTPS"=>URI::HTTPS, "LDAP"=>URI::LDAP, "FILE"=>URI::File, "FTP"=>URI::FTP, "MAILTO"=>URI::MailTo, "LDAPS"=>URI::LDAPS}

I actually think the style of keys in this list is wrong (why is it upper case?) and in addition, the listed schemes don't all make sense for async-http. So I propose we have our own list of supported schemes?

postmodern commented 2 months ago

I switched my code to using lowercase String values, which matches what URI::Generic.build() accepts. Still, if you give it a Symbol, it should probably either be converted to a String or a TypeError or ArgumentError should be raised saying "unsupported scheme".

ioquatix commented 2 months ago

I've merged a fix for this. LMK if you think it's appropriate.