Commit cae585b4 authored by Mark Chao's avatar Mark Chao

Validate MR branch names

Prevents refspec as branch name, which would bypass branch protection
when used in conjunction with rebase.

HEAD seems to be a special case with lots of occurrence,
so it is considered valid for now.

Another special case is `refs/head/*`, which can be imported.
parent 204dca44
...@@ -586,6 +586,8 @@ class MergeRequest < ApplicationRecord ...@@ -586,6 +586,8 @@ class MergeRequest < ApplicationRecord
return return
end end
[:source_branch, :target_branch].each { |attr| validate_branch_name(attr) }
if opened? if opened?
similar_mrs = target_project similar_mrs = target_project
.merge_requests .merge_requests
...@@ -606,6 +608,16 @@ class MergeRequest < ApplicationRecord ...@@ -606,6 +608,16 @@ class MergeRequest < ApplicationRecord
end end
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 def validate_target_project
return true if target_project.merge_requests_enabled? return true if target_project.merge_requests_enabled?
......
---
title: Prevent invalid branch for merge request
merge_request:
author:
type: security
...@@ -5,12 +5,15 @@ ...@@ -5,12 +5,15 @@
module Gitlab module Gitlab
module GitRefValidator module GitRefValidator
extend self extend self
EXPANDED_PREFIXES = %w[refs/heads/ refs/remotes/].freeze
DISALLOWED_PREFIXES = %w[-].freeze
# Validates a given name against the git reference specification # Validates a given name against the git reference specification
# #
# Returns true for a valid reference name, false otherwise # Returns true for a valid reference name, false otherwise
def validate(ref_name) def validate(ref_name)
not_allowed_prefixes = %w(refs/heads/ refs/remotes/ -) return false if ref_name.start_with?(*(EXPANDED_PREFIXES + DISALLOWED_PREFIXES))
return false if ref_name.start_with?(*not_allowed_prefixes)
return false if ref_name == 'HEAD' return false if ref_name == 'HEAD'
begin begin
...@@ -19,5 +22,21 @@ module Gitlab ...@@ -19,5 +22,21 @@ module Gitlab
return false return false
end end
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
end end
...@@ -76,7 +76,7 @@ describe 'issuable list' do ...@@ -76,7 +76,7 @@ describe 'issuable list' do
create(:issue, project: project, author: user) create(:issue, project: project, author: user)
else else
create(:merge_request, source_project: project, source_branch: generate(:branch)) 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') 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) create(:merge_request, title: FFaker::Lorem.sentence, source_project: project, source_branch: source_branch, head_pipeline: pipeline)
end end
......
...@@ -5,6 +5,7 @@ describe Gitlab::BitbucketImport::Importer do ...@@ -5,6 +5,7 @@ describe Gitlab::BitbucketImport::Importer do
before do before do
stub_omniauth_provider('bitbucket') stub_omniauth_provider('bitbucket')
stub_feature_flags(stricter_mr_branch_name: false)
end end
let(:statuses) do let(:statuses) do
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::GitRefValidator do describe Gitlab::GitRefValidator do
it { expect(described_class.validate('feature/new')).to be_truthy } using RSpec::Parameterized::TableSyntax
it { expect(described_class.validate('implement_@all')).to be_truthy }
it { expect(described_class.validate('my_new_feature')).to be_truthy } context '.validate' do
it { expect(described_class.validate('my-branch')).to be_truthy } it { expect(described_class.validate('feature/new')).to be true }
it { expect(described_class.validate('#1')).to be_truthy } it { expect(described_class.validate('implement_@all')).to be true }
it { expect(described_class.validate('feature/refs/heads/foo')).to be_truthy } it { expect(described_class.validate('my_new_feature')).to be true }
it { expect(described_class.validate('feature/~new/')).to be_falsey } it { expect(described_class.validate('my-branch')).to be true }
it { expect(described_class.validate('feature/^new/')).to be_falsey } it { expect(described_class.validate('#1')).to be true }
it { expect(described_class.validate('feature/:new/')).to be_falsey } it { expect(described_class.validate('feature/refs/heads/foo')).to be true }
it { expect(described_class.validate('feature/?new/')).to be_falsey } it { expect(described_class.validate('feature/~new/')).to be false }
it { expect(described_class.validate('feature/*new/')).to be_falsey } it { expect(described_class.validate('feature/^new/')).to be false }
it { expect(described_class.validate('feature/[new/')).to be_falsey } it { expect(described_class.validate('feature/:new/')).to be false }
it { expect(described_class.validate('feature/new/')).to be_falsey } it { expect(described_class.validate('feature/?new/')).to be false }
it { expect(described_class.validate('feature/new.')).to be_falsey } it { expect(described_class.validate('feature/*new/')).to be false }
it { expect(described_class.validate('feature\@{')).to be_falsey } it { expect(described_class.validate('feature/[new/')).to be false }
it { expect(described_class.validate('feature\new')).to be_falsey } it { expect(described_class.validate('feature/new/')).to be false }
it { expect(described_class.validate('feature//new')).to be_falsey } it { expect(described_class.validate('feature/new.')).to be false }
it { expect(described_class.validate('feature new')).to be_falsey } it { expect(described_class.validate('feature\@{')).to be false }
it { expect(described_class.validate('refs/heads/')).to be_falsey } it { expect(described_class.validate('feature\new')).to be false }
it { expect(described_class.validate('refs/remotes/')).to be_falsey } it { expect(described_class.validate('feature//new')).to be false }
it { expect(described_class.validate('refs/heads/feature')).to be_falsey } it { expect(described_class.validate('feature new')).to be false }
it { expect(described_class.validate('refs/remotes/origin')).to be_falsey } it { expect(described_class.validate('refs/heads/')).to be false }
it { expect(described_class.validate('-')).to be_falsey } it { expect(described_class.validate('refs/remotes/')).to be false }
it { expect(described_class.validate('-branch')).to be_falsey } it { expect(described_class.validate('refs/heads/feature')).to be false }
it { expect(described_class.validate('.tag')).to be_falsey } it { expect(described_class.validate('refs/remotes/origin')).to be false }
it { expect(described_class.validate('my branch')).to be_falsey } it { expect(described_class.validate('-')).to be false }
it { expect(described_class.validate("\xA0\u0000\xB0")).to be_falsey } 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 end
...@@ -150,6 +150,42 @@ describe MergeRequest do ...@@ -150,6 +150,42 @@ describe MergeRequest do
end end
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 context 'for forks' do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:fork1) { fork_project(project) } let(:fork1) { fork_project(project) }
......
...@@ -963,7 +963,7 @@ describe Ci::CreatePipelineService do ...@@ -963,7 +963,7 @@ describe Ci::CreatePipelineService do
let(:merge_request) do let(:merge_request) do
create(:merge_request, create(:merge_request,
source_project: project, source_project: project,
source_branch: ref_name, source_branch: Gitlab::Git.ref_name(ref_name),
target_project: project, target_project: project,
target_branch: 'master') target_branch: 'master')
end end
...@@ -994,7 +994,7 @@ describe Ci::CreatePipelineService do ...@@ -994,7 +994,7 @@ describe Ci::CreatePipelineService do
let(:merge_request) do let(:merge_request) do
create(:merge_request, create(:merge_request,
source_project: project, source_project: project,
source_branch: ref_name, source_branch: Gitlab::Git.ref_name(ref_name),
target_project: project, target_project: project,
target_branch: 'master') target_branch: 'master')
end end
...@@ -1023,7 +1023,7 @@ describe Ci::CreatePipelineService do ...@@ -1023,7 +1023,7 @@ describe Ci::CreatePipelineService do
let(:merge_request) do let(:merge_request) do
create(:merge_request, create(:merge_request,
source_project: project, source_project: project,
source_branch: ref_name, source_branch: Gitlab::Git.ref_name(ref_name),
target_project: project, target_project: project,
target_branch: 'master') target_branch: 'master')
end 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