summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/merge_request.rb12
-rw-r--r--changelogs/unreleased/security-60039.yml5
-rw-r--r--lib/gitlab/git_ref_validator.rb23
-rw-r--r--spec/features/issuables/issuable_list_spec.rb2
-rw-r--r--spec/lib/gitlab/bitbucket_import/importer_spec.rb1
-rw-r--r--spec/lib/gitlab/git_ref_validator_spec.rb92
-rw-r--r--spec/models/merge_request_spec.rb36
-rw-r--r--spec/services/ci/create_pipeline_service_spec.rb6
8 files changed, 144 insertions, 33 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 311ba1ce6bd..81a8622bfe7 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -588,6 +588,8 @@ class MergeRequest < ApplicationRecord
return
end
+ [:source_branch, :target_branch].each { |attr| validate_branch_name(attr) }
+
if opened?
similar_mrs = target_project
.merge_requests
@@ -608,6 +610,16 @@ class MergeRequest < ApplicationRecord
end
end
+ def validate_branch_name(attr)
+ return unless changes_include?(attr)
+
+ branch = read_attribute(attr)
+
+ return unless branch
+
+ errors.add(attr) unless Gitlab::GitRefValidator.validate_merge_request_branch(branch)
+ end
+
def validate_target_project
return true if target_project.merge_requests_enabled?
diff --git a/changelogs/unreleased/security-60039.yml b/changelogs/unreleased/security-60039.yml
new file mode 100644
index 00000000000..5edbf32ec97
--- /dev/null
+++ b/changelogs/unreleased/security-60039.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent invalid branch for merge request
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/git_ref_validator.rb b/lib/gitlab/git_ref_validator.rb
index 3f13ebeb9d0..dfff6823689 100644
--- a/lib/gitlab/git_ref_validator.rb
+++ b/lib/gitlab/git_ref_validator.rb
@@ -5,12 +5,15 @@
module Gitlab
module GitRefValidator
extend self
+
+ EXPANDED_PREFIXES = %w[refs/heads/ refs/remotes/].freeze
+ DISALLOWED_PREFIXES = %w[-].freeze
+
# Validates a given name against the git reference specification
#
# Returns true for a valid reference name, false otherwise
def validate(ref_name)
- not_allowed_prefixes = %w(refs/heads/ refs/remotes/ -)
- return false if ref_name.start_with?(*not_allowed_prefixes)
+ return false if ref_name.start_with?(*(EXPANDED_PREFIXES + DISALLOWED_PREFIXES))
return false if ref_name == 'HEAD'
begin
@@ -19,5 +22,21 @@ module Gitlab
return false
end
end
+
+ def validate_merge_request_branch(ref_name)
+ return false if ref_name.start_with?(*DISALLOWED_PREFIXES)
+
+ expanded_name = if ref_name.start_with?(*EXPANDED_PREFIXES)
+ ref_name
+ else
+ "refs/heads/#{ref_name}"
+ end
+
+ begin
+ Rugged::Reference.valid_name?(expanded_name)
+ rescue ArgumentError
+ return false
+ end
+ end
end
end
diff --git a/spec/features/issuables/issuable_list_spec.rb b/spec/features/issuables/issuable_list_spec.rb
index 7b6e9cd66b2..225b858742d 100644
--- a/spec/features/issuables/issuable_list_spec.rb
+++ b/spec/features/issuables/issuable_list_spec.rb
@@ -76,7 +76,7 @@ describe 'issuable list' do
create(:issue, project: project, author: user)
else
create(:merge_request, source_project: project, source_branch: generate(:branch))
- source_branch = FFaker::Name.name
+ source_branch = FFaker::Lorem.characters(8)
pipeline = create(:ci_empty_pipeline, project: project, ref: source_branch, status: %w(running failed success).sample, sha: 'any')
create(:merge_request, title: FFaker::Lorem.sentence, source_project: project, source_branch: source_branch, head_pipeline: pipeline)
end
diff --git a/spec/lib/gitlab/bitbucket_import/importer_spec.rb b/spec/lib/gitlab/bitbucket_import/importer_spec.rb
index a02c00e3340..2e90f6c7f71 100644
--- a/spec/lib/gitlab/bitbucket_import/importer_spec.rb
+++ b/spec/lib/gitlab/bitbucket_import/importer_spec.rb
@@ -5,6 +5,7 @@ describe Gitlab::BitbucketImport::Importer do
before do
stub_omniauth_provider('bitbucket')
+ stub_feature_flags(stricter_mr_branch_name: false)
end
let(:statuses) do
diff --git a/spec/lib/gitlab/git_ref_validator_spec.rb b/spec/lib/gitlab/git_ref_validator_spec.rb
index 3ab04a1c46d..b63389af29f 100644
--- a/spec/lib/gitlab/git_ref_validator_spec.rb
+++ b/spec/lib/gitlab/git_ref_validator_spec.rb
@@ -1,31 +1,69 @@
require 'spec_helper'
describe Gitlab::GitRefValidator do
- it { expect(described_class.validate('feature/new')).to be_truthy }
- it { expect(described_class.validate('implement_@all')).to be_truthy }
- it { expect(described_class.validate('my_new_feature')).to be_truthy }
- it { expect(described_class.validate('my-branch')).to be_truthy }
- it { expect(described_class.validate('#1')).to be_truthy }
- it { expect(described_class.validate('feature/refs/heads/foo')).to be_truthy }
- it { expect(described_class.validate('feature/~new/')).to be_falsey }
- it { expect(described_class.validate('feature/^new/')).to be_falsey }
- it { expect(described_class.validate('feature/:new/')).to be_falsey }
- it { expect(described_class.validate('feature/?new/')).to be_falsey }
- it { expect(described_class.validate('feature/*new/')).to be_falsey }
- it { expect(described_class.validate('feature/[new/')).to be_falsey }
- it { expect(described_class.validate('feature/new/')).to be_falsey }
- it { expect(described_class.validate('feature/new.')).to be_falsey }
- it { expect(described_class.validate('feature\@{')).to be_falsey }
- it { expect(described_class.validate('feature\new')).to be_falsey }
- it { expect(described_class.validate('feature//new')).to be_falsey }
- it { expect(described_class.validate('feature new')).to be_falsey }
- it { expect(described_class.validate('refs/heads/')).to be_falsey }
- it { expect(described_class.validate('refs/remotes/')).to be_falsey }
- it { expect(described_class.validate('refs/heads/feature')).to be_falsey }
- it { expect(described_class.validate('refs/remotes/origin')).to be_falsey }
- it { expect(described_class.validate('-')).to be_falsey }
- it { expect(described_class.validate('-branch')).to be_falsey }
- it { expect(described_class.validate('.tag')).to be_falsey }
- it { expect(described_class.validate('my branch')).to be_falsey }
- it { expect(described_class.validate("\xA0\u0000\xB0")).to be_falsey }
+ using RSpec::Parameterized::TableSyntax
+
+ context '.validate' do
+ it { expect(described_class.validate('feature/new')).to be true }
+ it { expect(described_class.validate('implement_@all')).to be true }
+ it { expect(described_class.validate('my_new_feature')).to be true }
+ it { expect(described_class.validate('my-branch')).to be true }
+ it { expect(described_class.validate('#1')).to be true }
+ it { expect(described_class.validate('feature/refs/heads/foo')).to be true }
+ it { expect(described_class.validate('feature/~new/')).to be false }
+ it { expect(described_class.validate('feature/^new/')).to be false }
+ it { expect(described_class.validate('feature/:new/')).to be false }
+ it { expect(described_class.validate('feature/?new/')).to be false }
+ it { expect(described_class.validate('feature/*new/')).to be false }
+ it { expect(described_class.validate('feature/[new/')).to be false }
+ it { expect(described_class.validate('feature/new/')).to be false }
+ it { expect(described_class.validate('feature/new.')).to be false }
+ it { expect(described_class.validate('feature\@{')).to be false }
+ it { expect(described_class.validate('feature\new')).to be false }
+ it { expect(described_class.validate('feature//new')).to be false }
+ it { expect(described_class.validate('feature new')).to be false }
+ it { expect(described_class.validate('refs/heads/')).to be false }
+ it { expect(described_class.validate('refs/remotes/')).to be false }
+ it { expect(described_class.validate('refs/heads/feature')).to be false }
+ it { expect(described_class.validate('refs/remotes/origin')).to be false }
+ it { expect(described_class.validate('-')).to be false }
+ it { expect(described_class.validate('-branch')).to be false }
+ it { expect(described_class.validate('+foo:bar')).to be false }
+ it { expect(described_class.validate('foo:bar')).to be false }
+ it { expect(described_class.validate('.tag')).to be false }
+ it { expect(described_class.validate('my branch')).to be false }
+ it { expect(described_class.validate("\xA0\u0000\xB0")).to be false }
+ end
+
+ context '.validate_merge_request_branch' do
+ it { expect(described_class.validate_merge_request_branch('HEAD')).to be true }
+ it { expect(described_class.validate_merge_request_branch('feature/new')).to be true }
+ it { expect(described_class.validate_merge_request_branch('implement_@all')).to be true }
+ it { expect(described_class.validate_merge_request_branch('my_new_feature')).to be true }
+ it { expect(described_class.validate_merge_request_branch('my-branch')).to be true }
+ it { expect(described_class.validate_merge_request_branch('#1')).to be true }
+ it { expect(described_class.validate_merge_request_branch('feature/refs/heads/foo')).to be true }
+ it { expect(described_class.validate_merge_request_branch('feature/~new/')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature/^new/')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature/:new/')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature/?new/')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature/*new/')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature/[new/')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature/new/')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature/new.')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature\@{')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature\new')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature//new')).to be false }
+ it { expect(described_class.validate_merge_request_branch('feature new')).to be false }
+ it { expect(described_class.validate_merge_request_branch('refs/heads/master')).to be true }
+ it { expect(described_class.validate_merge_request_branch('refs/heads/')).to be false }
+ it { expect(described_class.validate_merge_request_branch('refs/remotes/')).to be false }
+ it { expect(described_class.validate_merge_request_branch('-')).to be false }
+ it { expect(described_class.validate_merge_request_branch('-branch')).to be false }
+ it { expect(described_class.validate_merge_request_branch('+foo:bar')).to be false }
+ it { expect(described_class.validate_merge_request_branch('foo:bar')).to be false }
+ it { expect(described_class.validate_merge_request_branch('.tag')).to be false }
+ it { expect(described_class.validate_merge_request_branch('my branch')).to be false }
+ it { expect(described_class.validate_merge_request_branch("\xA0\u0000\xB0")).to be false }
+ end
end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index c72b6e9033d..cb806d663da 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -173,6 +173,42 @@ describe MergeRequest do
end
end
+ context 'for branch' do
+ before do
+ stub_feature_flags(stricter_mr_branch_name: false)
+ end
+
+ using RSpec::Parameterized::TableSyntax
+
+ where(:branch_name, :valid) do
+ 'foo' | true
+ 'foo:bar' | false
+ '+foo:bar' | false
+ 'foo bar' | false
+ '-foo' | false
+ 'HEAD' | true
+ 'refs/heads/master' | true
+ end
+
+ with_them do
+ it "validates source_branch" do
+ subject = build(:merge_request, source_branch: branch_name, target_branch: 'master')
+
+ subject.valid?
+
+ expect(subject.errors.added?(:source_branch)).to eq(!valid)
+ end
+
+ it "validates target_branch" do
+ subject = build(:merge_request, source_branch: 'master', target_branch: branch_name)
+
+ subject.valid?
+
+ expect(subject.errors.added?(:target_branch)).to eq(!valid)
+ end
+ end
+ end
+
context 'for forks' do
let(:project) { create(:project) }
let(:fork1) { fork_project(project) }
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 9a3ac75e418..867692d4d64 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -973,7 +973,7 @@ describe Ci::CreatePipelineService do
let(:merge_request) do
create(:merge_request,
source_project: project,
- source_branch: ref_name,
+ source_branch: Gitlab::Git.ref_name(ref_name),
target_project: project,
target_branch: 'master')
end
@@ -1004,7 +1004,7 @@ describe Ci::CreatePipelineService do
let(:merge_request) do
create(:merge_request,
source_project: project,
- source_branch: ref_name,
+ source_branch: Gitlab::Git.ref_name(ref_name),
target_project: project,
target_branch: 'master')
end
@@ -1033,7 +1033,7 @@ describe Ci::CreatePipelineService do
let(:merge_request) do
create(:merge_request,
source_project: project,
- source_branch: ref_name,
+ source_branch: Gitlab::Git.ref_name(ref_name),
target_project: project,
target_branch: 'master')
end
View Lorry Controller Status