Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sec-WebSocket-Protocol #81

Closed
navossoc opened this issue Jun 9, 2019 · 6 comments
Closed

Sec-WebSocket-Protocol #81

navossoc opened this issue Jun 9, 2019 · 6 comments

Comments

@navossoc
Copy link
Contributor

navossoc commented Jun 9, 2019

Hi there!
I'm not exactly sure, but I think this behavior is wrong (or misleading).

Let me explain:
I'm testing the Sec-WebSocket-Protocol, trying to require a custom protocol.

	u := ws.Upgrader{
		Protocol: func(protocol []byte) bool {
			log.Println("Protocol", string(protocol))
			return string(protocol) == "binary"
		},
	}

If my client is using the protocol binary everything works perfectly (as expected).
As soon I change the protocol on my client to binary2 for example, my function Protocol evaluates to false and the client rejects the connection...

The problem happens only when I omit the Sec-WebSocket-Protocol, because the client didn't sent any value, the Protocol func on the server is never called, so the connection is accepted.

The request MAY include a header field with the name |Sec-WebSocket-Protocol|. If present, this value indicates one or more comma-separated subprotocol the client wishes to speak, ordered by preference. The elements that comprise this value MUST be non-empty strings with characters in the range U+0021 to U+007E not including separator characters as defined in [RFC2616] and MUST all be unique strings. The ABNF for the value of this header field is 1#token, where the definitions of constructs and rules are as given in [RFC2616].

Let me know if you need more details.

How can I enforce a subprotocol? The only way I can think about is using the handshake return value on Upgrader.Upgrade and then closing the connection.

@gobwas
Copy link
Owner

gobwas commented Jun 9, 2019

Hi @navossoc,

From the RFC:
6. If the response includes a |Sec-WebSocket-Protocol| header field
and this header field indicates the use of a subprotocol that was
not present in the client's handshake (the server has indicated a
subprotocol not requested by the client), the client MUST Fail
the WebSocket Connection
.

@gobwas gobwas closed this as completed Jun 9, 2019
@gobwas
Copy link
Owner

gobwas commented Jun 9, 2019

In other words you can not “enforce” to use some subprotocol which client does not know. This mechanism is intended to make both sides of connection agreed on the subprotocol.

@gobwas
Copy link
Owner

gobwas commented Jun 9, 2019

Also, if your client knows how to deal with both binary and binary2 subprotocols, they could both passed as a comma separated list (ordered be preference) during handshake.

@navossoc
Copy link
Contributor Author

navossoc commented Jun 9, 2019

Thanks for your answer, but I'm still not sure if this is totally accurate.
I know, if the client can't handle the subprotocol it can't be enforced, but at least the connection should not be completed.

For example, this websocket server:

wscat --connect wss://upp203a.ig.com/lightstreamer (fail/403) (I meant this case, server expect a subprotocol, client didn't sent any)
wscat --connect wss://upp203a.ig.com/lightstreamer -s randomprotocol (fail/403) (wrong subprotocol, ok, already handled by gobwas/ws)
wscat --connect wss://upp203a.ig.com/lightstreamer -s js.lightstreamer.com (succeed/101)

You can check it here: https://www.ig.com/us

In this case, the client didn't sent a Sec-WebSocket-Protocol as per RFC it MAY send it or not. So it is ok.
Now, the server only knows how to speak subprotocol js.lightstreamer.com so it send back a 403.

That is what I was trying to achieve.

To be honest, I believe I didn't read in the RFC a case like this.

Do you know any other websocket servers that are using a subprotocol? I'll try to find more tomorrow for testing.

@gobwas
Copy link
Owner

gobwas commented Jun 9, 2019

It looks like OnBeforeUpgrade callback will work for you – negotiated subprotocol could be checked there and appropriate error may be returned.

@gobwas
Copy link
Owner

gobwas commented Jun 9, 2019

I mean something like this:

func upgrade(conn net.Conn) error {
    var negotiatedBinary bool
    _, err := (ws.Upgrader{
        OnBeforeUpgrade: func() (ws.HandshakeHeader, error) {
            if !negotiatedBinary {
                return nil, ws.RejectConnectionError(
                    ws.RejectionCode(403),
                )
            }
            return
        },
        Protocol: func(p []byte) bool {
            if bytes.Equal(p, []byte("binary2")) {
                negotiatedBinary = true
                return true
            }
            return false
        },
    }).Upgrade(conn)
    return err
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants