Merge branch 'security-60039' into 'master'

Disallow invalid MR branch name

See merge request gitlab/gitlabhq!3052

5 files changed, 106 insertions, 31 deletions

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