Don't leak information about the existence of accounts in SessionsController (lynndylanhurley#1600)

* Don't leak information about the existence of accounts in DeivseTokenAuth::SessionsController

* Refactor SessionsController

Author: moritzhoeppner

app/controllers/devise_token_auth/sessions_controller.rb

@@ -11,11 +11,7 @@ def new
     end
 
     def create
-      # Check
-      field = (resource_params.keys.map(&:to_sym) & resource_class.authentication_keys).first
-
-      @resource = nil
-      if field
+      if field = (resource_params.keys.map(&:to_sym) & resource_class.authentication_keys).first
         q_value = get_case_insensitive_field_from_resource_params(field)
 
         @resource = find_resource(field, q_value)
@@ -34,13 +30,14 @@ def create
         yield @resource if block_given?
 
         render_create_success
-      elsif @resource && !(!@resource.respond_to?(:active_for_authentication?) || @resource.active_for_authentication?)
+      elsif @resource && !Devise.paranoid && !(!@resource.respond_to?(:active_for_authentication?) || @resource.active_for_authentication?)
         if @resource.respond_to?(:locked_at) && @resource.locked_at
           render_create_error_account_locked
         else
           render_create_error_not_confirmed
         end
       else
+        hash_password_in_paranoid_mode
         render_create_error_bad_credentials
       end
     end
@@ -144,5 +141,13 @@ def create_and_assign_token
         @resource.save!
       end
     end
+
+    def hash_password_in_paranoid_mode
+      # In order to avoid timing attacks in paranoid mode, we want the password hash to be
+      # calculated even if no resource has been found. Devise's DatabaseAuthenticatable warden
+      # strategy handles this case similarly:
+      # https://github.com/heartcombo/devise/blob/main/lib/devise/strategies/database_authenticatable.rb
+      resource_class.new.password = resource_params[:password] if Devise.paranoid
+    end
   end
 end

test/controllers/devise_token_auth/sessions_controller_test.rb

@@ -310,23 +310,47 @@ def @controller.reset_session
     end
 
     describe 'Unconfirmed user' do
-      before do
-        @unconfirmed_user = create(:user)
-        post :create, params: { email: @unconfirmed_user.email,
-                                password: @unconfirmed_user.password }
-        @resource = assigns(:resource)
-        @data = JSON.parse(response.body)
-      end
+      describe 'Without paranoid mode' do
+        before do
+          @unconfirmed_user = create(:user)
+          post :create, params: { email: @unconfirmed_user.email,
+                                  password: @unconfirmed_user.password }
+          @resource = assigns(:resource)
+          @data = JSON.parse(response.body)
+        end
 
-      test 'request should fail' do
-        assert_equal 401, response.status
+        test 'request should fail' do
+          assert_equal 401, response.status
+        end
+
+        test 'response should contain errors' do
+          assert @data['errors']
+          assert_equal @data['errors'],
+                      [I18n.t('devise_token_auth.sessions.not_confirmed',
+                              email: @unconfirmed_user.email)]
+        end
       end
+      
+      describe 'With paranoid mode' do
+        before do
+          @unconfirmed_user = create(:user)
+          swap Devise, paranoid: true do
+            post :create, params: { email: @unconfirmed_user.email,
+                                    password: @unconfirmed_user.password }
+          end
+          @resource = assigns(:resource)
+          @data = JSON.parse(response.body)
+        end
 
-      test 'response should contain errors' do
-        assert @data['errors']
-        assert_equal @data['errors'],
-                     [I18n.t('devise_token_auth.sessions.not_confirmed',
-                             email: @unconfirmed_user.email)]
+        test 'request should fail' do
+          assert_equal 401, response.status
+        end
+
+        test 'response should contain errors that do not leak the existence of the account' do
+          assert @data['errors']
+          assert_equal @data['errors'],
+                      [I18n.t('devise_token_auth.sessions.bad_credentials')]
+        end
       end
     end
 
@@ -375,20 +399,42 @@ def @controller.reset_session
     end
 
     describe 'Non-existing user' do
-      before do
-        post :create,
-             params: { email: -> { Faker::Internet.email },
-                       password: -> { Faker::Number.number(10) } }
-        @resource = assigns(:resource)
-        @data = JSON.parse(response.body)
-      end
+      describe 'Without paranoid mode' do
+        before do
+          post :create,
+              params: { email: -> { Faker::Internet.email },
+                        password: -> { Faker::Number.number(10) } }
+          @resource = assigns(:resource)
+          @data = JSON.parse(response.body)
+        end
 
-      test 'request should fail' do
-        assert_equal 401, response.status
+        test 'request should fail' do
+          assert_equal 401, response.status
+        end
+
+        test 'response should contain errors' do
+          assert @data['errors']
+        end
       end
 
-      test 'response should contain errors' do
-        assert @data['errors']
+      describe 'With paranoid mode' do
+        before do
+          mock_hash = '$2a$04$MUWADkfA6MHXDdWHoep6QOvX1o0Y56pNqt3NMWQ9zCRwKSp1HZJba'
+          @bcrypt_mock = MiniTest::Mock.new
+          @bcrypt_mock.expect(:call, mock_hash, [Object, String])
+
+          swap Devise, paranoid: true do
+            BCrypt::Engine.stub :hash_secret, @bcrypt_mock do
+              post :create,
+                  params: { email: -> { Faker::Internet.email },
+                            password: -> { Faker::Number.number(10) } }
+            end
+          end
+        end
+        
+        test 'password should be hashed' do
+          @bcrypt_mock.verify
+        end
       end
     end
 
@@ -472,21 +518,44 @@ def @controller.reset_session
       end
 
       describe 'locked user' do
-        before do
-          @locked_user = create(:lockable_user, :locked)
-          post :create,
-               params: { email: @locked_user.email,
-                         password: @locked_user.password }
-          @data = JSON.parse(response.body)
-        end
+        describe 'Without paranoid mode' do
+          before do
+            @locked_user = create(:lockable_user, :locked)
+            post :create,
+                params: { email: @locked_user.email,
+                          password: @locked_user.password }
+            @data = JSON.parse(response.body)
+          end
 
-        test 'request should fail' do
-          assert_equal 401, response.status
+          test 'request should fail' do
+            assert_equal 401, response.status
+          end
+
+          test 'response should contain errors' do
+            assert @data['errors']
+            assert_equal @data['errors'], [I18n.t('devise.mailer.unlock_instructions.account_lock_msg')]
+          end
         end
 
-        test 'response should contain errors' do
-          assert @data['errors']
-          assert_equal @data['errors'], [I18n.t('devise.mailer.unlock_instructions.account_lock_msg')]
+        describe 'With paranoid mode' do
+          before do
+            @locked_user = create(:lockable_user, :locked)
+            swap Devise, paranoid: true do
+              post :create,
+                  params: { email: @locked_user.email,
+                            password: @locked_user.password }
+            end
+            @data = JSON.parse(response.body)
+          end
+
+          test 'request should fail' do
+            assert_equal 401, response.status
+          end
+
+          test 'response should contain errors that do not leak the existence of the account' do
+            assert @data['errors']
+            assert_equal @data['errors'], [I18n.t('devise_token_auth.sessions.bad_credentials')]
+          end
         end
       end