Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does Sorcery invalidate session tokens after logout? #156

Closed
GildedHonour opened this issue Oct 28, 2018 · 16 comments
Closed

Does Sorcery invalidate session tokens after logout? #156

GildedHonour opened this issue Oct 28, 2018 · 16 comments
Labels
bug Something isn't working help wanted Community assistance requested

Comments

@GildedHonour
Copy link

GildedHonour commented Oct 28, 2018

I've not found that in the code. Does it?

In our project it's not invalidated and can be reused if you save it.
However, it does log out the user.

@joshbuker joshbuker added high priority Extra attention is needed help wanted Community assistance requested labels Oct 29, 2018
@joshbuker
Copy link
Member

I believe that reset_session is supposed to be invalidating the session_id upon logout.

https://github.com/Sorcery/sorcery/blob/master/lib/sorcery/controller.rb#L71

This does seem like a good thing to be 100% on however.

@GildedHonour
Copy link
Author

GildedHonour commented Oct 29, 2018 via email

@joshbuker
Copy link
Member

I've tested this and have been unable to replicate the issue. After logging out, the cookie does appear to be properly invalidated.

Can you please provide more detail on how to replicate the issue? Specifics on your session controller and any cookie configurations would be helpful.

@GildedHonour
Copy link
Author

the cookie does appear

but if you save its content and then re-create it?

@GildedHonour
Copy link
Author

captura de pantalla 2018-11-05 a la s 18 11 51

@joshbuker
Copy link
Member

joshbuker commented Nov 8, 2018

@GildedHonour Following those steps fails to reproduce the issue using Sorcery 0.12.0. Once logout is called the session cookie is invalidated and setting it again on the same or different browser results in the user not being logged in.

Have you tested to ensure that users can't set arbitrary session cookies on your application? (does it use secure cookies?) Edit: Secure cookies isn't the setting I was thinking of.

@daan-b
Copy link

daan-b commented Nov 13, 2018

Devise is also struggling with this issue.

Basically we need to add an session-token to the user model. When we store the user-id in the session we need to append the session-token also. Those combined validates a valid user. When the user logout whe regenerate the session-token so the session value is invalid.

See the solution at Makandra

@joshbuker
Copy link
Member

@daan-b All three solutions are on the application side here, so this should be a documentation update rather than a code change in Sorcery. Thanks for those links!

@daan-b
Copy link

daan-b commented Nov 13, 2018

@athix I think option 2 is the right option and I think this should be in the core of Sorcery.

Something like:

def auto_login(user, _should_remember = false) session[:user] = {id: user.id.to_s, token: user.session_token} @current_user = user end

def login_from_session @current_user = if session[:user] user_class.sorcery_adapter.find_by(id: session[:user][:id], session_token: session[:user][:session_token]) end end

def reset_sorcery_session reset_session # protect from session fixation attacks user.regenerate_session_token end

Update
In functionality we need an extra column in the user table. When user login we generate an token en put that token along with the user_id in the session. When the user logout we generate a new token and store that in the database. That way the ID isn't matching the session_token in the database and the session is invalid.

@joshbuker
Copy link
Member

@daan-b That sounds reasonable to me.

@joshbuker joshbuker changed the title Does it invalidate the session cookie after loging out? Does Sorcery invalidate session tokens after logout? Nov 13, 2018
@bzf
Copy link
Contributor

bzf commented Nov 23, 2018

@daan-b If you only have a single row per current valid user, wouldn't that prevent users from being able to sign in from multiple devices and without signing out from everywhere else?

Maybe you could have a has_many :sessions relation between User and Session instead and only destroy the current row when the user signs out?

@daan-b
Copy link

daan-b commented Nov 23, 2018

@bzf Yeah that's true. But I think you should regenerate the token only at logout. If a user closes the window there is no problem. If the user explicitly choses to logout, then he's logged out everywhere.

@joshbuker joshbuker removed the high priority Extra attention is needed label Dec 14, 2018
@Sanjay-Chaudhary
Copy link

I am also having the same issue. The server side session cookies does not get invalidated. After logout i am still able to use the session cookie and get access to those pages which should only be accessed after login.

reset_session option did not work and did not invalidated the session cookies after logout.

This issue was raised in the pen test as well. So can you please help in solving this issue. Is there a roundabout for this.

@joshbuker joshbuker added the bug Something isn't working label Feb 20, 2019
@OscarBarrett
Copy link

As mentioned in the devise issue, this is unavoidable when using the cookie store - cookie-based sessions are replayable. The session data is not stored server side, only in the cookie. The cookie itself has enough information to identify the user and consider them as logged in.
It's not something that Sorcery should need to fix.

If you want session data to be handled server-side, you should use an alternative session store, such as:

However IMO it's not something that you should really worry about. By default the session data in the cookie is encrypted and is marked as httponly so cannot be read via javascript. The ActiveRecord session store was removed from rails due to performance concerns, with redis you need to consider and handle potential failures, and tracking a single session per user in your database is not a great user experience when users may be accessing your platform from multiple devices.

As an example: Netflix uses cookie-based sessions. If you log out but someone has your cookie they can use it to access your account. They have a "sign out of all devices" feature which will invalidate all sessions, and Sorcery also supports this with invalidate_active_sessions!.

@mladenilic
Copy link
Contributor

I agree with @OscarBarrett. This is a reasonable tradeoff when using cookie session store. If this is seen as security concern, I'd say that the best way to avoid it is to move away from cookie session store.

A slight improvement on what @daan-b suggested (storing session token in the database) could however offer some potentially nice features. Since users can have more than one session, we would need an association on user model that would keep track of all the sessions across multiple devices.

Sorcery could then track activity per session and also allowing user to not only log out of current session, but also revoke any other session that bound to his account – allowing developers to build something along the lines of GitHub's security settings page.

I don't see this issue as a bug and in that sense I think that it should be closed. On the other hand I am open to discussion on whether the "multiple sessions" should be added to sorcery as a new module.

@joshbuker
Copy link
Member

I'll close this for now because it seems like the topic has been covered pretty well here. If anyone would like to tackle the multiple sessions feature, please feel free to open a pull request or issue to start a new discussion about that specifically. Likewise, if anyone would like additional guidance on their particular situation with session cookies, feel free to open a new issue with additional details.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Community assistance requested
Projects
None yet
Development

No branches or pull requests

7 participants