Add support for Sec-WebSocket-Protocol to HTTP::WebSocketHandler#16574
Conversation
Sec-WebSocket-Protocol to HTTP::WebSocketHandler
| requested_protocols = request.headers["Sec-WebSocket-Protocol"]? | ||
| return nil unless requested_protocols | ||
| if supported_protocols = @protocols | ||
| requested_protocols.split(",").each do |protocol| | ||
| protocol = protocol.strip | ||
| if supported_protocols.includes? protocol | ||
| return protocol | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Suggestion: prefer early returns.
| requested_protocols = request.headers["Sec-WebSocket-Protocol"]? | |
| return nil unless requested_protocols | |
| if supported_protocols = @protocols | |
| requested_protocols.split(",").each do |protocol| | |
| protocol = protocol.strip | |
| if supported_protocols.includes? protocol | |
| return protocol | |
| end | |
| end | |
| end | |
| return unless requested_protocols = request.headers["Sec-WebSocket-Protocol"]? | |
| return unless supported_protocols = @protocols | |
| requested_protocols.split(',').each do |protocol| | |
| protocol = protocol.strip | |
| return protocol if protocol.in?(supported_protocols) | |
| end |
|
I'd like to see the possibility to have regex in the supported protocol list. Sometimes you'd like to "catch all", like I have a feeling that even though we have the IANA WebSocket Subprotocol Name Registry not all implementations will follow this strictly. E.g. a client will send If I can configure the handler to accept any matching |
|
@spuun I'm not sure about this... It seems like a relatively simple addition. But then it also adds a bit of complexity and I find it hard to assess. Maybe we could defer this to a follow-up discussion? Websocket subprotocol matching is intentionally very simple. Adding regex to that makes it carry quite a bit more weight and seems overpowered. It may also open opportunities for DoS. Potential alternatives would be to allow specifying a custom handler for subprotocol matching. That would give users full flexibility to implement matching however they want. It might be even more overpowered, though. |
|
A customizable subprotocol handler proc actually sounds great 🤔 |
I decided to go with #protocol and protocols instead of subprotocols as was selected in crystal-lang#16574. The decision was based on the JavaScript WebSocket API which uses #protocol to read the negotiated subprotocol, if any. Part of crystal-lang#15574
…er (#1637) This fixes some parts that #1621 missed. The specs added in #1621 did only verify that the websocket handler picked the right client handler based on `Sec-WebSocket-Protocol`. What they didn't verify was if `Sec-WebSocket-Protocol` was sent back to the client. Because of how Crystal's `HTTP::WebSocketHandler` and `HTTP::Server:Response#upgrade` works I just copied `HTTP::WebSocketHandler` to make my own fixed version. crystal-lang/crystal#16574 would fix this in the stdlib.
…er (#1637) This fixes some parts that #1621 missed. The specs added in #1621 did only verify that the websocket handler picked the right client handler based on `Sec-WebSocket-Protocol`. What they didn't verify was if `Sec-WebSocket-Protocol` was sent back to the client. Because of how Crystal's `HTTP::WebSocketHandler` and `HTTP::Server:Response#upgrade` works I just copied `HTTP::WebSocketHandler` to make my own fixed version. crystal-lang/crystal#16574 would fix this in the stdlib.
…er (#1637) This fixes some parts that #1621 missed. The specs added in #1621 did only verify that the websocket handler picked the right client handler based on `Sec-WebSocket-Protocol`. What they didn't verify was if `Sec-WebSocket-Protocol` was sent back to the client. Because of how Crystal's `HTTP::WebSocketHandler` and `HTTP::Server:Response#upgrade` works I just copied `HTTP::WebSocketHandler` to make my own fixed version. crystal-lang/crystal#16574 would fix this in the stdlib.
| if supported_protocols.includes? protocol | ||
| return protocol | ||
| end |
There was a problem hiding this comment.
Suggestion: simplification
| if supported_protocols.includes? protocol | |
| return protocol | |
| end | |
| return protocol if protocol.in?(supported_protocols) |
Co-authored-by: Julien Portalier <julien@portalier.com>
Closes #15547