Engine.IO gets session from client, not cookie #359

Closed
polidore opened this Issue Mar 13, 2013 · 11 comments

Comments

Projects
None yet
3 participants
Contributor

polidore commented Mar 13, 2013

I originally wrote this giant issue when I didn't understand the problem: #356

I closed it because there was too much speculation in it.

I now think there is a bug with the engine.io implementation of websockets in the new socketstream. The wrapper on this line:

https://github.com/socketstream/socketstream/blob/master/src/websocket/transports/engineio/wrapper.js#L20

assumes that connect.sid is always readable by the client. It is not. When a session expires or is invalidated or when a client is moved to a backup system, connect will return a new session cookie with httpOnly set to true when it invalidates the old cookie. It will update it a few more times to remove the httpOnly, but during this process, the engine.io wrapper will crash because the above linked line fails.

The way it worked before with socket.io was much better! Instead of decrypting the client session and sending it in the get string, it would just look at the cookie in the server and grab the session from there.

I think this change was a move backward.

Contributor

paulbjensen commented Mar 13, 2013

Hi @polidore,

Thanks for the issue.

I don't think that this is an issue with the cookie key, as the connect.sid cookie key has been used within SocketStream since the socket.io implementation, see:

sessionId = cookie['connect.sid'].split('.')[0]
. That said, there should probably be a way to specify a custom cookie key.

I'd like to try and replicate the app setup that you've used for your app, because when I developed ss-engine.io I naturally assumed that you would want to share session data across all running instances of the app, so that client reconnections use the same session, and do not lose channel subscriptions. I recall that you're not sharing sessions between servers in your case, so I'd like to try and fix this issue to cater for that use case.

Contributor

polidore commented Mar 13, 2013

Hey, paul. Sorry about the tone of the issue above. I didn't mean to come off as I think I did!

Anyway, I changed the code to use the HTTP header to get the session instead of sending it over the websocket. I'm not very experienced in this area, so take a look and see what you think-- you may want to adjust it a bit, but I think this is a better way to do it in general.

thanks again!

Contributor

polidore commented Mar 13, 2013

If you want to test it, just blow away your sessions in redis / mongo while your connected to your app and refresh. Prior to this change, you'd hit the exception here without fail:

https://github.com/socketstream/socketstream/blob/master/src/websocket/transports/engineio/wrapper.js#L24

Contributor

paulbjensen commented Mar 13, 2013

Hi, no worries, I get the frustration when working software breaks.

I will give it a go tomorrow morning, and checkout your pull request as well.

Contributor

polidore commented Mar 25, 2013

Hey any thoughts on this? I've been using it for a while without issue.

owenb commented Mar 25, 2013

Hey Ben

Apologies, I've been so engrossed in 0.4 development these last few weeks I've not been spending as much time on 0.3 issues as I should.

Your patch looks good, but I'm always ultra-cautious when it comes to changing things like this as inevitably it will break something none of us have considered.

As soon as I get chance I'll try it out myself and think hard about anything which could go wrong. If all looks good I'll merge it in.

Also, I'll be in touch shortly regarding Angular integration in 0.4. I think I'm onto something pretty good but want to advance it a bit more before showing you.

Cheers,

Owen

Contributor

polidore commented Mar 25, 2013

sounds great. thanks.

On Mon, Mar 25, 2013 at 2:20 PM, Owen Barnes notifications@github.comwrote:

Hey Ben

Apologies, I've been so engrossed in 0.4 development these last few weeks
I've not been spending as much time on 0.3 issues as I should.

Your patch looks good, but I'm always ultra-cautious when it comes to
changing things like this as inevitably it will break something none of us
have considered.

As soon as I get chance I'll try it out myself and think hard about
anything which could go wrong. If all looks good I'll merge it in.

Also, I'll be in touch shortly regarding Angular integration in 0.4. I
think I'm onto something pretty good but want to advance it a bit more
before showing you.

Cheers,

Owen


Reply to this email directly or view it on GitHubhttps://github.com/socketstream/socketstream/issues/359#issuecomment-15411159
.

owenb commented Mar 30, 2013

Hi Ben

I'm working on Sessions support in 0.4 today and I've started thinking about the two approaches here.

  1. The client-side JS reads the session cookie and sends it over the WS (what we have today in 0.3)
  2. We try to obtain the session id server-side by looking at the HTTP headers (what we used to have in socket.io)

While I agree 2 is more reliable if the transport supports it (last time I checked, SockJS doesn't), it has a big problem: it requires the client to be running in a browser.

The latest version of 0.4 (which I will push within the next week) has separate modules for the client and server 'realtime' components. Best of all, the client can run anywhere - on the browser, or in another Node process. Thus there is no longer any need for ss-console as you can simple connect to the server using the same client library and use a REPL to invoke RPC commands etc.

All this works great already - without sessions support. Figuring out how to add this is the hard part.

I'm going to do some experimenting with ideas today and let you know. Ideally I'd like to find a secure and reliable way to do method 1 properly, even if cookies expire or don't exist at all.

Owen

Contributor

polidore commented Mar 30, 2013

I think #1 works as long as you suppress the httpOnly flag set by connect
on expiring cookies.
On Mar 30, 2013 10:16 AM, "Owen Barnes" notifications@github.com wrote:

Hi Ben

I'm working on Sessions support in 0.4 today and I've started thinking
about the two approaches here.

  1. The client-side JS reads the session cookie and sends it over the
    WS (what we have today in 0.3)
  2. We try to obtain the session id server-side by looking at the HTTP
    headers (what we used to have in socket.io)

While I agree 2 is more reliable if the transport supports it (last time I
checked, SockJS doesn't), it has a big problem: it requires the client to
be running in a browser.

The latest version of 0.4 (which I will push within the next week) has
separate modules for the client and server 'realtime' components. Best of
all, the client can run anywhere - on the browser, or in another Node
process. Thus there is no longer any need for ss-console as you can simple
connect to the server using the same client library and use a REPL to
invoke RPC commands etc.

All this works great already - without sessions support. Figuring out how
to add this is the hard part.

I'm going to do some experimenting with ideas today and let you know.
Ideally I'd like to find a secure and reliable way to do method 1 properly,
even if cookies expire or don't exist at all.

Owen


Reply to this email directly or view it on GitHubhttps://github.com/socketstream/socketstream/issues/359#issuecomment-15675182
.

Contributor

paulbjensen commented Sep 28, 2013

Hi,

Was this issue resolved by the work on the Websockets (#397)?

Contributor

polidore commented Sep 28, 2013

Yes, thank you.
On Sep 28, 2013 7:23 AM, "Paul Jensen" notifications@github.com wrote:

Hi,

Was this issue resolved by the work on the Websockets (#397#397
)?


Reply to this email directly or view it on GitHubhttps://github.com/socketstream/socketstream/issues/359#issuecomment-25296382
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment