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_.
Created attachment 134001 [details] Patch
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 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.
(In reply to comment #2) > Use Vector<>::contains(). Hahaha, good point. I missed the obvious :-D
(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!
(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.
Created attachment 134047 [details] Patch
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 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.
(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.
(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.
(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.
Created attachment 134479 [details] Patch
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().
Created attachment 134481 [details] Patch
Comment on attachment 134481 [details] Patch ok
Comment on attachment 134481 [details] Patch Clearing flags on attachment: 134481 Committed r112499: <http://trac.webkit.org/changeset/112499>
All reviewed patches have been landed. Closing bug.