Ignore HTTP_VERSION headers since Rack misbehaves

[mattrobenolt]

See: https://github.com/rack/rack/blob/028438f/lib/rack/handler/cgi.rb#L29

Right now, raven captures this HTTP_VERSION header, and correctly assumes this was a Version header sent from the client. According to the CGI spec, HTTP_* is reserved for client values only.

Environment variables with names beginning with "HTTP_" contain header data read from the client, if the protocol used was HTTP.

So we just compare against env['SERVER_PROTOCOL'] to counteract it's poor behavior and make sure we don't ignore a legitimate Version header sent from the client.

[mattrobenolt]

It seems that this also applies to WEBrick.

[nateberkopec]

Correct me if I'm wrong, but it looks like those handlers are only setting HTTP_VERSION to $SERVER_PROTOCOL if the client did not provide a Version header. Here's where it's set in Webrick: https://github.com/ruby/ruby/blob/832c74f428db6c5bd6e575e1f6ea7fe0891c84d2/lib/webrick/httprequest.rb#L375

If the client sends it's own Version header, I don't think the linked handlers will overwrite it.

[mattrobenolt]

This is true.

The problem for raven-ruby is that we're reporting a Version header from the client when the client didn't send it, so it's confused.

This is also the reason for the check value == server_protocol. This makes sure the client didn't send the value to override it.

[mattrobenolt]

I guess my tl;dr is that we are falsely reporting this header when the client isn't sending it purely because Rack defines it as HTTP_VERSION which conflicts with the CGI convention of HTTP_* being client headers.

[nateberkopec]

Aha! Now I understand. What if the client is sending a Version header equal to a Rack server's SERVER_PROTOCOL?

[mattrobenolt]

imo this is less likely to happen. :) This would basically have to be HTTP/1.1. I've never seen SERVER_PROTOCOL to be anything different. It's not affected by the client sending GET / HTTP/1.0 or anything like that fwiw.

[mattrobenolt]

fwiw, I've opened up two issues with Rack to address this on their side:

rack/rack#969
rack/rack#970

[mattrobenolt]

Ignore my comment about WEBrick since that's not a problem here. :) It's only Rack.

[mattrobenolt]

Further down the rabbit hole here:

So turns out that Unicorn literally doesn't allow the client to set a Version header. See: https://github.com/defunkt/unicorn/blob/422a657a5f6dfb69f44feabd6429f2904ca03fa8/ext/unicorn_http/unicorn_http.rl#L237-L240

Purely to avoid conflicting with this magical HTTP_VERSION value.

[nateberkopec]

We'll see what Rack does, but I think the reasoning in rack/rack#970 makes more sense than merging an imperfect solution at our end.

[mattrobenolt]

@nateberkopec so I would have been inclined to agree with that, but digging deeper into this rabbit hole, it's unlikely that these things are going to change in Rack.

This is even supported behavior in Unicorn: https://github.com/defunkt/unicorn/blob/422a657a5f6dfb69f44feabd6429f2904ca03fa8/ext/unicorn_http/unicorn_http.rl#L237-L240

They completely ignore a Version header coming in from a client just to avoid this conflict.

I'm just inclined to do this on our end, since a Version header is extremely uncommon in any case + the improbability of it also being set to exactly HTTP/1.1 is even more unlikely.

Right now, this makes every request being logged through a Rack server to have an extraneous header attached in Sentry, which is a bit odd.

[mattrobenolt]

I'm just pretty sure there's a deeper rooted problem here with regards to HTTP_VERSION, that I'm unlikely to influence and change. :) It's riddled throughout unicorn and rack both, and I haven't even dug into other servers.

This just seems like a thing we should skip on our side.

[mattrobenolt]

@nateberkopec You know more here than me, but I'd like to get this merged unless you have a major objection.

[nateberkopec]

@mattrobenolt Go for it.

[mattrobenolt]

Thanks. 🍰