Commit 5d805eaa authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot

Merge branch 'security-60039-11-10' into '11-10-stable'

Disallow invalid MR branch name

See merge request gitlab/gitlabhq!3094
parents 90b7724e cec5d2e8
......@@ -620,6 +620,8 @@ class MergeRequest < ApplicationRecord
return
end
[:source_branch, :target_branch].each { |attr| validate_branch_name(attr) }
if opened?
similar_mrs = target_project
.merge_requests
......@@ -640,6 +642,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?
......
---
title: Prevent invalid branch for merge request
merge_request:
author:
type: security
......@@ -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
......@@ -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
......
......@@ -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
......
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
......@@ -150,6 +150,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) }
......
......@@ -922,7 +922,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
......@@ -953,7 +953,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
......@@ -982,7 +982,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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment