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

Merge branch 'security-60039' into 'master'

Disallow invalid MR branch name

See merge request gitlab/gitlabhq!3052
parents e5b88d88 c7e8f5c6
...@@ -588,6 +588,8 @@ class MergeRequest < ApplicationRecord ...@@ -588,6 +588,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
...@@ -608,6 +610,16 @@ class MergeRequest < ApplicationRecord ...@@ -608,6 +610,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
...@@ -173,6 +173,42 @@ describe MergeRequest do ...@@ -173,6 +173,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) }
......
...@@ -973,7 +973,7 @@ describe Ci::CreatePipelineService do ...@@ -973,7 +973,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
...@@ -1004,7 +1004,7 @@ describe Ci::CreatePipelineService do ...@@ -1004,7 +1004,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
...@@ -1033,7 +1033,7 @@ describe Ci::CreatePipelineService do ...@@ -1033,7 +1033,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