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

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

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

app/presenters/ci/stage_presenter.rb

+# 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

app/views/projects/stage/_stage.html.haml

+- 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 }
      

changelogs/unreleased/330787-omits-trailing-slash-when-checking-for-allowed-requests.yml

+---
+title: Omit trailing slash when checking allowed requests in the read-only middleware
+merge_request: 61641
+author:
+type: fixed

changelogs/unreleased/sh-avoid-trailing-slash-in-proxy.yml

+---
+title: Omit trailing slash when proxying pre-authorized routes with no suffix
+merge_request: 61638
+author:
+type: fixed

changelogs/unreleased/sh-fix-nplus-one-pipelines-show.yml

+---
+title: Fix N+1 SQL queries in PipelinesController#show
+merge_request: 60794
+author:
+type: fixed

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])

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) }
 

spec/presenters/ci/stage_presenter_spec.rb

+# 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

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' } }
 

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), ""
 	}
 

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 {