Merge pull request #5043 from maestrano/increment-failed-attempts-concurency

Backport CVE-2019-5421 fix to 3.x

CHANGELOG.md

@@ -1,4 +1,7 @@
-### Unreleased
+### 3.5.10 - 2016-05-15
+
+* bug fixes
+  * Fix overwriting the remember_token when a valid one already exists (by @ralinchimev).
 
 ### 3.5.9 - 2016-05-02
 
@@ -17,18 +20,18 @@
   * Fix the `extend_remember_period` configuration. When set to `false` it does
     not update the cookie expiration anymore.(by @ulissesalmeida)
 
-### 3.5.6 - 2016-01-02
+### 3.5.6 - 2016-02-01
 
 * bug fixes
   * Fix type coercion of the rememberable timestamp stored on cookies.
 
-### 3.5.5 - 2016-22-01
+### 3.5.5 - 2016-01-22
 
 * bug fixes
   * Bring back remember_expired? implementation
   * Ensure timeouts are not triggered if remember me is being used
 
-### 3.5.4 - 2016-18-01
+### 3.5.4 - 2016-01-18
 
 * bug fixes
   * Store creation timestamps on remember cookies

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    devise (3.5.9)
+    devise (3.5.10)
       bcrypt (~> 3.0)
       orm_adapter (~> 0.1)
       railties (>= 3.2.6, < 5)
@@ -184,4 +184,4 @@ DEPENDENCIES
   webrat (= 0.7.3)
 
 BUNDLED WITH
-   1.11.2
+   1.12.3

app/controllers/devise/sessions_controller.rb

@@ -2,7 +2,7 @@ class Devise::SessionsController < DeviseController
   prepend_before_filter :require_no_authentication, only: [:new, :create]
   prepend_before_filter :allow_params_authentication!, only: :create
   prepend_before_filter :verify_signed_out_user, only: :destroy
-  prepend_before_filter only: [:create, :destroy] { request.env["devise.skip_timeout"] = true }
+  prepend_before_filter(only: [:create, :destroy]) { request.env["devise.skip_timeout"] = true }
 
   # GET /resource/sign_in
   def new

lib/devise/models/lockable.rb

@@ -99,8 +99,8 @@ module Devise
         if super && !access_locked?
           true
         else
-          self.failed_attempts ||= 0
-          self.failed_attempts += 1
+          self.class.increment_counter(:failed_attempts, id)
+          reload
           if attempts_exceeded?
             lock_access! unless access_locked?
           else

lib/devise/models/rememberable.rb

@@ -48,7 +48,7 @@ module Devise
       # TODO: We were used to receive a extend period argument but we no longer do.
       # Remove this for Devise 4.0.
       def remember_me!(*)
-        self.remember_token = self.class.remember_token if respond_to?(:remember_token)
+        self.remember_token ||= self.class.remember_token if respond_to?(:remember_token)
         self.remember_created_at ||= Time.now.utc
         save(validate: false) if self.changed?
       end

lib/devise/version.rb

@@ -1,3 +1,3 @@
 module Devise
-  VERSION = "3.5.9".freeze
+  VERSION = "3.5.10".freeze
 end

test/models/lockable_test.rb

@@ -37,6 +37,17 @@ class LockableTest < ActiveSupport::TestCase
     end
   end
 
+  test "should read failed_attempts from database when incrementing" do
+    user = create_user
+    initial_failed_attempts = user.failed_attempts
+    same_user = User.find(user.id)
+
+    user.valid_for_authentication?{ false }
+    same_user.valid_for_authentication?{ false }
+
+    assert_equal initial_failed_attempts + 2, user.reload.failed_attempts
+  end
+
   test 'should be valid for authentication with a unlocked user' do
     user = create_user
     user.lock_access!

test/models/rememberable_test.rb

@@ -16,6 +16,18 @@ class RememberableTest < ActiveSupport::TestCase
     assert user.remember_created_at
   end
 
+  test 'remember_me should not generate a new token if valid token exists' do
+    user = create_user
+    user.singleton_class.send(:attr_accessor, :remember_token)
+    User.to_adapter.expects(:find_first).returns(nil)
+
+    user.remember_me!
+    existing_token = user.remember_token
+
+    user.remember_me!
+    assert_equal existing_token, user.remember_token
+  end
+
   test 'forget_me should not clear remember token if using salt' do
     user = create_user
     user.remember_me!