From 1cfe0037f92486c95847e76279aeffec4e37b2f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Wed, 27 Apr 2022 13:44:48 +0200 Subject: [PATCH 1/2] 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 726f46f2e..7f03479da 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -18,25 +18,17 @@ class UsersController < ApplicationController 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 @@ class UsersController < ApplicationController 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 @@ class UsersController < ApplicationController 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 @@ class UsersController < ApplicationController 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 85c70f18d..3b88d883b 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -127,21 +127,34 @@ describe UsersController, :type => :controller do 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 + + 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 - expect(@user).to receive(:update_with_password).with(hash_including(password_params)) - allow(@controller).to receive(:current_user).and_return(@user) 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 From 43ee2dbb50bef30e23583776cf1e8821b59aa4b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Wed, 27 Apr 2022 19:48:42 +0200 Subject: [PATCH 2/2] Do not allow to mass assign OTP fields on user edit page --- app/controllers/users_controller.rb | 2 - .../two_factor_authentications/_activate.haml | 1 - spec/controllers/users_controller_spec.rb | 42 +++++++++++++------ 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 7f03479da..99a0297a0 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -140,8 +140,6 @@ class UsersController < ApplicationController :auto_follow_back_aspect_id, :getting_started, :post_default_public, - :otp_required_for_login, - :otp_secret, email_preferences: UserPreference::VALID_EMAIL_TYPES.map(&:to_sym) ) end diff --git a/app/views/two_factor_authentications/_activate.haml b/app/views/two_factor_authentications/_activate.haml index ef6b0a7ff..960f48e6b 100644 --- a/app/views/two_factor_authentications/_activate.haml +++ b/app/views/two_factor_authentications/_activate.haml @@ -6,6 +6,5 @@ .well= t("two_factor_auth.deactivated.status") = form_for "user", url: two_factor_authentication_path, html: {method: :post} do |f| - = f.hidden_field :otp_required_for_login, value: true .clearfix.form-group= f.submit t("two_factor_auth.deactivated.change_button"), class: "btn btn-primary pull-right" diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 3b88d883b..ac3e79727 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -110,21 +110,20 @@ describe UsersController, :type => :controller do end end - describe '#update' do - before do - @params = { :id => @user.id, - :user => { :diaspora_handle => "notreal@stuff.com" } } - end + describe "#update" do + context "with random params" do + let(:params) { {id: @user.id, user: {diaspora_handle: "notreal@stuff.com"}} } - it "doesn't overwrite random attributes" do - expect { - put :update, params: @params - }.not_to change(@user, :diaspora_handle) - end + it "doesn't overwrite random attributes" do + expect { + put :update, params: params + }.not_to change(@user, :diaspora_handle) + end - it 'renders the user edit page' do - put :update, params: @params - expect(response).to render_template('edit') + it "renders the user edit page" do + put :update, params: params + expect(response).to render_template('edit') + end end describe "password updates" do @@ -158,6 +157,23 @@ describe UsersController, :type => :controller do end end + context "with otp params" do + let(:otp_params) { {otp_required_for_login: false, otp_secret: "mykey"} } + let(:params) { {id: @user.id, user: otp_params} } + + before do + allow(@controller).to receive(:current_user).and_return(@user) + allow(@user).to receive(:update_attributes) + end + + it "does not accept the params" do + put :update, params: params + + expect(@user).not_to have_received(:update_attributes) + .with(hash_including(:otp_required_for_login, :otp_secret)) + end + end + describe 'language' do it "allows the user to change their language" do old_language = 'en'