Missing headers in HTTP/1.x upgrade response

[wtn]

I've been using Async::Websocket::Adapters::Rails (source) with Puma and it's very useful. But when Puma is behind HAProxy, WebSocket connection negotiation fails with status code 502.

I found this HAProxy blog post, which provides the following guidance:

The most important part is the Connection: Upgrade header which [lets the client inform the server that it wants to change protocols to the one indicated] by [the] Upgrade: websocket header.

This requirement agrees with RFC 7231 section 6.2.2, which says

The server MUST generate an Upgrade header field in the response that indicates which protocol(s) will be switched to immediately after the empty line that terminates the 101 response.

The MDN Web Docs have a note that

Connection: upgrade must be set whenever Upgrade is set.

I added both headers to the HTTP/1.x upgrade response in https://github.com/wtn/async-websocket/commit/672d54bf1f0af5828c7e16775b588053700508f5, and that resolved the problem.

However, this change resulted in warnings when running the tests, so I opened this issue to discuss.

% bake test
 1.41s     warn: Protocol::Rack::Response [ec=0xcf8] [pid=95526] [2023-05-03]
               | Ignoring protocol-level headers: [["connection", "upgrade"], ["upgrade", "websocket"]]
 1.41s     warn: Protocol::Rack::Response [ec=0xd70] [pid=95526] [2023-05-03]
               | Ignoring protocol-level headers: [["connection", "upgrade"], ["upgrade", "websocket"]]
 1.42s     warn: Protocol::Rack::Response [ec=0xdd4] [pid=95526] [2023-05-03]
               | Ignoring protocol-level headers: [["connection", "upgrade"], ["upgrade", "websocket"]]

[ioquatix]

Upgrading a connection is HTTP version specific. HTTP/0.9 - HTTP/1.1 use the upgrade: websocket response header to indicate the upgrade was successful while HTTP/2 uses :protocol: websocket pseudo header.

Because of this, we need to generate a HTTP version specific response.

In Async::WebSocket, this is handled by the bifurcation UpgradeRequest/UpgradeResponse and ConnectRequest/ConnectResponse (source).

To avoid having to specify these headers explicitly, Protocol::HTTP::Request#protocol and Protocol::HTTP::Response#protocol exist. There are some subtle differences:

• HTTP/1.x will set the request protocol and it will be an array with more than one protocol. You must return the selected protocol in Response#protocol. To accept the "upgrade" with selected protocol, you must respond with 101 Switching Protocols and a suitable upgrade header.
• HTTP/2 sets the request protocol which is only a single value, and may ignore the Response#protocol. To accept the "connect" with protocol, you just response with 200 OK.

So, the reason for the Request#protcol and Response#protocol is to hide some of this complexity from the user. However it's not a perfect abstraction as explained above the actual behaviour is quite different between HTTP/1 and HTTP/2.

This is why you get such a warning: Protocol::Rack::Response does not want you setting headers that affect the underlying protocol, and instead wants you to use the metadata fields it understands, which it will map to the underlying version specific behaviour.

In this case, what you want is "rack.protocol" => "websocket" as a response header. If you are interested, this is discussed in more detail here: rack/rack#1946 however I have not had success getting other servers on board, yet.

There are tests here which validate the correct behaviour of a wide range of servers: https://github.com/socketry/rack-conform/blob/main/test/rack/conform/websocket.rb however this will only work on Rack 3+.

In terms of your code, the Response#protocol is set as the last argument here: https://github.com/wtn/async-websocket/commit/672d54bf1f0af5828c7e16775b588053700508f5#diff-73bff7d916a1d7037490608f2accca552ba2eb36ee8a3e5dbd119e359c21d081R30

The way that gets mapped back out is by Protocol::Rack here: https://github.com/socketry/protocol-rack/blob/708582d65242ee0d1a512073a631e798a49f03ba/lib/protocol/rack/adapter/rack2.rb#L125-L128

The best behaviour would be for puma to detect this and set the appropriate connection and upgrade headers (as outlined in the above issue for Rack).

[ioquatix]

If you are running on Puma (and Rack 3+), the following compatibility middleware should work:

class RackProtocolToConnectionUpgrade
  def initialize(app)
    @app = app
  end

  def call(env)
    status, headers, body = @app.call(env)
    if protocol = headers.delete('rack.protocol')
      headers['upgrade'] = protocol
      headers['connection'] = 'upgrade'
    end
    
    return status, headers, body
  end
end

I only eyeballed the code, but this should be the gist of it. If you are on Rack 2.x, you need to use Rack::Utils::HeaderHash to extract the headers.

[wtn]

Thank you for your thorough explanation.

I can confirm that the compatibility middleware resolves the issue. For safety, I'm only setting the headers when env['SERVER_PROTOCOL'] is HTTP/1.1.

Now the concept described in rack/rack#1946 totally makes sense. I agree in principle that the app server (Puma, Falcon, WEBrick, etc.) should be required to manage connection and other hop-by-hop header fields. So I support making rack.protocol a required part of the Rack spec.

[ioquatix]

Please add your feedback to that issue if you have time.

I'm happy this fixed your issue.