Bug 82307 – [WebSocket]Browser must fail connection if Sec-WebSocket-Protocol mismatched.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Li Yin]

From RFC 6455: 
http://tools.ietf.org/html/rfc6455#section-4.1
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_.

[Li Yin]

Created attachment 134001 [details]
Patch

[Yuta Kitamura]

Comment on attachment 134001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134001&action=review

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:737
> +            size_t i;
> +            for (i = 0; i < result.size(); ++i) {
> +                if (serverWebSocketProtocol == result[i])
> +                    break;
> +            }
> +            if (i == result.size()) {
> +                m_failureReason = "Error during WebSocket handshake: Sec-WebSocket-Protocol mismatch";
> +                return false;
> +            }

Use Vector<>::contains().

> LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-mismatch-protocol-header.html:21
> +function doTest(curTestNumber)

nit: We usually don't prefer abbreviated names like "cur".

> LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-mismatch-protocol-header.html:40
> +            return ;

nit: The space before semicolon is not necessary.

[Benjamin Poulain]

Comment on attachment 134001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134001&action=review

As for the previous patches. I do not know enough about the state of WebSocket to r+ so I'll leave the final review to a more knowledgeable reviewer.

> Source/WebCore/ChangeLog:9
> +        From RFC6455, if the WebSocket openhanding respond included the mismatched
> +        Sec-WebSocket-Protocol header field, the client must fail the WebSocket Connection.

I think it is useful when you also add a link to the spec.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:729
> +            size_t i;

The variable name "i" is a bad name in this context. We use "i" when it is stopped an obvious what it is.

Here, you should use something like serverWebSocketProtocolIndex or better.

> LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-mismatch-protocol-header.html:24
> +    if (protocolCase[curTestNumber] === "")
> +        ws = new WebSocket(url);

How does this case happen?

All the tests are run with integers. The strict equality does not seems to mach any of the invocation.

> LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-mismatch-protocol-header_wsh.py:13
> +    raise handshake.AbortedByUserException('Abort the connection') # Prevents pywebsocket from sending its own handshake message.

The comment should probably be on the above line if you want to follow python's style.

[Benjamin Poulain]

(In reply to comment #2)
> Use Vector<>::contains().

Hahaha, good point. I missed the obvious :-D

[Li Yin]

(In reply to comment #4)
> (In reply to comment #2)
> > Use Vector<>::contains().
> 
> Hahaha, good point. I missed the obvious :-D
Yeah, good point, thank you!

[Li Yin]

(In reply to comment #3)
> > Source/WebCore/ChangeLog:9
> > +        From RFC6455, if the WebSocket openhanding respond included the mismatched
> > +        Sec-WebSocket-Protocol header field, the client must fail the WebSocket Connection.
> 
> I think it is useful when you also add a link to the spec.
Yeah, I will add the linked address in the ChangeLog

> > LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-mismatch-protocol-header.html:24
> > +    if (protocolCase[curTestNumber] === "")
> > +        ws = new WebSocket(url);
> 
> How does this case happen?
> 
> All the tests are run with integers. The strict equality does not seems to mach any of the invocation.
When the curTestNumber is 0, the protocolCase[curTestNumber] equals "", protocolCase[curTestNumber] === "" returns true.
 
> > +    raise handshake.AbortedByUserException('Abort the connection') # Prevents pywebsocket from sending its own handshake message.
> 
> The comment should probably be on the above line if you want to follow python's style.
This style should be accepted, many current test case used this style, such as long-invalid-header_wsh.py, unmasked-frames_wsh.py etc.

[Li Yin]

Created attachment 134047 [details]
Patch

[Yuta Kitamura]

Comment on attachment 134047 [details]
Patch

Thanks for doing this. This patch looks OK to me. Please wait for comments from a WebKit reviewer.

[Kent Tamura]

Comment on attachment 134047 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134047&action=review

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:728
> +            m_clientProtocol.split(String(", "), result);

Using a string literal ", " looks dangerous.  If we changed the protocol string generation code so that it used " , ", this check would not work.

[Li Yin]

(In reply to comment #9)
> (From update of attachment 134047 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134047&action=review
> 
> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:728
> > +            m_clientProtocol.split(String(", "), result);
> 
> Using a string literal ", " looks dangerous.  If we changed the protocol string generation code so that it used " , ", this check would not work.

It should be matched with joinStrings(protocols, ", "), which can be found in WebSocket::connect.

[Li Yin]

(In reply to comment #9)
> 
> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:728
> > +            m_clientProtocol.split(String(", "), result);
> 
> Using a string literal ", " looks dangerous.  If we changed the protocol string generation code so that it used " , ", this check would not work.

This String was constructed by browser itself, not from network. So it would be safe.
In addition, if the protocol spec will be updated, the constructing methods also need be improved.

[Kent Tamura]

(In reply to comment #11)
> (In reply to comment #9)
> > 
> > > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:728
> > > +            m_clientProtocol.split(String(", "), result);
> > 
> > Using a string literal ", " looks dangerous.  If we changed the protocol string generation code so that it used " , ", this check would not work.
> 
> This String was constructed by browser itself, not from network. So it would be safe.
> In addition, if the protocol spec will be updated, the constructing methods also need be improved.

Yeah, I know.  Someone might want to change ", " in WebSocket::connect() in the future.
We should have a constant variable for the separator, or WebSocket should pass unjoined protocol tokens to WebSocketHandshake.

[Li Yin]

Created attachment 134479 [details]
Patch

[Kent Tamura]

Comment on attachment 134479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134479&action=review

> Source/WebCore/Modules/websockets/WebSocket.cpp:154
> +static const char* webSocketSubProtocolSeperator = ", ";
> +
> +const char* WebSocket::getSubProtocolSeperator()
> +{
> +    return webSocketSubProtocolSeperator;

We don't need webSocketSubProtocolSeparator const variable.
return ", "; is ok.

We don't add "get" prefix for such function.  This should be named as subProtocolSeparator().

[Li Yin]

Created attachment 134481 [details]
Patch

[Kent Tamura]

Comment on attachment 134481 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 134481 [details]
Patch

Clearing flags on attachment: 134481

Committed r112499: <http://trac.webkit.org/changeset/112499>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.