crash on malformed (HTTP/2?) 301

[doriantaylor]

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]

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

[doriantaylor]

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

[matthewbennink]

We encountered this recently with a 307 Temporary Redirect. The site being fetched did not return a Location header, but instead returned HTML with a <script> tag that sets document.cookie and then calls location.reload(), which then results in 200 OK with the site's contents.

The following was enough to help resolve our issue in case it's useful for anyone else. In short, we stop following redirects the moment we don't have a Location header. (Note that blank? is from Rails/ActiveSupport.)

class HandleRedirectWithoutLocation
  def response(client)
    if client.redirect? && client.response_header.location.blank?
      client.req.redirects = 0
    end
  end
end
EventMachine::HttpRequest.use(HandleRedirectWithoutLocation)

And per https://stackoverflow.com/questions/16194988/for-which-3xx-http-codes-is-the-location-header-mandatory, the Location header doesn't seem to be required, so it's a bit of an unknown and I think the caller perhaps needs to decide the best way to handle this scenario. If the library did anything, I think it'd make sense to stop following redirects per the fix in the prior GH comment, but it does change the behavior a bit.