diff options
17 files changed, 209 insertions, 76 deletions
diff --git a/app/models/packages/package.rb b/app/models/packages/package.rb index d6633456de4..e455d541b32 100644 --- a/app/models/packages/package.rb +++ b/app/models/packages/package.rb @@ -35,6 +35,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? } 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 0bbe73d6f7e..3c963bf85e0 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 f3b7fb5ed45..f39b5fb2783 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -139,8 +139,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 d8d16673623..07bcb64810d 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 unbounded_semver_regex # See the official regex: https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb index 5c264aefb92..6eb38a09f04 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -307,6 +307,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 '.semver_regex' do subject { described_class.semver_regex } diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb index 4170bf595f0..303e6cf6958 100644 --- a/spec/models/packages/package_spec.rb +++ b/spec/models/packages/package_spec.rb @@ -107,6 +107,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 9879fc53461..4ab9e8197a7 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -223,7 +223,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 |