Merge branch 'security-60039' into 'master'

Disallow invalid MR branch name

See merge request gitlab/gitlabhq!3052

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