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

sign_out doesn't properly clear cookie-based sessions #3031

Closed
trianglegrrl opened this issue May 16, 2014 · 20 comments
Closed

sign_out doesn't properly clear cookie-based sessions #3031

trianglegrrl opened this issue May 16, 2014 · 20 comments

Comments

@trianglegrrl
Copy link

We have a Rails 4.0.5 app with Devise 3.2.4.

For some reason, when we call SessionsController#destroy, it correctly logs the current user out, but it does not correctly destroy the session when the OurApp::Application.config.session_store is set to :cookie_store. Installing the new activerecord-session_store gem and changing session_store to active_record_store resolves the issue.

Here's the order of what happens:

  1. We DELETE /users/sign_out.
  2. #destroy gets called, and does seem to nuke the session. current_user becomes nil, and Warden doesn't know about any users. However, the session cookie is not removed from the browser.
  3. after_sign_out_path_for returns the new_user_session_url, just like we asked it to.
  4. The page redirects to the new_user_session_url.
  5. We hit SessionsController#new. However, Devise somehow thinks the user is still logged in, and treats them as such, by moving on to 6:
  6. after_sign_in_path_for gets called. This is the step that we know shouldn't be happening, but we can't figure out how it's getting here. It should never get called, because the user was logged out in step 2 above.

With the :cookie_store, if we manually delete the session cookie from the browser and then hit the #destroy endpoint, it works. If we use the active_record_store, none of this happens. It behaves as expected.

I've tried reverting to several previous Devise versions to see when it was introduced, but the behaviour is the same all the way back to 3.

If I can provide any more information to help in troubleshooting, please let me know. We've tried stepping into and all the way through the Devise code and nothing is jumping out at us.

@lucasmazza
Copy link
Contributor

Can you please provide a sample application that reproduces the error?

@trianglegrrl
Copy link
Author

Yup, I'll try to whip something up this weekend.

@josevalim
Copy link
Contributor

Let us know if there is a way to reproduce the issue and we will gladly repoen the issue. Thank you!

@mrchess
Copy link

mrchess commented Sep 6, 2014

@trianglegrrl Omg I love you. I've been trying to figure out this exact issue for a couple days now as I think I have this exact same problem. I just implemented your suggestions and I pray the login issues go away.

Basically users would be able to create accounts, but after they sign out, they could not sign in again without triggering a password reset (does pw reset clear the session?). Will keep you posted.

I'm also on Rails 4.0.5, 3.1.1 Devise.

EDIT: Fwiw i took the opportunity to migrate over to a redis based session store.

@oliverjesse
Copy link

This is happening to me with Rails 4.1.4 and Devise 3.2.4. The behavior is exactly as described above: the browser session cookie is never removed. Do you need me to publish a sample application to reopen this? I need to use cookie-based sessions I'm afraid =(

@josevalim
Copy link
Contributor

Yes we need a way to reproduce the issue otherwise we can be chasing what is happening for quite a while. :)

@oliverjesse
Copy link

ok thanks, will do!

@mrchess
Copy link

mrchess commented Sep 14, 2014

as a follow up, i would have published a sample but i never figured out how to get to the root of the problem. 😕 i just circumvented it by switching to redis sessions, and disabling the authenticity token via skip_before_filter :verify_authenticity_token in the SessionsController

@iantropov
Copy link

@trianglegrrl I think that it isn't bug - take a look - http://maverickblogging.com/logout-is-broken-by-default-ruby-on-rails-web-applications/

@trianglegrrl
Copy link
Author

@iantropov That sounds like exactly the issue! Thanks for the link.

@arvinddoraiswamy
Copy link

Isn't this bad though? Shouldn't this be fixed so cookies are invalidated on the server instead?

  1. Log in.
  2. Get a _app_session cookie. Copy this cookie.
  3. Work
  4. Log out
  5. Cookie is destroyed in the browser
  6. Do NOT log in but replay an old request with the cookie in Step 2
  7. I can access the feature and its data

IMO this is not good at all. Have I missed something completely? Or is devise still expected to behave this way.

Yes, I know I can write my own stuff and store sessions in the DB or whatever but ideally Devise should handle this seamlessly. :)

@josevalim
Copy link
Contributor

No, we cannot handle this. This is the whole trade-off of using cookie-based sessions. They are replayable. Just use another storage if you don't want this behaviour.

@arvinddoraiswamy
Copy link

Okay thanks :)

@kinnrot
Copy link

kinnrot commented Sep 13, 2015

There is no way to make this cookie invalid when user logs out?

@josevalim
Copy link
Contributor

There is, use another session store instead of cookies.

@jamesfzhang
Copy link

I am experiencing a similar problem, but with session store rails/activerecord-session_store#80

@fatosmorina
Copy link

I found an article demonstrating a solution without having to change the storage:

You can add a session_token column to your devise model (e.g. User) and override Devise #authenticatable_salt method to contain your session token:

class User < ApplicationRecord

  devise :database_authenticatable, :recoverable, :rememberable
         
  def authenticatable_salt
    "#{super}#{session_token}"
  end

  def invalidate_all_sessions!
    self.session_token = SecureRandom.hex
  end
  
  ...
end

Now you need to invalidate the session cookie by resetting the session_token of a user on logout:

class Users::SessionsController < Devise::SessionsController
  
  def destroy
    current_user.invalidate_all_sessions!
    super
  end

end

I hope that helps.

eoinkelly added a commit to ackama/rails-template that referenced this issue Jun 3, 2020
The default devise behaviour does not invalidate session cookies after logout
so if an attacker can retrieve a cookie from a session they can use it
even if the user has logged out.

We have used variations of this change successfully on a number of Rails
apps in production.

References

* heartcombo/devise#3031
* http://maverickblogging.com/logout-is-broken-by-default-ruby-on-rails-web-applications/
* https://makandracards.com/makandra/53562-devise-invalidating-all-sessions-for-a-user

Fixes #91
@sandipsubedi
Copy link

I found an article demonstrating a solution without having to change the storage:

You can add a session_token column to your devise model (e.g. User) and override Devise #authenticatable_salt method to contain your session token:

Anyone had luck with this? For some reason, it's not working for me.

@vlad-psh
Copy link

vlad-psh commented Aug 26, 2021

I found an article demonstrating a solution without having to change the storage:

You can add a session_token column to your devise model (e.g. User) and override Devise #authenticatable_salt method to contain your session token:

Anyone had luck with this? For some reason, it's not working for me.

@sandipsubedi You're right, this method isn't working.

authenticatable_salt is only used when user logs in and when rememberable module refreshes user session.
It won't be verified if user already has valid (or, well, stolen) session cookie.

Devise already has remember_user_token invalidation mechanism based on remember_created_at column. So there is no reason to add additional custom session_token column.

Just use any store other than :cookie_store as José Valim suggested above.

@PhilippeChab
Copy link

PhilippeChab commented May 13, 2024

authenticatable_salt is only used when user logs in and when rememberable module refreshes user session.

Maybe I'm wrong, but I don't think this is true. It also depends on what you exactly mean by "logs in".

Devise mainly uses the authenticate method in warden here, which proceeds in this order:

  • Tries to serialize the user from the session here. This part uses the authenticatable_salt in devise here.
  • Performs any candidate strategies to try and sign in the user here.

Am I missing something?

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

No branches or pull requests