Merge branch 'fix/mirror-user' into 'security-9-3-ee'

Fix mirroring and CI exploit

See merge request !518

app/controllers/concerns/safe_mirror_params.rb

+module SafeMirrorParams
+  extend ActiveSupport::Concern
+
+  included do
+    helper_method :default_mirror_users
+  end
+
+  private
+
+  def valid_mirror_user?(mirror_params)
+    return true unless mirror_params[:mirror_user_id].present?
+
+    default_mirror_users.map(&:id).include?(mirror_params[:mirror_user_id].to_i)
+  end
+
+  def default_mirror_users
+    [current_user, @project.mirror_user].compact.uniq
+  end
+end

app/controllers/projects/imports_controller.rb

 class Projects::ImportsController < Projects::ApplicationController
   include ContinueParams
+  include SafeMirrorParams
 
   # Authorize
   before_action :authorize_admin_project!
@@ -11,7 +12,7 @@ class Projects::ImportsController < Projects::ApplicationController
   end
 
   def create
-    if @project.update_attributes(import_params)
+    if @project.update_attributes(safe_import_params)
       @project.reload.import_schedule
     end
 
@@ -67,4 +68,10 @@ class Projects::ImportsController < Projects::ApplicationController
   def import_params
     params.require(:project).permit(:import_url, :mirror, :mirror_user_id)
   end
+
+  def safe_import_params
+    return import_params if valid_mirror_user?(import_params)
+
+    import_params.merge(mirror_user_id: current_user.id)
+  end
 end

app/controllers/projects/mirrors_controller.rb

 class Projects::MirrorsController < Projects::ApplicationController
   include RepositorySettingsRedirect
+  include SafeMirrorParams
   # Authorize
   before_action :authorize_admin_project!, except: [:update_now]
   before_action :authorize_push_code!, only: [:update_now]
@@ -12,7 +13,7 @@ class Projects::MirrorsController < Projects::ApplicationController
   end
 
   def update
-    if @project.update_attributes(mirror_params)
+    if @project.update_attributes(safe_mirror_params)
       if @project.mirror?
         @project.force_import_job!
 
@@ -51,4 +52,10 @@ class Projects::MirrorsController < Projects::ApplicationController
     params.require(:project).permit(:mirror, :import_url, :mirror_user_id,
                                     :mirror_trigger_builds, remote_mirrors_attributes: [:url, :id, :enabled])
   end
+
+  def safe_mirror_params
+    return mirror_params if valid_mirror_user?(mirror_params)
+
+    mirror_params.merge(mirror_user_id: current_user.id)
+  end
 end

app/controllers/projects/settings/repository_controller.rb

 module Projects
   module Settings
     class RepositoryController < Projects::ApplicationController
+      include SafeMirrorParams
+
       before_action :authorize_admin_project!
 
       prepend ::EE::Projects::Settings::RepositoryController

app/helpers/mirror_helper.rb

@@ -4,4 +4,8 @@ module MirrorHelper
     message << "<br>To discard the local changes and overwrite the branch with the upstream version, delete it here and choose 'Update Now' above." if can?(current_user, :push_code, @project)
     message
   end
+
+  def options_for_mirror_user
+    options_from_collection_for_select(default_mirror_users, :id, :name, @project.mirror_user_id || current_user.id)
+  end
 end

app/views/projects/mirrors/_show.html.haml

@@ -36,12 +36,11 @@
         = render "projects/mirrors/instructions"
       .form-group
         = f.label :mirror_user_id, "Mirror user", class: "label-light"
-        = users_select_tag("project[mirror_user_id]", class: 'input-large', selected: @project.mirror_user_id || current_user.id,
-          first_user: true, current_user: true, push_code_to_protected_branches: true)
+        = select_tag('project[mirror_user_id]', options_for_mirror_user, class: "select2 lg", required: true)
         .help-block
           This user will be the author of all events in the activity feed that are the result of an update,
           like new branches being created or new commits being pushed to existing branches.
-          They need to have at least master access to this project.
+          You can only assign yourself to be the mirror user.
       - if @project.builds_enabled?
         = render "shared/mirror_trigger_builds_setting", f: f
       = f.submit 'Save changes', class: 'btn btn-create', name: 'update_remote_mirror'

app/views/shared/_mirror_trigger_builds_setting.html.haml

@@ -6,3 +6,4 @@
       Trigger pipelines when branches or tags are updated from the upstream repository.
       Depending on the activity of the upstream repository, this may greatly increase the load on your CI runners.
       Only enable this if you know they can handle the load.
+      <strong>CI will run using the credentials assigned above.</strong>

changelogs/unreleased-ee/fix-mirror-user.yml

+---
+title: Prevent mirror user to be assigned to users other than the current one
+merge_request:
+author:

spec/controllers/projects/imports_controller_spec.rb

@@ -2,16 +2,15 @@ require 'spec_helper'
 
 describe Projects::ImportsController do
   let(:user) { create(:user) }
+  let(:project) { create(:empty_project) }
+
+  before do
+    sign_in(user)
+    project.team << [user, :master]
+  end
 
   describe 'GET #show' do
     context 'when repository does not exists' do
-      let(:project) { create(:empty_project) }
-
-      before do
-        sign_in(user)
-        project.team << [user, :master]
-      end
-
       it 'renders template' do
         get :show, namespace_id: project.namespace.to_param, project_id: project
 
@@ -28,11 +27,6 @@ describe Projects::ImportsController do
     context 'when repository exists' do
       let(:project) { create(:project_empty_repo, import_url: 'https://github.com/vim/vim.git') }
 
-      before do
-        sign_in(user)
-        project.team << [user, :master]
-      end
-
       context 'when import is in progress' do
         before do
           project.update_attribute(:import_status, :started)
@@ -125,4 +119,23 @@ describe Projects::ImportsController do
       end
     end
   end
+
+  context 'POST #create' do
+    context 'mirror user is not the current user' do
+      it 'should only assign the current user' do
+        allow_any_instance_of(EE::Project).to receive(:add_import_job)
+
+        new_user = create(:user)
+        project.add_master(new_user)
+
+        post :create, namespace_id: project.namespace.to_param,
+                      project_id: project,
+                      project: { mirror: true, mirror_user_id: new_user.id, import_url: 'http://local.dev' },
+                      format: :json
+
+        expect(project.reload.mirror).to eq(true)
+        expect(project.reload.mirror_user.id).to eq(user.id)
+      end
+    end
+  end
 end

spec/controllers/projects/mirrors_controller_spec.rb

@@ -71,6 +71,20 @@ describe Projects::MirrorsController do
           expect(project.reload.mirror).to eq(true)
           expect(project.reload.import_url).to eq('http://local.dev')
         end
+
+        context 'mirror user is not the current user' do
+          it 'should only assign the current user' do
+            expect_any_instance_of(EE::Project).to receive(:force_import_job!)
+
+            new_user = create(:user)
+            project.add_master(new_user)
+
+            do_put(project, mirror: true, mirror_user_id: new_user.id, import_url: 'http://local.dev')
+
+            expect(project.reload.mirror).to eq(true)
+            expect(project.reload.mirror_user.id).to eq(project.owner.id)
+          end
+        end
       end
     end
   end

spec/features/projects/settings/ee/push_rules_settings_spec.rb

@@ -60,7 +60,7 @@ describe 'Project settings > [EE] repository', feature: true do
 
         click_button('Save changes')
 
-        expect(find('.select2-chosen')).to have_content(user2.name)
+        expect(find('.select2-chosen')).to have_content(user.name)
       end
     end
   end