diff --git a/Changelog.md b/Changelog.md index 5c55dde29..a9966d0a5 100644 --- a/Changelog.md +++ b/Changelog.md @@ -2,6 +2,7 @@ ## Security * Bump Rails to 5.2.7 to address [CVE-2022-22577](https://discuss.rubyonrails.org/t/cve-2022-22577-possible-xss-vulnerability-in-action-pack/80533) and [CVE-2022-27777](https://discuss.rubyonrails.org/t/cve-2022-27777-possible-xss-vulnerability-in-action-view-tag-helpers/80534) [#8350](https://github.com/diaspora/diaspora/pull/8350) +* Do not allow the user to mass assign their own password and 2fa settings alongside other parameters. Reported by Breno Vitório (@brenu) - thank you! [#8351](https://github.com/diaspora/diaspora/pull/8351) ## Refactor diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 726f46f2e..99a0297a0 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, @@ -152,12 +140,17 @@ 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 - # 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 +170,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/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 85c70f18d..ac3e79727 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -110,38 +110,67 @@ describe UsersController, :type => :controller do end end - describe '#update' do - before do - @params = { :id => @user.id, - :user => { :diaspora_handle => "notreal@stuff.com" } } + 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 "renders the user edit page" do + put :update, params: params + expect(response).to render_template('edit') + end 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') - 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 + + 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