Websocket support for Sec-WebSocket-Protocol response header

Hi guys, I was trying to look into the websocket handhsake and the missing subprotocol header (Sec-WebSocket-Protocol)

to clarify the issue, here is an example of client request headers:

Host: localhost:8080 
Connection: Upgrade 
Pragma: no-cache
Cache-Control: no-cache
Upgrade: websocket
Origin: http://localhost:3000
Sec-WebSocket-Version: 13
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.108 Safari/537.36
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9,he;q=0.8
Cookie: Webstorm-96ae2b1d=39201d8b-8d82-475f-945c-5098b40bc88d; textwrapon=false; textautoformat=false; wysiwyg=textarea; DA-Token=%22f7e3150e-2e16-43b3-8bfb-9632ed38c7b4%22; io=WzwgBGdpfnJGKKEiAAAC
Sec-WebSocket-Key: LK2Qjb+GReKaqQrn4zEvsA==
Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits
Sec-WebSocket-Protocol: graphql-ws

the current server response headers:

Upgrade: websocket
Connection: Upgrade
Sec-WebSocket-Accept: n9TkYN9oiYi2IV7tKQn2QCPQUMk=

according to the https://tools.ietf.org/html/rfc6455#section-1.2 the server should respond with Sec-WebSocket-Protocol, specifiying the supported websocket subprotocol

The relevant code is in http4s-websocket WebsocketHandshake

serverHandshake method gets the request headers and returns the response headers. current implementation takes care of Sec-WebSocket-Accept header:

 /** Checks the headers received from the client and if they are valid, generates response headers */
  def serverHandshake(headers: Traversable[(String, String)]): Either[(Int, String), Seq[(String, String)]] = {
    if (!headers.exists { case (k, v) => k.equalsIgnoreCase("Host")}) {
      Left(-1, "Missing Host Header")
    } else if (!headers.exists { case (k, v) => k.equalsIgnoreCase("Connection") && valueContains("Upgrade", v)}) {
      Left(-1, "Bad Connection header")
    } else if (!headers.exists { case (k, v) => k.equalsIgnoreCase("Upgrade") && v.equalsIgnoreCase("websocket") }) {
      Left(-1, "Bad Upgrade header")
    } else if (!headers.exists { case (k, v) => k.equalsIgnoreCase("Sec-WebSocket-Version") && valueContains("13", v) }) {
      Left(-1, "Bad Websocket Version header")
    } // we are past most of the 'just need them' headers
    else headers.find{ case (k, v) =>
      k.equalsIgnoreCase("Sec-WebSocket-Key") && decodeLen(v) == 16
    }.map { case (_, v) =>
      val respHeaders = Seq(
        ("Upgrade", "websocket"),
        ("Connection", "Upgrade"),
        ("Sec-WebSocket-Accept", genAcceptKey(v))
      )

      Right(respHeaders)
    }.getOrElse(Left(-1, "Bad Sec-WebSocket-Key header"))
  }

here is the relevant section from the RFC related to the subprotocol header:

The |Sec-WebSocket-Protocol| request-header field can be
   used to indicate what subprotocols are acceptable to the client.
   The server selects one or none of the acceptable protocols and echoes
   that value in its handshake to indicate that it has selected that
   protocol.

according to this, the selected subprotocol should come from the application implementor currently serverHandshake only gets the request headers and has no access to response data. The response data comes from http4s-server websocket:

 def WS[F[_]](
      read: Stream[F, WebSocketFrame],
      write: Sink[F, WebSocketFrame],
      status: F[Response[F]])(implicit F: Functor[F]): F[Response[F]] =
    status.map(_.withAttribute(AttributeEntry(websocketKey[F], Websocket(read, write))))

  def WS[F[_]](read: Stream[F, WebSocketFrame], write: Sink[F, WebSocketFrame])(
      implicit F: Monad[F],
      W: EntityEncoder[F, String]): F[Response[F]] =
    WS(read, write, Response[F](Status.NotImplemented).withBody("This is a WebSocket route."))

the default server response is an error NotImplemented this error response is intercepted by http4s-blaze-server WebSocketSupport.renderResponse

val ws = resp.attributes.get(org.http4s.server.websocket.websocketKey[F])

if ws is defined on the response, the code flows throw WebsocketHandshake.serverHandshake and the 501 error never gets to the client.

I would like to get your guidence here as to how would the implementation report the selected subprotocol to WebsocketHandshake.serverHandshake the ws endpoint implementation gets the request

case req@GET -> Root / "ws"  =>

\\[server code that handles ws connection request and creats read stream and write sink]

WS(read, write)

I could add the required headers to the response like this:

WS(d, e).map(r => r.copy(headers = r.headers ++ Headers(List(Header("Sec-WebSocket-Protocol","graphql-ws")))))

that will make sure the subrptocol header exists on the response now blaze-server WebSocketSupport.renderResponse can use the resp headers and merge "Sec-WebSocket-Protocol" with the headers returned by

WebsocketHandshake.serverHandshake(hdrs)

that way the only code change would have to be in WebsocketSupport.scala:50 when StringBuilder builds the actual response "Sec-WebSocket-Protocol" header from resp can be appended.

I will happily take this in case you approve. waiting for your feedback.

Thanks Jonathan

I don't like the parameter name status on WS. It should maybe be called fallbackResponse. That fallback response is rendered if a non-websocket request is made to the endpoint. Adding the Sec-WebSocket-Protocol to that response would result in those headers being shown in the fallback response.

We could add an protocol: Option[String] parameter to WS and the WebSocket case class to indicate which protocol was selected. This could then be rendered by the WebSocketSupport. This fits your problem well, but is rather special case.

The RFC mentions that other headers can be present. We could instead add headers as an additional parameter to WS. To string them through, it could be added on to the WebSocket case class, though it's questionable why this model would include only some response headers and not, say, Sec-WebSocket-Accept. Another option would be to string them through in a new response attribute, and have the WebSocketSupport look for and render them.

What do you think?

I think fallbackResponse exposes implementation details. as a user, I understood response as something that affects the WS handshake response (seeing that the default is a NotImplemented error came as a surprise). I'm not sure if that response is ever returned to the caller. if ws attribute is defined on the response, the code will always try to handshake and in case of an error, a new BadRequest response is rendered.

I see your point of Sec-WebSocket-Accept, being another header that is handled differently than Sec-WebSocket-Protocol. however, from the perspective of an API user, I expect Sec-WebSocket-Accept to be taken care of internally by the library. I wouldn't know how to provide its value. This is why I tend towards your protocol: Option[String] suggestion, but as I see the response as a natural piece of the WS API, I feel it is natural to use that response as the container of any ws handshake attribute that may come in the future.

If we see that response as WS-Handshake-Response, it suddenly feels natural to add any ws handshake related header to that response. In the scope of blaze-server WebSocketSupport.renderResponse, looking for specific ws handshake headers seems legit.

My perspective is limited to the very few code fragments I reviewed, I will totally follow your guidance here.

I think you've got the gist, but I'll walk through that with links, if only to refresh my own memory.

The signature of an HttpService requires that we return an F[Response[F]]. There is no special "web socket service". So we need to embed the WebSocket as an attribute in a response.

If there is no web socket on the response, it is rendered here. This is so the same service can support web socket and non-websocket routes.

If the service returned a response with a web socket, but the request was not a web socket request, then we render the response here. This is where you would typically see the NotImplemented response: someone made an HTTP request to an endpoint designed for web sockets. You can embed a web socket on any response, so an endpoint might just as well do something like SSE or a static view of the resource at the URI.

If the service returned a response with a web socket, and it's a web socket request, we try the handshake. If the handshake fails, we return a canned response. There isn't really any way to customize that, but that's a whole separate discussion.

If the service returns a response with a websocket, and it's a web socket request, and the handshake succeeds, we do the upgrade. This is the response you want to customize, and can't.

Now, if I understand your proposal, you are suggesting that the response that contains the web socket should not be the fallback response, but rather the successful response to a handshake. Maybe that's legit. But allowing that successful response to have anything but a 101 Status or an empty EntityBody is non-sensical. The only legitimate thing to customize on this response is headers.

How about this:

val DefaultNonWebSocketResponse = Response[F](Status.NotImplemented).withBody("This is a WebSocket route.")
val DefaultHandshakeFailureResponse = Response[F](Status.BadRequest).withBody("WebSocket handshake failed.")

case class WebSocketContext[F](
   webSocket: WebSocket[F],
   headers: Headers,
   failureResponse: F[Response[F]]
)

def websocketKey[F[_]]: AttributeKey[WebsocketContext[F]] =
  Keys.WebSocket.asInstanceOf[AttributeKey[WebsocketContext[F]]]

def WebSocketResponse(
    send: Stream[F, WebSocketFrame],
    receive: Sink[F, WebSocketFrame],
    headers: Headers.empty, // Sec-WebSocket-Protocol, Cookies, etc. go here
    onNonWebSocketRequest: F[Response[F]] = DefaultNonWebSocketResponse,
    onHandshakeFailure: F[Response[F]] = DefaultHandshakeFailureResponse): F[Response[F]] = {

    onNonWebSocketRequest.map(
      _.withAttribute(AttributeEntry(websocketKey[F], WebsocketContext(
        WebSocket(send, receive),
        headers,
        onHandshakeFailure))))
}

That's still the same basic model where the fallback response is the container, so things slide through on servers that don't support websockets or to clients that don't request a websocket. But now:

You can customize headers on the happy path without setting invalid statuses or bodies.
You can customize the handshake failure message
Everything WebSocketSupport needs for the above is stuffed into one attribute.

And your route looks something like this:

case GET -> Root / "socket" =>
   WebSocketResponse(send, receive, Headers(`Sec-WebSocket-Protocol`("chat"))))

Thanks for this walkthrough. I like your solution and I will start working on it.

will probably ping you on gitter for few technical questions if thats ok.

http4s / http4s-websocket

Websocket support for Sec-WebSocket-Protocol response header #7