Upgrader does not close connection if Sec-WebSocket-Protocol headers are mismatched (improper handling of response headers)

[jjakob]

Say I want to limit my allowed protocols server-side to only the specified ones. Using the chat example code, I add:

respHeader := make(http.Header)
respHeader.Add("Sec-WebSocket-Protocol", "gorilla-chat-example")
conn, err := upgrader.Upgrade(w, r, respHeader)

My understanding is that this is the way the documentation says it should be done.

However, the results are different and the connection is still upgraded, though it then later closes (assuming it's browser-side due to mismatching protocols in the headers), but not before successfully passing through and registering in the hub.

Here are the headers:
request headers:

GET /ws HTTP/1.1
Host: localhost:8080
User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,/;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Sec-WebSocket-Version: 13
Origin: http://localhost:8080
Sec-WebSocket-Protocol: gorilla-chat-example2
Sec-WebSocket-Extensions: permessage-deflate
Sec-WebSocket-Key: +kU4w5hYMAwpNrJiD1fgNQ==
Connection: keep-alive, Upgrade
Pragma: no-cache
Cache-Control: no-cache
Upgrade: websocket

response headers:
HTTP/1.1 101 Switching Protocols
Upgrade: websocket
Connection: Upgrade
Sec-WebSocket-Accept: kKw5eJDu4h01xApbZpKNVPKqlSE=
Sec-WebSocket-Protocol: gorilla-chat-example

RFC on this topic: https://tools.ietf.org/html/rfc6455#section-4.2.2
(...) The value chosen MUST be derived
from the client's handshake, specifically by selecting one of
the values from the |Sec-WebSocket-Protocol| field that the
server is willing to use for this connection (if any). If the
client's handshake did not contain such a header field or if
the server does not agree to any of the client's requested
subprotocols, the only acceptable value is null. The absence
of such a field is equivalent to the null value (meaning that
if the server does not wish to agree to one of the suggested
subprotocols, it MUST NOT send back a |Sec-WebSocket-Protocol|
header field in its response). (...)

So returning a Sec-WebSocket-Protocol header in the response that is different than in the client's request does not conform to this RFC. Though the exact behaviour that the server must follow is not defined (or I can't find it), I'd assume it should send a 4xx error response instead, using "return u.returnError(w, r, http.StatusBadRequest, ...)" like in the other error cases.

The relevant checking is done in server.go:147, 182, 193, 201.

I've also tried setting it in the Upgrader struct:

var upgrader = websocket.Upgrader{
	ReadBufferSize:  1024,
	WriteBufferSize: 1024,
	Subprotocols:    []string{"gorilla-chat-example"},
}

but then the behavior is even weirder, with the response including no protocol header at all if no matching protocol is found, but the connection establishes and works without errors. (selectSubprotocol does indeed return a null string, as it should. Perhaps a third case should be added, one if the protocols are mismatched and the server should return an error?)

[jjakob]

@garyburd @kisielk
I've submitted a PR with the fix: #401

[garyburd]

So returning a Sec-WebSocket-Protocol header in the response that is different than in the client's request does not conform to this RFC.

When the application specified Sec-WebSocket-Protocol header is used, it is assumed that the application handled the subprotocol negotiation and did so correctly.

Is there another scenario where upgrade will return a subprotocol different from the client's request?

Though the exact behaviour that the server must follow is not defined (or I can't find it), I'd assume it should send a 4xx error response instead, using "return u.returnError(w, r, http.StatusBadRequest, ...)" like in the other error cases.

EDIT: this section updated

The builtin negotiation assumes that it's OK to proceed with no negotiated subprotocol. In this case, the client and server use a default protocol.

If the server application handles the negotiation, then the server application can choose to return a 4xx response or proceed with a default protocol.

I've also tried setting it in the Upgrader struct: Subprotocols: []string{"gorilla-chat-example"}, but then the behavior is even weirder, with the response including no protocol header at all if no matching protocol is found, but the connection establishes and works without errors.

This is working as intended. The client and server did not agree on a subprotocol, so they proceed with a default protocol.

If the server application requires a subprotocol, then the server application should implement the negotiation.

[jjakob]

When the application specified Sec-WebSocket-Protocol header is used, it is assumed that the application handled the subprotocol negotiation and did so correctly.

Makes sense. However it would be nice to mention this in the documentation, as it's unclear when (and why) they should be defined in the struct Subprotocols or passed as an argument to the Upgrade function call.

Is there another scenario where upgrade will return a subprotocol different from the client's request?

Not that I could see, no.

The builtin negotiation assumes that it's OK to proceed with no negotiated subprotocol. In this case, the client and server use a default protocol.

So, if no match is found between the client's requested subprotocols and the ones defined in the struct Subprotocols variable, a call to Upgrade will still upgrade the conection, but without any subprotocols header (or with a header, but an empty one, e.g. "")?

I've also tried setting it in the Upgrader struct: Subprotocols: []string{"gorilla-chat-example"}, but then the behavior is even weirder, with the response including no protocol header at all if no matching protocol is found, but the connection establishes and works without errors.

This is working as intended. The client and server did not agree on a subprotocol, so they proceed with a default protocol.

None of this is documented, which is why I was having such a hard time understanding the way it's supposed to work. It would be very helpful to get this documented, especially to someone who's never worked with Websockets and has no idea how the negotiation process should work. Rummaging through the code (unhelpfully) and reading the RFCs (which tell you how the protocol should behave, but not the details) can only get you so far...

Guess I'll be moving the negotiation into the application code.

Thank you for your help, sorry for the inconvenience!

[garyburd]

The documentation does need improvement. I'll open an issue for that.

What is your application scenario? Do you want to require a match between the client requested protocols and the server provided protocols and fail if there's no match?

[jjakob]

Thanks, I'll take a look if I can contribute.

Yup, the scenario is to enforce a match, the server has to serve two types of clients and fail if the client requests no subprotocol. So basically the app would have to parse the request headers and see if there's a subprotocol match before calling Upgrade. On the client side (in the browser javascript), parse the response headers and close the connection if the server offers an incompatible subprotocol. Thus there's no way in which an incompatible client could connect and send garbage to the app (it's more of a safety measure than anything else)

[garyburd]

The builtin support was designed for the scenario where clients and servers continue to operate with each other as new application protocols are defined over time.

How about adding a "require match" flag to Upgrader?

[jjakob]

Seems reasonable, adding it to the struct would be the most logical place to put it, as it would be non-breaking.
If undefined or false, it would remain backwards-compatible, if true, it would behave as per my PR.
Also, in my PR, adding new subprotocols to Upgrader wouldn't break existing clients as they'd still match on one of the included protocols. Only in the case that a client requests something that the server doesn't have would it return en error. Perhaps the logic could be modified as to make ResponseHeader's protocol override the matching function (now selectSubprotocol still wants a match between the client's requested and responseHeader's subprotocols).

[garyburd]

In my proposal, the server returns a 4xx response when the flag is true, Upgrader.Subprotcols != nil and there's no match with client requested protocols.

As I think about it, it's better to specify subprotocol negotiation options with an int instead of a bool. This allows future enhancements without adding yet another field. The value 0 uses the current logic.

Perhaps the logic could be modified as to make ResponseHeader's protocol override the matching function

The change in priority can break a server application that specifies both Upgrader.Subprotocols and the Sec-Websocket-Protocol response header.

Upgrade should return error when the Upgrader.Subprotocols != nil && responseHeader.Get("Sec-Websocket-Protocol") != "", but this change can also break applications.

Only in the case that a client requests something that the server doesn't have would it return en error

Can you give a scenario where this is useful? This prevents a new client from working with an old server.

[ghost]

How about adding the following field to Upgrader?

    // NegotiateSubprotocol returns the negotiated subprotocol for the handshake
    // request. If the returned string is "", then the the Sec-Websocket-Protocol header
    // is not included in the handshake response.  If the function returns an error, then
    // Upgrade responds to the client with http.StatusBadRequest. 
    //
    // If this function is not nil, then the Upgrader.Subportocols field is ignored.
    NegotiateSubprotocol func(r *http.Request) (string, error)

[garyburd]

@stevenscott89 Yeah, that plus function that implements current negotiation is a better design. I don't like that it deprecates the Subprotocols field, but so be it.

The field documentation should mention the websocket.Subprotocols method and that returned protocol must be on of the protocols requested by the client or "".