From 1cfe0037f92486c95847e76279aeffec4e37b2f1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jonne=20Ha=C3=9F?= <me@jhass.eu>
Date: Wed, 27 Apr 2022 13:44:48 +0200
Subject: [PATCH] Do not allow the user to mass assign their own password
 alongside other parameters
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Much thanks to Breno Vitório (@brenu) for the report!
---
 app/controllers/users_controller.rb       | 39 ++++++++++-------------
 spec/controllers/users_controller_spec.rb | 27 ++++++++++++----
 2 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 726f46f2e9b..7f03479dad4 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -18,25 +18,17 @@ def privacy_settings
   end
 
   def update
-    password_changed = false
-    user_data = user_params
     @user = current_user
 
-    if user_data
-      # change password
-      if params[:change_password]
-        password_changed = change_password(user_data)
-      else
-        update_user(user_data)
-      end
+    if params[:change_password] && user_password_params
+      password_changed = change_password(user_password_params)
+      return redirect_to new_user_session_path if password_changed
+    elsif user_params
+      update_user(user_params)
     end
 
-    if password_changed
-      redirect_to new_user_session_path
-    else
-      set_email_preferences
-      render :edit
-    end
+    set_email_preferences
+    render :edit
   end
 
   def update_privacy_settings
@@ -137,13 +129,9 @@ def auth_token
 
   private
 
-  # rubocop:disable Metrics/MethodLength
   def user_params
     params.fetch(:user).permit(
       :email,
-      :current_password,
-      :password,
-      :password_confirmation,
       :language,
       :color_theme,
       :disable_mail,
@@ -157,7 +145,14 @@ def user_params
       email_preferences: UserPreference::VALID_EMAIL_TYPES.map(&:to_sym)
     )
   end
-  # rubocop:enable Metrics/MethodLength
+
+  def user_password_params
+    params.fetch(:user).permit(
+      :current_password,
+      :password,
+      :password_confirmation
+    )
+  end
 
   def update_user(user_data)
     if user_data[:email_preferences]
@@ -177,8 +172,8 @@ def update_user(user_data)
     end
   end
 
-  def change_password(user_data)
-    if @user.update_with_password(user_data)
+  def change_password(password_params)
+    if @user.update_with_password(password_params)
       flash[:notice] = t("users.update.password_changed")
       true
     else
diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb
index 85c70f18deb..3b88d883b94 100644
--- a/spec/controllers/users_controller_spec.rb
+++ b/spec/controllers/users_controller_spec.rb
@@ -127,21 +127,34 @@
       expect(response).to render_template('edit')
     end
 
-    describe 'password updates' do
+    describe "password updates" do
       let(:password_params) do
-        {:current_password => 'bluepin7',
-         :password => "foobaz",
-         :password_confirmation => "foobaz"}
+        {current_password: "bluepin7", password: "foobaz", password_confirmation: "foobaz"}
       end
 
       let(:params) do
-        {id: @user.id, user: password_params, change_password: 'Change Password'}
+        {id: @user.id, user: password_params, change_password: "Change Password"}
       end
 
-      it "uses devise's update with password" do
-        expect(@user).to receive(:update_with_password).with(hash_including(password_params))
+      before do
         allow(@controller).to receive(:current_user).and_return(@user)
+        allow(@user).to receive(:update_with_password)
+        allow(@user).to receive(:update_attributes)
+      end
+
+      it "uses devise's update with password" do
         put :update, params: params
+
+        expect(@user).to have_received(:update_with_password).with(hash_including(password_params))
+        expect(@user).not_to have_received(:update_attributes).with(hash_including(password_params))
+      end
+
+      it "does not update the password without the change_password param" do
+        put :update, params: params.except(:change_password).deep_merge(user: {language: "de"})
+
+        expect(@user).not_to have_received(:update_with_password).with(hash_including(password_params))
+        expect(@user).not_to have_received(:update_attributes).with(hash_including(password_params))
+        expect(@user).to have_received(:update_attributes).with(hash_including(language: "de"))
       end
     end