Merge branch 'next-minor' into develop

Changelog.md

@@ -39,6 +39,10 @@ Although the chat was never enabled per default and was marked as experimental,
 
 # 0.7.17.0
 
+## 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
 
 ## Bug fixes

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)

Gemfile.lock

@@ -2,25 +2,25 @@ GEM
   remote: https://rubygems.org/
   remote: https://gems.diasporafoundation.org/
   specs:
-    actioncable (5.2.6.2)
+    actioncable (5.2.7.1)
-      actionpack (= 5.2.6.2)
+      actionpack (= 5.2.7.1)
       nio4r (~> 2.0)
       websocket-driver (>= 0.6.1)
-    actionmailer (5.2.6.2)
+    actionmailer (5.2.7.1)
-      actionpack (= 5.2.6.2)
+      actionpack (= 5.2.7.1)
-      actionview (= 5.2.6.2)
+      actionview (= 5.2.7.1)
-      activejob (= 5.2.6.2)
+      activejob (= 5.2.7.1)
       mail (~> 2.5, >= 2.5.4)
       rails-dom-testing (~> 2.0)
-    actionpack (5.2.6.2)
+    actionpack (5.2.7.1)
-      actionview (= 5.2.6.2)
+      actionview (= 5.2.7.1)
-      activesupport (= 5.2.6.2)
+      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)
+    actionview (5.2.7.1)
-      activesupport (= 5.2.6.2)
+      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)
+    activejob (5.2.7.1)
-      activesupport (= 5.2.6.2)
+      activesupport (= 5.2.7.1)
       globalid (>= 0.3.6)
-    activemodel (5.2.6.2)
+    activemodel (5.2.7.1)
-      activesupport (= 5.2.6.2)
+      activesupport (= 5.2.7.1)
-    activerecord (5.2.6.2)
+    activerecord (5.2.7.1)
-      activemodel (= 5.2.6.2)
+      activemodel (= 5.2.7.1)
-      activesupport (= 5.2.6.2)
+      activesupport (= 5.2.7.1)
       arel (>= 9.0)
     activerecord-import (1.1.0)
       activerecord (>= 3.2)
-    activestorage (5.2.6.2)
+    activestorage (5.2.7.1)
-      actionpack (= 5.2.6.2)
+      actionpack (= 5.2.7.1)
-      activerecord (= 5.2.6.2)
+      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)
@@ -143,7 +143,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)
@@ -340,7 +340,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)
@@ -398,7 +398,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)
@@ -533,18 +533,18 @@ GEM
       rack
     rack-test (1.1.0)
       rack (>= 1.0, < 3)
-    rails (5.2.6.2)
+    rails (5.2.7.1)
-      actioncable (= 5.2.6.2)
+      actioncable (= 5.2.7.1)
-      actionmailer (= 5.2.6.2)
+      actionmailer (= 5.2.7.1)
-      actionpack (= 5.2.6.2)
+      actionpack (= 5.2.7.1)
-      actionview (= 5.2.6.2)
+      actionview (= 5.2.7.1)
-      activejob (= 5.2.6.2)
+      activejob (= 5.2.7.1)
-      activemodel (= 5.2.6.2)
+      activemodel (= 5.2.7.1)
-      activerecord (= 5.2.6.2)
+      activerecord (= 5.2.7.1)
-      activestorage (= 5.2.6.2)
+      activestorage (= 5.2.7.1)
-      activesupport (= 5.2.6.2)
+      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)
@@ -595,9 +595,9 @@ GEM
     rails-timeago (2.19.1)
       actionpack (>= 3.1)
       activesupport (>= 3.1)
-    railties (5.2.6.2)
+    railties (5.2.7.1)
-      actionpack (= 5.2.6.2)
+      actionpack (= 5.2.7.1)
-      activesupport (= 5.2.6.2)
+      activesupport (= 5.2.7.1)
       method_source
       rake (>= 0.8.7)
       thor (>= 0.19.0, < 2.0)
@@ -877,7 +877,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)!

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
+    if params[:change_password] && user_password_params
-      # change password
+      password_changed = change_password(user_password_params)
-      if params[:change_password]
+      return redirect_to new_user_session_path if password_changed
-        password_changed = change_password(user_data)
+    elsif user_params
-      else
+      update_user(user_params)
-        update_user(user_data)
-      end
     end
 
-    if password_changed
+    set_email_preferences
-      redirect_to new_user_session_path
+    render :edit
-    else
-      set_email_preferences
-      render :edit
-    end
   end
 
   def update_privacy_settings
@@ -132,13 +124,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,
@@ -147,14 +135,19 @@ class UsersController < ApplicationController
       :auto_follow_back_aspect_id,
       :getting_started,
       :post_default_public,
-      :otp_required_for_login,
-      :otp_secret,
       :exported_photos_file,
       :export,
       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]
@@ -176,8 +169,8 @@ class UsersController < ApplicationController
     end
   end
 
-  def change_password(user_data)
+  def change_password(password_params)
-    if @user.update_with_password(user_data)
+    if @user.update_with_password(password_params)
       flash[:notice] = t("users.update.password_changed")
       true
     else

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"

spec/controllers/users_controller_spec.rb

@@ -114,38 +114,67 @@ describe UsersController, :type => :controller do
     end
   end
 
-  describe '#update' do
+  describe "#update" do
-    before do
+    context "with random params" do
-      @params  = { :id => @user.id,
+      let(:params) { {id: @user.id, user: {diaspora_handle: "notreal@stuff.com"}} }
-                  :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
+    describe "password updates" 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
       let(:password_params) do
-        {:current_password => 'bluepin7',
+        {current_password: "bluepin7", password: "foobaz", password_confirmation: "foobaz"}
-         :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