diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-11-02 18:23:43 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-11-02 18:23:43 +0000 |
commit | b8110282a17d805dc28fb94f2d62f164bd1c3665 (patch) | |
tree | 722c853c14f1eff27ae260f2ad331c219b3ddd63 | |
parent | bfbbc52faaae2a1a06e065511a1a8661203e868a (diff) | |
download | gitlab-ce-b8110282a17d805dc28fb94f2d62f164bd1c3665.tar.gz |
Add latest changes from gitlab-org/gitlab@13-4-stable-ee
37 files changed, 534 insertions, 120 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d8186407f5..02faf92eb98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,21 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 13.4.5 (2020-11-02) + +### Security (9 changes) + +- Add CSRF protection to runner pause and resume. !1021 +- Do not expose Terraform state record in API. +- Path traversal to RCE via LFS upload. +- Update container_repository_name_regex to prevent catastrophic backtracking. +- Validate nuget package names. +- Prevent private repo from being accessed via internal Kubernetes API. +- Validate each upload param key in multipart.rb. +- Fix XSS vulnerability for job build dependencies. +- Fix unauthorized user is able to access schedule pipeline variables and values. + + ## 13.4.4 (2020-10-15) ### Fixed (2 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 24a9ec7aa95..2150631c08c 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -13.4.4
\ No newline at end of file +13.4.5
\ No newline at end of file diff --git a/app/assets/javascripts/jobs/components/job_app.vue b/app/assets/javascripts/jobs/components/job_app.vue index 00ff3fb939d..c6adf2f231f 100644 --- a/app/assets/javascripts/jobs/components/job_app.vue +++ b/app/assets/javascripts/jobs/components/job_app.vue @@ -1,8 +1,7 @@ <script> -/* eslint-disable vue/no-v-html */ import { throttle, isEmpty } from 'lodash'; import { mapGetters, mapState, mapActions } from 'vuex'; -import { GlLoadingIcon, GlIcon } from '@gitlab/ui'; +import { GlLoadingIcon, GlIcon, GlSafeHtmlDirective as SafeHtml } from '@gitlab/ui'; import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils'; import { isScrolledToBottom } from '~/lib/utils/scroll_utils'; import { polyfillSticky } from '~/lib/utils/sticky'; @@ -36,6 +35,9 @@ export default { GlLoadingIcon, SharedRunner: () => import('ee_component/jobs/components/shared_runner_limit_block.vue'), }, + directives: { + SafeHtml, + }, mixins: [delayedJobMixin], props: { artifactHelpUrl: { @@ -223,7 +225,7 @@ export default { </div> <callout v-if="shouldRenderHeaderCallout"> - <div v-html="job.callout_message"></div> + <div v-safe-html="job.callout_message"></div> </callout> </header> <!-- EO Header Section --> 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/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb index 2b8522539b4..49255fa6d0b 100644 --- a/app/serializers/build_details_entity.rb +++ b/app/serializers/build_details_entity.rb @@ -136,7 +136,7 @@ class BuildDetailsEntity < JobEntity docs_url = "https://docs.gitlab.com/ce/ci/yaml/README.html#dependencies" [ - failure_message.html_safe, + failure_message, help_message(docs_url).html_safe ].join("<br />") end 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/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index dba5177718d..ca02cc03bfd 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -79,11 +79,7 @@ module Projects # Directories on disk move_project_folders(project) - # Move missing group labels to project - Labels::TransferService.new(current_user, @old_group, project).execute - - # Move missing group milestones - Milestones::TransferService.new(current_user, @old_group, project).execute + transfer_missing_group_resources(@old_group) # Move uploads move_project_uploads(project) @@ -103,6 +99,12 @@ module Projects refresh_permissions end + def transfer_missing_group_resources(group) + Labels::TransferService.new(current_user, group, project).execute + + Milestones::TransferService.new(current_user, group, project).execute + end + def allowed_transfer?(current_user, project) @new_namespace && can?(current_user, :change_namespace, project) && diff --git a/app/services/terraform/remote_state_handler.rb b/app/services/terraform/remote_state_handler.rb index d2c44d4a265..7e79cb9e007 100644 --- a/app/services/terraform/remote_state_handler.rb +++ b/app/services/terraform/remote_state_handler.rb @@ -23,6 +23,8 @@ module Terraform state.save! unless state.destroyed? end + + nil end def lock! diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index 654bb15378c..411d8b2614f 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -5,6 +5,10 @@ class GitlabUploader < CarrierWave::Uploader::Base class_attribute :options + PROTECTED_METHODS = %i(filename cache_dir work_dir store_dir).freeze + + ObjectNotReadyError = Class.new(StandardError) + class << self # DSL setter def storage_options(options) @@ -33,6 +37,8 @@ class GitlabUploader < CarrierWave::Uploader::Base delegate :base_dir, :file_storage?, to: :class + before :cache, :protect_from_path_traversal! + def initialize(model, mounted_as = nil, **uploader_context) super(model, mounted_as) end @@ -121,6 +127,9 @@ class GitlabUploader < CarrierWave::Uploader::Base # For example, `FileUploader` builds the storage path based on the associated # project model's `path_with_namespace` value, which can change when the # project or its containing namespace is moved or renamed. + # + # When implementing this method, raise `ObjectNotReadyError` if the model + # does not yet exist, as it will be tested in `#protect_from_path_traversal!` def dynamic_segment raise(NotImplementedError) end @@ -138,4 +147,21 @@ class GitlabUploader < CarrierWave::Uploader::Base def pathname @pathname ||= Pathname.new(path) end + + # Protect against path traversal attacks + # This takes a list of methods to test for path traversal, e.g. ../../ + # and checks each of them. This uses `.send` so that any potential errors + # don't block the entire set from being tested. + # + # @param [CarrierWave::SanitizedFile] + # @return [Nil] + # @raise [Gitlab::Utils::PathTraversalAttackError] + def protect_from_path_traversal!(file) + PROTECTED_METHODS.each do |method| + Gitlab::Utils.check_path_traversal!(self.send(method)) # rubocop: disable GitlabSecurity/PublicSend + + rescue ObjectNotReadyError + # Do nothing. This test was attempted before the file was ready for that method + end + end end diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index 47976c909e8..83dc1030606 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -4,7 +4,6 @@ class JobArtifactUploader < GitlabUploader extend Workhorse::UploadPath include ObjectStorage::Concern - ObjectNotReadyError = Class.new(StandardError) UnknownFileLocationError = Class.new(StandardError) storage_options Gitlab.config.artifacts @@ -24,7 +23,9 @@ class JobArtifactUploader < GitlabUploader private def dynamic_segment - raise ObjectNotReadyError, 'JobArtifact is not ready' unless model.id + # This now tests model.created_at because it can for some reason be nil in the test suite, + # and it's not clear if this is intentional or not + raise ObjectNotReadyError, 'JobArtifact is not ready' unless model.id && model.created_at if model.hashed_path? hashed_path diff --git a/app/uploaders/packages/package_file_uploader.rb b/app/uploaders/packages/package_file_uploader.rb index 28545b9fcdf..61fbe2b4c49 100644 --- a/app/uploaders/packages/package_file_uploader.rb +++ b/app/uploaders/packages/package_file_uploader.rb @@ -20,6 +20,8 @@ class Packages::PackageFileUploader < GitlabUploader private def dynamic_segment + raise ObjectNotReadyError, "Package model not ready" unless model.id + Gitlab::HashedPath.new('packages', model.package.id, 'files', model.id, root_hash: model.package.project_id) end end 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/revert-42465-and-42343.yml b/changelogs/unreleased/revert-42465-and-42343.yml deleted file mode 100644 index 4c7342c9d0d..00000000000 --- a/changelogs/unreleased/revert-42465-and-42343.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: 'Revert 42465 and 42343: Expanded collapsed diff files' -merge_request: 43361 -author: -type: other 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/api/internal/kubernetes.rb b/lib/api/internal/kubernetes.rb index 6d5dfd086e7..2e14a7783e5 100644 --- a/lib/api/internal/kubernetes.rb +++ b/lib/api/internal/kubernetes.rb @@ -85,7 +85,7 @@ module API # TODO sort out authorization for real # https://gitlab.com/gitlab-org/gitlab/-/issues/220912 - if !project || !project.public? + unless Ability.allowed?(nil, :download_code, project) not_found! end diff --git a/lib/api/terraform/state.rb b/lib/api/terraform/state.rb index 7063a3d08b5..d582f3f90f3 100644 --- a/lib/api/terraform/state.rb +++ b/lib/api/terraform/state.rb @@ -39,7 +39,6 @@ module API env['api.format'] = :binary # this bypasses json serialization body state.latest_file.read - status :ok end end @@ -53,8 +52,10 @@ module API remote_state_handler.handle_with_lock do |state| state.update_file!(CarrierWaveStringFile.new(data), version: params[:serial]) - status :ok end + + body false + status :ok end desc 'Delete a terraform state of a certain name' @@ -64,8 +65,10 @@ module API remote_state_handler.handle_with_lock do |state| state.destroy! - status :ok end + + body false + status :ok end desc 'Lock a terraform state of a certain name' diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb index 8e6ac7610f2..e637c0ce304 100644 --- a/lib/gitlab/middleware/multipart.rb +++ b/lib/gitlab/middleware/multipart.rb @@ -31,6 +31,7 @@ module Gitlab RACK_ENV_KEY = 'HTTP_GITLAB_WORKHORSE_MULTIPART_FIELDS' JWT_PARAM_SUFFIX = '.gitlab-workhorse-upload' JWT_PARAM_FIXED_KEY = 'upload' + REWRITTEN_FIELD_NAME_MAX_LENGTH = 10000.freeze class Handler def initialize(env, message) @@ -41,6 +42,8 @@ module Gitlab def with_open_files @rewritten_fields.each do |field, tmp_path| + raise "invalid field: #{field.inspect}" unless valid_field_name?(field) + parsed_field = Rack::Utils.parse_nested_query(field) raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1 @@ -108,6 +111,17 @@ module Gitlab private + def valid_field_name?(name) + # length validation + return false if name.size >= REWRITTEN_FIELD_NAME_MAX_LENGTH + + # brackets validation + return false if name.include?('[]') || name.start_with?('[', ']') + return false unless ::Gitlab::Utils.valid_brackets?(name, allow_nested: false) + + true + end + def package_allowed_paths packages_config = ::Gitlab.config.packages return [] unless allow_packages_storage_path?(packages_config) @@ -140,6 +154,8 @@ module Gitlab class HandlerForJWTParams < Handler def with_open_files @rewritten_fields.keys.each do |field| + raise "invalid field: #{field.inspect}" unless valid_field_name?(field) + parsed_field = Rack::Utils.parse_nested_query(field) raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1 diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 8e23ac6aca5..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 @@ -139,7 +143,7 @@ module Gitlab # See https://github.com/docker/distribution/blob/master/reference/regexp.go. # def container_repository_name_regex - @container_repository_regex ||= %r{\A[a-z0-9]+((?:[._/]|__|[-]{0,10})[a-z0-9]+)*\Z} + @container_repository_regex ||= %r{\A[a-z0-9]+(([._/]|__|-*)[a-z0-9])*\z} end ## diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index e2d93e7cd29..3df54e74b4f 100644 --- a/lib/gitlab/utils.rb +++ b/lib/gitlab/utils.rb @@ -10,6 +10,8 @@ module Gitlab # Also see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24223#note_284122580 # It also checks for ALT_SEPARATOR aka '\' (forward slash) def check_path_traversal!(path) + return unless path.is_a?(String) + path = decode_path(path) path_regex = /(\A(\.{1,2})\z|\A\.\.[\/\\]|[\/\\]\.\.\z|[\/\\]\.\.[\/\\]|\n)/ @@ -208,5 +210,33 @@ module Gitlab def stable_sort_by(list) list.sort_by.with_index { |x, idx| [yield(x), idx] } end + + # Check for valid brackets (`[` and `]`) in a string using this aspects: + # * open brackets count == closed brackets count + # * (optionally) reject nested brackets via `allow_nested: false` + # * open / close brackets coherence, eg. ][[] -> invalid + def valid_brackets?(string = '', allow_nested: true) + # remove everything except brackets + brackets = string.remove(/[^\[\]]/) + + return true if brackets.empty? + # balanced counts check + return false if brackets.size.odd? + + unless allow_nested + # nested brackets check + return false if brackets.include?('[[') || brackets.include?(']]') + end + + # open / close brackets coherence check + untrimmed = brackets + loop do + trimmed = untrimmed.gsub('[]', '') + return true if trimmed.empty? + return false if trimmed == untrimmed + + untrimmed = trimmed + end + end end end diff --git a/spec/features/file_uploads/multipart_invalid_uploads_spec.rb b/spec/features/file_uploads/multipart_invalid_uploads_spec.rb new file mode 100644 index 00000000000..c7490f88db6 --- /dev/null +++ b/spec/features/file_uploads/multipart_invalid_uploads_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Invalid uploads that must be rejected', :api, :js do + include_context 'file upload requests helpers' + + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user, :admin) } + let_it_be(:personal_access_token) { create(:personal_access_token, user: user) } + + context 'invalid upload key', :capybara_ignore_server_errors do + let(:api_path) { "/projects/#{project.id}/packages/nuget/" } + let(:url) { capybara_url(api(api_path)) } + let(:file) { fixture_file_upload('spec/fixtures/dk.png') } + + subject do + HTTParty.put( + url, + basic_auth: { user: user.username, password: personal_access_token.token }, + body: body + ) + end + + RSpec.shared_examples 'by rejecting uploads with an invalid key' do + RSpec.shared_examples 'rejecting invalid keys' do |key_name:| + context "with invalid key #{key_name}" do + let(:body) { { key_name => file, 'package[test][name]' => 'test' } } + + it { expect { subject }.not_to change { Packages::Package.nuget.count } } + + it { expect(subject.code).to eq(500) } + + it { expect(subject.body).to include("invalid field: \"#{key_name}\"") } + end + end + + it_behaves_like 'rejecting invalid keys', key_name: 'package[test' + it_behaves_like 'rejecting invalid keys', key_name: 'package[]test' + it_behaves_like 'rejecting invalid keys', key_name: '[]' + it_behaves_like 'rejecting invalid keys', key_name: '[package]test' + it_behaves_like 'rejecting invalid keys', key_name: 'package][test]]' + it_behaves_like 'rejecting invalid keys', key_name: 'package[test[nested]]' + it_behaves_like 'rejecting invalid keys', key_name: 'x' * 11000 + end + + it_behaves_like 'handling file uploads', 'by rejecting uploads with an invalid key' + end +end diff --git a/spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb b/spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb index 875e3820011..a1e9ac6e425 100644 --- a/spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb +++ b/spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb @@ -123,15 +123,46 @@ RSpec.describe Gitlab::Middleware::Multipart do end end - context 'with invalid key in parameters' do + context 'with an invalid upload key' do include_context 'with one temporary file for multipart' - let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) } - let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'wrong_key', filename: filename, remote_id: remote_id) } + RSpec.shared_examples 'rejecting the invalid key' do |key_in_header:, key_in_upload_params:, error_message:| + let(:rewritten_fields) { rewritten_fields_hash(key_in_header => uploaded_filepath) } + let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: key_in_upload_params, filename: filename, remote_id: remote_id) } - it 'raises an error' do - expect { subject }.to raise_error(RuntimeError, 'Empty JWT param: file.gitlab-workhorse-upload') + it 'raises an error' do + expect { subject }.to raise_error(RuntimeError, error_message) + end end + + it_behaves_like 'rejecting the invalid key', + key_in_header: 'file', + key_in_upload_params: 'wrong_key', + error_message: 'Empty JWT param: file.gitlab-workhorse-upload' + it_behaves_like 'rejecting the invalid key', + key_in_header: 'user[avatar', + key_in_upload_params: 'user[avatar]', + error_message: 'invalid field: "user[avatar"' + it_behaves_like 'rejecting the invalid key', + key_in_header: '[user]avatar', + key_in_upload_params: 'user[avatar]', + error_message: 'invalid field: "[user]avatar"' + it_behaves_like 'rejecting the invalid key', + key_in_header: 'user[]avatar', + key_in_upload_params: 'user[avatar]', + error_message: 'invalid field: "user[]avatar"' + it_behaves_like 'rejecting the invalid key', + key_in_header: 'user[avatar[image[url]]]', + key_in_upload_params: 'user[avatar]', + error_message: 'invalid field: "user[avatar[image[url]]]"' + it_behaves_like 'rejecting the invalid key', + key_in_header: '[]', + key_in_upload_params: 'user[avatar]', + error_message: 'invalid field: "[]"' + it_behaves_like 'rejecting the invalid key', + key_in_header: 'x' * 11000, + key_in_upload_params: 'user[avatar]', + error_message: "invalid field: \"#{'x' * 11000}\"" end context 'with a modified JWT payload' do diff --git a/spec/lib/gitlab/middleware/multipart_with_handler_spec.rb b/spec/lib/gitlab/middleware/multipart_with_handler_spec.rb index 742a5639ace..8c2af775574 100644 --- a/spec/lib/gitlab/middleware/multipart_with_handler_spec.rb +++ b/spec/lib/gitlab/middleware/multipart_with_handler_spec.rb @@ -139,6 +139,58 @@ RSpec.describe Gitlab::Middleware::Multipart do subject end end + + context 'with invalid key in header' do + include_context 'with one temporary file for multipart' + + RSpec.shared_examples 'rejecting the invalid key' do |key_in_header:, key_in_upload_params:, error_message:| + let(:rewritten_fields) { rewritten_fields_hash(key_in_header => uploaded_filepath) } + let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: key_in_upload_params, filename: filename, remote_id: remote_id) } + + it 'raises an error' do + expect { subject }.to raise_error(RuntimeError, error_message) + end + end + + it_behaves_like 'rejecting the invalid key', + key_in_header: 'user[avatar', + key_in_upload_params: 'user[avatar]', + error_message: 'invalid field: "user[avatar"' + it_behaves_like 'rejecting the invalid key', + key_in_header: '[user]avatar', + key_in_upload_params: 'user[avatar]', + error_message: 'invalid field: "[user]avatar"' + it_behaves_like 'rejecting the invalid key', + key_in_header: 'user[]avatar', + key_in_upload_params: 'user[avatar]', + error_message: 'invalid field: "user[]avatar"' + it_behaves_like 'rejecting the invalid key', + key_in_header: 'user[avatar[image[url]]]', + key_in_upload_params: 'user[avatar]', + error_message: 'invalid field: "user[avatar[image[url]]]"' + it_behaves_like 'rejecting the invalid key', + key_in_header: '[]', + key_in_upload_params: 'user[avatar]', + error_message: 'invalid field: "[]"' + it_behaves_like 'rejecting the invalid key', + key_in_header: 'x' * 11000, + key_in_upload_params: 'user[avatar]', + error_message: "invalid field: \"#{'x' * 11000}\"" + end + + context 'with key with unbalanced brackets in header' do + include_context 'with one temporary file for multipart' + + let(:invalid_key) { 'user[avatar' } + let(:rewritten_fields) { rewritten_fields_hash( invalid_key => uploaded_filepath) } + let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'user[avatar]', filename: filename, remote_id: remote_id) } + + it 'builds no UploadedFile' do + expect(app).not_to receive(:call) + + expect { subject }.to raise_error(RuntimeError, "invalid field: \"#{invalid_key}\"") + end + end end end end diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb index 88c3315150b..a853dd0758c 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -107,11 +107,16 @@ RSpec.describe Gitlab::Regex do it { is_expected.to match('my/awesome/image-1') } it { is_expected.to match('my/awesome/image.test') } it { is_expected.to match('my/awesome/image--test') } - # docker distribution allows for infinite `-` - # https://github.com/docker/distribution/blob/master/reference/regexp.go#L13 - # but we have a range of 0,10 to add a reasonable limit. - it { is_expected.not_to match('my/image-----------test') } + it { is_expected.to match('my/image__test') } + # this example tests for catastrophic backtracking + it { is_expected.to match('user1/project/a_bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb------------x') } + it { is_expected.not_to match('user1/project/a_bbbbb-------------') } it { is_expected.not_to match('my/image-.test') } + it { is_expected.not_to match('my/image___test') } + it { is_expected.not_to match('my/image_.test') } + it { is_expected.not_to match('my/image_-test') } + it { is_expected.not_to match('my/image..test') } + it { is_expected.not_to match('my/image\ntest') } it { is_expected.not_to match('.my/image') } it { is_expected.not_to match('my/image.') } end @@ -317,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/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index 1eaceec1d8a..36257a0605b 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/spec/lib/gitlab/utils_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Gitlab::Utils do + using RSpec::Parameterized::TableSyntax + delegate :to_boolean, :boolean_to_yes_no, :slugify, :random_string, :which, :ensure_array_from_string, :to_exclusive_sentence, :bytes_to_megabytes, :append_path, :check_path_traversal!, :allowlisted?, :check_allowed_absolute_path!, :decode_path, :ms_to_round_sec, to: :described_class @@ -50,6 +52,10 @@ RSpec.describe Gitlab::Utils do expect(check_path_traversal!('dir/..foo.rb')).to eq('dir/..foo.rb') expect(check_path_traversal!('dir/.foo.rb')).to eq('dir/.foo.rb') end + + it 'does nothing for a non-string' do + expect(check_path_traversal!(nil)).to be_nil + end end describe '.allowlisted?' do @@ -448,4 +454,29 @@ RSpec.describe Gitlab::Utils do end end end + + describe '.valid_brackets?' do + where(:input, :allow_nested, :valid) do + 'no brackets' | true | true + 'no brackets' | false | true + 'user[avatar]' | true | true + 'user[avatar]' | false | true + 'user[avatar][friends]' | true | true + 'user[avatar][friends]' | false | true + 'user[avatar[image[url]]]' | true | true + 'user[avatar[image[url]]]' | false | false + 'user[avatar[]friends]' | true | true + 'user[avatar[]friends]' | false | false + 'user[avatar]]' | true | false + 'user[avatar]]' | false | false + 'user][avatar]]' | true | false + 'user][avatar]]' | false | false + 'user[avatar' | true | false + 'user[avatar' | false | false + end + + with_them do + it { expect(described_class.valid_brackets?(input, allow_nested: allow_nested)).to eq(valid) } + end + end end 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/requests/api/internal/kubernetes_spec.rb b/spec/requests/api/internal/kubernetes_spec.rb index f669483b5a4..a532b8e59f2 100644 --- a/spec/requests/api/internal/kubernetes_spec.rb +++ b/spec/requests/api/internal/kubernetes_spec.rb @@ -166,6 +166,16 @@ RSpec.describe API::Internal::Kubernetes do ) ) end + + context 'repository is for project members only' do + let(:project) { create(:project, :public, :repository_private) } + + it 'returns 404' do + send_request(params: { id: project.id }, headers: { 'Authorization' => "Bearer #{agent_token.token}" }) + + expect(response).to have_gitlab_http_status(:not_found) + end + end end context 'project is private' do @@ -190,7 +200,7 @@ RSpec.describe API::Internal::Kubernetes do context 'project does not exist' do it 'returns 404' do - send_request(params: { id: 0 }, headers: { 'Authorization' => "Bearer #{agent_token.token}" }) + send_request(params: { id: non_existing_record_id }, headers: { 'Authorization' => "Bearer #{agent_token.token}" }) expect(response).to have_gitlab_http_status(:not_found) end diff --git a/spec/requests/api/terraform/state_spec.rb b/spec/requests/api/terraform/state_spec.rb index 8d128bd911f..3466921bff6 100644 --- a/spec/requests/api/terraform/state_spec.rb +++ b/spec/requests/api/terraform/state_spec.rb @@ -125,6 +125,7 @@ RSpec.describe API::Terraform::State do expect { request }.to change { Terraform::State.count }.by(0) expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)).to be_empty end context 'on Unicorn', :unicorn do @@ -132,6 +133,7 @@ RSpec.describe API::Terraform::State do expect { request }.to change { Terraform::State.count }.by(0) expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)).to be_empty end end end @@ -167,6 +169,7 @@ RSpec.describe API::Terraform::State do expect { request }.to change { Terraform::State.count }.by(1) expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)).to be_empty end context 'on Unicorn', :unicorn do @@ -174,6 +177,7 @@ RSpec.describe API::Terraform::State do expect { request }.to change { Terraform::State.count }.by(1) expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)).to be_empty end end end @@ -206,10 +210,11 @@ RSpec.describe API::Terraform::State do context 'with maintainer permissions' do let(:current_user) { maintainer } - it 'deletes the state' do + it 'deletes the state and returns empty body' do expect { request }.to change { Terraform::State.count }.by(-1) expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)).to be_empty 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/services/terraform/remote_state_handler_spec.rb b/spec/services/terraform/remote_state_handler_spec.rb index c47367feb14..ca392849d49 100644 --- a/spec/services/terraform/remote_state_handler_spec.rb +++ b/spec/services/terraform/remote_state_handler_spec.rb @@ -42,17 +42,17 @@ RSpec.describe Terraform::RemoteStateHandler do describe '#handle_with_lock' do it 'allows to modify a state using database locking' do - state = subject.handle_with_lock do |state| + record = nil + subject.handle_with_lock do |state| + record = state state.name = 'updated-name' end - expect(state.name).to eq 'updated-name' + expect(record.reload.name).to eq 'updated-name' end - it 'returns the state object itself' do - state = subject.handle_with_lock - - expect(state.name).to eq 'my-state' + it 'returns nil' do + expect(subject.handle_with_lock).to be_nil end end @@ -70,11 +70,13 @@ RSpec.describe Terraform::RemoteStateHandler do it 'handles a locked state using exclusive read lock' do handler.lock! - state = handler.handle_with_lock do |state| + record = nil + handler.handle_with_lock do |state| + record = state state.name = 'new-name' end - expect(state.name).to eq 'new-name' + expect(record.reload.name).to eq 'new-name' end it 'raises exception if lock has not been acquired before' do diff --git a/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb b/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb index 12bcbb8b812..7126d3ace96 100644 --- a/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb +++ b/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb @@ -14,6 +14,7 @@ end RSpec.shared_examples "builds correct paths" do |**patterns| let(:patterns) { patterns } + let(:fixture) { File.join('spec', 'fixtures', 'rails_sample.jpg') } before do allow(subject).to receive(:filename).and_return('<filename>') @@ -55,4 +56,15 @@ RSpec.shared_examples "builds correct paths" do |**patterns| let(:target) { subject.class } end end + + describe "path traversal exploits" do + before do + allow(subject).to receive(:filename).and_return("3bc58d54542d6a5efffa9a87554faac0254f73f675b337899ea869f6d38b7371/122../../../../../../../../.ssh/authorized_keys") + end + + it "throws an exception" do + expect { subject.cache!(fixture_file_upload(fixture)) }.to raise_error(Gitlab::Utils::PathTraversalAttackError) + expect { subject.store!(fixture_file_upload(fixture)) }.to raise_error(Gitlab::Utils::PathTraversalAttackError) + end + end end diff --git a/spec/uploaders/import_export_uploader_spec.rb b/spec/uploaders/import_export_uploader_spec.rb index b1fdcf067c6..cb7a89193e6 100644 --- a/spec/uploaders/import_export_uploader_spec.rb +++ b/spec/uploaders/import_export_uploader_spec.rb @@ -24,9 +24,14 @@ RSpec.describe ImportExportUploader do include_context 'with storage', described_class::Store::REMOTE - it_behaves_like 'builds correct paths', - store_dir: %r[import_export_upload/import_file/], - upload_path: %r[import_export_upload/import_file/] + patterns = { + store_dir: %r[import_export_upload/import_file/], + upload_path: %r[import_export_upload/import_file/] + } + + it_behaves_like 'builds correct paths', patterns do + let(:fixture) { File.join('spec', 'fixtures', 'group_export.tar.gz') } + end describe '#move_to_store' do it 'returns false' do 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 |