From d8bddb16624f34600069bb5d3540960b25176381 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Wed, 10 Apr 2019 11:39:45 +0800 Subject: 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. --- app/models/merge_request.rb | 12 +++ changelogs/unreleased/security-60039.yml | 5 ++ lib/gitlab/git_ref_validator.rb | 23 +++++- spec/features/issuables/issuable_list_spec.rb | 2 +- spec/lib/gitlab/bitbucket_import/importer_spec.rb | 1 + spec/lib/gitlab/git_ref_validator_spec.rb | 92 ++++++++++++++++------- spec/models/merge_request_spec.rb | 36 +++++++++ 7 files changed, 141 insertions(+), 30 deletions(-) create mode 100644 changelogs/unreleased/security-60039.yml diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 458c57c1dc6..368772a5cf4 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -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? 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 e1a2bae5fe8..fbe87aaefa1 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..6fc41cd64f9 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_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('+foo:bar')).to be_falsey } + it { expect(described_class.validate('foo:bar')).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 } + end + + context '.validate_merge_request_branch' do + it { expect(described_class.validate_merge_request_branch('HEAD')).to be_truthy } + it { expect(described_class.validate_merge_request_branch('feature/new')).to be_truthy } + it { expect(described_class.validate_merge_request_branch('implement_@all')).to be_truthy } + it { expect(described_class.validate_merge_request_branch('my_new_feature')).to be_truthy } + it { expect(described_class.validate_merge_request_branch('my-branch')).to be_truthy } + it { expect(described_class.validate_merge_request_branch('#1')).to be_truthy } + it { expect(described_class.validate_merge_request_branch('feature/refs/heads/foo')).to be_truthy } + it { expect(described_class.validate_merge_request_branch('feature/~new/')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature/^new/')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature/:new/')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature/?new/')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature/*new/')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature/[new/')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature/new/')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature/new.')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature\@{')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature\new')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature//new')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('feature new')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('refs/heads/master')).to be_truthy } + it { expect(described_class.validate_merge_request_branch('refs/heads/')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('refs/remotes/')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('-')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('-branch')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('+foo:bar')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('foo:bar')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('.tag')).to be_falsey } + it { expect(described_class.validate_merge_request_branch('my branch')).to be_falsey } + it { expect(described_class.validate_merge_request_branch("\xA0\u0000\xB0")).to be_falsey } + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 6f34ef9c1bc..2b78e1e361e 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -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) } -- cgit v1.2.1 From 76469b508234c0528cbb5afb22e2dd776c11d56b Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Mon, 22 Apr 2019 14:57:08 +0900 Subject: Use full ref when creating MR pipeline in specs Continuation of 426488b7218e85ce69868ae4628801af2322b74a --- spec/services/ci/create_pipeline_service_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 866d709d446..0f5554fa577 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -923,7 +923,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 @@ -954,7 +954,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 @@ -983,7 +983,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 -- cgit v1.2.1 From c7e8f5c613754a7221d6b2f0b0e154b75c55dd84 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Fri, 3 May 2019 03:01:57 +0800 Subject: Refactor spec to not use truthy or falsey --- spec/lib/gitlab/git_ref_validator_spec.rb | 116 +++++++++++++++--------------- 1 file changed, 58 insertions(+), 58 deletions(-) diff --git a/spec/lib/gitlab/git_ref_validator_spec.rb b/spec/lib/gitlab/git_ref_validator_spec.rb index 6fc41cd64f9..b63389af29f 100644 --- a/spec/lib/gitlab/git_ref_validator_spec.rb +++ b/spec/lib/gitlab/git_ref_validator_spec.rb @@ -4,66 +4,66 @@ describe Gitlab::GitRefValidator do using RSpec::Parameterized::TableSyntax context '.validate' 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('+foo:bar')).to be_falsey } - it { expect(described_class.validate('foo:bar')).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 } + 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_truthy } - it { expect(described_class.validate_merge_request_branch('feature/new')).to be_truthy } - it { expect(described_class.validate_merge_request_branch('implement_@all')).to be_truthy } - it { expect(described_class.validate_merge_request_branch('my_new_feature')).to be_truthy } - it { expect(described_class.validate_merge_request_branch('my-branch')).to be_truthy } - it { expect(described_class.validate_merge_request_branch('#1')).to be_truthy } - it { expect(described_class.validate_merge_request_branch('feature/refs/heads/foo')).to be_truthy } - it { expect(described_class.validate_merge_request_branch('feature/~new/')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature/^new/')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature/:new/')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature/?new/')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature/*new/')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature/[new/')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature/new/')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature/new.')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature\@{')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature\new')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature//new')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('feature new')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('refs/heads/master')).to be_truthy } - it { expect(described_class.validate_merge_request_branch('refs/heads/')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('refs/remotes/')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('-')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('-branch')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('+foo:bar')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('foo:bar')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('.tag')).to be_falsey } - it { expect(described_class.validate_merge_request_branch('my branch')).to be_falsey } - it { expect(described_class.validate_merge_request_branch("\xA0\u0000\xB0")).to be_falsey } + 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 -- cgit v1.2.1 From b0fbf001dab134b6638411f0be209bc0d1460519 Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Fri, 3 May 2019 15:09:20 +0200 Subject: Fix url redaction for issue links Add changelog entry Add missing href to all redactor specs and removed href assignment Remove obsolete spec If original_content is given, it should be used for link content --- ...ity-fix-project-existence-disclosure-master.yml | 5 ++++ lib/banzai/redactor.rb | 7 +++-- spec/lib/banzai/redactor_spec.rb | 32 ++++++++++++---------- 3 files changed, 28 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/security-fix-project-existence-disclosure-master.yml diff --git a/changelogs/unreleased/security-fix-project-existence-disclosure-master.yml b/changelogs/unreleased/security-fix-project-existence-disclosure-master.yml new file mode 100644 index 00000000000..084439c71d9 --- /dev/null +++ b/changelogs/unreleased/security-fix-project-existence-disclosure-master.yml @@ -0,0 +1,5 @@ +--- +title: Fix url redaction for issue links +merge_request: +author: +type: security diff --git a/lib/banzai/redactor.rb b/lib/banzai/redactor.rb index 7db5f5e1f7d..c2da7fec7cc 100644 --- a/lib/banzai/redactor.rb +++ b/lib/banzai/redactor.rb @@ -70,8 +70,11 @@ module Banzai # Build the raw tag just with a link as href and content if # it's originally a link pattern. We shouldn't return a plain text href. original_link = - if link_reference == 'true' && href = original_content - %(#{href}) + if link_reference == 'true' + href = node.attr('href') + content = original_content + + %(#{content}) end # The reference should be replaced by the original link's content, diff --git a/spec/lib/banzai/redactor_spec.rb b/spec/lib/banzai/redactor_spec.rb index aaeec953e4b..718649e0e10 100644 --- a/spec/lib/banzai/redactor_spec.rb +++ b/spec/lib/banzai/redactor_spec.rb @@ -13,10 +13,10 @@ describe Banzai::Redactor do it 'redacts an array of documents' do doc1 = Nokogiri::HTML - .fragment('foo') + .fragment('foo') doc2 = Nokogiri::HTML - .fragment('bar') + .fragment('bar') redacted_data = redactor.redact([doc1, doc2]) @@ -27,7 +27,7 @@ describe Banzai::Redactor do end it 'replaces redacted reference with inner HTML' do - doc = Nokogiri::HTML.fragment("foo") + doc = Nokogiri::HTML.fragment("foo") redactor.redact([doc]) expect(doc.to_html).to eq('foo') end @@ -35,20 +35,24 @@ describe Banzai::Redactor do context 'when data-original attribute provided' do let(:original_content) { 'foo' } it 'replaces redacted reference with original content' do - doc = Nokogiri::HTML.fragment("bar") + doc = Nokogiri::HTML.fragment("bar") redactor.redact([doc]) expect(doc.to_html).to eq(original_content) end - end - - it 'returns tag with original href if it is originally a link reference' do - href = 'http://localhost:3000' - doc = Nokogiri::HTML - .fragment("#{href}") - redactor.redact([doc]) + it 'does not replace redacted reference with original content if href is given' do + html = "Marge" + doc = Nokogiri::HTML.fragment(html) + redactor.redact([doc]) + expect(doc.to_html).to eq('Marge') + end - expect(doc.to_html).to eq('http://localhost:3000') + it 'uses the original content as the link content if given' do + html = "Marge" + doc = Nokogiri::HTML.fragment(html) + redactor.redact([doc]) + expect(doc.to_html).to eq('Homer') + end end end @@ -61,7 +65,7 @@ describe Banzai::Redactor do end it 'redacts an issue attached' do - doc = Nokogiri::HTML.fragment("foo") + doc = Nokogiri::HTML.fragment("foo") redactor.redact([doc]) @@ -69,7 +73,7 @@ describe Banzai::Redactor do end it 'redacts an external issue' do - doc = Nokogiri::HTML.fragment("foo") + doc = Nokogiri::HTML.fragment("foo") redactor.redact([doc]) -- cgit v1.2.1 From b6424b378d3fd79a78c597f1c3d630ab2245f460 Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Tue, 14 May 2019 13:16:30 +0200 Subject: Fix confidential issue label disclosure on milestone view Add changelog entry Method should be public Use milestonish method Use render data to filter labels Add specs for label visibility on milestone --- app/controllers/concerns/milestone_actions.rb | 8 ++++- ...-confidential-issue-label-visibility-master.yml | 5 ++++ .../projects/milestones_controller_spec.rb | 34 ++++++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-fix-confidential-issue-label-visibility-master.yml diff --git a/app/controllers/concerns/milestone_actions.rb b/app/controllers/concerns/milestone_actions.rb index cfff154c3dd..8b8b7db72f8 100644 --- a/app/controllers/concerns/milestone_actions.rb +++ b/app/controllers/concerns/milestone_actions.rb @@ -26,16 +26,22 @@ module MilestoneActions end end + # rubocop:disable Gitlab/ModuleWithInstanceVariables def labels respond_to do |format| format.html { redirect_to milestone_redirect_path } format.json do + milestone_labels = @milestone.issue_labels_visible_by_user(current_user) + render json: tabs_json("shared/milestones/_labels_tab", { - labels: @milestone.labels.map { |label| label.present(issuable_subject: @milestone.parent) } # rubocop:disable Gitlab/ModuleWithInstanceVariables + labels: milestone_labels.map do |label| + label.present(issuable_subject: @milestone.parent) + end }) end end end + # rubocop:enable Gitlab/ModuleWithInstanceVariables private diff --git a/changelogs/unreleased/security-fix-confidential-issue-label-visibility-master.yml b/changelogs/unreleased/security-fix-confidential-issue-label-visibility-master.yml new file mode 100644 index 00000000000..adfd8e1298f --- /dev/null +++ b/changelogs/unreleased/security-fix-confidential-issue-label-visibility-master.yml @@ -0,0 +1,5 @@ +--- +title: Fix confidential issue label disclosure on milestone view +merge_request: +author: +type: security diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb index f8470a94f98..767cee7d54a 100644 --- a/spec/controllers/projects/milestones_controller_spec.rb +++ b/spec/controllers/projects/milestones_controller_spec.rb @@ -175,6 +175,40 @@ describe Projects::MilestonesController do end end + describe '#labels' do + render_views + + context 'as json' do + let!(:guest) { create(:user, username: 'guest1') } + let!(:group) { create(:group, :public) } + let!(:project) { create(:project, :public, group: group) } + let!(:label) { create(:label, title: 'test_label_on_private_issue', project: project) } + let!(:confidential_issue) { create(:labeled_issue, confidential: true, project: project, milestone: milestone, labels: [label]) } + + it 'does not render labels of private issues if user has no access' do + sign_in(guest) + + get :labels, params: { namespace_id: group.id, project_id: project.id, id: milestone.iid }, format: :json + + expect(response).to have_gitlab_http_status(200) + expect(response.content_type).to eq 'application/json' + + expect(json_response['html']).not_to include(label.title) + end + + it 'does render labels of private issues if user has access' do + sign_in(user) + + get :labels, params: { namespace_id: group.id, project_id: project.id, id: milestone.iid }, format: :json + + expect(response).to have_gitlab_http_status(200) + expect(response.content_type).to eq 'application/json' + + expect(json_response['html']).to include(label.title) + end + end + end + context 'promotion succeeds' do before do group.add_developer(user) -- cgit v1.2.1 From 1be66c4a098290d72cb19b4c844a9bee4eff630b Mon Sep 17 00:00:00 2001 From: Alexandru Croitor Date: Thu, 9 May 2019 16:09:04 +0300 Subject: Hide issue title on unsubscribe for anonymous users --- app/helpers/notifications_helper.rb | 4 + app/views/sent_notifications/unsubscribe.html.haml | 2 +- .../security-unsubscribing-from-issue.yml | 5 + .../sent_notifications_controller_spec.rb | 109 +++++++++++++++++++-- 4 files changed, 109 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/security-unsubscribing-from-issue.yml diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index a7ce7667916..11b9cf22142 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -100,4 +100,8 @@ module NotificationsHelper css_class: "icon notifications-icon js-notifications-icon" ) end + + def show_unsubscribe_title?(noteable) + can?(current_user, "read_#{noteable.to_ability_name}".to_sym, noteable) + end end diff --git a/app/views/sent_notifications/unsubscribe.html.haml b/app/views/sent_notifications/unsubscribe.html.haml index ca392e1adfc..22fcfcda297 100644 --- a/app/views/sent_notifications/unsubscribe.html.haml +++ b/app/views/sent_notifications/unsubscribe.html.haml @@ -1,6 +1,6 @@ - noteable = @sent_notification.noteable - noteable_type = @sent_notification.noteable_type.titleize.downcase -- noteable_text = %(#{noteable.title} (#{noteable.to_reference})) +- noteable_text = show_unsubscribe_title?(noteable) ? %(#{noteable.title} (#{noteable.to_reference})) : %(#{noteable.to_reference}) - page_title _("Unsubscribe"), noteable_text, noteable_type.pluralize, @sent_notification.project.full_name %h3.page-title diff --git a/changelogs/unreleased/security-unsubscribing-from-issue.yml b/changelogs/unreleased/security-unsubscribing-from-issue.yml new file mode 100644 index 00000000000..3a33a457c69 --- /dev/null +++ b/changelogs/unreleased/security-unsubscribing-from-issue.yml @@ -0,0 +1,5 @@ +--- +title: Hide confidential issue title on unsubscribe for anonymous users +merge_request: +author: +type: security diff --git a/spec/controllers/sent_notifications_controller_spec.rb b/spec/controllers/sent_notifications_controller_spec.rb index 2b9df71aa3a..89857a9d21b 100644 --- a/spec/controllers/sent_notifications_controller_spec.rb +++ b/spec/controllers/sent_notifications_controller_spec.rb @@ -4,15 +4,31 @@ require 'rails_helper' describe SentNotificationsController do let(:user) { create(:user) } - let(:project) { create(:project) } - let(:sent_notification) { create(:sent_notification, project: project, noteable: issue, recipient: user) } + let(:project) { create(:project, :public) } + let(:private_project) { create(:project, :private) } + let(:sent_notification) { create(:sent_notification, project: target_project, noteable: noteable, recipient: user) } let(:issue) do - create(:issue, project: project, author: user) do |issue| - issue.subscriptions.create(user: user, project: project, subscribed: true) + create(:issue, project: target_project) do |issue| + issue.subscriptions.create(user: user, project: target_project, subscribed: true) end end + let(:confidential_issue) do + create(:issue, project: target_project, confidential: true) do |issue| + issue.subscriptions.create(user: user, project: target_project, subscribed: true) + end + end + + let(:merge_request) do + create(:merge_request, source_project: target_project, target_project: target_project) do |mr| + mr.subscriptions.create(user: user, project: target_project, subscribed: true) + end + end + + let(:noteable) { issue } + let(:target_project) { project } + describe 'GET unsubscribe' do context 'when the user is not logged in' do context 'when the force param is passed' do @@ -34,20 +50,93 @@ describe SentNotificationsController do end context 'when the force param is not passed' do + render_views + before do get(:unsubscribe, params: { id: sent_notification.reply_key }) end - it 'does not unsubscribe the user' do - expect(issue.subscribed?(user, project)).to be_truthy + shared_examples 'unsubscribing as anonymous' do + it 'does not unsubscribe the user' do + expect(noteable.subscribed?(user, target_project)).to be_truthy + end + + it 'does not set the flash message' do + expect(controller).not_to set_flash[:notice] + end + + it 'renders unsubscribe page' do + expect(response.status).to eq(200) + expect(response).to render_template :unsubscribe + end end - it 'does not set the flash message' do - expect(controller).not_to set_flash[:notice] + context 'when project is public' do + context 'when unsubscribing from issue' do + let(:noteable) { issue } + + it 'shows issue title' do + expect(response.body).to include(issue.title) + end + + it_behaves_like 'unsubscribing as anonymous' + end + + context 'when unsubscribing from confidential issue' do + let(:noteable) { confidential_issue } + + it 'does not show issue title' do + expect(response.body).not_to include(confidential_issue.title) + expect(response.body).to include(confidential_issue.to_reference) + end + + it_behaves_like 'unsubscribing as anonymous' + end + + context 'when unsubscribing from merge request' do + let(:noteable) { merge_request } + + it 'shows merge request title' do + expect(response.body).to include(merge_request.title) + end + + it_behaves_like 'unsubscribing as anonymous' + end end - it 'redirects to the login page' do - expect(response).to render_template :unsubscribe + context 'when project is not public' do + let(:target_project) { private_project } + + context 'when unsubscribing from issue' do + let(:noteable) { issue } + + it 'shows issue title' do + expect(response.body).not_to include(issue.title) + end + + it_behaves_like 'unsubscribing as anonymous' + end + + context 'when unsubscribing from confidential issue' do + let(:noteable) { confidential_issue } + + it 'does not show issue title' do + expect(response.body).not_to include(confidential_issue.title) + expect(response.body).to include(confidential_issue.to_reference) + end + + it_behaves_like 'unsubscribing as anonymous' + end + + context 'when unsubscribing from merge request' do + let(:noteable) { merge_request } + + it 'shows merge request title' do + expect(response.body).not_to include(merge_request.title) + end + + it_behaves_like 'unsubscribing as anonymous' + end end end end -- cgit v1.2.1 From b70b43d07ec27c6410e4a8d7ad417662a8823f8f Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 20 May 2019 11:08:31 -0300 Subject: Resolve: Milestones leaked via search API Fix milestone titles being leaked using search API when users cannot read milestones --- app/models/project.rb | 12 ++++++ .../security-fix_milestones_search_api_leak.yml | 5 +++ lib/gitlab/project_search_results.rb | 6 +++ lib/gitlab/search_results.rb | 28 +++++++++++-- spec/lib/gitlab/search_results_spec.rb | 24 +++++++++++ spec/models/project_spec.rb | 17 ++++++++ spec/requests/api/search_spec.rb | 46 ++++++++++++++++++++-- 7 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/security-fix_milestones_search_api_leak.yml diff --git a/app/models/project.rb b/app/models/project.rb index ab4da61dcf8..4ca14d1c2ac 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -406,6 +406,7 @@ class Project < ApplicationRecord scope :with_builds_enabled, -> { with_feature_enabled(:builds) } scope :with_issues_enabled, -> { with_feature_enabled(:issues) } scope :with_issues_available_for_user, ->(current_user) { with_feature_available_for_user(:issues, current_user) } + scope :with_merge_requests_available_for_user, ->(current_user) { with_feature_available_for_user(:merge_requests, current_user) } scope :with_merge_requests_enabled, -> { with_feature_enabled(:merge_requests) } scope :with_remote_mirrors, -> { joins(:remote_mirrors).where(remote_mirrors: { enabled: true }).distinct } @@ -596,6 +597,17 @@ class Project < ApplicationRecord def group_ids joins(:namespace).where(namespaces: { type: 'Group' }).select(:namespace_id) end + + # Returns ids of projects with milestones available for given user + # + # Used on queries to find milestones which user can see + # For example: Milestone.where(project_id: ids_with_milestone_available_for(user)) + def ids_with_milestone_available_for(user) + with_issues_enabled = with_issues_available_for_user(user).select(:id) + with_merge_requests_enabled = with_merge_requests_available_for_user(user).select(:id) + + from_union([with_issues_enabled, with_merge_requests_enabled]).select(:id) + end end def all_pipelines diff --git a/changelogs/unreleased/security-fix_milestones_search_api_leak.yml b/changelogs/unreleased/security-fix_milestones_search_api_leak.yml new file mode 100644 index 00000000000..5691550b602 --- /dev/null +++ b/changelogs/unreleased/security-fix_milestones_search_api_leak.yml @@ -0,0 +1,5 @@ +--- +title: 'Resolve: Milestones leaked via search API' +merge_request: +author: +type: security diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb index 78337518988..0f3b97e2317 100644 --- a/lib/gitlab/project_search_results.rb +++ b/lib/gitlab/project_search_results.rb @@ -138,6 +138,12 @@ module Gitlab project end + def filter_milestones_by_project(milestones) + return Milestone.none unless Ability.allowed?(@current_user, :read_milestone, @project) + + milestones.where(project_id: project.id) # rubocop: disable CodeReuse/ActiveRecord + end + def repository_project_ref @repository_project_ref ||= repository_ref || project.default_branch end diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index 4a097a00101..7c1e6b1baff 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -103,9 +103,11 @@ module Gitlab # rubocop: disable CodeReuse/ActiveRecord def milestones - milestones = Milestone.where(project_id: project_ids_relation) - milestones = milestones.search(query) - milestones.reorder('milestones.updated_at DESC') + milestones = Milestone.search(query) + + milestones = filter_milestones_by_project(milestones) + + milestones.reorder('updated_at DESC') end # rubocop: enable CodeReuse/ActiveRecord @@ -123,6 +125,26 @@ module Gitlab 'projects' end + # Filter milestones by authorized projects. + # For performance reasons project_id is being plucked + # to be used on a smaller query. + # + # rubocop: disable CodeReuse/ActiveRecord + def filter_milestones_by_project(milestones) + project_ids = + milestones.where(project_id: project_ids_relation) + .select(:project_id).distinct + .pluck(:project_id) + + return Milestone.none if project_ids.nil? + + authorized_project_ids_relation = + Project.where(id: project_ids).ids_with_milestone_available_for(current_user) + + milestones.where(project_id: authorized_project_ids_relation) + end + # rubocop: enable CodeReuse/ActiveRecord + # rubocop: disable CodeReuse/ActiveRecord def project_ids_relation limit_projects.select(:id).reorder(nil) diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb index 312aa3be490..3d27156b356 100644 --- a/spec/lib/gitlab/search_results_spec.rb +++ b/spec/lib/gitlab/search_results_spec.rb @@ -256,4 +256,28 @@ describe Gitlab::SearchResults do expect(results.objects('merge_requests')).not_to include merge_request end + + context 'milestones' do + it 'returns correct set of milestones' do + private_project_1 = create(:project, :private) + private_project_2 = create(:project, :private) + internal_project = create(:project, :internal) + public_project_1 = create(:project, :public) + public_project_2 = create(:project, :public, :issues_disabled, :merge_requests_disabled) + private_project_1.add_developer(user) + # milestones that should not be visible + create(:milestone, project: private_project_2, title: 'Private project without access milestone') + create(:milestone, project: public_project_2, title: 'Public project with milestones disabled milestone') + # milestones that should be visible + milestone_1 = create(:milestone, project: private_project_1, title: 'Private project with access milestone', state: 'closed') + milestone_2 = create(:milestone, project: internal_project, title: 'Internal project milestone') + milestone_3 = create(:milestone, project: public_project_1, title: 'Public project with milestones enabled milestone') + # Global search scope takes user authorized projects, internal projects and public projects. + limit_projects = ProjectsFinder.new(current_user: user).execute + + milestones = described_class.new(user, limit_projects, 'milestone').objects('milestones') + + expect(milestones).to match_array([milestone_1, milestone_2, milestone_3]) + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 425096d7e80..ba2411d0b95 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3170,6 +3170,23 @@ describe Project do end end + describe '.ids_with_milestone_available_for' do + let!(:user) { create(:user) } + + it 'returns project ids with milestones available for user' do + project_1 = create(:project, :public, :merge_requests_disabled, :issues_disabled) + project_2 = create(:project, :public, :merge_requests_disabled) + project_3 = create(:project, :public, :issues_disabled) + project_4 = create(:project, :public) + project_4.project_feature.update(issues_access_level: ProjectFeature::PRIVATE, merge_requests_access_level: ProjectFeature::PRIVATE ) + + project_ids = described_class.ids_with_milestone_available_for(user).pluck(:id) + + expect(project_ids).to include(project_2.id, project_3.id) + expect(project_ids).not_to include(project_1.id, project_4.id) + end + end + describe '.with_feature_available_for_user' do let(:user) { create(:user) } let(:feature) { MergeRequest } diff --git a/spec/requests/api/search_spec.rb b/spec/requests/api/search_spec.rb index 49672591b3b..42e0efa10b7 100644 --- a/spec/requests/api/search_spec.rb +++ b/spec/requests/api/search_spec.rb @@ -70,11 +70,30 @@ describe API::Search do context 'for milestones scope' do before do create(:milestone, project: project, title: 'awesome milestone') + end + + context 'when user can read project milestones' do + before do + get api('/search', user), params: { scope: 'milestones', search: 'awesome' } + end - get api('/search', user), params: { scope: 'milestones', search: 'awesome' } + it_behaves_like 'response is correct', schema: 'public_api/v4/milestones' end - it_behaves_like 'response is correct', schema: 'public_api/v4/milestones' + context 'when user cannot read project milestones' do + before do + project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE) + project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE) + end + + it 'returns empty array' do + get api('/search', user), params: { scope: 'milestones', search: 'awesome' } + + milestones = JSON.parse(response.body) + + expect(milestones).to be_empty + end + end end context 'for users scope' do @@ -318,11 +337,30 @@ describe API::Search do context 'for milestones scope' do before do create(:milestone, project: project, title: 'awesome milestone') + end + + context 'when user can read milestones' do + before do + get api("/projects/#{project.id}/search", user), params: { scope: 'milestones', search: 'awesome' } + end - get api("/projects/#{project.id}/search", user), params: { scope: 'milestones', search: 'awesome' } + it_behaves_like 'response is correct', schema: 'public_api/v4/milestones' end - it_behaves_like 'response is correct', schema: 'public_api/v4/milestones' + context 'when user cannot read project milestones' do + before do + project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE) + project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE) + end + + it 'returns empty array' do + get api("/projects/#{project.id}/search", user), params: { scope: 'milestones', search: 'awesome' } + + milestones = JSON.parse(response.body) + + expect(milestones).to be_empty + end + end end context 'for users scope' do -- cgit v1.2.1 From fab6a50f17d15d21a157d4d561f41527fa943f27 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Wed, 12 Dec 2018 14:45:55 +0000 Subject: Prevent password sign in restriction bypass --- app/controllers/sessions_controller.rb | 9 ++++++ .../security-jej-prevent-web-sign-in-bypass.yml | 5 ++++ spec/controllers/sessions_controller_spec.rb | 34 +++++++++++++++++++++- 3 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-jej-prevent-web-sign-in-bypass.yml diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 6fea61cf45d..a841859621e 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -18,6 +18,7 @@ class SessionsController < Devise::SessionsController prepend_before_action :store_redirect_uri, only: [:new] prepend_before_action :ldap_servers, only: [:new, :create] prepend_before_action :require_no_authentication_without_flash, only: [:new, :create] + prepend_before_action :ensure_password_authentication_enabled!, if: :password_based_login?, only: [:create] before_action :auto_sign_in_with_provider, only: [:new] before_action :load_recaptcha @@ -138,6 +139,14 @@ class SessionsController < Devise::SessionsController end # rubocop: enable CodeReuse/ActiveRecord + def ensure_password_authentication_enabled! + render_403 unless Gitlab::CurrentSettings.password_authentication_enabled_for_web? + end + + def password_based_login? + user_params[:login].present? || user_params[:password].present? + end + def user_params params.require(:user).permit(:login, :password, :remember_me, :otp_attempt, :device_response) end diff --git a/changelogs/unreleased/security-jej-prevent-web-sign-in-bypass.yml b/changelogs/unreleased/security-jej-prevent-web-sign-in-bypass.yml new file mode 100644 index 00000000000..02773fa1d7c --- /dev/null +++ b/changelogs/unreleased/security-jej-prevent-web-sign-in-bypass.yml @@ -0,0 +1,5 @@ +--- +title: Prevent bypass of restriction disabling web password sign in +merge_request: +author: +type: security diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 6bcff7f975c..9c4ddce5409 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -58,7 +58,26 @@ describe SessionsController do it 'authenticates user correctly' do post(:create, params: { user: user_params }) - expect(subject.current_user). to eq user + expect(subject.current_user).to eq user + end + + context 'with password authentication disabled' do + before do + stub_application_setting(password_authentication_enabled_for_web: false) + end + + it 'does not sign in the user' do + post(:create, params: { user: user_params }) + + expect(@request.env['warden']).not_to be_authenticated + expect(subject.current_user).to be_nil + end + + it 'returns status 403' do + post(:create, params: { user: user_params }) + + expect(response.status).to eq 403 + end end it 'creates an audit log record' do @@ -153,6 +172,19 @@ describe SessionsController do end end + context 'with password authentication disabled' do + before do + stub_application_setting(password_authentication_enabled_for_web: false) + end + + it 'allows 2FA stage of non-password login' do + authenticate_2fa(otp_attempt: user.current_otp) + + expect(@request.env['warden']).to be_authenticated + expect(subject.current_user).to eq user + end + end + ## # See #14900 issue # -- cgit v1.2.1 From 3d4821a8e76d49b388b218824714d3bcb8c54dbf Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Thu, 11 Apr 2019 18:26:16 +0300 Subject: Hide password on import by url form --- app/controllers/concerns/import_url_params.rb | 17 +++++++++ app/controllers/projects/imports_controller.rb | 7 +++- app/controllers/projects_controller.rb | 2 + app/views/shared/_import_form.html.haml | 29 ++++++++++++-- app/workers/concerns/project_import_options.rb | 2 +- ...y-id-leaked-password-in-import-url-frontend.yml | 5 +++ lib/gitlab/url_sanitizer.rb | 4 ++ locale/gitlab.pot | 9 +++++ .../controllers/concerns/import_url_params_spec.rb | 44 ++++++++++++++++++++++ .../projects/imports_controller_spec.rb | 15 ++++++++ spec/lib/gitlab/url_sanitizer_spec.rb | 34 +++++++++++++++++ 11 files changed, 161 insertions(+), 7 deletions(-) create mode 100644 app/controllers/concerns/import_url_params.rb create mode 100644 changelogs/unreleased/security-id-leaked-password-in-import-url-frontend.yml create mode 100644 spec/controllers/concerns/import_url_params_spec.rb diff --git a/app/controllers/concerns/import_url_params.rb b/app/controllers/concerns/import_url_params.rb new file mode 100644 index 00000000000..765654ca2cb --- /dev/null +++ b/app/controllers/concerns/import_url_params.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module ImportUrlParams + def import_url_params + { import_url: import_params_to_full_url(params[:project]) } + end + + def import_params_to_full_url(params) + Gitlab::UrlSanitizer.new( + params[:import_url], + credentials: { + user: params[:import_url_user], + password: params[:import_url_password] + } + ).full_url + end +end diff --git a/app/controllers/projects/imports_controller.rb b/app/controllers/projects/imports_controller.rb index 4640be015de..afbf9fd7720 100644 --- a/app/controllers/projects/imports_controller.rb +++ b/app/controllers/projects/imports_controller.rb @@ -2,6 +2,7 @@ class Projects::ImportsController < Projects::ApplicationController include ContinueParams + include ImportUrlParams # Authorize before_action :authorize_admin_project! @@ -67,10 +68,12 @@ class Projects::ImportsController < Projects::ApplicationController end def import_params_attributes - [:import_url] + [] end def import_params - params.require(:project).permit(import_params_attributes) + params.require(:project) + .permit(import_params_attributes) + .merge(import_url_params) end end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index e88c46144ef..12db493978b 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -7,6 +7,7 @@ class ProjectsController < Projects::ApplicationController include PreviewMarkdown include SendFileUpload include RecordUserLastActivity + include ImportUrlParams prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) } @@ -333,6 +334,7 @@ class ProjectsController < Projects::ApplicationController def project_params(attributes: []) params.require(:project) .permit(project_params_attributes + attributes) + .merge(import_url_params) end def project_params_attributes diff --git a/app/views/shared/_import_form.html.haml b/app/views/shared/_import_form.html.haml index 3ee713cf499..75fec53b29c 100644 --- a/app/views/shared/_import_form.html.haml +++ b/app/views/shared/_import_form.html.haml @@ -1,11 +1,32 @@ - ci_cd_only = local_assigns.fetch(:ci_cd_only, false) +- import_url = Gitlab::UrlSanitizer.new(f.object.import_url) .form-group.import-url-data - = f.label :import_url, class: 'label-bold' do - %span - = _('Git repository URL') + .form-group + = f.label :import_url, class: 'label-bold' do + %span + = _('Git repository URL') + = f.text_field :import_url, value: import_url.sanitized_url, + autocomplete: 'off', class: 'form-control', placeholder: 'https://gitlab.company.com/group/project.git', required: true - = f.text_field :import_url, autocomplete: 'off', class: 'form-control', placeholder: 'https://username:password@gitlab.company.com/group/project.git', required: true + .form-group#import_url_auth_method + = label :import_url_auth_method, class: 'label-bold' do + %span + = _('Authentication method') + = select_tag :import_url_auth_method, options_for_select([[_('None'), 'none'], [_('Username and Password'), 'username-and-password']]), class: 'form-control' + + .div#import_url_auth_group{ style: 'display: none' } + .form-group + = f.label :import_url_user, class: 'label-bold' do + %span + = _('Username (optional)') + = f.text_field :import_url_user, value: import_url.user, class: 'form-control', required: false, autocomplete: 'new-password' + + .form-group + = f.label :import_url_password, class: 'label-bold' do + %span + = _('Git repository password') + = f.password_field :import_url_password, class: 'form-control', required: false, autocomplete: 'new-password', placeholder: 'Basic Auth Password' .info-well.prepend-top-20 .well-segment diff --git a/app/workers/concerns/project_import_options.rb b/app/workers/concerns/project_import_options.rb index 2baf768bfd1..53b0459c48c 100644 --- a/app/workers/concerns/project_import_options.rb +++ b/app/workers/concerns/project_import_options.rb @@ -3,7 +3,7 @@ module ProjectImportOptions extend ActiveSupport::Concern - IMPORT_RETRY_COUNT = 5 + IMPORT_RETRY_COUNT = 0 included do sidekiq_options retry: IMPORT_RETRY_COUNT, status_expiration: StuckImportJobsWorker::IMPORT_JOBS_EXPIRATION diff --git a/changelogs/unreleased/security-id-leaked-password-in-import-url-frontend.yml b/changelogs/unreleased/security-id-leaked-password-in-import-url-frontend.yml new file mode 100644 index 00000000000..df636ec37fb --- /dev/null +++ b/changelogs/unreleased/security-id-leaked-password-in-import-url-frontend.yml @@ -0,0 +1,5 @@ +--- +title: Add extra fields for handling basic auth on import by url page +merge_request: +author: +type: security diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb index 880712de5fe..215454fe63c 100644 --- a/lib/gitlab/url_sanitizer.rb +++ b/lib/gitlab/url_sanitizer.rb @@ -47,6 +47,10 @@ module Gitlab @credentials ||= { user: @url.user.presence, password: @url.password.presence } end + def user + credentials[:user] + end + def full_url @full_url ||= generate_full_url.to_s end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b1fe186d96a..99e7bb5c9a0 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4584,6 +4584,9 @@ msgstr "" msgid "Git repository URL" msgstr "" +msgid "Git repository password" +msgstr "" + msgid "Git revision" msgstr "" @@ -10789,6 +10792,12 @@ msgstr "" msgid "UserProfile|Your projects can be available publicly, internally, or privately, at your choice." msgstr "" +msgid "Username (optional)" +msgstr "" + +msgid "Username and Password" +msgstr "" + msgid "Username is already taken." msgstr "" diff --git a/spec/controllers/concerns/import_url_params_spec.rb b/spec/controllers/concerns/import_url_params_spec.rb new file mode 100644 index 00000000000..fc5dfb5263f --- /dev/null +++ b/spec/controllers/concerns/import_url_params_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ImportUrlParams do + let(:import_url_params) do + controller = OpenStruct.new(params: params).extend(described_class) + controller.import_url_params + end + + context 'url and password separately provided' do + let(:params) do + ActionController::Parameters.new(project: { + import_url: 'https://url.com', + import_url_user: 'user', import_url_password: 'password' + }) + end + + describe '#import_url_params' do + it 'returns hash with import_url' do + expect(import_url_params).to eq( + import_url: "https://user:password@url.com" + ) + end + end + end + + context 'url with provided empty credentials' do + let(:params) do + ActionController::Parameters.new(project: { + import_url: 'https://user:password@url.com', + import_url_user: '', import_url_password: '' + }) + end + + describe '#import_url_params' do + it 'does not change the url' do + expect(import_url_params).to eq( + import_url: "https://user:password@url.com" + ) + end + end + end +end diff --git a/spec/controllers/projects/imports_controller_spec.rb b/spec/controllers/projects/imports_controller_spec.rb index 8d88ee7dfd6..bdc81efe3bc 100644 --- a/spec/controllers/projects/imports_controller_spec.rb +++ b/spec/controllers/projects/imports_controller_spec.rb @@ -122,4 +122,19 @@ describe Projects::ImportsController do end end end + + describe 'POST #create' do + let(:params) { { import_url: 'https://github.com/vim/vim.git', import_url_user: 'user', import_url_password: 'password' } } + let(:project) { create(:project) } + + before do + allow(RepositoryImportWorker).to receive(:perform_async) + + post :create, params: { project: params, namespace_id: project.namespace.to_param, project_id: project } + end + + it 'sets import_url to the project' do + expect(project.reload.import_url).to eq('https://user:password@github.com/vim/vim.git') + end + end end diff --git a/spec/lib/gitlab/url_sanitizer_spec.rb b/spec/lib/gitlab/url_sanitizer_spec.rb index 5861e6955a6..7242255d535 100644 --- a/spec/lib/gitlab/url_sanitizer_spec.rb +++ b/spec/lib/gitlab/url_sanitizer_spec.rb @@ -115,6 +115,40 @@ describe Gitlab::UrlSanitizer do end end + describe '#user' do + context 'credentials in hash' do + it 'overrides URL-provided user' do + sanitizer = described_class.new('http://a:b@example.com', credentials: { user: 'c', password: 'd' }) + + expect(sanitizer.user).to eq('c') + end + end + + context 'credentials in URL' do + where(:url, :user) do + 'http://foo:bar@example.com' | 'foo' + 'http://foo:bar:baz@example.com' | 'foo' + 'http://:bar@example.com' | nil + 'http://foo:@example.com' | 'foo' + 'http://foo@example.com' | 'foo' + 'http://:@example.com' | nil + 'http://@example.com' | nil + 'http://example.com' | nil + + # Other invalid URLs + nil | nil + '' | nil + 'no' | nil + end + + with_them do + subject { described_class.new(url).user } + + it { is_expected.to eq(user) } + end + end + end + describe '#full_url' do context 'credentials in hash' do where(:credentials, :userinfo) do -- cgit v1.2.1 From c7903542683eaa5427a5d30adad8550f0754bdfa Mon Sep 17 00:00:00 2001 From: Sam Bigelow Date: Sat, 13 Apr 2019 12:00:10 -0400 Subject: Handling password on import by url page --- app/views/shared/_import_form.html.haml | 20 +++++++------------- app/workers/concerns/project_import_options.rb | 2 +- locale/gitlab.pot | 11 ++++------- spec/javascripts/projects/project_new_spec.js | 14 ++++++++++++-- 4 files changed, 24 insertions(+), 23 deletions(-) diff --git a/app/views/shared/_import_form.html.haml b/app/views/shared/_import_form.html.haml index 75fec53b29c..d0f9374e832 100644 --- a/app/views/shared/_import_form.html.haml +++ b/app/views/shared/_import_form.html.haml @@ -1,7 +1,7 @@ - ci_cd_only = local_assigns.fetch(:ci_cd_only, false) - import_url = Gitlab::UrlSanitizer.new(f.object.import_url) -.form-group.import-url-data +.import-url-data .form-group = f.label :import_url, class: 'label-bold' do %span @@ -9,24 +9,18 @@ = f.text_field :import_url, value: import_url.sanitized_url, autocomplete: 'off', class: 'form-control', placeholder: 'https://gitlab.company.com/group/project.git', required: true - .form-group#import_url_auth_method - = label :import_url_auth_method, class: 'label-bold' do - %span - = _('Authentication method') - = select_tag :import_url_auth_method, options_for_select([[_('None'), 'none'], [_('Username and Password'), 'username-and-password']]), class: 'form-control' - - .div#import_url_auth_group{ style: 'display: none' } - .form-group + .row + .form-group.col-md-6 = f.label :import_url_user, class: 'label-bold' do %span = _('Username (optional)') = f.text_field :import_url_user, value: import_url.user, class: 'form-control', required: false, autocomplete: 'new-password' - .form-group + .form-group.col-md-6 = f.label :import_url_password, class: 'label-bold' do %span - = _('Git repository password') - = f.password_field :import_url_password, class: 'form-control', required: false, autocomplete: 'new-password', placeholder: 'Basic Auth Password' + = _('Password (optional)') + = f.password_field :import_url_password, class: 'form-control', required: false, autocomplete: 'new-password' .info-well.prepend-top-20 .well-segment @@ -34,7 +28,7 @@ %li = _('The repository must be accessible over http://, https:// or git://.').html_safe %li - = _('If your HTTP repository is not publicly accessible, add authentication information to the URL: https://username:password@gitlab.company.com/group/project.git.').html_safe + = _('If your HTTP repository is not publicly accessible, add your credentials.') %li = import_will_timeout_message(ci_cd_only) %li diff --git a/app/workers/concerns/project_import_options.rb b/app/workers/concerns/project_import_options.rb index 53b0459c48c..2baf768bfd1 100644 --- a/app/workers/concerns/project_import_options.rb +++ b/app/workers/concerns/project_import_options.rb @@ -3,7 +3,7 @@ module ProjectImportOptions extend ActiveSupport::Concern - IMPORT_RETRY_COUNT = 0 + IMPORT_RETRY_COUNT = 5 included do sidekiq_options retry: IMPORT_RETRY_COUNT, status_expiration: StuckImportJobsWorker::IMPORT_JOBS_EXPIRATION diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 99e7bb5c9a0..6ba55560b4c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4584,9 +4584,6 @@ msgstr "" msgid "Git repository URL" msgstr "" -msgid "Git repository password" -msgstr "" - msgid "Git revision" msgstr "" @@ -5036,7 +5033,7 @@ msgstr "" msgid "If this was a mistake you can leave the %{source_type}." msgstr "" -msgid "If your HTTP repository is not publicly accessible, add authentication information to the URL: https://username:password@gitlab.company.com/group/project.git." +msgid "If your HTTP repository is not publicly accessible, add your credentials." msgstr "" msgid "ImageDiffViewer|2-up" @@ -6772,6 +6769,9 @@ msgstr "" msgid "Password" msgstr "" +msgid "Password (optional)" +msgstr "" + msgid "Password authentication is unavailable." msgstr "" @@ -10795,9 +10795,6 @@ msgstr "" msgid "Username (optional)" msgstr "" -msgid "Username and Password" -msgstr "" - msgid "Username is already taken." msgstr "" diff --git a/spec/javascripts/projects/project_new_spec.js b/spec/javascripts/projects/project_new_spec.js index b61e0ac872f..106a3ba94e4 100644 --- a/spec/javascripts/projects/project_new_spec.js +++ b/spec/javascripts/projects/project_new_spec.js @@ -10,7 +10,17 @@ describe('New Project', () => { setFixtures(`
- +
+ +
+
+
+ +
+
+ +
+
@@ -119,7 +129,7 @@ describe('New Project', () => { }); it('changes project path for HTTPS URL in $projectImportUrl', () => { - $projectImportUrl.val('https://username:password@gitlab.company.com/group/project.git'); + $projectImportUrl.val('https://gitlab.company.com/group/project.git'); projectNew.deriveProjectPathFromUrl($projectImportUrl); -- cgit v1.2.1 From 88241108c4d9807e5c312b11c910b3072bc6f120 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Thu, 30 May 2019 12:51:04 +0000 Subject: Update CHANGELOG.md for 11.9.12 [ci skip] --- CHANGELOG.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88521222b8a..c31af2488f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -478,6 +478,24 @@ entry. - Removes EE differences for environment_item.vue. +## 11.9.12 (2019-05-30) + +### Security (12 changes, 1 of them is from the community) + +- Protect Gitlab::HTTP against DNS rebinding attack. +- Fix project visibility level validation. (Peter Marko) +- Update Knative version. +- Add DNS rebinding protection settings. +- Prevent XSS injection in note imports. +- Prevent invalid branch for merge request. +- Filter relative links in wiki for XSS. +- Fix confidential issue label disclosure on milestone view. +- Fix url redaction for issue links. +- Resolve: Milestones leaked via search API. +- Prevent bypass of restriction disabling web password sign in. +- Hide confidential issue title on unsubscribe for anonymous users. + + ## 11.9.10 (2019-04-26) ### Security (5 changes) -- cgit v1.2.1 From a9bcddee4c2653cbf2254d893299393e3778e7df Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 21 Apr 2019 12:03:26 +0200 Subject: Protect Gitlab::HTTP against DNS rebinding attack Gitlab::HTTP now resolves the hostname only once, verifies the IP is not blocked, and then uses the same IP to perform the actual request, while passing the original hostname in the `Host` header and SSL SNI field. --- .../unreleased/dm-http-hostname-override.yml | 5 ++ config/initializers/hipchat_client_patch.rb | 6 +- config/initializers/http_hostname_override.rb | 49 ++++++++++++ lib/gitlab/http.rb | 2 +- lib/gitlab/http_connection_adapter.rb | 37 +++++++++ lib/gitlab/proxy_http_connection_adapter.rb | 36 --------- lib/gitlab/url_blocker.rb | 61 +++++++++++---- .../projects/ci/lints_controller_spec.rb | 4 +- .../gitlab/ci/config/external/file/remote_spec.rb | 14 ++-- spec/lib/gitlab/ci/config/external/mapper_spec.rb | 4 +- .../gitlab/ci/config/external/processor_spec.rb | 20 +++-- spec/lib/gitlab/ci/config_spec.rb | 5 +- spec/lib/gitlab/ci/yaml_processor_spec.rb | 4 +- spec/lib/gitlab/http_connection_adapter_spec.rb | 88 ++++++++++++++++++++++ spec/lib/gitlab/http_spec.rb | 28 ++++++- .../web_upload_strategy_spec.rb | 4 +- spec/lib/gitlab/url_blocker_spec.rb | 48 +++++++++++- spec/lib/mattermost/session_spec.rb | 7 +- .../project_services/assembla_service_spec.rb | 6 +- .../models/project_services/bamboo_service_spec.rb | 3 +- .../project_services/buildkite_service_spec.rb | 10 +-- .../project_services/campfire_service_spec.rb | 24 +++--- .../pivotaltracker_service_spec.rb | 10 ++- .../project_services/pushover_service_spec.rb | 6 +- .../project_services/teamcity_service_spec.rb | 3 +- spec/requests/api/system_hooks_spec.rb | 8 +- .../lfs_pointers/lfs_download_service_spec.rb | 19 +++-- spec/services/submit_usage_ping_service_spec.rb | 4 +- spec/services/web_hook_service_spec.rb | 10 ++- spec/support/helpers/stub_requests.rb | 40 ++++++++++ 30 files changed, 447 insertions(+), 118 deletions(-) create mode 100644 changelogs/unreleased/dm-http-hostname-override.yml create mode 100644 config/initializers/http_hostname_override.rb create mode 100644 lib/gitlab/http_connection_adapter.rb delete mode 100644 lib/gitlab/proxy_http_connection_adapter.rb create mode 100644 spec/lib/gitlab/http_connection_adapter_spec.rb create mode 100644 spec/support/helpers/stub_requests.rb diff --git a/changelogs/unreleased/dm-http-hostname-override.yml b/changelogs/unreleased/dm-http-hostname-override.yml new file mode 100644 index 00000000000..f84f36a0010 --- /dev/null +++ b/changelogs/unreleased/dm-http-hostname-override.yml @@ -0,0 +1,5 @@ +--- +title: Protect Gitlab::HTTP against DNS rebinding attack +merge_request: +author: +type: security diff --git a/config/initializers/hipchat_client_patch.rb b/config/initializers/hipchat_client_patch.rb index 1879ecb15fb..51bd48af320 100644 --- a/config/initializers/hipchat_client_patch.rb +++ b/config/initializers/hipchat_client_patch.rb @@ -2,14 +2,14 @@ # This monkey patches the HTTParty used in https://github.com/hipchat/hipchat-rb. module HipChat class Client - connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter + connection_adapter ::Gitlab::HTTPConnectionAdapter end class Room - connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter + connection_adapter ::Gitlab::HTTPConnectionAdapter end class User - connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter + connection_adapter ::Gitlab::HTTPConnectionAdapter end end diff --git a/config/initializers/http_hostname_override.rb b/config/initializers/http_hostname_override.rb new file mode 100644 index 00000000000..58dd380326f --- /dev/null +++ b/config/initializers/http_hostname_override.rb @@ -0,0 +1,49 @@ +# This override allows passing `@hostname_override` to the SNI protocol, +# which is used to lookup the correct SSL certificate in the +# request handshake process. +# +# Given we've forced the HTTP request to be sent to the resolved +# IP address in a few scenarios (e.g.: `Gitlab::HTTP` through +# `Gitlab::UrlBlocker.validate!`), we need to provide the _original_ +# hostname via SNI in order to have a clean connection setup. +# +# This is ultimately needed in order to avoid DNS rebinding attacks +# through HTTP requests. +# +class OpenSSL::SSL::SSLContext + attr_accessor :hostname_override +end + +class OpenSSL::SSL::SSLSocket + module HostnameOverride + # rubocop: disable Gitlab/ModuleWithInstanceVariables + def hostname=(hostname) + super(@context.hostname_override || hostname) + end + + def post_connection_check(hostname) + super(@context.hostname_override || hostname) + end + # rubocop: enable Gitlab/ModuleWithInstanceVariables + end + + prepend HostnameOverride +end + +class Net::HTTP + attr_accessor :hostname_override + SSL_IVNAMES << :@hostname_override + SSL_ATTRIBUTES << :hostname_override + + module HostnameOverride + def addr_port + return super unless hostname_override + + addr = hostname_override + default_port = use_ssl? ? Net::HTTP.https_default_port : Net::HTTP.http_default_port + default_port == port ? addr : "#{addr}:#{port}" + end + end + + prepend HostnameOverride +end diff --git a/lib/gitlab/http.rb b/lib/gitlab/http.rb index bcd9e2be35f..55ed7e7e837 100644 --- a/lib/gitlab/http.rb +++ b/lib/gitlab/http.rb @@ -11,7 +11,7 @@ module Gitlab include HTTParty # rubocop:disable Gitlab/HTTParty - connection_adapter ProxyHTTPConnectionAdapter + connection_adapter HTTPConnectionAdapter def self.perform_request(http_method, path, options, &block) super diff --git a/lib/gitlab/http_connection_adapter.rb b/lib/gitlab/http_connection_adapter.rb new file mode 100644 index 00000000000..9ccf0653903 --- /dev/null +++ b/lib/gitlab/http_connection_adapter.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +# This class is part of the Gitlab::HTTP wrapper. Depending on the value +# of the global setting allow_local_requests_from_hooks_and_services this adapter +# will allow/block connection to internal IPs and/or urls. +# +# This functionality can be overridden by providing the setting the option +# allow_local_requests = true in the request. For example: +# Gitlab::HTTP.get('http://www.gitlab.com', allow_local_requests: true) +# +# This option will take precedence over the global setting. +module Gitlab + class HTTPConnectionAdapter < HTTParty::ConnectionAdapter + def connection + begin + @uri, hostname = Gitlab::UrlBlocker.validate!(uri, allow_local_network: allow_local_requests?, + allow_localhost: allow_local_requests?) + rescue Gitlab::UrlBlocker::BlockedUrlError => e + raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}" + end + + super.tap do |http| + http.hostname_override = hostname if hostname + end + end + + private + + def allow_local_requests? + options.fetch(:allow_local_requests, allow_settings_local_requests?) + end + + def allow_settings_local_requests? + Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? + end + end +end diff --git a/lib/gitlab/proxy_http_connection_adapter.rb b/lib/gitlab/proxy_http_connection_adapter.rb deleted file mode 100644 index a64cb47e77e..00000000000 --- a/lib/gitlab/proxy_http_connection_adapter.rb +++ /dev/null @@ -1,36 +0,0 @@ -# frozen_string_literal: true - -# This class is part of the Gitlab::HTTP wrapper. Depending on the value -# of the global setting allow_local_requests_from_hooks_and_services this adapter -# will allow/block connection to internal IPs and/or urls. -# -# This functionality can be overridden by providing the setting the option -# allow_local_requests = true in the request. For example: -# Gitlab::HTTP.get('http://www.gitlab.com', allow_local_requests: true) -# -# This option will take precedence over the global setting. -module Gitlab - class ProxyHTTPConnectionAdapter < HTTParty::ConnectionAdapter - def connection - unless allow_local_requests? - begin - Gitlab::UrlBlocker.validate!(uri, allow_local_network: false) - rescue Gitlab::UrlBlocker::BlockedUrlError => e - raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}" - end - end - - super - end - - private - - def allow_local_requests? - options.fetch(:allow_local_requests, allow_settings_local_requests?) - end - - def allow_settings_local_requests? - Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? - end - end -end diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 641ba70ef83..81935f2b052 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -8,38 +8,52 @@ module Gitlab BlockedUrlError = Class.new(StandardError) class << self - def validate!(url, ports: [], schemes: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false) - return true if url.nil? + # Validates the given url according to the constraints specified by arguments. + # + # ports - Raises error if the given URL port does is not between given ports. + # allow_localhost - Raises error if URL resolves to a localhost IP address and argument is true. + # allow_local_network - Raises error if URL resolves to a link-local address and argument is true. + # ascii_only - Raises error if URL has unicode characters and argument is true. + # enforce_user - Raises error if URL user doesn't start with alphanumeric characters and argument is true. + # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true. + # + # Returns an array with [, ]. + def validate!(url, ports: [], schemes: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false) # rubocop:disable Metrics/CyclomaticComplexity + return [nil, nil] if url.nil? # Param url can be a string, URI or Addressable::URI uri = parse_url(url) validate_html_tags!(uri) if enforce_sanitization - # Allow imports from the GitLab instance itself but only from the configured ports - return true if internal?(uri) - + hostname = uri.hostname port = get_port(uri) - validate_scheme!(uri.scheme, schemes) - validate_port!(port, ports) if ports.any? - validate_user!(uri.user) if enforce_user - validate_hostname!(uri.hostname) - validate_unicode_restriction!(uri) if ascii_only + + unless internal?(uri) + validate_scheme!(uri.scheme, schemes) + validate_port!(port, ports) if ports.any? + validate_user!(uri.user) if enforce_user + validate_hostname!(hostname) + validate_unicode_restriction!(uri) if ascii_only + end begin - addrs_info = Addrinfo.getaddrinfo(uri.hostname, port, nil, :STREAM).map do |addr| + addrs_info = Addrinfo.getaddrinfo(hostname, port, nil, :STREAM).map do |addr| addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr end rescue SocketError - return true + return [uri, nil] end + # Allow url from the GitLab instance itself but only for the configured hostname and ports + return enforce_uri_hostname(addrs_info, uri, hostname) if internal?(uri) + validate_localhost!(addrs_info) unless allow_localhost validate_loopback!(addrs_info) unless allow_localhost validate_local_network!(addrs_info) unless allow_local_network validate_link_local!(addrs_info) unless allow_local_network - true + enforce_uri_hostname(addrs_info, uri, hostname) end def blocked_url?(*args) @@ -52,6 +66,27 @@ module Gitlab private + # Returns the given URI with IP address as hostname and the original hostname respectively + # in an Array. + # + # It checks whether the resolved IP address matches with the hostname. If not, it changes + # the hostname to the resolved IP address. + # + # The original hostname is used to validate the SSL, given in that scenario + # we'll be making the request to the IP address, instead of using the hostname. + def enforce_uri_hostname(addrs_info, uri, hostname) + address = addrs_info.first + ip_address = address&.ip_address + + if ip_address && ip_address != hostname + uri = uri.dup + uri.hostname = ip_address + return [uri, hostname] + end + + [uri, nil] + end + def get_port(uri) uri.port || uri.default_port end diff --git a/spec/controllers/projects/ci/lints_controller_spec.rb b/spec/controllers/projects/ci/lints_controller_spec.rb index 323a32575af..cc6ac83ca38 100644 --- a/spec/controllers/projects/ci/lints_controller_spec.rb +++ b/spec/controllers/projects/ci/lints_controller_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe Projects::Ci::LintsController do + include StubRequests + let(:project) { create(:project, :repository) } let(:user) { create(:user) } @@ -70,7 +72,7 @@ describe Projects::Ci::LintsController do context 'with a valid gitlab-ci.yml' do before do - WebMock.stub_request(:get, remote_file_path).to_return(body: remote_file_content) + stub_full_request(remote_file_path).to_return(body: remote_file_content) project.add_developer(user) post :create, params: { namespace_id: project.namespace, project_id: project, content: content } diff --git a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb index d8a61618e77..46d68097fff 100644 --- a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe Gitlab::Ci::Config::External::File::Remote do + include StubRequests + let(:context) { described_class::Context.new(nil, '12345', nil, Set.new) } let(:params) { { remote: location } } let(:remote_file) { described_class.new(params, context) } @@ -46,7 +48,7 @@ describe Gitlab::Ci::Config::External::File::Remote do describe "#valid?" do context 'when is a valid remote url' do before do - WebMock.stub_request(:get, location).to_return(body: remote_file_content) + stub_full_request(location).to_return(body: remote_file_content) end it 'returns true' do @@ -92,7 +94,7 @@ describe Gitlab::Ci::Config::External::File::Remote do describe "#content" do context 'with a valid remote file' do before do - WebMock.stub_request(:get, location).to_return(body: remote_file_content) + stub_full_request(location).to_return(body: remote_file_content) end it 'returns the content of the file' do @@ -114,7 +116,7 @@ describe Gitlab::Ci::Config::External::File::Remote do let(:location) { 'https://asdasdasdaj48ggerexample.com' } before do - WebMock.stub_request(:get, location).to_raise(SocketError.new('Some HTTP error')) + stub_full_request(location).to_raise(SocketError.new('Some HTTP error')) end it 'is nil' do @@ -144,7 +146,7 @@ describe Gitlab::Ci::Config::External::File::Remote do context 'when timeout error has been raised' do before do - WebMock.stub_request(:get, location).to_timeout + stub_full_request(location).to_timeout end it 'returns error message about a timeout' do @@ -154,7 +156,7 @@ describe Gitlab::Ci::Config::External::File::Remote do context 'when HTTP error has been raised' do before do - WebMock.stub_request(:get, location).to_raise(Gitlab::HTTP::Error) + stub_full_request(location).to_raise(Gitlab::HTTP::Error) end it 'returns error message about a HTTP error' do @@ -164,7 +166,7 @@ describe Gitlab::Ci::Config::External::File::Remote do context 'when response has 404 status' do before do - WebMock.stub_request(:get, location).to_return(body: remote_file_content, status: 404) + stub_full_request(location).to_return(body: remote_file_content, status: 404) end it 'returns error message about a timeout' do diff --git a/spec/lib/gitlab/ci/config/external/mapper_spec.rb b/spec/lib/gitlab/ci/config/external/mapper_spec.rb index 136974569de..e068b786b02 100644 --- a/spec/lib/gitlab/ci/config/external/mapper_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe Gitlab::Ci::Config::External::Mapper do + include StubRequests + set(:project) { create(:project, :repository) } set(:user) { create(:user) } @@ -18,7 +20,7 @@ describe Gitlab::Ci::Config::External::Mapper do end before do - WebMock.stub_request(:get, remote_url).to_return(body: file_content) + stub_full_request(remote_url).to_return(body: file_content) end describe '#process' do diff --git a/spec/lib/gitlab/ci/config/external/processor_spec.rb b/spec/lib/gitlab/ci/config/external/processor_spec.rb index 0f58a4f1d44..856187371e1 100644 --- a/spec/lib/gitlab/ci/config/external/processor_spec.rb +++ b/spec/lib/gitlab/ci/config/external/processor_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe Gitlab::Ci::Config::External::Processor do + include StubRequests + set(:project) { create(:project, :repository) } set(:another_project) { create(:project, :repository) } set(:user) { create(:user) } @@ -42,7 +44,7 @@ describe Gitlab::Ci::Config::External::Processor do let(:values) { { include: remote_file, image: 'ruby:2.2' } } before do - WebMock.stub_request(:get, remote_file).to_raise(SocketError.new('Some HTTP error')) + stub_full_request(remote_file).and_raise(SocketError.new('Some HTTP error')) end it 'raises an error' do @@ -75,7 +77,7 @@ describe Gitlab::Ci::Config::External::Processor do end before do - WebMock.stub_request(:get, remote_file).to_return(body: external_file_content) + stub_full_request(remote_file).to_return(body: external_file_content) end it 'appends the file to the values' do @@ -145,7 +147,7 @@ describe Gitlab::Ci::Config::External::Processor do allow_any_instance_of(Gitlab::Ci::Config::External::File::Local) .to receive(:fetch_local_content).and_return(local_file_content) - WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content) + stub_full_request(remote_file).to_return(body: remote_file_content) end it 'appends the files to the values' do @@ -191,7 +193,8 @@ describe Gitlab::Ci::Config::External::Processor do end it 'takes precedence' do - WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content) + stub_full_request(remote_file).to_return(body: remote_file_content) + expect(processor.perform[:image]).to eq('ruby:2.2') end end @@ -231,7 +234,8 @@ describe Gitlab::Ci::Config::External::Processor do HEREDOC end - WebMock.stub_request(:get, 'http://my.domain.com/config.yml').to_return(body: 'remote_build: { script: echo Hello World }') + stub_full_request('http://my.domain.com/config.yml') + .to_return(body: 'remote_build: { script: echo Hello World }') end context 'when project is public' do @@ -273,8 +277,10 @@ describe Gitlab::Ci::Config::External::Processor do context 'when config includes an external configuration file via SSL web request' do before do - stub_request(:get, 'https://sha256.badssl.com/fake.yml').to_return(body: 'image: ruby:2.6', status: 200) - stub_request(:get, 'https://self-signed.badssl.com/fake.yml') + stub_full_request('https://sha256.badssl.com/fake.yml', ip_address: '8.8.8.8') + .to_return(body: 'image: ruby:2.6', status: 200) + + stub_full_request('https://self-signed.badssl.com/fake.yml', ip_address: '8.8.8.9') .to_raise(OpenSSL::SSL::SSLError.new('SSL_connect returned=1 errno=0 state=error: certificate verify failed (self signed certificate)')) end diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index 092e9f242b7..7f336ee853e 100644 --- a/spec/lib/gitlab/ci/config_spec.rb +++ b/spec/lib/gitlab/ci/config_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Gitlab::Ci::Config do + include StubRequests + set(:user) { create(:user) } let(:config) do @@ -216,8 +218,7 @@ describe Gitlab::Ci::Config do end before do - WebMock.stub_request(:get, remote_location) - .to_return(body: remote_file_content) + stub_full_request(remote_location).to_return(body: remote_file_content) allow(project.repository) .to receive(:blob_data_at).and_return(local_file_content) diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index 0d998d89d73..29276d5b686 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' module Gitlab module Ci describe YamlProcessor do + include StubRequests + subject { described_class.new(config, user: nil) } describe '#build_attributes' do @@ -648,7 +650,7 @@ module Gitlab end before do - WebMock.stub_request(:get, 'https://gitlab.com/awesome-project/raw/master/.before-script-template.yml') + stub_full_request('https://gitlab.com/awesome-project/raw/master/.before-script-template.yml') .to_return( status: 200, headers: { 'Content-Type' => 'application/json' }, diff --git a/spec/lib/gitlab/http_connection_adapter_spec.rb b/spec/lib/gitlab/http_connection_adapter_spec.rb new file mode 100644 index 00000000000..af033be8e5b --- /dev/null +++ b/spec/lib/gitlab/http_connection_adapter_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::HTTPConnectionAdapter do + describe '#connection' do + context 'when local requests are not allowed' do + it 'sets up the connection' do + uri = URI('https://example.org') + + connection = described_class.new(uri).connection + + expect(connection).to be_a(Net::HTTP) + expect(connection.address).to eq('93.184.216.34') + expect(connection.hostname_override).to eq('example.org') + expect(connection.addr_port).to eq('example.org') + expect(connection.port).to eq(443) + end + + it 'raises error when it is a request to local address' do + uri = URI('http://172.16.0.0/12') + + expect { described_class.new(uri).connection } + .to raise_error(Gitlab::HTTP::BlockedUrlError, + "URL 'http://172.16.0.0/12' is blocked: Requests to the local network are not allowed") + end + + it 'raises error when it is a request to localhost address' do + uri = URI('http://127.0.0.1') + + expect { described_class.new(uri).connection } + .to raise_error(Gitlab::HTTP::BlockedUrlError, + "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed") + end + + context 'when port different from URL scheme is used' do + it 'sets up the addr_port accordingly' do + uri = URI('https://example.org:8080') + + connection = described_class.new(uri).connection + + expect(connection.address).to eq('93.184.216.34') + expect(connection.hostname_override).to eq('example.org') + expect(connection.addr_port).to eq('example.org:8080') + expect(connection.port).to eq(8080) + end + end + end + + context 'when local requests are allowed' do + it 'sets up the connection' do + uri = URI('https://example.org') + + connection = described_class.new(uri, allow_local_requests: true).connection + + expect(connection).to be_a(Net::HTTP) + expect(connection.address).to eq('93.184.216.34') + expect(connection.hostname_override).to eq('example.org') + expect(connection.addr_port).to eq('example.org') + expect(connection.port).to eq(443) + end + + it 'sets up the connection when it is a local network' do + uri = URI('http://172.16.0.0/12') + + connection = described_class.new(uri, allow_local_requests: true).connection + + expect(connection).to be_a(Net::HTTP) + expect(connection.address).to eq('172.16.0.0') + expect(connection.hostname_override).to be(nil) + expect(connection.addr_port).to eq('172.16.0.0') + expect(connection.port).to eq(80) + end + + it 'sets up the connection when it is localhost' do + uri = URI('http://127.0.0.1') + + connection = described_class.new(uri, allow_local_requests: true).connection + + expect(connection).to be_a(Net::HTTP) + expect(connection.address).to eq('127.0.0.1') + expect(connection.hostname_override).to be(nil) + expect(connection.addr_port).to eq('127.0.0.1') + expect(connection.port).to eq(80) + end + end + end +end diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb index 6c37c157f5d..158f77cab2c 100644 --- a/spec/lib/gitlab/http_spec.rb +++ b/spec/lib/gitlab/http_spec.rb @@ -1,6 +1,28 @@ require 'spec_helper' describe Gitlab::HTTP do + include StubRequests + + context 'when allow_local_requests' do + it 'sends the request to the correct URI' do + stub_full_request('https://example.org:8080', ip_address: '8.8.8.8').to_return(status: 200) + + described_class.get('https://example.org:8080', allow_local_requests: false) + + expect(WebMock).to have_requested(:get, 'https://8.8.8.8:8080').once + end + end + + context 'when not allow_local_requests' do + it 'sends the request to the correct URI' do + stub_full_request('https://example.org:8080') + + described_class.get('https://example.org:8080', allow_local_requests: true) + + expect(WebMock).to have_requested(:get, 'https://8.8.8.9:8080').once + end + end + describe 'allow_local_requests_from_hooks_and_services is' do before do WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success') @@ -21,6 +43,8 @@ describe Gitlab::HTTP do context 'if allow_local_requests set to true' do it 'override the global value and allow requests to localhost or private network' do + stub_full_request('http://localhost:3003') + expect { described_class.get('http://localhost:3003', allow_local_requests: true) }.not_to raise_error end end @@ -32,6 +56,8 @@ describe Gitlab::HTTP do end it 'allow requests to localhost' do + stub_full_request('http://localhost:3003') + expect { described_class.get('http://localhost:3003') }.not_to raise_error end @@ -49,7 +75,7 @@ describe Gitlab::HTTP do describe 'handle redirect loops' do before do - WebMock.stub_request(:any, "http://example.org").to_raise(HTTParty::RedirectionTooDeep.new("Redirection Too Deep")) + stub_full_request("http://example.org", method: :any).to_raise(HTTParty::RedirectionTooDeep.new("Redirection Too Deep")) end it 'handles GET requests' do diff --git a/spec/lib/gitlab/import_export/after_export_strategies/web_upload_strategy_spec.rb b/spec/lib/gitlab/import_export/after_export_strategies/web_upload_strategy_spec.rb index 7c4ac62790e..21a227335cd 100644 --- a/spec/lib/gitlab/import_export/after_export_strategies/web_upload_strategy_spec.rb +++ b/spec/lib/gitlab/import_export/after_export_strategies/web_upload_strategy_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do + include StubRequests + let(:example_url) { 'http://www.example.com' } let(:strategy) { subject.new(url: example_url, http_method: 'post') } let!(:project) { create(:project, :with_export) } @@ -35,7 +37,7 @@ describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do context 'when upload fails' do it 'stores the export error' do - stub_request(:post, example_url).to_return(status: [404, 'Page not found']) + stub_full_request(example_url, method: :post).to_return(status: [404, 'Page not found']) strategy.execute(user, project) diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 445a56ab0d8..416f8cfece9 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -2,6 +2,52 @@ require 'spec_helper' describe Gitlab::UrlBlocker do + describe '#validate!' do + context 'when URI is nil' do + let(:import_url) { nil } + + it 'returns no URI and hostname' do + uri, hostname = described_class.validate!(import_url) + + expect(uri).to be(nil) + expect(hostname).to be(nil) + end + end + + context 'when URI is internal' do + let(:import_url) { 'http://localhost' } + + it 'returns URI and no hostname' do + uri, hostname = described_class.validate!(import_url) + + expect(uri).to eq(Addressable::URI.parse('http://[::1]')) + expect(hostname).to eq('localhost') + end + end + + context 'when the URL hostname is a domain' do + let(:import_url) { 'https://example.org' } + + it 'returns URI and hostname' do + uri, hostname = described_class.validate!(import_url) + + expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34')) + expect(hostname).to eq('example.org') + end + end + + context 'when the URL hostname is an IP address' do + let(:import_url) { 'https://93.184.216.34' } + + it 'returns URI and no hostname' do + uri, hostname = described_class.validate!(import_url) + + expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34')) + expect(hostname).to be(nil) + end + end + end + describe '#blocked_url?' do let(:ports) { Project::VALID_IMPORT_PORTS } @@ -208,7 +254,7 @@ describe Gitlab::UrlBlocker do end def stub_domain_resolv(domain, ip) - address = double(ip_address: ip, ipv4_private?: true, ipv6_link_local?: false, ipv4_loopback?: false, ipv6_loopback?: false) + address = double(ip_address: ip, ipv4_private?: true, ipv6_link_local?: false, ipv4_loopback?: false, ipv6_loopback?: false, ipv4?: false) allow(Addrinfo).to receive(:getaddrinfo).with(domain, any_args).and_return([address]) allow(address).to receive(:ipv6_v4mapped?).and_return(false) end diff --git a/spec/lib/mattermost/session_spec.rb b/spec/lib/mattermost/session_spec.rb index 77fea5b2d24..346455067a7 100644 --- a/spec/lib/mattermost/session_spec.rb +++ b/spec/lib/mattermost/session_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' describe Mattermost::Session, type: :request do include ExclusiveLeaseHelpers + include StubRequests let(:user) { create(:user) } @@ -24,7 +25,7 @@ describe Mattermost::Session, type: :request do let(:location) { 'http://location.tld' } let(:cookie_header) {'MMOAUTH=taskik8az7rq8k6rkpuas7htia; Path=/;'} let!(:stub) do - WebMock.stub_request(:get, "#{mattermost_url}/oauth/gitlab/login") + stub_full_request("#{mattermost_url}/oauth/gitlab/login") .to_return(headers: { 'location' => location, 'Set-Cookie' => cookie_header }, status: 302) end @@ -63,7 +64,7 @@ describe Mattermost::Session, type: :request do end before do - WebMock.stub_request(:get, "#{mattermost_url}/signup/gitlab/complete") + stub_full_request("#{mattermost_url}/signup/gitlab/complete") .with(query: hash_including({ 'state' => state })) .to_return do |request| post "/oauth/token", @@ -80,7 +81,7 @@ describe Mattermost::Session, type: :request do end end - WebMock.stub_request(:post, "#{mattermost_url}/api/v4/users/logout") + stub_full_request("#{mattermost_url}/api/v4/users/logout", method: :post) .to_return(headers: { Authorization: 'token thisworksnow' }, status: 200) end diff --git a/spec/models/project_services/assembla_service_spec.rb b/spec/models/project_services/assembla_service_spec.rb index 7742e33e901..2c86c0ec7be 100644 --- a/spec/models/project_services/assembla_service_spec.rb +++ b/spec/models/project_services/assembla_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe AssemblaService do + include StubRequests + describe "Associations" do it { is_expected.to belong_to :project } it { is_expected.to have_one :service_hook } @@ -23,12 +25,12 @@ describe AssemblaService do ) @sample_data = Gitlab::DataBuilder::Push.build_sample(project, user) @api_url = 'https://atlas.assembla.com/spaces/project_name/github_tool?secret_key=verySecret' - WebMock.stub_request(:post, @api_url) + stub_full_request(@api_url, method: :post) end it "calls Assembla API" do @assembla_service.execute(@sample_data) - expect(WebMock).to have_requested(:post, @api_url).with( + expect(WebMock).to have_requested(:post, stubbed_hostname(@api_url)).with( body: /#{@sample_data[:before]}.*#{@sample_data[:after]}.*#{project.path}/ ).once end diff --git a/spec/models/project_services/bamboo_service_spec.rb b/spec/models/project_services/bamboo_service_spec.rb index 08c510f09df..65d227a17f9 100644 --- a/spec/models/project_services/bamboo_service_spec.rb +++ b/spec/models/project_services/bamboo_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe BambooService, :use_clean_rails_memory_store_caching do include ReactiveCachingHelpers + include StubRequests let(:bamboo_url) { 'http://gitlab.com/bamboo' } @@ -257,7 +258,7 @@ describe BambooService, :use_clean_rails_memory_store_caching do end def stub_bamboo_request(url, status, body) - WebMock.stub_request(:get, url).to_return( + stub_full_request(url).to_return( status: status, headers: { 'Content-Type' => 'application/json' }, body: body diff --git a/spec/models/project_services/buildkite_service_spec.rb b/spec/models/project_services/buildkite_service_spec.rb index 091d4d8f695..ca196069055 100644 --- a/spec/models/project_services/buildkite_service_spec.rb +++ b/spec/models/project_services/buildkite_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe BuildkiteService, :use_clean_rails_memory_store_caching do include ReactiveCachingHelpers + include StubRequests let(:project) { create(:project) } @@ -110,10 +111,9 @@ describe BuildkiteService, :use_clean_rails_memory_store_caching do body ||= %q({"status":"success"}) buildkite_full_url = 'https://gitlab.buildkite.com/status/secret-sauce-status-token.json?commit=123' - WebMock.stub_request(:get, buildkite_full_url).to_return( - status: status, - headers: { 'Content-Type' => 'application/json' }, - body: body - ) + stub_full_request(buildkite_full_url) + .to_return(status: status, + headers: { 'Content-Type' => 'application/json' }, + body: body) end end diff --git a/spec/models/project_services/campfire_service_spec.rb b/spec/models/project_services/campfire_service_spec.rb index bf4c52fc7ab..0d3dd89e93b 100644 --- a/spec/models/project_services/campfire_service_spec.rb +++ b/spec/models/project_services/campfire_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe CampfireService do + include StubRequests + describe 'Associations' do it { is_expected.to belong_to :project } it { is_expected.to have_one :service_hook } @@ -49,39 +51,37 @@ describe CampfireService do it "calls Campfire API to get a list of rooms and speak in a room" do # make sure a valid list of rooms is returned body = File.read(Rails.root + 'spec/fixtures/project_services/campfire/rooms.json') - WebMock.stub_request(:get, @rooms_url).with(basic_auth: @auth).to_return( + + stub_full_request(@rooms_url).with(basic_auth: @auth).to_return( body: body, status: 200, headers: @headers ) + # stub the speak request with the room id found in the previous request's response speak_url = 'https://project-name.campfirenow.com/room/123/speak.json' - WebMock.stub_request(:post, speak_url).with(basic_auth: @auth) + stub_full_request(speak_url, method: :post).with(basic_auth: @auth) @campfire_service.execute(@sample_data) - expect(WebMock).to have_requested(:get, @rooms_url).once - expect(WebMock).to have_requested(:post, speak_url).with( - body: /#{project.path}.*#{@sample_data[:before]}.*#{@sample_data[:after]}/ - ).once + expect(WebMock).to have_requested(:get, stubbed_hostname(@rooms_url)).once + expect(WebMock).to have_requested(:post, stubbed_hostname(speak_url)) + .with(body: /#{project.path}.*#{@sample_data[:before]}.*#{@sample_data[:after]}/).once end it "calls Campfire API to get a list of rooms but shouldn't speak in a room" do # return a list of rooms that do not contain a room named 'test-room' body = File.read(Rails.root + 'spec/fixtures/project_services/campfire/rooms2.json') - WebMock.stub_request(:get, @rooms_url).with(basic_auth: @auth).to_return( + stub_full_request(@rooms_url).with(basic_auth: @auth).to_return( body: body, status: 200, headers: @headers ) - # we want to make sure no request is sent to the /speak endpoint, here is a basic - # regexp that matches this endpoint - speak_url = 'https://verySecret:X@project-name.campfirenow.com/room/.*/speak.json' @campfire_service.execute(@sample_data) - expect(WebMock).to have_requested(:get, @rooms_url).once - expect(WebMock).not_to have_requested(:post, /#{speak_url}/) + expect(WebMock).to have_requested(:get, 'https://8.8.8.9/rooms.json').once + expect(WebMock).not_to have_requested(:post, '*/room/.*/speak.json') end end end diff --git a/spec/models/project_services/pivotaltracker_service_spec.rb b/spec/models/project_services/pivotaltracker_service_spec.rb index 773b8b7890f..dde46c82df6 100644 --- a/spec/models/project_services/pivotaltracker_service_spec.rb +++ b/spec/models/project_services/pivotaltracker_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe PivotaltrackerService do + include StubRequests + describe 'Associations' do it { is_expected.to belong_to :project } it { is_expected.to have_one :service_hook } @@ -53,12 +55,12 @@ describe PivotaltrackerService do end before do - WebMock.stub_request(:post, url) + stub_full_request(url, method: :post) end it 'posts correct message' do service.execute(push_data) - expect(WebMock).to have_requested(:post, url).with( + expect(WebMock).to have_requested(:post, stubbed_hostname(url)).with( body: { 'source_commit' => { 'commit_id' => '21c12ea', @@ -85,14 +87,14 @@ describe PivotaltrackerService do service.execute(push_data(branch: 'master')) service.execute(push_data(branch: 'v10')) - expect(WebMock).to have_requested(:post, url).twice + expect(WebMock).to have_requested(:post, stubbed_hostname(url)).twice end it 'does not post message if branch is not in the list' do service.execute(push_data(branch: 'mas')) service.execute(push_data(branch: 'v11')) - expect(WebMock).not_to have_requested(:post, url) + expect(WebMock).not_to have_requested(:post, stubbed_hostname(url)) end end end diff --git a/spec/models/project_services/pushover_service_spec.rb b/spec/models/project_services/pushover_service_spec.rb index d2a45f48705..380f02739bc 100644 --- a/spec/models/project_services/pushover_service_spec.rb +++ b/spec/models/project_services/pushover_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe PushoverService do + include StubRequests + describe 'Associations' do it { is_expected.to belong_to :project } it { is_expected.to have_one :service_hook } @@ -57,13 +59,13 @@ describe PushoverService do sound: sound ) - WebMock.stub_request(:post, api_url) + stub_full_request(api_url, method: :post, ip_address: '8.8.8.8') end it 'calls Pushover API' do pushover.execute(sample_data) - expect(WebMock).to have_requested(:post, api_url).once + expect(WebMock).to have_requested(:post, 'https://8.8.8.8/1/messages.json').once end end end diff --git a/spec/models/project_services/teamcity_service_spec.rb b/spec/models/project_services/teamcity_service_spec.rb index 96dccae733b..1c434b25205 100644 --- a/spec/models/project_services/teamcity_service_spec.rb +++ b/spec/models/project_services/teamcity_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe TeamcityService, :use_clean_rails_memory_store_caching do include ReactiveCachingHelpers + include StubRequests let(:teamcity_url) { 'http://gitlab.com/teamcity' } @@ -212,7 +213,7 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do body ||= %Q({"build":{"status":"#{build_status}","id":"666"}}) - WebMock.stub_request(:get, teamcity_full_url).with(basic_auth: auth).to_return( + stub_full_request(teamcity_full_url).with(basic_auth: auth).to_return( status: status, headers: { 'Content-Type' => 'application/json' }, body: body diff --git a/spec/requests/api/system_hooks_spec.rb b/spec/requests/api/system_hooks_spec.rb index b6e8d74c2e9..0e2f3face71 100644 --- a/spec/requests/api/system_hooks_spec.rb +++ b/spec/requests/api/system_hooks_spec.rb @@ -1,12 +1,14 @@ require 'spec_helper' describe API::SystemHooks do + include StubRequests + let(:user) { create(:user) } let(:admin) { create(:admin) } let!(:hook) { create(:system_hook, url: "http://example.com") } before do - stub_request(:post, hook.url) + stub_full_request(hook.url, method: :post) end describe "GET /hooks" do @@ -68,6 +70,8 @@ describe API::SystemHooks do end it 'sets default values for events' do + stub_full_request('http://mep.mep', method: :post) + post api('/hooks', admin), params: { url: 'http://mep.mep' } expect(response).to have_gitlab_http_status(201) @@ -78,6 +82,8 @@ describe API::SystemHooks do end it 'sets explicit values for events' do + stub_full_request('http://mep.mep', method: :post) + post api('/hooks', admin), params: { url: 'http://mep.mep', diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index f4470b50753..75d534c59bf 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -2,6 +2,8 @@ require 'spec_helper' describe Projects::LfsPointers::LfsDownloadService do + include StubRequests + let(:project) { create(:project) } let(:lfs_content) { SecureRandom.random_bytes(10) } let(:oid) { Digest::SHA256.hexdigest(lfs_content) } @@ -62,7 +64,7 @@ describe Projects::LfsPointers::LfsDownloadService do describe '#execute' do context 'when file download succeeds' do before do - WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + stub_full_request(download_link).to_return(body: lfs_content) end it_behaves_like 'lfs object is created' @@ -104,7 +106,7 @@ describe Projects::LfsPointers::LfsDownloadService do let(:size) { 1 } before do - WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + stub_full_request(download_link).to_return(body: lfs_content) end it_behaves_like 'no lfs object is created' @@ -118,7 +120,7 @@ describe Projects::LfsPointers::LfsDownloadService do context 'when downloaded lfs file has a different oid' do before do - WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + stub_full_request(download_link).to_return(body: lfs_content) allow_any_instance_of(Digest::SHA256).to receive(:hexdigest).and_return('foobar') end @@ -136,7 +138,7 @@ describe Projects::LfsPointers::LfsDownloadService do let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials) } before do - WebMock.stub_request(:get, download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content) + stub_full_request(download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content) end it 'the request adds authorization headers' do @@ -149,7 +151,7 @@ describe Projects::LfsPointers::LfsDownloadService do let(:local_request_setting) { true } before do - WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + stub_full_request(download_link, ip_address: '192.168.2.120').to_return(body: lfs_content) end it_behaves_like 'lfs object is created' @@ -173,7 +175,8 @@ describe Projects::LfsPointers::LfsDownloadService do with_them do before do - WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) + stub_full_request(download_link, ip_address: '192.168.2.120') + .to_return(status: 301, headers: { 'Location' => redirect_link }) end it_behaves_like 'no lfs object is created' @@ -184,8 +187,8 @@ describe Projects::LfsPointers::LfsDownloadService do let(:redirect_link) { "http://example.com/"} before do - WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) - WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content) + stub_full_request(download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) + stub_full_request(redirect_link).to_return(body: lfs_content) end it_behaves_like 'lfs object is created' diff --git a/spec/services/submit_usage_ping_service_spec.rb b/spec/services/submit_usage_ping_service_spec.rb index 78df9bf96bf..653f17a4324 100644 --- a/spec/services/submit_usage_ping_service_spec.rb +++ b/spec/services/submit_usage_ping_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe SubmitUsagePingService do + include StubRequests + context 'when usage ping is disabled' do before do stub_application_setting(usage_ping_enabled: false) @@ -99,7 +101,7 @@ describe SubmitUsagePingService do end def stub_response(body) - stub_request(:post, 'https://version.gitlab.com/usage_data') + stub_full_request('https://version.gitlab.com/usage_data', method: :post) .to_return( headers: { 'Content-Type' => 'application/json' }, body: body.to_json diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 75ba2479b63..37bafc0c002 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe WebHookService do + include StubRequests + let(:project) { create(:project) } let(:project_hook) { create(:project_hook) } let(:headers) do @@ -67,11 +69,11 @@ describe WebHookService do let(:project_hook) { create(:project_hook, url: 'https://demo:demo@example.org/') } it 'uses the credentials' do - WebMock.stub_request(:post, url) + stub_full_request(url, method: :post) service_instance.execute - expect(WebMock).to have_requested(:post, url).with( + expect(WebMock).to have_requested(:post, stubbed_hostname(url)).with( headers: headers.merge('Authorization' => 'Basic ZGVtbzpkZW1v') ).once end @@ -82,11 +84,11 @@ describe WebHookService do let(:project_hook) { create(:project_hook, url: 'https://demo@example.org/') } it 'uses the credentials anyways' do - WebMock.stub_request(:post, url) + stub_full_request(url, method: :post) service_instance.execute - expect(WebMock).to have_requested(:post, url).with( + expect(WebMock).to have_requested(:post, stubbed_hostname(url)).with( headers: headers.merge('Authorization' => 'Basic ZGVtbzo=') ).once end diff --git a/spec/support/helpers/stub_requests.rb b/spec/support/helpers/stub_requests.rb new file mode 100644 index 00000000000..5cad35282c0 --- /dev/null +++ b/spec/support/helpers/stub_requests.rb @@ -0,0 +1,40 @@ +module StubRequests + IP_ADDRESS_STUB = '8.8.8.9'.freeze + + # Fully stubs a request using WebMock class. This class also + # stubs the IP address the URL is translated to (DNS lookup). + # + # It expects the final request to go to the `ip_address` instead the given url. + # That's primarily a DNS rebind attack prevention of Gitlab::HTTP + # (see: Gitlab::UrlBlocker). + # + def stub_full_request(url, ip_address: IP_ADDRESS_STUB, port: 80, method: :get) + stub_dns(url, ip_address: ip_address, port: port) + + url = stubbed_hostname(url, hostname: ip_address) + WebMock.stub_request(method, url) + end + + def stub_dns(url, ip_address:, port: 80) + url = parse_url(url) + socket = Socket.sockaddr_in(port, ip_address) + addr = Addrinfo.new(socket) + + # See Gitlab::UrlBlocker + allow(Addrinfo).to receive(:getaddrinfo) + .with(url.hostname, url.port, nil, :STREAM) + .and_return([addr]) + end + + def stubbed_hostname(url, hostname: IP_ADDRESS_STUB) + url = parse_url(url) + url.hostname = hostname + url.to_s + end + + private + + def parse_url(url) + url.is_a?(URI) ? url : URI(url) + end +end -- cgit v1.2.1 From a1a0f8e6b017f57060bc94d14fd4d37d8756e47d Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Wed, 29 May 2019 13:43:07 -0300 Subject: Add DNS rebinding protection settings --- app/helpers/application_settings_helper.rb | 1 + app/models/application_setting_implementation.rb | 1 + .../admin/application_settings/_outbound.html.haml | 8 +++++ ...g_protection_enabled_to_application_settings.rb | 23 ++++++++++++++ db/schema.rb | 3 +- lib/gitlab.rb | 5 ++++ lib/gitlab/http_connection_adapter.rb | 9 +++++- lib/gitlab/url_blocker.rb | 34 ++++++++++++++------- locale/gitlab.pot | 6 ++++ spec/features/admin/admin_settings_spec.rb | 5 +++- spec/lib/gitlab/http_connection_adapter_spec.rb | 32 ++++++++++++++++++++ spec/lib/gitlab/url_blocker_spec.rb | 35 ++++++++++++++++++++++ spec/lib/gitlab_spec.rb | 30 +++++++++++++++++++ 13 files changed, 179 insertions(+), 13 deletions(-) create mode 100644 db/migrate/20190529142545_add_dns_rebinding_protection_enabled_to_application_settings.rb diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 971d1052824..4469118f065 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -160,6 +160,7 @@ module ApplicationSettingsHelper :akismet_api_key, :akismet_enabled, :allow_local_requests_from_hooks_and_services, + :dns_rebinding_protection_enabled, :archive_builds_in_human_readable, :authorized_keys_enabled, :auto_devops_enabled, diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index e51619b0f9c..904d650ef96 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -21,6 +21,7 @@ module ApplicationSettingImplementation after_sign_up_text: nil, akismet_enabled: false, allow_local_requests_from_hooks_and_services: false, + dns_rebinding_protection_enabled: true, authorized_keys_enabled: true, # TODO default to false if the instance is configured to use AuthorizedKeysCommand container_registry_token_expire_delay: 5, default_artifacts_expire_in: '30 days', diff --git a/app/views/admin/application_settings/_outbound.html.haml b/app/views/admin/application_settings/_outbound.html.haml index f4bfb5af385..dd56bb99a06 100644 --- a/app/views/admin/application_settings/_outbound.html.haml +++ b/app/views/admin/application_settings/_outbound.html.haml @@ -8,4 +8,12 @@ = f.label :allow_local_requests_from_hooks_and_services, class: 'form-check-label' do Allow requests to the local network from hooks and services + .form-group + .form-check + = f.check_box :dns_rebinding_protection_enabled, class: 'form-check-input' + = f.label :dns_rebinding_protection_enabled, class: 'form-check-label' do + = _('Enforce DNS rebinding attack protection') + %span.form-text.text-muted + = _('Resolves IP addresses once and uses them to submit requests') + = f.submit 'Save changes', class: "btn btn-success" diff --git a/db/migrate/20190529142545_add_dns_rebinding_protection_enabled_to_application_settings.rb b/db/migrate/20190529142545_add_dns_rebinding_protection_enabled_to_application_settings.rb new file mode 100644 index 00000000000..8835dc8b7ba --- /dev/null +++ b/db/migrate/20190529142545_add_dns_rebinding_protection_enabled_to_application_settings.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddDnsRebindingProtectionEnabledToApplicationSettings < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:application_settings, :dns_rebinding_protection_enabled, + :boolean, + default: true, + allow_null: false) + end + + def down + remove_column(:application_settings, :dns_rebinding_protection_enabled) + end +end diff --git a/db/schema.rb b/db/schema.rb index bb59af540fe..9facc1c4bb2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190524062810) do +ActiveRecord::Schema.define(version: 20190529142545) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -193,6 +193,7 @@ ActiveRecord::Schema.define(version: 20190524062810) do t.integer "elasticsearch_replicas", default: 1, null: false t.text "encrypted_lets_encrypt_private_key" t.text "encrypted_lets_encrypt_private_key_iv" + t.boolean "dns_rebinding_protection_enabled", default: true, null: false t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree end diff --git a/lib/gitlab.rb b/lib/gitlab.rb index 3f107fbbf3b..ccaf06c5d6a 100644 --- a/lib/gitlab.rb +++ b/lib/gitlab.rb @@ -40,6 +40,7 @@ module Gitlab SUBDOMAIN_REGEX = %r{\Ahttps://[a-z0-9]+\.gitlab\.com\z}.freeze VERSION = File.read(root.join("VERSION")).strip.freeze INSTALLATION_TYPE = File.read(root.join("INSTALLATION_TYPE")).strip.freeze + HTTP_PROXY_ENV_VARS = %w(http_proxy https_proxy HTTP_PROXY HTTPS_PROXY).freeze def self.com? # Check `gl_subdomain?` as well to keep parity with gitlab.com @@ -66,6 +67,10 @@ module Gitlab end end + def self.http_proxy_env? + HTTP_PROXY_ENV_VARS.any? { |name| ENV[name] } + end + def self.process_name return 'sidekiq' if Sidekiq.server? return 'console' if defined?(Rails::Console) diff --git a/lib/gitlab/http_connection_adapter.rb b/lib/gitlab/http_connection_adapter.rb index 9ccf0653903..41eab3658bc 100644 --- a/lib/gitlab/http_connection_adapter.rb +++ b/lib/gitlab/http_connection_adapter.rb @@ -14,7 +14,8 @@ module Gitlab def connection begin @uri, hostname = Gitlab::UrlBlocker.validate!(uri, allow_local_network: allow_local_requests?, - allow_localhost: allow_local_requests?) + allow_localhost: allow_local_requests?, + dns_rebind_protection: dns_rebind_protection?) rescue Gitlab::UrlBlocker::BlockedUrlError => e raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}" end @@ -30,6 +31,12 @@ module Gitlab options.fetch(:allow_local_requests, allow_settings_local_requests?) end + def dns_rebind_protection? + return false if Gitlab.http_proxy_env? + + Gitlab::CurrentSettings.dns_rebinding_protection_enabled? + end + def allow_settings_local_requests? Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? end diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 81935f2b052..9a8df719827 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -18,7 +18,21 @@ module Gitlab # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true. # # Returns an array with [, ]. - def validate!(url, ports: [], schemes: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false) # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/ParameterLists + def validate!( + url, + ports: [], + schemes: [], + allow_localhost: false, + allow_local_network: true, + ascii_only: false, + enforce_user: false, + enforce_sanitization: false, + dns_rebind_protection: true) + # rubocop:enable Metrics/CyclomaticComplexity + # rubocop:enable Metrics/ParameterLists + return [nil, nil] if url.nil? # Param url can be a string, URI or Addressable::URI @@ -45,15 +59,17 @@ module Gitlab return [uri, nil] end + protected_uri_with_hostname = enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection) + # Allow url from the GitLab instance itself but only for the configured hostname and ports - return enforce_uri_hostname(addrs_info, uri, hostname) if internal?(uri) + return protected_uri_with_hostname if internal?(uri) validate_localhost!(addrs_info) unless allow_localhost validate_loopback!(addrs_info) unless allow_localhost validate_local_network!(addrs_info) unless allow_local_network validate_link_local!(addrs_info) unless allow_local_network - enforce_uri_hostname(addrs_info, uri, hostname) + protected_uri_with_hostname end def blocked_url?(*args) @@ -74,17 +90,15 @@ module Gitlab # # The original hostname is used to validate the SSL, given in that scenario # we'll be making the request to the IP address, instead of using the hostname. - def enforce_uri_hostname(addrs_info, uri, hostname) + def enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection) address = addrs_info.first ip_address = address&.ip_address - if ip_address && ip_address != hostname - uri = uri.dup - uri.hostname = ip_address - return [uri, hostname] - end + return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != hostname - [uri, nil] + uri = uri.dup + uri.hostname = ip_address + [uri, hostname] end def get_port(uri) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0d47c574120..b550553f303 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3775,6 +3775,9 @@ msgstr "" msgid "Ends at (UTC)" msgstr "" +msgid "Enforce DNS rebinding attack protection" +msgstr "" + msgid "Enter at least three characters to search" msgstr "" @@ -8286,6 +8289,9 @@ msgstr "" msgid "Resolved by %{resolvedByName}" msgstr "" +msgid "Resolves IP addresses once and uses them to submit requests" +msgstr "" + msgid "Response metrics (AWS ELB)" msgstr "" diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index f9950b5b03f..c4dbe23f6b4 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -332,16 +332,19 @@ describe 'Admin updates settings' do end context 'Network page' do - it 'Enable outbound requests' do + it 'Changes Outbound requests settings' do visit network_admin_application_settings_path page.within('.as-outbound') do check 'Allow requests to the local network from hooks and services' + # Enabled by default + uncheck 'Enforce DNS rebinding attack protection' click_button 'Save changes' end expect(page).to have_content "Application settings saved successfully" expect(Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services).to be true + expect(Gitlab::CurrentSettings.dns_rebinding_protection_enabled).to be false end end diff --git a/spec/lib/gitlab/http_connection_adapter_spec.rb b/spec/lib/gitlab/http_connection_adapter_spec.rb index af033be8e5b..930d1f62272 100644 --- a/spec/lib/gitlab/http_connection_adapter_spec.rb +++ b/spec/lib/gitlab/http_connection_adapter_spec.rb @@ -47,6 +47,38 @@ describe Gitlab::HTTPConnectionAdapter do end end + context 'when DNS rebinding protection is disabled' do + it 'sets up the connection' do + stub_application_setting(dns_rebinding_protection_enabled: false) + + uri = URI('https://example.org') + + connection = described_class.new(uri).connection + + expect(connection).to be_a(Net::HTTP) + expect(connection.address).to eq('example.org') + expect(connection.hostname_override).to eq(nil) + expect(connection.addr_port).to eq('example.org') + expect(connection.port).to eq(443) + end + end + + context 'when http(s) environment variable is set' do + it 'sets up the connection' do + stub_env('https_proxy' => 'https://my.proxy') + + uri = URI('https://example.org') + + connection = described_class.new(uri).connection + + expect(connection).to be_a(Net::HTTP) + expect(connection.address).to eq('example.org') + expect(connection.hostname_override).to eq(nil) + expect(connection.addr_port).to eq('example.org') + expect(connection.port).to eq(443) + end + end + context 'when local requests are allowed' do it 'sets up the connection' do uri = URI('https://example.org') diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 416f8cfece9..253366e0789 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -46,6 +46,41 @@ describe Gitlab::UrlBlocker do expect(hostname).to be(nil) end end + + context 'disabled DNS rebinding protection' do + context 'when URI is internal' do + let(:import_url) { 'http://localhost' } + + it 'returns URI and no hostname' do + uri, hostname = described_class.validate!(import_url, dns_rebind_protection: false) + + expect(uri).to eq(Addressable::URI.parse('http://localhost')) + expect(hostname).to be(nil) + end + end + + context 'when the URL hostname is a domain' do + let(:import_url) { 'https://example.org' } + + it 'returns URI and no hostname' do + uri, hostname = described_class.validate!(import_url, dns_rebind_protection: false) + + expect(uri).to eq(Addressable::URI.parse('https://example.org')) + expect(hostname).to eq(nil) + end + end + + context 'when the URL hostname is an IP address' do + let(:import_url) { 'https://93.184.216.34' } + + it 'returns URI and no hostname' do + uri, hostname = described_class.validate!(import_url, dns_rebind_protection: false) + + expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34')) + expect(hostname).to be(nil) + end + end + end end describe '#blocked_url?' do diff --git a/spec/lib/gitlab_spec.rb b/spec/lib/gitlab_spec.rb index 767b5779a79..e075904b0cc 100644 --- a/spec/lib/gitlab_spec.rb +++ b/spec/lib/gitlab_spec.rb @@ -109,4 +109,34 @@ describe Gitlab do expect(described_class.ee?).to eq(false) end end + + describe '.http_proxy_env?' do + it 'returns true when lower case https' do + stub_env('https_proxy', 'https://my.proxy') + + expect(described_class.http_proxy_env?).to eq(true) + end + + it 'returns true when upper case https' do + stub_env('HTTPS_PROXY', 'https://my.proxy') + + expect(described_class.http_proxy_env?).to eq(true) + end + + it 'returns true when lower case http' do + stub_env('http_proxy', 'http://my.proxy') + + expect(described_class.http_proxy_env?).to eq(true) + end + + it 'returns true when upper case http' do + stub_env('HTTP_PROXY', 'http://my.proxy') + + expect(described_class.http_proxy_env?).to eq(true) + end + + it 'returns false when not set' do + expect(described_class.http_proxy_env?).to eq(false) + end + end end -- cgit v1.2.1