Bug 64344 - WebSocket: Implement hybi handshake
Summary: WebSocket: Implement hybi handshake
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: Yuta Kitamura
URL:
Keywords:
Depends on:
Blocks: 50099
  Show dependency treegraph
 
Reported: 2011-07-11 23:33 PDT by Yuta Kitamura
Modified: 2011-07-13 22:33 PDT (History)
4 users (show)

See Also:


Attachments
Patch (42.95 KB, patch)
2011-07-12 00:58 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v2 (42.89 KB, patch)
2011-07-12 04:48 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v3 (42.79 KB, patch)
2011-07-12 04:52 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v4 (44.43 KB, patch)
2011-07-13 01:06 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuta Kitamura 2011-07-11 23:33:56 PDT
Implement hybi WebSocket handshake.

http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-10#section-5.1
Comment 1 Yuta Kitamura 2011-07-12 00:58:36 PDT
Created attachment 100447 [details]
Patch
Comment 2 Kent Tamura 2011-07-12 03:43:11 PDT
Comment on attachment 100447 [details]
Patch

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

> LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-no-accept-header_wsh.py:5
> +    # message += 'Sec-WebSocket-Accept: %s\r\n' % compute_accept(request.headers_in['Sec-WebSocket-Key'])[0]

Is this line needed? If not, please remove instead of comment-out.

> Source/WebCore/websockets/WebSocketHandshake.cpp:169
> +    unsigned char key[16];
> +    cryptographicallyRandomValues(key, 16);
> +    return base64Encode(reinterpret_cast<char*>(key), 16);

Please define const variable for the magic number '16'.

> Source/WebCore/websockets/WebSocketHandshake.cpp:180
> +    Vector<uint8_t, 20> hash;
> +    sha1.computeHash(hash);
> +    return base64Encode(reinterpret_cast<const char*>(hash.data()), 20);

We had better have
   const size_t Sha1HashSize = 20;
here or SHA1.h.
Comment 3 Yuta Kitamura 2011-07-12 04:48:55 PDT
Created attachment 100467 [details]
Patch v2
Comment 4 Yuta Kitamura 2011-07-12 04:49:38 PDT
Comment on attachment 100467 [details]
Patch v2

Oops, I forgot to address the first comment. Ignore this patch.
Comment 5 Yuta Kitamura 2011-07-12 04:52:07 PDT
Created attachment 100468 [details]
Patch v3
Comment 6 Yuta Kitamura 2011-07-12 04:54:40 PDT
Comment on attachment 100447 [details]
Patch

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

>> LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-no-accept-header_wsh.py:5
>> +    # message += 'Sec-WebSocket-Accept: %s\r\n' % compute_accept(request.headers_in['Sec-WebSocket-Key'])[0]
> 
> Is this line needed? If not, please remove instead of comment-out.

Removed.

>> Source/WebCore/websockets/WebSocketHandshake.cpp:169
>> +    return base64Encode(reinterpret_cast<char*>(key), 16);
> 
> Please define const variable for the magic number '16'.

Defined "nonceSize".

>> Source/WebCore/websockets/WebSocketHandshake.cpp:180
>> +    return base64Encode(reinterpret_cast<const char*>(hash.data()), 20);
> 
> We had better have
>    const size_t Sha1HashSize = 20;
> here or SHA1.h.

Defined a constant inside this function for now. Added a FIXME comment to tell it really should be moved to SHA1.h.
Comment 7 Yuta Kitamura 2011-07-13 01:06:46 PDT
Created attachment 100640 [details]
Patch v4
Comment 8 Kent Tamura 2011-07-13 21:32:34 PDT
Comment on attachment 100640 [details]
Patch v4

Looks ok.
Comment 9 WebKit Review Bot 2011-07-13 22:32:59 PDT
Comment on attachment 100640 [details]
Patch v4

Clearing flags on attachment: 100640

Committed r90979: <http://trac.webkit.org/changeset/90979>
Comment 10 WebKit Review Bot 2011-07-13 22:33:04 PDT
All reviewed patches have been landed.  Closing bug.