Puma treats the HTTP protocol version as an header, loses track of real protocol version when there is a client-provided header named 'version'

[amuta]

Describe the bug
Puma treats the HTTP protocol version as an header and if there is a client-provided header named 'version' Puma adds the two values, later Puma uses this value when matching which protocol to respond, having now a bogus value it will always fallback to HTTP/1.0.

There is a related issue that was closed:

Issue 1565 - Requests to Puma hanging due to issues with Keep-Alive, Content-Length, and HTTP_VERSION headers

And this comment pretty much sums what is happening
#1565 (comment)

Unfortunately I don't know how to dig deeper to figure out where the "doubled" version is coming from

It comes from here, set via this and lives in Ragel here and here

As part of the HTTP spec every client-provided header gets transformed to uppercase, has - replaces with _ and gets the prefix of HTTP_ added. In the case of a Version header that'll transform it to HTTP_VERSION.

The problem is we're already setting HTTP_VERSION to HTTP/1.1 in Puma (via the C/Java extensions) we fall into the RFC's behaviour for when you set the same header more than once: We add the client header value to create HTTP/1.1, HTTP/1.1. You could send a request with Version:Blabbedy and get HTTP_VERSION: HTTP/1.1, Blabbedy grimacing

Though it's not in the Rack spec, this is also a problem in Rack itself (rack/rack#970). We can fix Puma and we can open PRs on Thin/Unicorn too but I'd love some thoughts from the maintainers of Puma on it before I do

The related Rack issue was fixed on the 3.0.0 version rack/rack#1691

Puma config:
NA

To Reproduce

Start puma hello.ru server.

> echo 'run lambda { |env| [200, {"Content-Type" => "text/plain"}, ["Hello World"]] }' > hello.ru
> puma hello.ru

Test without version header - Puma responds with HTTP/1.1:

time curl -v -k http://localhost:9292 
*   Trying 127.0.0.1:9292...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 9292 (#0)
> GET / HTTP/1.1
> Host: localhost:9292
> User-Agent: curl/7.68.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: text/plain
< Content-Length: 11
< 
* Connection #0 to host localhost left intact
Hello World
real	0m0,010s
user	0m0,001s
sys	0m0,009s

Test with version header - Puma responds with HTTP/1.0:

time curl -v -k http://localhost:9292 --header 'version: abc'
*   Trying 127.0.0.1:9292...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 9292 (#0)
> GET / HTTP/1.1
> Host: localhost:9292
> User-Agent: curl/7.68.0
> Accept: */*
> version: abc
> 
* Mark bundle as not supporting multiuse
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
< Content-Type: text/plain
< Content-Length: 11
< 
* Closing connection 0
Hello World
real	0m0,011s
user	0m0,004s
sys	0m0,007s

Desktop (please complete the following information):

• OS: Ubuntu 20.4
• Puma Version 5.6.4

[MSP-Greg]

Thanks for the report. Interesting, in that a version header is somewhat odd in terms of mention of it in recent documents. Regardless, changing the following line:

puma/lib/puma/request.rb

Line 402 in 6481347

http_11 = env[HTTP_VERSION] == HTTP_11

to

http_11 = env.fetch(HTTP_VERSION, '').start_with? HTTP_11

seems to fix the issue you've provided?

[amuta]

@MSP-Greg that should be a clever and quick way to fix the wrong protocol on response.

There is also the problem with the missing/bogus 'version' header when it is client-provided.

I think the best way would be having another env 'PROTOCOL_VERSION' for the HTTP protocol version, and then using the HTTP_ prefix strictly for the request headers.

[MSP-Greg]

There is also the problem with the missing/bogus 'version' header when it is client-provided.

Obviously, the response protocol should not be changed to HTTP/1.0. But as to changes to header handling, not sure what to do.

Maybe others have stronger opinions, but given that specs are non-existent or vague...

[kikoreis]

(Puma newbie so apologies in advance if my suggestions are very misguided :-)

I'm curious what the rationale is for using HTTP_VERSION to hold the protocol version and simultaneously prefixing user-provided headers with HTTP_ — that seems like a recipe for a collision. We could use a special prefix for Puma-defined values — for instance, _HTTP_VERSION, or go the route @amuta is describing, using something like PROTOCOL_VERSION which isn't prefixed with HTTP_. Either would work.

AIUI both options would however break compatibility for anyone who is relying on HTTP_VERSION's current semantics. It's probably a good idea to indicate a deprecation for a stable cycle, and provide a warning as well if a Version: header is provided by the end user as it will be silently clobbered.

At any rate, I think the immediate fix suggested above makes sense — it would at least avoid a very confusing failure mode, as messing with the protocol version causes problems with upstream/downstream elements in the request processing chain.

[jeremyevans]

As the linked issue indicates, SERVER_PROTOCOL should now be used for the HTTP version used for the request. For backwards compatibility with existing applications, which often use HTTP_VERSION, since that is what both Puma and Unicorn support, the Rack 3 spec states that if HTTP_VERSION is present in env, it must match SERVER_PROTOCOL. This does mean that there isn't a way to handle an version request header currently (I recommend servers ignore it). Possibly we can drop the HTTP_VERSION requirement in Rack 4, but I think it is too risky to do so in Rack 3. However, Rack 3 is still in draft form, and we welcome feedback regarding this.