Merge branch '11-8-stable-patch-8' into '11-8-stable'

Prepare 11.8.8 release

See merge request gitlab-org/gitlab-ce!27573

.gitlab-ci.yml

@@ -315,7 +315,7 @@ cloud-native-image:
   variables:
     GIT_DEPTH: "1"
   cache: {}
-  when: always
+  when: manual
   script:
     - gem install gitlab --no-document
     - CNG_PROJECT_PATH="gitlab-org/build/CNG" BUILD_TRIGGER_TOKEN=$CI_JOB_TOKEN ./scripts/trigger-build cng

app/assets/javascripts/pipelines/pipeline_details_mediator.js

@@ -19,6 +19,7 @@ export default class pipelinesMediator {
     this.poll = new Poll({
       resource: this.service,
       method: 'getPipeline',
+      data: this.store.state.expandedPipelines ? this.getExpandedParameters() : undefined,
       successCallback: this.successCallback.bind(this),
       errorCallback: this.errorCallback.bind(this),
     });
@@ -56,6 +57,19 @@ export default class pipelinesMediator {
       .getPipeline()
       .then(response => this.successCallback(response))
       .catch(() => this.errorCallback())
-      .finally(() => this.poll.restart());
+      .finally(() =>
+        this.poll.restart(
+          this.store.state.expandedPipelines ? this.getExpandedParameters() : undefined,
+        ),
+      );
+  }
+
+  /**
+   * Backend expects paramets in the following format: `expanded[]=id&expanded[]=id`
+   */
+  getExpandedParameters() {
+    return {
+      expanded: this.store.state.expandedPipelines,
+    };
   }
 }

app/assets/javascripts/pipelines/services/pipeline_service.js

@@ -5,8 +5,8 @@ export default class PipelineService {
     this.pipeline = endpoint;
   }
-  getPipeline() {
-    return axios.get(this.pipeline);
+  getPipeline(params) {
+    return axios.get(this.pipeline, { params });
   }
   // eslint-disable-next-line class-methods-use-this

app/models/merge_request_diff.rb

@@ -301,6 +301,11 @@ class MergeRequestDiff < ActiveRecord::Base
   private
+  def encode_in_base64?(diff_text)
+    (diff_text.encoding == Encoding::BINARY && !diff_text.ascii_only?) ||
+      diff_text.include?("\0")
+  end
+
   def create_merge_request_diff_files(diffs)
     rows =
       if has_attribute?(:external_diff) && Gitlab.config.external_diffs.enabled
@@ -353,7 +358,7 @@ class MergeRequestDiff < ActiveRecord::Base
       diff_hash.tap do |hash|
         diff_text = hash[:diff]
-        if diff_text.encoding == Encoding::BINARY && !diff_text.ascii_only?
+        if encode_in_base64?(diff_text)
           hash[:binary] = true
           hash[:diff] = [diff_text].pack('m0')
         end

app/validators/sha_validator.rb

@@ -2,7 +2,7 @@
 class ShaValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
-    return if value.blank? || value.match(/\A\h{40}\z/)
+    return if value.blank? || Commit.valid_hash?(value)
     record.errors.add(attribute, 'is not a valid SHA')
   end

app/views/projects/buttons/_clone.html.haml

@@ -7,7 +7,7 @@
     = sprite_icon("arrow-down", css_class: "icon")
   %ul.p-3.dropdown-menu.dropdown-menu-right.dropdown-menu-large.dropdown-menu-selectable.clone-options-dropdown.qa-clone-options
     - if ssh_enabled?
-      %li.pb-2
+      %li
         %label.label-bold
           = _('Clone with SSH')
         .input-group
@@ -16,7 +16,7 @@
             = clipboard_button(target: '#ssh_project_clone', title: _("Copy URL to clipboard"), class: "input-group-text btn-default btn-clipboard")
             = render_if_exists 'projects/buttons/geo'
     - if http_enabled?
-      %li
+      %li.pt-2
         %label.label-bold
           = _('Clone with %{http_label}') % { http_label: gitlab_config.protocol.upcase }
         .input-group
@@ -24,5 +24,6 @@
           .input-group-append
             = clipboard_button(target: '#http_project_clone', title: _("Copy URL to clipboard"), class: "input-group-text btn-default btn-clipboard")
             = render_if_exists 'projects/buttons/geo'
+    = render_if_exists 'projects/buttons/kerberos_clone_field'
 = render_if_exists 'shared/geo_info_modal', project: project

app/views/shared/_mobile_clone_panel.html.haml

@@ -13,3 +13,4 @@
     - if http_enabled?
       %li
         = dropdown_item_with_description(http_copy_label, project.http_url_to_repo, href: project.http_url_to_repo, data: { clone_type: 'http' })
+    = render_if_exists 'shared/mobile_kerberos_clone'

changelogs/unreleased/fj-58804-fix-bitbucket-import.yml

+---
+title: Fix bug in BitBucket imports with SHA shorter than 40 chars
+merge_request: 26050
+author:
+type: fixed

changelogs/unreleased/sh-handle-null-bytes-in-merge-request-diffs.yml

+---
+title: Fix error creating a merge request when diff includes a null byte
+merge_request: 26190
+author:
+type: fixed

changelogs/unreleased/sh-revert-rack-request-health-checks.yml

+---
+title: Fix health checks not working behind load balancers
+merge_request: 26055
+author:
+type: fixed

lib/gitlab/middleware/basic_health_check.rb

@@ -24,7 +24,13 @@ module Gitlab
       def call(env)
         return @app.call(env) unless env['PATH_INFO'] == HEALTH_PATH
-        request = ActionDispatch::Request.new(env)
+        # We should be using ActionDispatch::Request instead of
+        # Rack::Request to be consistent with Rails, but due to a Rails
+        # bug described in
+        # https://gitlab.com/gitlab-org/gitlab-ce/issues/58573#note_149799010
+        # hosts behind a load balancer will only see 127.0.0.1 for the
+        # load balancer's IP.
+        request = Rack::Request.new(env)
         return OK_RESPONSE if client_ip_whitelisted?(request)

lib/gitlab/request_context.rb

@@ -13,7 +13,13 @@ module Gitlab
     end
     def call(env)
-      req = ActionDispatch::Request.new(env)
+      # We should be using ActionDispatch::Request instead of
+      # Rack::Request to be consistent with Rails, but due to a Rails
+      # bug described in
+      # https://gitlab.com/gitlab-org/gitlab-ce/issues/58573#note_149799010
+      # hosts behind a load balancer will only see 127.0.0.1 for the
+      # load balancer's IP.
+      req = Rack::Request.new(env)
       Gitlab::SafeRequestStore[:client_ip] = req.ip

spec/factories/ci/pipelines.rb

@@ -82,6 +82,12 @@ FactoryBot.define do
         end
       end
+      trait :with_job do
+        after(:build) do |pipeline, evaluator|
+          pipeline.builds << build(:ci_build, pipeline: pipeline, project: pipeline.project)
+        end
+      end
+
       trait :auto_devops_source do
         config_source { Ci::Pipeline.config_sources[:auto_devops_source] }
       end

spec/lib/gitlab/bitbucket_import/importer_spec.rb

@@ -95,6 +95,9 @@ describe Gitlab::BitbucketImport::Importer do
   subject { described_class.new(project) }
   describe '#import_pull_requests' do
+    let(:source_branch_sha) { sample.commits.last }
+    let(:target_branch_sha) { sample.commits.first }
+
     before do
       allow(subject).to receive(:import_wiki)
       allow(subject).to receive(:import_issues)
@@ -102,9 +105,9 @@ describe Gitlab::BitbucketImport::Importer do
       pull_request = instance_double(
         Bitbucket::Representation::PullRequest,
         iid: 10,
-        source_branch_sha: sample.commits.last,
+        source_branch_sha: source_branch_sha,
         source_branch_name: Gitlab::Git::BRANCH_REF_PREFIX + sample.source_branch,
-        target_branch_sha: sample.commits.first,
+        target_branch_sha: target_branch_sha,
         target_branch_name: Gitlab::Git::BRANCH_REF_PREFIX + sample.target_branch,
         title: 'This is a title',
         description: 'This is a test pull request',
@@ -162,6 +165,19 @@ describe Gitlab::BitbucketImport::Importer do
       expect(reply_note).to be_a(DiffNote)
       expect(reply_note.note).to eq(@reply.note)
     end
+
+    context "when branches' sha is not found in the repository" do
+      let(:source_branch_sha) { 'a' * Commit::MIN_SHA_LENGTH }
+      let(:target_branch_sha) { 'b' * Commit::MIN_SHA_LENGTH }
+
+      it 'uses the pull request sha references' do
+        expect { subject.execute }.to change { MergeRequest.count }.by(1)
+
+        merge_request_diff = MergeRequest.first.merge_request_diff
+        expect(merge_request_diff.head_commit_sha).to eq source_branch_sha
+        expect(merge_request_diff.start_commit_sha).to eq target_branch_sha
+      end
+    end
   end
   context 'issues statuses' do

spec/lib/gitlab/middleware/basic_health_check_spec.rb

@@ -28,6 +28,35 @@ describe Gitlab::Middleware::BasicHealthCheck do
       end
     end
+    context 'with X-Forwarded-For headers' do
+      let(:load_balancer_ip) { '1.2.3.4' }
+
+      before do
+        env['HTTP_X_FORWARDED_FOR'] = "#{load_balancer_ip}, 127.0.0.1"
+        env['REMOTE_ADDR'] = '127.0.0.1'
+        env['PATH_INFO'] = described_class::HEALTH_PATH
+      end
+
+      it 'returns 200 response when endpoint is allowed' do
+        allow(Settings.monitoring).to receive(:ip_whitelist).and_return([load_balancer_ip])
+        expect(app).not_to receive(:call)
+
+        response = middleware.call(env)
+
+        expect(response[0]).to eq(200)
+        expect(response[1]).to eq({ 'Content-Type' => 'text/plain' })
+        expect(response[2]).to eq(['GitLab OK'])
+      end
+
+      it 'returns 404 when whitelist is not configured' do
+        allow(Settings.monitoring).to receive(:ip_whitelist).and_return([])
+
+        response = middleware.call(env)
+
+        expect(response[0]).to eq(404)
+      end
+    end
+
     context 'whitelisted IP' do
       before do
         env['REMOTE_ADDR'] = '127.0.0.1'

spec/lib/gitlab/request_context_spec.rb

@@ -6,6 +6,31 @@ describe Gitlab::RequestContext do
     let(:app) { -> (env) {} }
     let(:env) { Hash.new }
+    context 'with X-Forwarded-For headers', :request_store do
+      let(:load_balancer_ip) { '1.2.3.4' }
+      let(:headers) do
+        {
+          'HTTP_X_FORWARDED_FOR' => "#{load_balancer_ip}, 127.0.0.1",
+          'REMOTE_ADDR' => '127.0.0.1'
+        }
+      end
+
+      let(:env) { Rack::MockRequest.env_for("/").merge(headers) }
+
+      it 'returns the load balancer IP' do
+        client_ip = nil
+
+        endpoint = proc do
+          client_ip = Gitlab::SafeRequestStore[:client_ip]
+          [200, {}, ["Hello"]]
+        end
+
+        Rails.application.middleware.build(endpoint).call(env)
+
+        expect(client_ip).to eq(load_balancer_ip)
+      end
+    end
+
     context 'when RequestStore::Middleware is used' do
       around do |example|
         RequestStore::Middleware.new(-> (env) { example.run }).call({})
@@ -15,7 +40,7 @@ describe Gitlab::RequestContext do
         let(:ip) { '192.168.1.11' }
         before do
-          allow_any_instance_of(ActionDispatch::Request).to receive(:ip).and_return(ip)
+          allow_any_instance_of(Rack::Request).to receive(:ip).and_return(ip)
           described_class.new(app).call(env)
         end

spec/models/merge_request_diff_spec.rb

 require 'spec_helper'
 describe MergeRequestDiff do
+  include RepoHelpers
+
   let(:diff_with_commits) { create(:merge_request).merge_request_diff }
   describe 'validations' do
@@ -194,6 +196,25 @@ describe MergeRequestDiff do
         expect(diff_file).to be_binary
         expect(diff_file.diff).to eq(mr_diff.compare.diffs(paths: [path]).to_a.first.diff)
       end
+
+      context 'with diffs that contain a null byte' do
+        let(:filename) { 'test-null.txt' }
+        let(:content) { "a" * 10000 + "\x00" }
+        let(:project) { create(:project, :repository) }
+        let(:branch) { 'null-data' }
+        let(:target_branch) { 'master' }
+
+        it 'saves diffs correctly' do
+          create_file_in_repo(project, target_branch, branch, filename, content)
+
+          mr_diff = create(:merge_request, target_project: project, source_project: project, source_branch: branch, target_branch: target_branch).merge_request_diff
+          diff_file = mr_diff.merge_request_diff_files.find_by(new_path: filename)
+
+          expect(diff_file).to be_binary
+          expect(diff_file.diff).to eq(mr_diff.compare.diffs(paths: [filename]).to_a.first.diff)
+          expect(diff_file.diff).to include(content)
+        end
+      end
     end
   end

spec/support/helpers/repo_helpers.rb

@@ -115,4 +115,18 @@ eos
       commits: commits
     )
   end
+
+  def create_file_in_repo(
+        project, start_branch, branch_name, filename, content,
+        commit_message: 'Add new content')
+    Files::CreateService.new(
+      project,
+      project.owner,
+      commit_message: commit_message,
+      start_branch: start_branch,
+      branch_name: branch_name,
+      file_path: filename,
+      file_content: content
+    ).execute
+  end
 end

spec/validators/sha_validator_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 describe ShaValidator do
   let(:validator) { described_class.new(attributes: [:base_commit_sha]) }
-  let(:merge_diff) { build(:merge_request_diff) }
+  let!(:merge_diff) { build(:merge_request_diff) }
   subject { validator.validate_each(merge_diff, :base_commit_sha, value) }
@@ -10,6 +10,8 @@ describe ShaValidator do
     let(:value) { nil }
     it 'does not add any error if value is empty' do
+      expect(Commit).not_to receive(:valid_hash?)
+
       subject
       expect(merge_diff.errors).to be_empty
@@ -19,7 +21,9 @@ describe ShaValidator do
   context 'with valid sha' do
     let(:value) { Digest::SHA1.hexdigest(SecureRandom.hex) }
-    it 'does not add any error if value is empty' do
+    it 'does not add any error' do
+      expect(Commit).to receive(:valid_hash?).and_call_original
+
       subject
       expect(merge_diff.errors).to be_empty
@@ -30,6 +34,7 @@ describe ShaValidator do
     let(:value) { 'foo' }
     it 'adds error to the record' do
+      expect(Commit).to receive(:valid_hash?).and_call_original
       expect(merge_diff.errors).to be_empty
       subject