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/3] 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 8e6f3b91d3f896112988c81e79d1edaf6c9588af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Wed, 27 Apr 2022 13:51:45 +0200 Subject: [PATCH 2/3] bump rails to 5.2.7 --- Gemfile | 2 +- Gemfile.lock | 80 ++++++++++++++++++++++++++-------------------------- 2 files changed, 41 insertions(+), 41 deletions(-) diff --git a/Gemfile b/Gemfile index 7dbd9f939..48d1d9136 100644 --- a/Gemfile +++ b/Gemfile @@ -2,7 +2,7 @@ source "https://rubygems.org" -gem "rails", "5.2.6.2" +gem "rails", "5.2.7.1" # Legacy Rails features, remove me! # responders (class level) diff --git a/Gemfile.lock b/Gemfile.lock index ae2d48981..c0fb9eecf 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -2,25 +2,25 @@ GEM remote: https://rubygems.org/ remote: https://gems.diasporafoundation.org/ specs: - actioncable (5.2.6.2) - actionpack (= 5.2.6.2) + actioncable (5.2.7.1) + actionpack (= 5.2.7.1) nio4r (~> 2.0) websocket-driver (>= 0.6.1) - actionmailer (5.2.6.2) - actionpack (= 5.2.6.2) - actionview (= 5.2.6.2) - activejob (= 5.2.6.2) + actionmailer (5.2.7.1) + actionpack (= 5.2.7.1) + actionview (= 5.2.7.1) + activejob (= 5.2.7.1) mail (~> 2.5, >= 2.5.4) rails-dom-testing (~> 2.0) - actionpack (5.2.6.2) - actionview (= 5.2.6.2) - activesupport (= 5.2.6.2) + actionpack (5.2.7.1) + actionview (= 5.2.7.1) + activesupport (= 5.2.7.1) rack (~> 2.0, >= 2.0.8) rack-test (>= 0.6.3) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.0.2) - actionview (5.2.6.2) - activesupport (= 5.2.6.2) + actionview (5.2.7.1) + activesupport (= 5.2.7.1) builder (~> 3.1) erubi (~> 1.4) rails-dom-testing (~> 2.0) @@ -28,22 +28,22 @@ GEM active_model_serializers (0.9.7) activemodel (>= 3.2) concurrent-ruby (~> 1.0) - activejob (5.2.6.2) - activesupport (= 5.2.6.2) + activejob (5.2.7.1) + activesupport (= 5.2.7.1) globalid (>= 0.3.6) - activemodel (5.2.6.2) - activesupport (= 5.2.6.2) - activerecord (5.2.6.2) - activemodel (= 5.2.6.2) - activesupport (= 5.2.6.2) + activemodel (5.2.7.1) + activesupport (= 5.2.7.1) + activerecord (5.2.7.1) + activemodel (= 5.2.7.1) + activesupport (= 5.2.7.1) arel (>= 9.0) activerecord-import (1.1.0) activerecord (>= 3.2) - activestorage (5.2.6.2) - actionpack (= 5.2.6.2) - activerecord (= 5.2.6.2) + activestorage (5.2.7.1) + actionpack (= 5.2.7.1) + activerecord (= 5.2.7.1) marcel (~> 1.0.0) - activesupport (5.2.6.2) + activesupport (5.2.7.1) concurrent-ruby (~> 1.0, >= 1.0.2) i18n (>= 0.7, < 2) minitest (~> 5.1) @@ -139,7 +139,7 @@ GEM compass (~> 1.0.0) sass-rails (< 5.1) sprockets (< 4.0) - concurrent-ruby (1.1.9) + concurrent-ruby (1.1.10) configurate (0.5.0) connection_pool (2.2.5) crack (0.4.5) @@ -337,7 +337,7 @@ GEM mime-types (~> 3.0) multi_xml (>= 0.5.2) httpclient (2.8.3) - i18n (1.9.1) + i18n (1.10.0) concurrent-ruby (~> 1.0) i18n-inflector (2.6.7) i18n (>= 0.4.1) @@ -392,7 +392,7 @@ GEM multi_json (~> 1.14) logging-rails (0.6.0) logging (>= 1.8) - loofah (2.14.0) + loofah (2.16.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) macaddr (1.7.2) @@ -527,18 +527,18 @@ GEM rack rack-test (1.1.0) rack (>= 1.0, < 3) - rails (5.2.6.2) - actioncable (= 5.2.6.2) - actionmailer (= 5.2.6.2) - actionpack (= 5.2.6.2) - actionview (= 5.2.6.2) - activejob (= 5.2.6.2) - activemodel (= 5.2.6.2) - activerecord (= 5.2.6.2) - activestorage (= 5.2.6.2) - activesupport (= 5.2.6.2) + rails (5.2.7.1) + actioncable (= 5.2.7.1) + actionmailer (= 5.2.7.1) + actionpack (= 5.2.7.1) + actionview (= 5.2.7.1) + activejob (= 5.2.7.1) + activemodel (= 5.2.7.1) + activerecord (= 5.2.7.1) + activestorage (= 5.2.7.1) + activesupport (= 5.2.7.1) bundler (>= 1.3.0) - railties (= 5.2.6.2) + railties (= 5.2.7.1) sprockets-rails (>= 2.0.0) rails-assets-autosize (4.0.2) rails-assets-backbone (1.3.3) @@ -602,9 +602,9 @@ GEM rails-timeago (2.19.1) actionpack (>= 3.1) activesupport (>= 3.1) - railties (5.2.6.2) - actionpack (= 5.2.6.2) - activesupport (= 5.2.6.2) + railties (5.2.7.1) + actionpack (= 5.2.7.1) + activesupport (= 5.2.7.1) method_source rake (>= 0.8.7) thor (>= 0.19.0, < 2.0) @@ -878,7 +878,7 @@ DEPENDENCIES rack-piwik (= 0.3.0) rack-rewrite (= 1.5.1) rack-ssl (= 1.4.1) - rails (= 5.2.6.2) + rails (= 5.2.7.1) rails-assets-autosize (= 4.0.2)! rails-assets-backbone (= 1.3.3)! rails-assets-blueimp-gallery (= 2.33.0)! 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 3/3] 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'