Summary: | WebSocket: Implement hybi handshake | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yuta Kitamura <yutak> | ||||||||||
Component: | WebCore Misc. | Assignee: | Yuta Kitamura <yutak> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, ap, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 50099 | ||||||||||||
Attachments: |
|
Description
Yuta Kitamura
2011-07-11 23:33:56 PDT
Created attachment 100447 [details]
Patch
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. Created attachment 100467 [details]
Patch v2
Comment on attachment 100467 [details]
Patch v2
Oops, I forgot to address the first comment. Ignore this patch.
Created attachment 100468 [details]
Patch v3
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. Created attachment 100640 [details]
Patch v4
Comment on attachment 100640 [details]
Patch v4
Looks ok.
Comment on attachment 100640 [details] Patch v4 Clearing flags on attachment: 100640 Committed r90979: <http://trac.webkit.org/changeset/90979> All reviewed patches have been landed. Closing bug. |