Bug 82307 - [WebSocket]Browser must fail connection if Sec-WebSocket-Protocol mismatched.
Summary: [WebSocket]Browser must fail connection if Sec-WebSocket-Protocol mismatched.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-27 01:26 PDT by Li Yin
Modified: 2012-03-28 21:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.45 KB, patch)
2012-03-27 01:49 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (7.36 KB, patch)
2012-03-27 06:52 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (9.84 KB, patch)
2012-03-28 19:09 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (9.06 KB, patch)
2012-03-28 19:31 PDT, Li Yin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Li Yin 2012-03-27 01:26:45 PDT
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_.
Comment 1 Li Yin 2012-03-27 01:49:41 PDT
Created attachment 134001 [details]
Patch
Comment 2 Yuta Kitamura 2012-03-27 02:37:59 PDT
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.
Comment 3 Benjamin Poulain 2012-03-27 02:39:25 PDT
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.
Comment 4 Benjamin Poulain 2012-03-27 02:41:13 PDT
(In reply to comment #2)
> Use Vector<>::contains().

Hahaha, good point. I missed the obvious :-D
Comment 5 Li Yin 2012-03-27 05:32:33 PDT
(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!
Comment 6 Li Yin 2012-03-27 06:38:39 PDT
(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.
Comment 7 Li Yin 2012-03-27 06:52:43 PDT
Created attachment 134047 [details]
Patch
Comment 8 Yuta Kitamura 2012-03-27 18:43:04 PDT
Comment on attachment 134047 [details]
Patch

Thanks for doing this. This patch looks OK to me. Please wait for comments from a WebKit reviewer.
Comment 9 Kent Tamura 2012-03-28 01:22:22 PDT
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.
Comment 10 Li Yin 2012-03-28 01:48:02 PDT
(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.
Comment 11 Li Yin 2012-03-28 01:53:55 PDT
(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.
Comment 12 Kent Tamura 2012-03-28 02:11:32 PDT
(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.
Comment 13 Li Yin 2012-03-28 19:09:21 PDT
Created attachment 134479 [details]
Patch
Comment 14 Kent Tamura 2012-03-28 19:14:39 PDT
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().
Comment 15 Li Yin 2012-03-28 19:31:06 PDT
Created attachment 134481 [details]
Patch
Comment 16 Kent Tamura 2012-03-28 20:05:40 PDT
Comment on attachment 134481 [details]
Patch

ok
Comment 17 WebKit Review Bot 2012-03-28 21:22:29 PDT
Comment on attachment 134481 [details]
Patch

Clearing flags on attachment: 134481

Committed r112499: <http://trac.webkit.org/changeset/112499>
Comment 18 WebKit Review Bot 2012-03-28 21:22:36 PDT
All reviewed patches have been landed.  Closing bug.