Add latest changes from gitlab-org/security/gitlab@14-9-stable-ee

6 files changed, 86 insertions, 8 deletions

diff --git a/Gemfile b/Gemfile
index ed18ae38318..7423bba8e62 100644
--- a/Gemfile
+++ b/Gemfile
@@ -67,7 +67,7 @@ gem 'akismet', '~> 3.0'
 gem 'invisible_captcha', '~> 1.1.0'
 
 # Two-factor authentication
-gem 'devise-two-factor', '~> 4.0.0'
+gem 'devise-two-factor', '~> 4.0.2'
 gem 'rqrcode-rails3', '~> 0.1.7'
 gem 'attr_encrypted', '~> 3.1.0'
 gem 'u2f', '~> 0.2.1'
diff --git a/Gemfile.lock b/Gemfile.lock
index 2b0057d353d..f97c2bf67ec 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -270,11 +270,11 @@ GEM
       railties (>= 4.1.0)
       responders
       warden (~> 1.2.3)
-    devise-two-factor (4.0.0)
-      activesupport (< 6.2)
+    devise-two-factor (4.0.2)
+      activesupport (< 7.1)
       attr_encrypted (>= 1.3, < 4, != 2)
       devise (~> 4.0)
-      railties (< 6.2)
+      railties (< 7.1)
       rotp (~> 6.0)
     diff-lcs (1.4.4)
     diff_match_patch (0.1.0)
@@ -1457,7 +1457,7 @@ DEPENDENCIES
   derailed_benchmarks
   device_detector
   devise (~> 4.7.2)
-  devise-two-factor (~> 4.0.0)
+  devise-two-factor (~> 4.0.2)
   diff_match_patch (~> 0.1.0)
   diffy (~> 3.3)
   discordrb-webhooks (~> 3.4)
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 4228da279a4..b9ba9d8e1b0 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -65,6 +65,8 @@ module Ci
     FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze
     MINUTES_COST_FACTOR_FIELDS = %i[public_projects_minutes_cost_factor private_projects_minutes_cost_factor].freeze
 
+    TAG_LIST_MAX_LENGTH = 50
+
     has_many :builds
     has_many :runner_projects, inverse_of: :runner, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
     has_many :projects, through: :runner_projects, disable_joins: true
@@ -520,6 +522,11 @@ module Ci
         errors.add(:tags_list,
           'can not be empty when runner is not allowed to pick untagged jobs')
       end
+
+      if tag_list_changed? && tag_list.count > TAG_LIST_MAX_LENGTH
+        errors.add(:tags_list,
+          "Too many tags specified. Please limit the number of tags to #{TAG_LIST_MAX_LENGTH}")
+      end
     end
 
     def no_projects
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 0620bb1ffc5..42187c3ef99 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -41,15 +41,25 @@ RSpec.describe Ci::Runner do
 
     context 'when runner is not allowed to pick untagged jobs' do
       context 'when runner does not have tags' do
+        let(:runner) { build(:ci_runner, tag_list: [], run_untagged: false) }
+
+        it 'is not valid' do
+          expect(runner).to be_invalid
+        end
+      end
+
+      context 'when runner has too many tags' do
+        let(:runner) { build(:ci_runner, tag_list: (1..::Ci::Runner::TAG_LIST_MAX_LENGTH + 1).map { |i| "tag#{i}" }, run_untagged: false) }
+
         it 'is not valid' do
-          runner = build(:ci_runner, tag_list: [], run_untagged: false)
           expect(runner).to be_invalid
         end
       end
 
       context 'when runner has tags' do
+        let(:runner) { build(:ci_runner, tag_list: ['tag'], run_untagged: false) }
+
         it 'is valid' do
-          runner = build(:ci_runner, tag_list: ['tag'], run_untagged: false)
           expect(runner).to be_valid
         end
       end
diff --git a/spec/requests/api/ci/runner/runners_post_spec.rb b/spec/requests/api/ci/runner/runners_post_spec.rb
index 1d553751eea..50ace7adccb 100644
--- a/spec/requests/api/ci/runner/runners_post_spec.rb
+++ b/spec/requests/api/ci/runner/runners_post_spec.rb
@@ -130,7 +130,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           }
         end
 
-        let_it_be(:new_runner) { create(:ci_runner) }
+        let_it_be(:new_runner) { build(:ci_runner) }
 
         it 'uses active value in registration' do
           expect_next_instance_of(::Ci::Runners::RegisterRunnerService) do |service|
@@ -183,6 +183,54 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           expect(response).to have_gitlab_http_status(:created)
           expect(::Ci::Runner.last.ip_address).to eq('123.111.123.111')
         end
+
+        context 'when tags parameter is provided' do
+          def request
+            post api('/runners'), params: {
+              token: registration_token,
+              tag_list: tag_list
+            }
+          end
+
+          context 'with number of tags above limit' do
+            let(:tag_list) { (1..::Ci::Runner::TAG_LIST_MAX_LENGTH + 1).map { |i| "tag#{i}" } }
+
+            it 'uses tag_list value in registration and returns error' do
+              expect_next_instance_of(::Ci::Runners::RegisterRunnerService) do |service|
+                expected_params = { tag_list: tag_list }.stringify_keys
+
+                expect(service).to receive(:execute)
+                  .once
+                  .with(registration_token, a_hash_including(expected_params))
+                  .and_call_original
+              end
+
+              request
+
+              expect(response).to have_gitlab_http_status(:bad_request)
+              expect(json_response.dig('message', 'tags_list')).to contain_exactly("Too many tags specified. Please limit the number of tags to #{::Ci::Runner::TAG_LIST_MAX_LENGTH}")
+            end
+          end
+
+          context 'with number of tags below limit' do
+            let(:tag_list) { (1..20).map { |i| "tag#{i}" } }
+
+            it 'uses tag_list value in registration and successfully creates runner' do
+              expect_next_instance_of(::Ci::Runners::RegisterRunnerService) do |service|
+                expected_params = { tag_list: tag_list }.stringify_keys
+
+                expect(service).to receive(:execute)
+                  .once
+                  .with(registration_token, a_hash_including(expected_params))
+                  .and_call_original
+              end
+
+              request
+
+              expect(response).to have_gitlab_http_status(:created)
+            end
+          end
+        end
       end
     end
   end
diff --git a/spec/requests/api/ci/runners_spec.rb b/spec/requests/api/ci/runners_spec.rb
index a1fda68b77b..d6ebc197ab0 100644
--- a/spec/requests/api/ci/runners_spec.rb
+++ b/spec/requests/api/ci/runners_spec.rb
@@ -396,6 +396,19 @@ RSpec.describe API::Ci::Runners do
           expect(shared_runner.reload.tag_list).to include('ruby2.1', 'pgsql', 'mysql')
         end
 
+        it 'unrelated runner attribute on an existing runner with too many tags' do
+          # This test ensures that it is possible to update any attribute on a runner that currently fails the
+          # validation that ensures that there aren't too many tags associated with a runner
+          existing_invalid_shared_runner = build(:ci_runner, :instance, tag_list: (1..::Ci::Runner::TAG_LIST_MAX_LENGTH + 1).map { |i| "tag#{i}" } )
+          existing_invalid_shared_runner.save!(validate: false)
+
+          active = existing_invalid_shared_runner.active
+          update_runner(existing_invalid_shared_runner.id, admin, paused: active)
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(existing_invalid_shared_runner.reload.active).to eq(!active)
+        end
+
         it 'runner untagged flag' do
           # Ensure tag list is non-empty before setting untagged to false.
           update_runner(shared_runner.id, admin, tag_list: ['ruby2.1', 'pgsql', 'mysql'])