diff options
| -rw-r--r-- | app/controllers/projects/pipelines_controller.rb | 2 | ||||
| -rw-r--r-- | app/models/ci/stage.rb | 7 | ||||
| -rw-r--r-- | app/presenters/ci/stage_presenter.rb | 32 | ||||
| -rw-r--r-- | app/views/projects/stage/_stage.html.haml | 6 | ||||
| -rw-r--r-- | changelogs/unreleased/330787-omits-trailing-slash-when-checking-for-allowed-requests.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-avoid-trailing-slash-in-proxy.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-fix-nplus-one-pipelines-show.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/middleware/read_only/controller.rb | 16 | ||||
| -rw-r--r-- | spec/controllers/projects/pipelines_controller_spec.rb | 33 | ||||
| -rw-r--r-- | spec/presenters/ci/stage_presenter_spec.rb | 49 | ||||
| -rw-r--r-- | spec/support/shared_examples/lib/gitlab/middleware/read_only_gitlab_instance_shared_examples.rb | 52 | ||||
| -rw-r--r-- | workhorse/internal/api/api.go | 5 | ||||
| -rw-r--r-- | workhorse/main_test.go | 6 |
13 files changed, 207 insertions, 16 deletions
diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index ee1e10221ec..9f326ef59f5 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -216,7 +216,7 @@ class Projects::PipelinesController < Projects::ApplicationController end def render_show - @stages = @pipeline.stages.with_latest_and_retried_statuses + @stages = @pipeline.stages respond_to do |format| format.html do diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index 9dd75150ac7..5ae97dcd495 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -6,6 +6,7 @@ module Ci include Importable include Ci::HasStatus include Gitlab::OptimisticLocking + include Presentable enum status: Ci::HasStatus::STATUSES_ENUM @@ -22,12 +23,6 @@ module Ci scope :ordered, -> { order(position: :asc) } scope :in_pipelines, ->(pipelines) { where(pipeline: pipelines) } scope :by_name, ->(names) { where(name: names) } - scope :with_latest_and_retried_statuses, -> do - includes( - latest_statuses: [:pipeline, project: :namespace], - retried_statuses: [:pipeline, project: :namespace] - ) - end with_options unless: :importing? do validates :project, presence: true diff --git a/app/presenters/ci/stage_presenter.rb b/app/presenters/ci/stage_presenter.rb new file mode 100644 index 00000000000..9ec3f8d153a --- /dev/null +++ b/app/presenters/ci/stage_presenter.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Ci + class StagePresenter < Gitlab::View::Presenter::Delegated + presents :stage + + def latest_ordered_statuses + preload_statuses(stage.statuses.latest_ordered) + end + + def retried_ordered_statuses + preload_statuses(stage.statuses.retried_ordered) + end + + private + + def preload_statuses(statuses) + loaded_statuses = statuses.load + statuses.tap do |statuses| + # rubocop: disable CodeReuse/ActiveRecord + ActiveRecord::Associations::Preloader.new.preload(preloadable_statuses(loaded_statuses), %w[pipeline tags job_artifacts_archive metadata]) + # rubocop: enable CodeReuse/ActiveRecord + end + end + + def preloadable_statuses(statuses) + statuses.reject do |status| + status.instance_of?(::GenericCommitStatus) || status.instance_of?(::Ci::Bridge) + end + end + end +end diff --git a/app/views/projects/stage/_stage.html.haml b/app/views/projects/stage/_stage.html.haml index 92bfd5a48a8..387c8fb3234 100644 --- a/app/views/projects/stage/_stage.html.haml +++ b/app/views/projects/stage/_stage.html.haml @@ -1,3 +1,5 @@ +- stage = stage.present(current_user: current_user) + %tr %th{ colspan: 10 } %strong @@ -6,8 +8,8 @@ = ci_icon_for_status(stage.status) = stage.name.titleize -= render stage.latest_statuses, stage: false, ref: false, pipeline_link: false, allow_retry: true -= render stage.retried_statuses, stage: false, ref: false, pipeline_link: false, retried: true += render stage.latest_ordered_statuses, stage: false, ref: false, pipeline_link: false, allow_retry: true += render stage.retried_ordered_statuses, stage: false, ref: false, pipeline_link: false, retried: true %tr %td{ colspan: 10 } diff --git a/changelogs/unreleased/330787-omits-trailing-slash-when-checking-for-allowed-requests.yml b/changelogs/unreleased/330787-omits-trailing-slash-when-checking-for-allowed-requests.yml new file mode 100644 index 00000000000..6e04471fa13 --- /dev/null +++ b/changelogs/unreleased/330787-omits-trailing-slash-when-checking-for-allowed-requests.yml @@ -0,0 +1,5 @@ +--- +title: Omit trailing slash when checking allowed requests in the read-only middleware +merge_request: 61641 +author: +type: fixed diff --git a/changelogs/unreleased/sh-avoid-trailing-slash-in-proxy.yml b/changelogs/unreleased/sh-avoid-trailing-slash-in-proxy.yml new file mode 100644 index 00000000000..3bea1874ff3 --- /dev/null +++ b/changelogs/unreleased/sh-avoid-trailing-slash-in-proxy.yml @@ -0,0 +1,5 @@ +--- +title: Omit trailing slash when proxying pre-authorized routes with no suffix +merge_request: 61638 +author: +type: fixed diff --git a/changelogs/unreleased/sh-fix-nplus-one-pipelines-show.yml b/changelogs/unreleased/sh-fix-nplus-one-pipelines-show.yml new file mode 100644 index 00000000000..ebaf2aee123 --- /dev/null +++ b/changelogs/unreleased/sh-fix-nplus-one-pipelines-show.yml @@ -0,0 +1,5 @@ +--- +title: Fix N+1 SQL queries in PipelinesController#show +merge_request: 60794 +author: +type: fixed diff --git a/lib/gitlab/middleware/read_only/controller.rb b/lib/gitlab/middleware/read_only/controller.rb index b11ee0afc10..226ef2041b2 100644 --- a/lib/gitlab/middleware/read_only/controller.rb +++ b/lib/gitlab/middleware/read_only/controller.rb @@ -83,7 +83,15 @@ module Gitlab end def route_hash - @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {} + @route_hash ||= Rails.application.routes.recognize_path(request_url, { method: request.request_method }) rescue {} + end + + def request_url + request.url.chomp('/') + end + + def request_path + @request_path ||= request.path.chomp('/') end def relative_url @@ -100,7 +108,7 @@ module Gitlab def workhorse_passthrough_route? # Calling route_hash may be expensive. Only do it if we think there's a possible match return false unless request.post? && - request.path.end_with?('.git/git-upload-pack') + request_path.end_with?('.git/git-upload-pack') ALLOWLISTED_GIT_READ_ONLY_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) end @@ -120,14 +128,14 @@ module Gitlab # https://gitlab.com/gitlab-org/gitlab/blob/master/app/controllers/repositories/lfs_api_controller.rb#L106 def lfs_batch_route? # Calling route_hash may be expensive. Only do it if we think there's a possible match - return unless request.path.end_with?('/info/lfs/objects/batch') + return unless request_path.end_with?('/info/lfs/objects/batch') ALLOWLISTED_GIT_LFS_BATCH_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) end def session_route? # Calling route_hash may be expensive. Only do it if we think there's a possible match - return false unless request.post? && request.path.end_with?('/users/sign_out', + return false unless request.post? && request_path.end_with?('/users/sign_out', '/admin/session', '/admin/session/destroy') ALLOWLISTED_SESSION_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 753223c5a4f..4a1d01f0e82 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -290,6 +290,39 @@ RSpec.describe Projects::PipelinesController do end end + describe 'GET #show' do + render_views + + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + + subject { get_pipeline_html } + + def get_pipeline_html + get :show, params: { namespace_id: project.namespace, project_id: project, id: pipeline }, format: :html + end + + def create_build_with_artifacts(stage, stage_idx, name) + create(:ci_build, :artifacts, :tags, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name) + end + + before do + create_build_with_artifacts('build', 0, 'job1') + create_build_with_artifacts('build', 0, 'job2') + end + + it 'avoids N+1 database queries', :request_store do + get_pipeline_html + + control_count = ActiveRecord::QueryRecorder.new { get_pipeline_html }.count + expect(response).to have_gitlab_http_status(:ok) + + create_build_with_artifacts('build', 0, 'job3') + + expect { get_pipeline_html }.not_to exceed_query_limit(control_count) + expect(response).to have_gitlab_http_status(:ok) + end + end + describe 'GET show.json' do let(:pipeline) { create(:ci_pipeline, project: project) } diff --git a/spec/presenters/ci/stage_presenter_spec.rb b/spec/presenters/ci/stage_presenter_spec.rb new file mode 100644 index 00000000000..368f03b0150 --- /dev/null +++ b/spec/presenters/ci/stage_presenter_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::StagePresenter do + let(:stage) { create(:ci_stage) } + let(:presenter) { described_class.new(stage) } + + let!(:build) { create(:ci_build, :tags, :artifacts, pipeline: stage.pipeline, stage: stage.name) } + let!(:retried_build) { create(:ci_build, :tags, :artifacts, :retried, pipeline: stage.pipeline, stage: stage.name) } + + before do + create(:generic_commit_status, pipeline: stage.pipeline, stage: stage.name) + end + + shared_examples 'preloaded associations for CI status' do + it 'preloads project' do + expect(presented_stage.association(:project)).to be_loaded + end + + it 'preloads build pipeline' do + expect(presented_stage.association(:pipeline)).to be_loaded + end + + it 'preloads build tags' do + expect(presented_stage.association(:tags)).to be_loaded + end + + it 'preloads build artifacts archive' do + expect(presented_stage.association(:job_artifacts_archive)).to be_loaded + end + + it 'preloads build artifacts metadata' do + expect(presented_stage.association(:metadata)).to be_loaded + end + end + + describe '#latest_ordered_statuses' do + subject(:presented_stage) { presenter.latest_ordered_statuses.second } + + it_behaves_like 'preloaded associations for CI status' + end + + describe '#retried_ordered_statuses' do + subject(:presented_stage) { presenter.retried_ordered_statuses.first } + + it_behaves_like 'preloaded associations for CI status' + end +end diff --git a/spec/support/shared_examples/lib/gitlab/middleware/read_only_gitlab_instance_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/middleware/read_only_gitlab_instance_shared_examples.rb index 5b3d30df739..0a07a56d417 100644 --- a/spec/support/shared_examples/lib/gitlab/middleware/read_only_gitlab_instance_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/middleware/read_only_gitlab_instance_shared_examples.rb @@ -70,6 +70,14 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do expect(subject).not_to disallow_request end + it 'expects a POST internal request with trailing slash to be allowed' do + expect(Rails.application.routes).not_to receive(:recognize_path) + response = request.post("/api/#{API::API.version}/internal/") + + expect(response).not_to be_redirect + expect(subject).not_to disallow_request + end + it 'expects a graphql request to be allowed' do response = request.post("/api/graphql") @@ -77,6 +85,13 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do expect(subject).not_to disallow_request end + it 'expects a graphql request with trailing slash to be allowed' do + response = request.post("/api/graphql/") + + expect(response).not_to be_redirect + expect(subject).not_to disallow_request + end + context 'relative URL is configured' do before do stub_config_setting(relative_url_root: '/gitlab') @@ -88,6 +103,13 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do expect(response).not_to be_redirect expect(subject).not_to disallow_request end + + it 'expects a graphql request with trailing slash to be allowed' do + response = request.post("/gitlab/api/graphql/") + + expect(response).not_to be_redirect + expect(subject).not_to disallow_request + end end context 'sidekiq admin requests' do @@ -119,6 +141,19 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do expect(response).not_to be_redirect expect(subject).not_to disallow_request end + + it 'allows requests with trailing slash' do + path = File.join(mounted_at, 'admin/sidekiq') + response = request.post("#{path}/") + + expect(response).not_to be_redirect + expect(subject).not_to disallow_request + + response = request.get("#{path}/") + + expect(response).not_to be_redirect + expect(subject).not_to disallow_request + end end end @@ -138,6 +173,14 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do expect(response).not_to be_redirect expect(subject).not_to disallow_request end + + it "expects a POST #{description} URL with trailing slash to be allowed" do + expect(Rails.application.routes).to receive(:recognize_path).and_call_original + response = request.post("#{path}/") + + expect(response).not_to be_redirect + expect(subject).not_to disallow_request + end end where(:description, :path) do @@ -153,11 +196,18 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do expect(response).to be_redirect expect(subject).to disallow_request end + + it "expects a POST #{description} URL with trailing slash not to be allowed" do + response = request.post("#{path}/") + + expect(response).to be_redirect + expect(subject).to disallow_request + end end end end - context 'json requests to a read-only GitLab instance' do + context 'JSON requests to a read-only GitLab instance' do let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'application/json' }, ['OK']] } } let(:content_json) { { 'CONTENT_TYPE' => 'application/json' } } diff --git a/workhorse/internal/api/api.go b/workhorse/internal/api/api.go index 445ca3a94cf..8ab83a1d986 100644 --- a/workhorse/internal/api/api.go +++ b/workhorse/internal/api/api.go @@ -168,7 +168,10 @@ func singleJoiningSlash(a, b string) string { // joinURLPath is taken from reverseproxy.go:joinURLPath func joinURLPath(a *url.URL, b string) (path string, rawpath string) { - if a.RawPath == "" && b == "" { + // Avoid adding a trailing slash if the suffix is empty + if b == "" { + return a.Path, a.RawPath + } else if a.RawPath == "" { return singleJoiningSlash(a.Path, b), "" } diff --git a/workhorse/main_test.go b/workhorse/main_test.go index 4e169b5112f..5729d2412bc 100644 --- a/workhorse/main_test.go +++ b/workhorse/main_test.go @@ -536,7 +536,11 @@ func TestApiContentTypeBlock(t *testing.T) { func TestAPIFalsePositivesAreProxied(t *testing.T) { goodResponse := []byte(`<html></html>`) ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) { - if r.Header.Get(secret.RequestHeader) != "" && r.Method != "GET" { + url := r.URL.String() + if url[len(url)-1] == '/' { + w.WriteHeader(500) + w.Write([]byte("PreAuthorize request included a trailing slash")) + } else if r.Header.Get(secret.RequestHeader) != "" && r.Method != "GET" { w.WriteHeader(500) w.Write([]byte("non-GET request went through PreAuthorize handler")) } else { |