diff options
author | Marin Jankovski <marin@gitlab.com> | 2019-04-23 16:57:11 +0000 |
---|---|---|
committer | Marin Jankovski <marin@gitlab.com> | 2019-04-23 16:57:11 +0000 |
commit | 9f5060bdd3f8c90359ab6e9daa45be63e834dcac (patch) | |
tree | b13051f7c909a213d9819e8ec89617ab25c2b70c | |
parent | 208890f11f8e957e1c780777c2225282ff0ed6dc (diff) | |
parent | b93b23cb403c8dd4b0a9e9ac89110d99e46a3e68 (diff) | |
download | gitlab-ce-9f5060bdd3f8c90359ab6e9daa45be63e834dcac.tar.gz |
Merge branch '11-8-stable-patch-8' into '11-8-stable'
Prepare 11.8.8 release
See merge request gitlab-org/gitlab-ce!27573
19 files changed, 179 insertions, 15 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 4e8453726a3..1f0e798e8a0 100644 --- a/.gitlab-ci.yml +++ b/.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 diff --git a/app/assets/javascripts/pipelines/pipeline_details_mediator.js b/app/assets/javascripts/pipelines/pipeline_details_mediator.js index bd1e1895660..d67d88c4dba 100644 --- a/app/assets/javascripts/pipelines/pipeline_details_mediator.js +++ b/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, + }; } } diff --git a/app/assets/javascripts/pipelines/services/pipeline_service.js b/app/assets/javascripts/pipelines/services/pipeline_service.js index a53a9cc8365..e44eb9cdfd1 100644 --- a/app/assets/javascripts/pipelines/services/pipeline_service.js +++ b/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 diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 481be2da8ac..be2f1319fd2 100644 --- a/app/models/merge_request_diff.rb +++ b/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 diff --git a/app/validators/sha_validator.rb b/app/validators/sha_validator.rb index 085fca4d65d..77e7cfa4f6b 100644 --- a/app/validators/sha_validator.rb +++ b/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 diff --git a/app/views/projects/buttons/_clone.html.haml b/app/views/projects/buttons/_clone.html.haml index 159d9e44e17..09f05b30433 100644 --- a/app/views/projects/buttons/_clone.html.haml +++ b/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 diff --git a/app/views/shared/_mobile_clone_panel.html.haml b/app/views/shared/_mobile_clone_panel.html.haml index 6e2527bd1a1..1e6b6f7c79b 100644 --- a/app/views/shared/_mobile_clone_panel.html.haml +++ b/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' diff --git a/changelogs/unreleased/fj-58804-fix-bitbucket-import.yml b/changelogs/unreleased/fj-58804-fix-bitbucket-import.yml new file mode 100644 index 00000000000..dc44c64a055 --- /dev/null +++ b/changelogs/unreleased/fj-58804-fix-bitbucket-import.yml @@ -0,0 +1,5 @@ +--- +title: Fix bug in BitBucket imports with SHA shorter than 40 chars +merge_request: 26050 +author: +type: fixed diff --git a/changelogs/unreleased/sh-handle-null-bytes-in-merge-request-diffs.yml b/changelogs/unreleased/sh-handle-null-bytes-in-merge-request-diffs.yml new file mode 100644 index 00000000000..01b6b08b61b --- /dev/null +++ b/changelogs/unreleased/sh-handle-null-bytes-in-merge-request-diffs.yml @@ -0,0 +1,5 @@ +--- +title: Fix error creating a merge request when diff includes a null byte +merge_request: 26190 +author: +type: fixed diff --git a/changelogs/unreleased/sh-revert-rack-request-health-checks.yml b/changelogs/unreleased/sh-revert-rack-request-health-checks.yml new file mode 100644 index 00000000000..5dd5e5b731c --- /dev/null +++ b/changelogs/unreleased/sh-revert-rack-request-health-checks.yml @@ -0,0 +1,5 @@ +--- +title: Fix health checks not working behind load balancers +merge_request: 26055 +author: +type: fixed diff --git a/lib/gitlab/middleware/basic_health_check.rb b/lib/gitlab/middleware/basic_health_check.rb index acf8c301b8f..84e49805428 100644 --- a/lib/gitlab/middleware/basic_health_check.rb +++ b/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) diff --git a/lib/gitlab/request_context.rb b/lib/gitlab/request_context.rb index d9811e036d3..f6d289476c5 100644 --- a/lib/gitlab/request_context.rb +++ b/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 diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb index 8a44ce52849..ee5d27355f1 100644 --- a/spec/factories/ci/pipelines.rb +++ b/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 diff --git a/spec/lib/gitlab/bitbucket_import/importer_spec.rb b/spec/lib/gitlab/bitbucket_import/importer_spec.rb index c432cc223b9..e1a2bae5fe8 100644 --- a/spec/lib/gitlab/bitbucket_import/importer_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/middleware/basic_health_check_spec.rb b/spec/lib/gitlab/middleware/basic_health_check_spec.rb index 187d903a5e1..86bdc479b66 100644 --- a/spec/lib/gitlab/middleware/basic_health_check_spec.rb +++ b/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' diff --git a/spec/lib/gitlab/request_context_spec.rb b/spec/lib/gitlab/request_context_spec.rb index fd443cc1f71..3ed57c2c916 100644 --- a/spec/lib/gitlab/request_context_spec.rb +++ b/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 diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index e530e0302f5..53f5307ea0b 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -1,6 +1,8 @@ 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 diff --git a/spec/support/helpers/repo_helpers.rb b/spec/support/helpers/repo_helpers.rb index 3c6956cf5e0..4af90f4af79 100644 --- a/spec/support/helpers/repo_helpers.rb +++ b/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 diff --git a/spec/validators/sha_validator_spec.rb b/spec/validators/sha_validator_spec.rb index b9242ef931e..fa3dd68df2d 100644 --- a/spec/validators/sha_validator_spec.rb +++ b/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 |