From e92dd4bb4fe852e0cd0896a3bd30c83135947181 Mon Sep 17 00:00:00 2001
From: Eoin Kelly <eoin@eoinkelly.info>
Date: Thu, 4 Jun 2020 06:49:41 +1200
Subject: [PATCH] Invalidate devise session cookies immediately on logout

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

* https://github.com/plataformatec/devise/issues/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
---
 .../controllers/users/sessions_controller.rb  | 21 +++++++
 .../requests/session_cookie_expiry_spec.rb    | 43 ++++++++++++++
 variants/devise/template.rb                   | 58 +++++++++++++++++++
 3 files changed, 122 insertions(+)
 create mode 100644 variants/devise/app/controllers/users/sessions_controller.rb
 create mode 100644 variants/devise/spec/requests/session_cookie_expiry_spec.rb

diff --git a/variants/devise/app/controllers/users/sessions_controller.rb b/variants/devise/app/controllers/users/sessions_controller.rb
new file mode 100644
index 00000000..a5592d75
--- /dev/null
+++ b/variants/devise/app/controllers/users/sessions_controller.rb
@@ -0,0 +1,21 @@
+module Users
+  class SessionsController < Devise::SessionsController
+    ##
+    # We want to make sure that when a user logs out (i.e. destroys their
+    # session) then the session cookie they had cannot be used again. We
+    # achieve this by overriding the built-in devise implementation of
+    # `Devise::SessionsController#destroy` action to invalidate all existing
+    # user sessions and then call `super` to run the built-in devise
+    # implementation of the method.
+    #
+    # References
+    #   * https://github.com/plataformatec/devise/issues/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
+    #
+    def destroy
+      current_user.invalidate_all_sessions!
+      super
+    end
+  end
+end
diff --git a/variants/devise/spec/requests/session_cookie_expiry_spec.rb b/variants/devise/spec/requests/session_cookie_expiry_spec.rb
new file mode 100644
index 00000000..b5c74d45
--- /dev/null
+++ b/variants/devise/spec/requests/session_cookie_expiry_spec.rb
@@ -0,0 +1,43 @@
+require "rails_helper"
+
+RSpec.describe "Session cookies are expired immediately after logout", type: :request do
+  let(:password) { SecureRandom.hex(16) }
+
+  let(:user) { FactoryBot.create(:user, password: password) }
+
+  it "session cookies are invalidated by logging out" do
+    # Sign in
+    post new_user_session_path, params: { "user[email]" => user.email, "user[password]" => password }
+
+    # When we view a page which requires a signed-in user ...
+    get edit_user_registration_path
+
+    # ... then it works.
+    expect(response).to have_http_status(200)
+    expect(response.body).to match(/Edit User/)
+
+    # Save the signed-in user's session cookie
+    session_cookie = response.headers["Set-Cookie"]
+
+    # Sign out
+    delete destroy_user_session_path
+
+    # When we try to view the same page as before ...
+    get edit_user_registration_path
+
+    # ... then we are redirected to the sign-in page
+    expect(response).to have_http_status(302)
+    expect(response.headers["Location"]).to eq(new_user_session_url)
+
+    # When we try to view the private page, this time passing the old session
+    # cookie ...
+    get edit_user_registration_path, headers: { "Cookie" => session_cookie }
+
+    # ... then we are still redirected to the sign-in page (this demonstrates
+    # that session cookies are properly invalidated when a user signs out)
+    expect(response).to have_http_status(302)
+    expect(response.headers["Location"]).to eq(new_user_session_url)
+  end
+end
+
+
diff --git a/variants/devise/template.rb b/variants/devise/template.rb
index bb909b2f..b0b5472f 100644
--- a/variants/devise/template.rb
+++ b/variants/devise/template.rb
@@ -114,6 +114,63 @@ def print_header(msg)
   ERB
 
 
+print_header "Fixing session cookie expiry"
+
+run "bundle exec rails g migration AddSessionTokenToUser session_token:string"
+run "bundle exec rails db:migrate"
+
+copy_file "app/controllers/users/sessions_controller.rb"
+
+gsub_file "config/routes.rb",
+          "devise_for :users",
+          <<~'EO_DEVISE'
+            devise_for :users, controllers: {
+              sessions: "users/sessions"
+            }
+          EO_DEVISE
+
+insert_into_file "app/models/user.rb", before: /^end/ do
+  <<~'RUBY'
+
+    ##
+    # The `session_token` attribute is used to build the Devise
+    # `authenticatable_salt` so changing the `session_token` has the effect of
+    # invalidating any existing sessions for the current user.
+    #
+    # This method is called by Users::SessionsController#destroy to make sure
+    # that when a user logs out (i.e. destroys their session) then the session
+    # cookie they had cannot be used again. This closes a security issue with
+    # cookie based sessions.
+    #
+    # References
+    #   * https://github.com/plataformatec/devise/issues/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
+    #
+    def invalidate_all_sessions!
+      update!(session_token: SecureRandom.hex(16))
+    end
+
+    ##
+    # devise calls this method to generate a salt for creating the session
+    # cookie. We override the built-in devise implementation (which comes from
+    # the devise `authenticable` module - see link below) to also include our
+    # `session_token` attribute. This means that whenever the session_token
+    # changes, the user's session cookie will be invalidated.
+    #
+    # `session_token` is `nil` until the user has signed out once. That is fine
+    # because we only care about making the `session_token` **different** after
+    # they logout so that the cookie is invalidated.
+    #
+    # References
+    #  * https://github.com/heartcombo/devise/blob/master/lib/devise/models/authenticatable.rb#L97-L98
+    #
+    def authenticatable_salt
+      "#{super}#{session_token}"
+    end
+  RUBY
+end
+
 print_header "Writing tests for you - you're welcome!"
 
 copy_file "spec/models/user_spec.rb", force: true
@@ -123,6 +180,7 @@ def print_header(msg)
 copy_file "spec/system/user_sign_in_feature_spec.rb"
 copy_file "spec/system/user_sign_up_feature_spec.rb"
 copy_file "spec/system/user_reset_password_feature_spec.rb"
+copy_file "spec/requests/session_cookie_expiry_spec.rb"
 
 print_header "Running rubocop -a to fix formatting in files generated by devise"
 run "bundle exec rubocop -aD"