core: avoid session rename when authenticating from websocket. #519

Closed
wants to merge 1 commit into from

4 participants

@kaos
Zotonic member

In case a postback results in calling z_auth:logon/2, we should not
rename the session if we have already sent a session cookie.
For properly detecting this, we need to store the set_session_id
flag in the session instead of in the context.

This is a rather minor fix, but possibly with side effects I'm not aware of, hence a review before committing it to master.

I've tried it on my dev setup, naturally. And so far I've not seen any issues with it.

What it solves in my case, specifically, is that it allows me to send a postback with
a signed logon request, which when verified results in a call to z_auth:logon (or signup, in case it's a unknown user).

I use it on my new mod_persona module, which won't work without this fix ;)
(or by solving the rename session issue some other way)

@kaos kaos core: avoid session rename when authenticating from websocket.
In case a postback results in calling z_auth:logon/2, we should not
rename the session if we have already sent a session cookie.
For properly detecting this, we need to store the `set_session_id`
flag in the session instead of in the context.
736497c
@kaos
Zotonic member

cc @mworrell any comments?

@kaos kaos referenced this pull request Feb 26, 2013
@arjan arjan mod_base: Add z_stream_restart() function to zotonic-1.0.js
When the session cookie changes (e.g. when doing an inline login
without a page refresh), the WebSocket stream needs to be restarted to
pick up the new cookie, otherwise the stream keeps using the previous
cookie for subsequent messages.
d7ecb70
@mworrell
Zotonic member

We should document that authenticating via websocket is insecure, due to session fixation attacks.
And document how to use XHR session authentication.

@mmzeeman
Zotonic member
@kaos
Zotonic member

What is session fixation attacks?

@mworrell
Zotonic member

session fixation: you go to a shared computer, open site A. Check the session cookies, copy them to your machine. Wait till someone else uses the shared computer, opens site A and logs on. Now your machine is also logged on as you know the session id.

@kaos
Zotonic member

Ah, ok. So not renaming the session after logging in is NOT a good idea, then.

@kaos
Zotonic member

Need to solve this some other way. Perhaps looking into using the ws restart arjan committed today: d7ecb70

@kaos kaos closed this Feb 26, 2013
@arjan
Zotonic member

After a log on, we can maybe just do the websocket restart.

z_context:add_script_page("z_websocket_restart();", Context),

Although in most cases the browser is redirected to another page so the restart doesnt make sense..

@kaos
Zotonic member

In my case (that I mentioned in the OP) I can trigger the restart after a successful logon from the server.

Hmm.. perhaps there's a way to detect in z_auth if the logon request comes from a websocket or not.. Ah, of course, just look at the controller handling the request!

So, if it is controller_websocket, then issue a z_websocket_restart() when logging on.. :)

@kaos
Zotonic member

Hmm... come to think of it, can't the websocket be closed from the server side just as well?

@mworrell
Zotonic member

Yes, it can - that will force the user agent to reopen it.

@mmzeeman
Zotonic member

Not sure if zotonic currently supports closing a websocket correctly. I've found that I can crash or hang chrome when the websocket process crashes…

@kaos
Zotonic member

Well, in case the websocket process crashes, could the issue at the other end be due to restart issues on the server side of things?

@mmzeeman
Zotonic member

The websocket spec on closing a websocket looks a bit tricky. A lot of SHOULD's and such.
There are no restarts for websocket processes on the server side. It just dies and goes away. Leaving it to the client to handle the situation. FF handled it correctly, so I suspected a Chrome bug. Didn't look into further because I had to finish something… Maybe it is already fixed, Chrome is a moving target.

@kaos kaos deleted the kaos:kaos/pu-rename_session_fix branch Oct 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment