Don't advertise support for non-working connection upgrade.

[ioquatix]

As discussed here: socketry/rack-conform#24

[ioquatix]

I have confirmed this fix is sufficient. For more context, in theory, we should also remove upgrade from the HTTP_CONNECTION header, but it's slow to do so. Alternatively, we could just delete everything from the upgrade header but leave it intact. However, I think this PR is good enough.

[casperisfine]

I don't see why Pitchfork should remove headers like this. If the app want to receive such headers it's fine by me.

[ioquatix]

The reason is, because it's nice to correctly advertise the capabilities of the server to the client application, and it's a good idea to follow the RFCs.

In the current scenario, a valid upgrade request can generate an invalid upgrade response due to the following code insisting on connection: close:

pitchfork/lib/pitchfork/http_response.rb

Line 53 in 79ba8e9

buf = "#{start}#{msg ? %Q(#{code} #{msg}) : status}\r\n" \

The RFCs specify how servers should behave:

https://datatracker.ietf.org/doc/html/rfc7230#section-6.7

A server MAY ignore a received Upgrade
header field if it wishes to continue using the current protocol on
that connection. Upgrade cannot be used to insist on a protocol
change.

A server that sends a 101 (Switching Protocols) response MUST send an
Upgrade header field to indicate the new protocol(s) to which the
connection is being switched

Example

run do |env|
  body = proc do |stream|
    stream.write("Hello World")
    stream.close
  end

  if env['HTTP_UPGRADE'] == 'foo'
    [101, {'connection' => 'upgrade', 'upgrade' => 'foo'}, body]
  else
    [200, {}, ['Ok']]
  end
end

Pitchfork will generate the following response:

* Request completely sent off
< HTTP/1.1 101 Switching Protocols
< Date: Tue, 02 Jul 2024 07:28:21 GMT
< Connection: close       *** this is the problem, the value needs to be "upgrade" ***
< upgrade: foo
< 
* Closing connection
Hello World⏎ 

which is invalid according the RFC.

I see various options to rectify this:

1. If you don't want to support connection upgrades, follow the RFCs advice: "A server MAY ignore a received Upgrade" and delete the HTTP_UPGRADE header before forwarding it to the Rack application, as per this PR.
2. Allow the connection: upgrade response header, e.g. sniff for the 101 status code or default to headers['connection'] ||= 'close'.

For the sake of simplicity, option 2 actually seems better to me.

[casperisfine]

it's nice to correctly advertise the capabilities of the server to the client application

IMO this has nothing to do with server capabilities. Upgrade is just an header, I'm forwarding it. I'm not gonna assume what the client and app want to do with it. If someone want to use the protocol upgrade for something compatible with Pitchfork execution model, I see no reason to prevent it.

The RFCs specify how servers should behave:

That's for the application to do that. I'm not gonna lie to the application. It's the app responsability to ignore the upgrade.

I really see no problem here.

[ioquatix]

That's for the application to do that. I'm not gonna lie to the application. It's the app responsability to ignore the upgrade.

How does the app know that the server doesn't support upgrade responses? In other words, how should rack-conform know that pitchfork won't correctly feed the response headers to the client?

[casperisfine]

Just mark it as unsupported in your test matrix.

[ioquatix]

Okay. FYI, I consider this broken behaviour.

[casperisfine]

FYI, I don't.