Improve WebSocket handshake validation #5327
Conversation
straight-shoota
force-pushed
the
jm-fix-websocket-key
branch
from
6a287f8
to
a2861a7
Compare
| @@ -285,4 +292,12 @@ class HTTP::WebSocket::Protocol | |||
|
|
|||
| raise ArgumentError.new("No host or path specified which are required.") | |||
| end | |||
|
|
|||
| def self.key_challenge(key) | |||
There was a problem hiding this comment.
Assume this is public because is used above (Protocol.key_challenge). In that case, perhaps will be better add a :nodoc: marker to avoid this showing up in the documentation?
Thank you.
There was a problem hiding this comment.
Yes, this needs to be accessible from WebSocketHandler.
The entire Protocol class is just internal implementation and not documented.
There was a problem hiding this comment.
Ah, missed that from the PR diff, thank you for your response!
|
Could this get a review from @ysbaddaden? |
| response = context.response | ||
|
|
||
| version = context.request.headers["Sec-WebSocket-Version"]? | ||
| unless version == WebSocket::Protocol::VERSION.to_s |
There was a problem hiding this comment.
Let's have WebSocket::Protocol::VERSION_STRING too, or something like that. I'd like to avoid a to_s call for each socket upgrade.
There was a problem hiding this comment.
Good catch 👍 It's only used as a string anyway, so I'll just change the datatype of the constant.
straight-shoota
force-pushed
the
jm-fix-websocket-key
branch
from
a2861a7
to
614bd13
Compare
|
@asterite Care to give this a ✔️ ? |
|
It seems that this PR wasn't rebased onto |
|
I think the commit should be rollbacked and reopen the PR. I just reach the issue while branching master. |
This reverts commit d3be42e.
|
Reverted the PR, |
|
I don't think you can reopen a merged PR. That'd be awkward. It's #6027 now. |
This PR adds the following changes in accordance with RCF 6455]
WebSocketHandlerverifies thatSec-WebSocket-Keyis present (previously this unchecked fetch resulted in aKeyError) and fails with400 Bad Requestif notWebSocketHandlerverifies thatSec-WebSocket-Versionis present and equals13and fails with426 Upgrade Requiredif notWebSocket::Protocolverifies thatSec-WebSocket-Acceptis present and contains the correct challenge response