diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-11-01 11:56:00 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-11-01 11:56:00 +0000 |
commit | f12386aa9acf19877161bfc77e55572f40509cc4 (patch) | |
tree | c54e5216f4a7a71fb446e7c3708cb6941a291ce2 | |
parent | b64b61bfe72c54fe4a7fdce34b2f1591e3822e5e (diff) | |
download | gitlab-ce-f12386aa9acf19877161bfc77e55572f40509cc4.tar.gz |
Add latest changes from gitlab-org/security/gitlab@15-5-stable-ee
-rw-r--r-- | app/models/ci/pipeline.rb | 2 | ||||
-rw-r--r-- | config/initializers/uri.rb | 3 | ||||
-rw-r--r-- | config/routes/repository_deprecated.rb | 8 | ||||
-rw-r--r-- | lib/gitlab/patch/uri.rb | 15 | ||||
-rw-r--r-- | spec/controllers/search_controller_spec.rb | 2 | ||||
-rw-r--r-- | spec/helpers/labels_helper_spec.rb | 11 | ||||
-rw-r--r-- | spec/lib/gitlab/patch/uri_spec.rb | 15 | ||||
-rw-r--r-- | spec/models/ci/pipeline_spec.rb | 159 | ||||
-rw-r--r-- | spec/routing/project_routing_spec.rb | 5 | ||||
-rw-r--r-- | spec/services/ci/retry_pipeline_service_spec.rb | 23 |
10 files changed, 194 insertions, 49 deletions
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 950e0a583bc..cc5ba41191b 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -1338,7 +1338,9 @@ module Ci end def reset_source_bridge!(current_user) + # break recursion when no source_pipeline bridge (first upstream pipeline) return unless bridge_waiting? + return unless current_user.can?(:update_pipeline, source_bridge.pipeline) source_bridge.pending! Ci::AfterRequeueJobService.new(project, current_user).execute(source_bridge) # rubocop:disable CodeReuse/ServiceClass diff --git a/config/initializers/uri.rb b/config/initializers/uri.rb new file mode 100644 index 00000000000..624f7c4d031 --- /dev/null +++ b/config/initializers/uri.rb @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +URI.singleton_class.prepend(Gitlab::Patch::Uri::ClassMethods) diff --git a/config/routes/repository_deprecated.rb b/config/routes/repository_deprecated.rb index e611b4f665b..32682000941 100644 --- a/config/routes/repository_deprecated.rb +++ b/config/routes/repository_deprecated.rb @@ -18,8 +18,12 @@ scope format: false do constraints: { id: Gitlab::PathRegex.git_reference_regex } get '/refs/:id/logs_tree/*path', - to: redirect('%{namespace_id}/%{project_id}/-/refs/%{id}/logs_tree/%{path}'), - constraints: { id: /.*/, path: /[^\0]*/ } + constraints: { id: /.*/, path: /[^\0]*/ }, + to: redirect { |params, _request| + path = params[:path] + path.gsub!('@', '-/') + Addressable::URI.escape("#{params[:namespace_id]}/#{params[:project_id]}/-/refs/#{params[:id]}/logs_tree/#{path}") + } scope constraints: { id: /[^\0]+/ } do # Deprecated. Keep for compatibility. diff --git a/lib/gitlab/patch/uri.rb b/lib/gitlab/patch/uri.rb new file mode 100644 index 00000000000..b3a34b5a68e --- /dev/null +++ b/lib/gitlab/patch/uri.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Gitlab + module Patch + module Uri + module ClassMethods + def parse(uri) + raise URI::InvalidURIError, "URI is too long" if uri && uri.to_s.length > 15_000 + + super + end + end + end + end +end diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index 7ab66b04a6e..392dc2229aa 100644 --- a/spec/controllers/search_controller_spec.rb +++ b/spec/controllers/search_controller_spec.rb @@ -395,7 +395,7 @@ RSpec.describe SearchController do it_behaves_like 'support for active record query timeouts', :autocomplete, { term: 'hello' }, :project, :json it 'returns an empty array when given abusive search term' do - get :autocomplete, params: { term: ('hal' * 9000), scope: 'projects' } + get :autocomplete, params: { term: ('hal' * 4000), scope: 'projects' } expect(response).to have_gitlab_http_status(:ok) expect(json_response).to match_array([]) end diff --git a/spec/helpers/labels_helper_spec.rb b/spec/helpers/labels_helper_spec.rb index 90366d7772c..85420d4afda 100644 --- a/spec/helpers/labels_helper_spec.rb +++ b/spec/helpers/labels_helper_spec.rb @@ -310,4 +310,15 @@ RSpec.describe LabelsHelper do end end end + + describe '#wrap_label_html' do + let(:project) { build_stubbed(:project) } + let(:xss_label) do + build_stubbed(:label, name: 'xsslabel', project: project, color: '"><img src=x onerror=prompt(1)>') + end + + it 'does not include the color' do + expect(wrap_label_html('xss', label: xss_label, small: false)).not_to include('color:') + end + end end diff --git a/spec/lib/gitlab/patch/uri_spec.rb b/spec/lib/gitlab/patch/uri_spec.rb new file mode 100644 index 00000000000..8232bb71fa4 --- /dev/null +++ b/spec/lib/gitlab/patch/uri_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Patch::Uri do + describe '#parse' do + it 'raises an error if the URI is too long' do + expect { URI.parse("https://example.com/#{'a' * 25_000}") }.to raise_error(URI::InvalidURIError) + end + + it 'does not raise an error if the URI is not too long' do + expect { URI.parse("https://example.com/#{'a' * 14_000}") }.not_to raise_error + end + end +end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index b2316949497..42b5210a080 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -5158,74 +5158,147 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end describe '#reset_source_bridge!' do - let(:pipeline) { create(:ci_pipeline, :created, project: project) } + subject(:reset_bridge) { pipeline.reset_source_bridge!(current_user) } - subject(:reset_bridge) { pipeline.reset_source_bridge!(project.first_owner) } + context 'with downstream pipeline' do + let_it_be(:owner) { project.first_owner } - context 'when the pipeline is a child pipeline and the bridge is depended' do - let!(:parent_pipeline) { create(:ci_pipeline) } - let!(:bridge) { create_bridge(parent_pipeline, pipeline, true) } + let!(:first_upstream_pipeline) { create(:ci_pipeline, user: owner) } + let_it_be_with_reload(:pipeline) { create(:ci_pipeline, :created, project: project, user: owner) } - it 'marks source bridge as pending' do - reset_bridge - - expect(bridge.reload).to be_pending + let!(:bridge) do + create_bridge( + upstream: first_upstream_pipeline, + downstream: pipeline, + depends: true + ) end - context 'when the parent pipeline has subsequent jobs after the bridge' do - let!(:after_bridge_job) { create(:ci_build, :skipped, pipeline: parent_pipeline, stage_idx: bridge.stage_idx + 1) } + context 'when the user has permissions for the processable' do + let(:current_user) { owner } - it 'marks subsequent jobs of the bridge as processable' do - reset_bridge + context 'when the downstream has strategy: depend' do + it 'marks source bridge as pending' do + expect { reset_bridge } + .to change { bridge.reload.status } + .to('pending') + end - expect(after_bridge_job.reload).to be_created - end - end + context 'with subsequent jobs' do + let!(:after_bridge_job) { add_bridge_dependant_job } + let!(:bridge_dependant_dag_job) { add_bridge_dependant_dag_job } - context 'when the parent pipeline has a dependent upstream pipeline' do - let!(:upstream_bridge) do - create_bridge(create(:ci_pipeline, project: create(:project)), parent_pipeline, true) - end + it 'changes subsequent job statuses to created' do + expect { reset_bridge } + .to change { after_bridge_job.reload.status } + .from('skipped').to('created') + .and change { bridge_dependant_dag_job.reload.status } + .from('skipped').to('created') + end - it 'marks all source bridges as pending' do - reset_bridge + context 'when the user is not the build user' do + let(:current_user) { create(:user) } - expect(bridge.reload).to be_pending - expect(upstream_bridge.reload).to be_pending - end - end - end + before do + project.add_maintainer(current_user) + end - context 'when the pipeline is a child pipeline and the bridge is not depended' do - let!(:parent_pipeline) { create(:ci_pipeline) } - let!(:bridge) { create_bridge(parent_pipeline, pipeline, false) } + it 'changes subsequent jobs user' do + expect { reset_bridge } + .to change { after_bridge_job.reload.user } + .from(owner).to(current_user) + .and change { bridge_dependant_dag_job.reload.user } + .from(owner).to(current_user) + end + end + end - it 'does not touch source bridge' do - reset_bridge + context 'when the upstream pipeline pipeline has a dependent upstream pipeline' do + let(:upstream_of_upstream) { create(:ci_pipeline, project: create(:project)) } + let!(:upstream_bridge) do + create_bridge( + upstream: upstream_of_upstream, + downstream: first_upstream_pipeline, + depends: true + ) + end + + it 'marks all source bridges as pending' do + expect { reset_bridge } + .to change { bridge.reload.status } + .from('skipped').to('pending') + .and change { upstream_bridge.reload.status } + .from('skipped').to('pending') + end + end - expect(bridge.reload).to be_success + context 'without strategy: depend' do + let!(:upstream_pipeline) { create(:ci_pipeline) } + let!(:bridge) do + create_bridge( + upstream: first_upstream_pipeline, + downstream: pipeline, + depends: false + ) + end + + it 'does not touch source bridge' do + expect { reset_bridge }.to not_change { bridge.status } + end + + context 'when the upstream pipeline has a dependent upstream pipeline' do + let!(:upstream_bridge) do + create_bridge( + upstream: create(:ci_pipeline, project: create(:project)), + downstream: first_upstream_pipeline, + depends: true + ) + end + + it 'does not touch any source bridge' do + expect { reset_bridge }.to not_change { bridge.status } + .and not_change { upstream_bridge.status } + end + end + end + end end - context 'when the parent pipeline has a dependent upstream pipeline' do - let!(:upstream_bridge) do - create_bridge(create(:ci_pipeline, project: create(:project)), parent_pipeline, true) + context 'when the user does not have permissions for the processable' do + let(:current_user) { create(:user) } + + it 'does not change bridge status' do + expect { reset_bridge }.to not_change { bridge.status } end - it 'does not touch any source bridge' do - reset_bridge + context 'with subsequent jobs' do + let!(:after_bridge_job) { add_bridge_dependant_job } + let!(:bridge_dependant_dag_job) { add_bridge_dependant_dag_job } - expect(bridge.reload).to be_success - expect(upstream_bridge.reload).to be_success + it 'does not change job statuses' do + expect { reset_bridge }.to not_change { after_bridge_job.reload.status } + .and not_change { bridge_dependant_dag_job.reload.status } + end end end end private - def create_bridge(upstream, downstream, depend = false) - options = depend ? { trigger: { strategy: 'depend' } } : {} + def add_bridge_dependant_job + create(:ci_build, :skipped, pipeline: first_upstream_pipeline, stage_idx: bridge.stage_idx + 1, user: owner) + end + + def add_bridge_dependant_dag_job + create(:ci_build, :skipped, name: 'dependant-build-1', pipeline: first_upstream_pipeline, user: owner).tap do |build| + create(:ci_build_need, build: build, name: bridge.name) + end + end + + def create_bridge(upstream:, downstream:, depends: false) + options = depends ? { trigger: { strategy: 'depend' } } : {} - bridge = create(:ci_bridge, pipeline: upstream, status: 'success', options: options) + bridge = create(:ci_bridge, pipeline: upstream, status: 'skipped', options: options, user: owner) create(:ci_sources_pipeline, pipeline: downstream, source_job: bridge) bridge diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index 9317a661188..875a54de3d1 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -189,6 +189,7 @@ RSpec.describe 'project routing' do end it 'to #logs_tree' do + expect(get('/gitlab/gitlabhq/-/refs/stable/logs_tree/..%2F..%2F..%2F..%2F..%2F@example.com/tree/a')).to route_to('projects/refs#logs_tree', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'stable', path: '../../../../../@example.com/tree/a') expect(get('/gitlab/gitlabhq/-/refs/stable/logs_tree')).to route_to('projects/refs#logs_tree', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'stable') expect(get('/gitlab/gitlabhq/-/refs/feature%2345/logs_tree')).to route_to('projects/refs#logs_tree', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'feature#45') expect(get('/gitlab/gitlabhq/-/refs/feature%2B45/logs_tree')).to route_to('projects/refs#logs_tree', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'feature+45') @@ -214,6 +215,10 @@ RSpec.describe 'project routing' do it_behaves_like 'redirecting a legacy path', '/gitlab/gitlabhq/refs/stable/logs_tree/new%0A%0Aline.txt', '/gitlab/gitlabhq/-/refs/stable/logs_tree/new%0A%0Aline.txt' + + it_behaves_like 'redirecting a legacy path', + '/gitlab/gitlabhq/refs/feature%2345/logs_tree/../../../../../@example.com/tree/a', + '/gitlab/gitlabhq/-/refs/feature#45/logs_tree/../../../../../-/example.com/tree/a' end describe Projects::MergeRequestsController, 'routing' do diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index 0a1e767539d..96437290ae3 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -313,10 +313,27 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do create(:ci_sources_pipeline, pipeline: pipeline, source_job: bridge) end - it 'marks source bridge as pending' do - service.execute(pipeline) + context 'without permission' do + it 'does nothing to the bridge' do + expect { service.execute(pipeline) }.to not_change { bridge.reload.status } + .and not_change { bridge.reload.user } + end + end + + context 'with permission' do + let!(:bridge_pipeline) { create(:ci_pipeline, project: create(:project)) } + let!(:bridge) do + create(:ci_bridge, :strategy_depend, status: 'success', pipeline: bridge_pipeline) + end - expect(bridge.reload).to be_pending + before do + bridge_pipeline.project.add_maintainer(user) + end + + it 'marks source bridge as pending' do + expect { service.execute(pipeline) }.to change { bridge.reload.status }.to('pending') + .and not_change { bridge.reload.user } + end end end |