Open foolip opened 4 years ago
@k7z45 you mentioned an implementation concern about the layering arising from limiting to one concurrent connection, but I don't think I understood it fully. Based on your prototyping work so far, what can you say about the merits of having or not having a limit on the number of connections?
Previously raised considerations that I can recall are:
The answer we have been tending to so far is that state is on the session, that only one connection is supported, but that it's possible to reconnect if a connection is dropped.
Yes, I agree with that. It doesn't make much sense to me to allow > 1 connection per session. If there was a use case for more than one it could be implemented using some middleware to combine multiple connections into a single connection down into the browser. But I can't immediately think of a use case for that.
Implementation concern wise, if we need to limit to one concurrent connection, we will need to keep track of a "global" mapping from session id to connection id. The workflow goes: http server serves an init session request -> session thread handles request and upon success modify session map with connection id -1 -> upon WebSocket connection request verify and modify session map to corresponding connection id -> upon WebSocket connection close, modify connection id back to -1 -> upon session end, erase the session id. The complication comes from http server on a different thread from where the session is created and passing the mapping and host information around. On the other hand, if we don't limit the connection and always allow connection, then we might not need to keep track of the session id to connection id mapping.
I'm not sure I fully understand. My mental model is as follows: On the thread that handles connections you need to know the active session ids in any case because otherwise you can't limit connections to active sessions; not doing that seems like a security concern. If you already know the active sessions, then you can just have a mapping on that thread from a session id to an object representing the connection, and refuse the connection if there's an existing entry in the map. What am I missing?
What I had in mind was to accept any session id as long as the path matches the pattern. I have not considered the security implication for doing that. I think it's still possible to hijack a session with the connection limit to 1(scanning session id and connect to it via WebSocket). But if there is legitimate security concern, then we would have to keep track of the session to connection information either way. add @JohnChen0
I believe the implementation should keep track of active session IDs regardless of whether multiple WebSocket connections per session are supported or not. The current prototype in ChromeDriver worked around this by allowing creation of WebSocket connection with any session ID, even if the specified session ID doesn't exist. I think it isn't a security issue, because any further commands received from such connections will be rejected. But we should report bad session ID while establishing the connection, instead of later when the client starts to use the connection.
As to the original question, I think we should start simple and support only one WebSocket connection per session. Supporting multiple connections will likely add complexity with little benefits.
This is an inline issue in https://w3c.github.io/webdriver-bidi/#transport.
In ChromeDriver establishing BiDi WebSocket connection Design by @k7z45 this question has come up and we should resolve it on the spec side so that it can be implemented and tested.