Add latest changes from gitlab-org/security/gitlab@13-4-stable-ee

17 files changed, 209 insertions, 76 deletions

diff --git a/app/models/packages/package.rb b/app/models/packages/package.rb
index bda11160957..48c40750db2 100644
--- a/app/models/packages/package.rb
+++ b/app/models/packages/package.rb
@@ -37,6 +37,7 @@ class Packages::Package < ApplicationRecord
   validate :package_already_taken, if: :npm?
   validates :version, format: { with: Gitlab::Regex.semver_regex }, if: -> { npm? || nuget? }
   validates :name, format: { with: Gitlab::Regex.conan_recipe_component_regex }, if: :conan?
+  validates :name, format: { with: Gitlab::Regex.nuget_package_name_regex }, if: :nuget?
   validates :version, format: { with: Gitlab::Regex.conan_recipe_component_regex }, if: :conan?
   validates :version, format: { with: Gitlab::Regex.maven_version_regex }, if: -> { version? && maven? }
   validates :version, format: { with: Gitlab::Regex.pypi_version_regex }, if: :pypi?
diff --git a/app/policies/ci/pipeline_schedule_policy.rb b/app/policies/ci/pipeline_schedule_policy.rb
index cf3f784f851..2ef5ffd6a5a 100644
--- a/app/policies/ci/pipeline_schedule_policy.rb
+++ b/app/policies/ci/pipeline_schedule_policy.rb
@@ -17,6 +17,7 @@ module Ci
     rule { can?(:admin_pipeline) | (can?(:update_build) & owner_of_schedule) }.policy do
       enable :update_pipeline_schedule
       enable :admin_pipeline_schedule
+      enable :read_pipeline_schedule_variables
     end
 
     rule { can?(:admin_pipeline_schedule) & ~owner_of_schedule }.policy do
diff --git a/app/services/packages/nuget/update_package_from_metadata_service.rb b/app/services/packages/nuget/update_package_from_metadata_service.rb
index f72b1386985..0109ee23c49 100644
--- a/app/services/packages/nuget/update_package_from_metadata_service.rb
+++ b/app/services/packages/nuget/update_package_from_metadata_service.rb
@@ -32,6 +32,8 @@ module Packages
             )
           end
         end
+      rescue ActiveRecord::RecordInvalid => e
+        raise InvalidMetadataError.new(e.message)
       end
 
       private
diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml
index a2b736c332c..ceeb525b4f3 100644
--- a/app/views/admin/runners/_runner.html.haml
+++ b/app/views/admin/runners/_runner.html.haml
@@ -69,10 +69,10 @@
           = sprite_icon('pencil')
       .btn-group
         - if runner.active?
-          = link_to [:pause, :admin, runner], method: :get, class: 'btn btn-default btn-svg has-tooltip', title: _('Pause'), ref: 'tooltip', aria: { label: _('Pause') }, data: { placement: 'top', container: 'body', confirm: _('Are you sure?') } do
+          = link_to [:pause, :admin, runner], method: :post, class: 'btn btn-default btn-svg has-tooltip', title: _('Pause'), ref: 'tooltip', aria: { label: _('Pause') }, data: { placement: 'top', container: 'body', confirm: _('Are you sure?') } do
             = sprite_icon('pause')
         - else
-          = link_to [:resume, :admin, runner], method: :get, class: 'btn btn-default btn-svg has-tooltip gl-px-3', title: _('Resume'), ref: 'tooltip', aria: { label: _('Resume') }, data: { placement: 'top', container: 'body'} do
+          = link_to [:resume, :admin, runner], method: :post, class: 'btn btn-default btn-svg has-tooltip gl-px-3', title: _('Resume'), ref: 'tooltip', aria: { label: _('Resume') }, data: { placement: 'top', container: 'body'} do
             = sprite_icon('play')
       .btn-group
         = link_to [:admin, runner], method: :delete, class: 'btn btn-danger has-tooltip', title: _('Remove'), ref: 'tooltip', aria: { label: _('Remove') }, data: { placement: 'top', container: 'body', confirm: _('Are you sure?') } do
diff --git a/changelogs/unreleased/security-10io-validate-nuget-package-name.yml b/changelogs/unreleased/security-10io-validate-nuget-package-name.yml
new file mode 100644
index 00000000000..aaf30711868
--- /dev/null
+++ b/changelogs/unreleased/security-10io-validate-nuget-package-name.yml
@@ -0,0 +1,5 @@
+---
+title: Validate nuget package names
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-runner-csrf-milestone-13-6.yml b/changelogs/unreleased/security-runner-csrf-milestone-13-6.yml
new file mode 100644
index 00000000000..4b661ecfdc9
--- /dev/null
+++ b/changelogs/unreleased/security-runner-csrf-milestone-13-6.yml
@@ -0,0 +1,5 @@
+---
+title: Add CSRF protection to runner pause and resume
+merge_request: 1021
+author:
+type: security
diff --git a/changelogs/unreleased/security-unauthorized-access-schedule-variables-milestone-13-6.yml b/changelogs/unreleased/security-unauthorized-access-schedule-variables-milestone-13-6.yml
new file mode 100644
index 00000000000..fc6702f8067
--- /dev/null
+++ b/changelogs/unreleased/security-unauthorized-access-schedule-variables-milestone-13-6.yml
@@ -0,0 +1,5 @@
+---
+title: Fix unauthorized user is able to access schedule pipeline variables and values
+merge_request:
+author:
+type: security
diff --git a/config/routes/admin.rb b/config/routes/admin.rb
index bac8247de2e..9c459014005 100644
--- a/config/routes/admin.rb
+++ b/config/routes/admin.rb
@@ -147,8 +147,8 @@ namespace :admin do
 
   resources :runners, only: [:index, :show, :update, :destroy] do
     member do
-      get :resume
-      get :pause
+      post :resume
+      post :pause
     end
 
     collection do
diff --git a/lib/api/ci/pipeline_schedules.rb b/lib/api/ci/pipeline_schedules.rb
index 1afdb0ad34c..ff32f4597c8 100644
--- a/lib/api/ci/pipeline_schedules.rb
+++ b/lib/api/ci/pipeline_schedules.rb
@@ -36,7 +36,7 @@ module API
           requires :pipeline_schedule_id, type: Integer, desc: 'The pipeline schedule id'
         end
         get ':id/pipeline_schedules/:pipeline_schedule_id' do
-          present pipeline_schedule, with: Entities::Ci::PipelineScheduleDetails
+          present pipeline_schedule, with: Entities::Ci::PipelineScheduleDetails, user: current_user
         end
 
         desc 'Create a new pipeline schedule' do
diff --git a/lib/api/entities/ci/pipeline_schedule_details.rb b/lib/api/entities/ci/pipeline_schedule_details.rb
index b233728b95b..d661a8940ea 100644
--- a/lib/api/entities/ci/pipeline_schedule_details.rb
+++ b/lib/api/entities/ci/pipeline_schedule_details.rb
@@ -5,7 +5,9 @@ module API
     module Ci
       class PipelineScheduleDetails < PipelineSchedule
         expose :last_pipeline, using: ::API::Entities::Ci::PipelineBasic
-        expose :variables, using: ::API::Entities::Ci::Variable
+        expose :variables,
+          using: ::API::Entities::Ci::Variable,
+          if: ->(schedule, options) { Ability.allowed?(options[:user], :read_pipeline_schedule_variables, schedule) }
       end
     end
   end
diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb
index df5e3163719..5660b80221b 100644
--- a/lib/gitlab/regex.rb
+++ b/lib/gitlab/regex.rb
@@ -46,6 +46,10 @@ module Gitlab
         maven_app_name_regex
       end
 
+      def nuget_package_name_regex
+        @nuget_package_name_regex ||= %r{\A[-+\.\_a-zA-Z0-9]+\z}.freeze
+      end
+
       def pypi_version_regex
         # See the official regex: https://github.com/pypa/packaging/blob/16.7/packaging/version.py#L159
 
diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb
index b4ed4f37c3b..a853dd0758c 100644
--- a/spec/lib/gitlab/regex_spec.rb
+++ b/spec/lib/gitlab/regex_spec.rb
@@ -322,6 +322,21 @@ RSpec.describe Gitlab::Regex do
     it { is_expected.not_to match('%2e%2e%2f1.2.3') }
   end
 
+  describe '.nuget_package_name_regex' do
+    subject { described_class.nuget_package_name_regex }
+
+    it { is_expected.to match('My.Package') }
+    it { is_expected.to match('My.Package.Mvc') }
+    it { is_expected.to match('MyPackage') }
+    it { is_expected.to match('My.23.Package') }
+    it { is_expected.to match('My23Package') }
+    it { is_expected.to match('runtime.my-test64.runtime.package.Mvc') }
+    it { is_expected.to match('my_package') }
+    it { is_expected.not_to match('My/package') }
+    it { is_expected.not_to match('../../../my_package') }
+    it { is_expected.not_to match('%2e%2e%2fmy_package') }
+  end
+
   describe '.pypi_version_regex' do
     subject { described_class.pypi_version_regex }
 
diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb
index ea1f75d04e7..78c2027d474 100644
--- a/spec/models/packages/package_spec.rb
+++ b/spec/models/packages/package_spec.rb
@@ -108,6 +108,21 @@ RSpec.describe Packages::Package, type: :model do
         it { is_expected.not_to allow_value('.foobar').for(:name) }
         it { is_expected.not_to allow_value('%foo%bar').for(:name) }
       end
+
+      context 'nuget package' do
+        subject { build_stubbed(:nuget_package) }
+
+        it { is_expected.to allow_value('My.Package').for(:name) }
+        it { is_expected.to allow_value('My.Package.Mvc').for(:name) }
+        it { is_expected.to allow_value('MyPackage').for(:name) }
+        it { is_expected.to allow_value('My.23.Package').for(:name) }
+        it { is_expected.to allow_value('My23Package').for(:name) }
+        it { is_expected.to allow_value('runtime.my-test64.runtime.package.Mvc').for(:name) }
+        it { is_expected.to allow_value('my_package').for(:name) }
+        it { is_expected.not_to allow_value('My/package').for(:name) }
+        it { is_expected.not_to allow_value('../../../my_package').for(:name) }
+        it { is_expected.not_to allow_value('%2e%2e%2fmy_package').for(:name) }
+      end
     end
 
     describe '#version' do
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index 0c457148b4d..79e569d2b08 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -137,7 +137,7 @@ RSpec.describe ProjectPolicy do
       it 'disallows all permissions except pipeline when the feature is disabled' do
         builds_permissions = [
           :create_build, :read_build, :update_build, :admin_build, :destroy_build,
-          :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule,
+          :create_pipeline_schedule, :read_pipeline_schedule_variables, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule,
           :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment,
           :create_cluster, :read_cluster, :update_cluster, :admin_cluster, :destroy_cluster,
           :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment
diff --git a/spec/requests/api/ci/pipeline_schedules_spec.rb b/spec/requests/api/ci/pipeline_schedules_spec.rb
index e0199b7b51c..4c8a356469d 100644
--- a/spec/requests/api/ci/pipeline_schedules_spec.rb
+++ b/spec/requests/api/ci/pipeline_schedules_spec.rb
@@ -97,46 +97,112 @@ RSpec.describe API::Ci::PipelineSchedules do
       pipeline_schedule.pipelines << build(:ci_pipeline, project: project)
     end
 
-    context 'authenticated user with valid permissions' do
-      it 'returns pipeline_schedule details' do
-        get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", developer)
-
+    matcher :return_pipeline_schedule_sucessfully do
+      match_unless_raises do |reponse|
         expect(response).to have_gitlab_http_status(:ok)
         expect(response).to match_response_schema('pipeline_schedule')
       end
+    end
 
-      it 'responds with 404 Not Found if requesting non-existing pipeline_schedule' do
-        get api("/projects/#{project.id}/pipeline_schedules/-5", developer)
+    shared_context 'request with project permissions' do
+      context 'authenticated user with project permisions' do
+        before do
+          project.add_maintainer(user)
+        end
 
-        expect(response).to have_gitlab_http_status(:not_found)
+        it 'returns pipeline_schedule details' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to return_pipeline_schedule_sucessfully
+          expect(json_response).to have_key('variables')
+        end
       end
     end
 
-    context 'authenticated user with invalid permissions' do
-      it 'does not return pipeline_schedules list' do
-        get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+    shared_examples 'request with schedule ownership' do
+      context 'authenticated user with pipeline schedule ownership' do
+        it 'returns pipeline_schedule details' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", developer)
 
-        expect(response).to have_gitlab_http_status(:not_found)
+          expect(response).to return_pipeline_schedule_sucessfully
+          expect(json_response).to have_key('variables')
+        end
       end
     end
 
-    context 'authenticated user with insufficient permissions' do
-      before do
-        project.add_guest(user)
+    shared_examples 'request with unauthenticated user' do
+      context 'with unauthenticated user' do
+        it 'does not return pipeline_schedule' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}")
+
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
       end
+    end
 
-      it 'does not return pipeline_schedules list' do
-        get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+    shared_examples 'request with non-existing pipeline_schedule' do
+      it 'responds with 404 Not Found if requesting non-existing pipeline_schedule' do
+        get api("/projects/#{project.id}/pipeline_schedules/-5", developer)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
-    context 'unauthenticated user' do
-      it 'does not return pipeline_schedules list' do
-        get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}")
+    context 'with private project' do
+      it_behaves_like 'request with schedule ownership'
+      it_behaves_like 'request with project permissions'
+      it_behaves_like 'request with unauthenticated user'
+      it_behaves_like 'request with non-existing pipeline_schedule'
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+      context 'authenticated user with no project permissions' do
+        it 'does not return pipeline_schedule' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+
+      context 'authenticated user with insufficient project permissions' do
+        before do
+          project.add_guest(user)
+        end
+
+        it 'does not return pipeline_schedule' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
+    context 'with public project' do
+      let_it_be(:project) { create(:project, :repository, :public, public_builds: false) }
+
+      it_behaves_like 'request with schedule ownership'
+      it_behaves_like 'request with project permissions'
+      it_behaves_like 'request with unauthenticated user'
+      it_behaves_like 'request with non-existing pipeline_schedule'
+
+      context 'authenticated user with no project permissions' do
+        it 'returns pipeline_schedule with no variables' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to return_pipeline_schedule_sucessfully
+          expect(json_response).not_to have_key('variables')
+        end
+      end
+
+      context 'authenticated user with insufficient project permissions' do
+        before do
+          project.add_guest(user)
+        end
+
+        it 'returns pipeline_schedule with no variables' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to return_pipeline_schedule_sucessfully
+          expect(json_response).not_to have_key('variables')
+        end
       end
     end
   end
diff --git a/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb b/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb
index b7c780c1ee2..92b493ed376 100644
--- a/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb
+++ b/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb
@@ -198,24 +198,26 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_
       it_behaves_like 'raising an', ::Packages::Nuget::MetadataExtractionService::ExtractionError
     end
 
-    context 'with package file with a blank package name' do
-      before do
-        allow(service).to receive(:package_name).and_return('')
-      end
+    context 'with an invalid package name' do
+      invalid_names = [
+        '',
+        'My/package',
+        '../../../my_package',
+        '%2e%2e%2fmy_package'
+      ]
 
-      it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
-    end
+      invalid_names.each do |invalid_name|
+        before do
+          allow(service).to receive(:package_name).and_return(invalid_name)
+        end
 
-    context 'with package file with a blank package version' do
-      before do
-        allow(service).to receive(:package_version).and_return('')
+        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
       end
-
-      it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
     end
 
     context 'with an invalid package version' do
       invalid_versions = [
+        '',
         '555',
         '1.2',
         '1./2.3',
@@ -224,13 +226,11 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_
       ]
 
       invalid_versions.each do |invalid_version|
-        it "raises an error for version #{invalid_version}" do
+        before do
           allow(service).to receive(:package_version).and_return(invalid_version)
-
-          expect { subject }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Version is invalid')
-          expect(package_file.file_name).not_to include(invalid_version)
-          expect(package_file.file.file.path).not_to include(invalid_version)
         end
+
+        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
       end
     end
   end
diff --git a/spec/workers/packages/nuget/extraction_worker_spec.rb b/spec/workers/packages/nuget/extraction_worker_spec.rb
index 35b5f1baed5..4703afc9413 100644
--- a/spec/workers/packages/nuget/extraction_worker_spec.rb
+++ b/spec/workers/packages/nuget/extraction_worker_spec.rb
@@ -13,6 +13,18 @@ RSpec.describe Packages::Nuget::ExtractionWorker, type: :worker do
 
     subject { described_class.new.perform(package_file_id) }
 
+    shared_examples 'handling the metadata error' do |exception_class: ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError|
+      it 'removes the package and the package file' do
+        expect(Gitlab::ErrorTracking).to receive(:log_exception).with(
+          instance_of(exception_class),
+          project_id: package.project_id
+        )
+        expect { subject }
+          .to change { Packages::Package.count }.by(-1)
+          .and change { Packages::PackageFile.count }.by(-1)
+      end
+    end
+
     context 'with valid package file' do
       it 'updates package and package file' do
         expect { subject }
@@ -48,46 +60,46 @@ RSpec.describe Packages::Nuget::ExtractionWorker, type: :worker do
         allow_any_instance_of(Zip::File).to receive(:glob).and_return([])
       end
 
-      it 'removes the package and the package file' do
-        expect(Gitlab::ErrorTracking).to receive(:log_exception).with(
-          instance_of(::Packages::Nuget::MetadataExtractionService::ExtractionError),
-          project_id: package.project_id
-        )
-        expect { subject }
-          .to change { Packages::Package.count }.by(-1)
-          .and change { Packages::PackageFile.count }.by(-1)
-      end
+      it_behaves_like 'handling the metadata error', exception_class: ::Packages::Nuget::MetadataExtractionService::ExtractionError
     end
 
-    context 'with package file with a blank package name' do
-      before do
-        allow_any_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService).to receive(:package_name).and_return('')
-      end
+    context 'with package with an invalid package name' do
+      invalid_names = [
+        '',
+        'My/package',
+        '../../../my_package',
+        '%2e%2e%2fmy_package'
+      ]
 
-      it 'removes the package and the package file' do
-        expect(Gitlab::ErrorTracking).to receive(:log_exception).with(
-          instance_of(::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError),
-          project_id: package.project_id
-        )
-        expect { subject }
-          .to change { Packages::Package.count }.by(-1)
-          .and change { Packages::PackageFile.count }.by(-1)
+      invalid_names.each do |invalid_name|
+        before do
+          allow_next_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService) do |service|
+            allow(service).to receive(:package_name).and_return(invalid_name)
+          end
+        end
+
+        it_behaves_like 'handling the metadata error'
       end
     end
 
-    context 'with package file with a blank package version' do
-      before do
-        allow_any_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService).to receive(:package_version).and_return('')
-      end
+    context 'with package with an invalid package version' do
+      invalid_versions = [
+        '',
+        '555',
+        '1.2',
+        '1./2.3',
+        '../../../../../1.2.3',
+        '%2e%2e%2f1.2.3'
+      ]
 
-      it 'removes the package and the package file' do
-        expect(Gitlab::ErrorTracking).to receive(:log_exception).with(
-          instance_of(::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError),
-          project_id: package.project_id
-        )
-        expect { subject }
-          .to change { Packages::Package.count }.by(-1)
-          .and change { Packages::PackageFile.count }.by(-1)
+      invalid_versions.each do |invalid_version|
+        before do
+          allow_next_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService) do |service|
+            allow(service).to receive(:package_version).and_return(invalid_version)
+          end
+        end
+
+        it_behaves_like 'handling the metadata error'
       end
     end
   end