ngauthier / tubesock

Websocket interface on Rack Hijack w/ Rails support
MIT License
620 stars 43 forks source link

Proper way to close a connection? #23

Open tyrbo opened 10 years ago

tyrbo commented 10 years ago

I've noticed if I access the hijacked controller from directly (and not through JS, where I call socket.close), the resource will sit loading forever.

How can I go about properly closing a socket from the hijacked controller? I've tried calling the close method, but run into an IOError issue. Is the close method the proper way of closing a socket?

My code is very similar to what you used for your chat example: https://github.com/ngauthier/sock-chat/blob/master/app/controllers/chat_controller.rb

ngauthier commented 10 years ago

Did you try tubesock.close from https://github.com/ngauthier/tubesock/blob/master/lib/tubesock.rb#L70 ?

tyrbo commented 10 years ago

Yep. When testing with a Puma server, Puma would end up dying from an uncaught IOError when I closed it. Rescuing it saved it from crashing, but yeah.

tyrbo commented 10 years ago

Might have been my code. I adjusted it and no longer get the IOError.

However, again if I open it in a browser it still hangs as if it's loading, even after I've sent the close method. Is there any way around that? I feel like the request shouldn't sit at pending after I've triggered the close method through tubesock.

I'm just concerned that someone could come along and send a request and have it just sit there, wasting resources. I can add some timeouts on the web server itself, but if there's a way to fix it in the Rails side of things, that'd be awesome.

ngauthier commented 10 years ago

I am not sure how a websocket endpoint is supposed to behave when a non-websocket connection comes in. Does it behave properly when you make a normal JS websocket connection?

tyrbo commented 10 years ago

I'm pretty new with websockets, but I removed the close call from the JavaScript side, and I still see the PING/PONG frames coming through when I watch the request in Chrome.

With the close call added back to JavaScript, the frames stop coming in as expected.

It seems like the connection remains open when I call tubesock.close whether I'm doing it through websockets or through a normal request.

ngauthier commented 10 years ago

Are you sure you're calling the close, and that it is the only socket open?

Also, please log when the socket opens, because maybe the websocket is re-establishing the connection when it's closed by the server?

I'm not really sure either, just trying to give some ideas.

On Tue, Mar 11, 2014 at 2:59 PM, tyrbo notifications@github.com wrote:

I'm pretty new with websockets, but I removed the close call from the JavaScript side, and I still see the PING/PONG frames coming through when I watch the request in Chrome.

With the close call added back to JavaScript, the frames stop coming in as expected.

It seems like the connection remains open when I call tubesock.close whether I'm doing it through websockets or through a normal request.

— Reply to this email directly or view it on GitHubhttps://github.com/ngauthier/tubesock/issues/23#issuecomment-37335718 .

tyrbo commented 10 years ago

Narrowed things down. I had originally been calling tubesock.close from a thread. Clearly a mistake. It would output the "Closed" message from the onclose handler, but it seems like the connection wasn't actually closed.

I've removed the threading stuff I had added and just called tubesock.close as soon as we are connected.

Back to the Puma crash:

Exiting /Users/Jon/.rvm/gems/ruby-2.1.0/gems/tubesock-0.2.2/lib/tubesock.rb:92:in `select': closed stream (IOError) from /Users/Jon/.rvm/gems/ruby-2.1.0/gems/tubesock-0.2.2/lib/tubesock.rb:92:in 'each_frame' from /Users/Jon/.rvm/gems/ruby-2.1.0/gems/tubesock-0.2.2/lib/tubesock.rb:63:in 'block in listen'

tyrbo commented 10 years ago

Seems like the current released version (0.2.2) doesn't have the rescues that the current github version has - including IOError.

tyrbo commented 10 years ago

Problem solved when using the latest from github. New issue:

/Users/Jon/.rvm/gems/ruby-2.1.0/bundler/gems/tubesock-1952e99cbae4/lib/tubesock.rb:96:in `select': Bad file descriptor (Errno::EBADF)

Here is the code in question: https://gist.github.com/tyrbo/83af7a6bc9d79c3c4884

Nothing too complicated, and the "onclose" handler kills the thread. Am I doing something wrong?

ngauthier commented 10 years ago

I'm not sure. It looks ok to me.

On Tue, Mar 11, 2014 at 3:57 PM, tyrbo notifications@github.com wrote:

Problem solved when using the latest from github. New issue:

/Users/Jon/.rvm/gems/ruby-2.1.0/bundler/gems/tubesock-1952e99cbae4/lib/tubesock.rb:96:in `select': Bad file descriptor (Errno::EBADF)

Here is the code in question: https://gist.github.com/tyrbo/83af7a6bc9d79c3c4884

Nothing too complicated, and the "onclose" handler kills the thread. Am I doing something wrong?

— Reply to this email directly or view it on GitHubhttps://github.com/ngauthier/tubesock/issues/23#issuecomment-37342269 .

jonathanhoskin commented 10 years ago

I've opened a PR to make tubesock.close less crashy on Puma.

teddythetwig commented 8 years ago

From what I understand on this thread, the main issue here is that a hijacked controller method is not responding properly to a regular HTTP request. According to the WebSocket specification, we should only be expecting an HTTP Upgrade request, which is not what you get when connecting to the WS endpoint over regular HTTP.

The opening handshake is intended to be compatible with HTTP-based server-side software and intermediaries, so that a single port can be used by both HTTP clients talking to that server and WebSocket clients talking to that server. To this end, the WebSocket client's handshake is an HTTP Upgrade request:

    GET /chat HTTP/1.1
    Host: server.example.com
    Upgrade: websocket
    Connection: Upgrade
    Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==
    Origin: http://example.com
    Sec-WebSocket-Protocol: chat, superchat
    Sec-WebSocket-Version: 13

In compliance with [RFC2616], header fields in the handshake may be sent by the client in any order, so the order in which different header fields are received is not significant.

The "Request-URI" of the GET method [RFC2616] is used to identify the endpoint of the WebSocket connection, both to allow multiple domains to be served from one IP address and to allow multiple WebSocket endpoints to be served by a single server.

The client includes the hostname in the |Host| header field of its handshake as per [RFC2616], so that both the client and the server can verify that they agree on which host is in use.

source

In response to @tyrbo i.e. a regular HTTP request connecting and just wasting cycles, I believe that is a legitimate concern. Not only is it a vulnerability, it doesn't provide definitive behavior in the case of his first question of why it is not closing.

I would propose that we modify the behavior of Tubesock to respond to an improper HTTP request with a 403, instead of trying to continue with the handshake. I'm not sure if this is the specified behavior, but I can't find anything definitive, and this sounds reasonable enough.

Let me know what you guys think.