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

Backport CVE-2019-5421 fix to 3.x

CHANGELOG.md

@@ -20,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

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

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!