crash on malformed (HTTP/2?) 301

doriantaylor commented 5 years ago

Getting a crash scenario from a 301 without a Location header. What appears to happen is that HttpConnection#redirect gets called with nil as a location which creates an empty hostname. Stack trace looks like this:

        7: from /var/lib/gems/2.5.0/gems/eventmachine-1.2.7/lib/eventmachine.rb:195:in `run'
        6: from /var/lib/gems/2.5.0/gems/eventmachine-1.2.7/lib/eventmachine.rb:195:in `run_machine'
        5: from /var/lib/gems/2.5.0/gems/eventmachine-1.2.7/lib/eventmachine.rb:1482:in `event_callback'
        4: from /home/dorian/clients/people/dorian-taylor/rb-em-http-request/lib/em-http/http_connection.rb:33:in `unbind'
        3: from /home/dorian/clients/people/dorian-taylor/rb-em-http-request/lib/em-http/http_connection.rb:231:in `unbind'
        2: from /var/lib/gems/2.5.0/gems/eventmachine-1.2.7/lib/em/connection.rb:686:in `reconnect'
        1: from /var/lib/gems/2.5.0/gems/eventmachine-1.2.7/lib/eventmachine.rb:795:in `reconnect'
/var/lib/gems/2.5.0/gems/eventmachine-1.2.7/lib/eventmachine.rb:795:in `connect_server': no implicit conversion of nil into String (TypeError)

• No checking for Location header in the client • No subsequent checking in the connection

The result is that an instance like this is created: #<Addressable::URI:0x2aaf7488f7a4 URI:/>, which makes @host be nil and thus crash with the type error cited above.

This should be easy enough to patch, the only question is what precisely—policywise—to patch it with.

igrigorik commented 5 years ago

Do you have an example test case to reproduce this behavior? I'm still a bit unclear on how this path is triggered.

doriantaylor commented 5 years ago

I wrote a quick CGI script to replicate it. It's up here: https://makethingsmakesense.com/bogus-redirect.cgi . Try hitting it and you'll see the crash.

This will fix it but I'm not sure if it's the most sensible solution:

            # issue #334: you can have a response code which is in the
            # "redirect" range *without* a Location header; indeed
            # this may not be aggressive enough (ie perhaps we should
            # also check if the header is well-formed before
            # committing more resources)
            if redirect? && @response_header.location
              @req.followed += 1

              @cookies.clear
              @cookies = @cookiejar.get(@response_header.location).map(&:to_s) if @req.pass_cookies

              @conn.redirect(self, @response_header.location)
            else
              succeed(self)
            end

igrigorik / em-http-request

crash on malformed (HTTP/2?) 301 #334