diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2019-03-04 21:36:37 +0300 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2019-03-04 21:36:37 +0300 |
commit | 6c3482d166955cd112b034598f4aaac57af544c3 (patch) | |
tree | d75e188edfbb455ec6f444b99c07f82420c6762b | |
parent | 72db8ae2b2d9bc79a96937f3b4943462b053af96 (diff) | |
parent | 9d046c8704c0e7df18d2f9e380e987d22b9a0b2e (diff) |
Merge branch 'security-50334' into 'master'
Fix git clone revealing private repo's presence
See merge request gitlab/gitlabhq!2937
-rw-r--r-- | changelogs/unreleased/security-50334.yml | 5 | ||||
-rw-r--r-- | config/routes/git_http.rb | 2 | ||||
-rw-r--r-- | lib/constraints/project_url_constrainer.rb | 3 | ||||
-rw-r--r-- | spec/lib/constraints/project_url_constrainer_spec.rb | 4 | ||||
-rw-r--r-- | spec/requests/git_http_spec.rb | 134 |
5 files changed, 82 insertions, 66 deletions
diff --git a/changelogs/unreleased/security-50334.yml b/changelogs/unreleased/security-50334.yml new file mode 100644 index 00000000000..828ef82b517 --- /dev/null +++ b/changelogs/unreleased/security-50334.yml @@ -0,0 +1,5 @@ +--- +title: Fix git clone revealing private repo's presence +merge_request: +author: +type: security diff --git a/config/routes/git_http.rb b/config/routes/git_http.rb index ec5c68f81df..a959d40881b 100644 --- a/config/routes/git_http.rb +++ b/config/routes/git_http.rb @@ -40,7 +40,7 @@ scope(path: '*namespace_id/:project_id', # /info/refs?service=git-receive-pack, but nothing else. # git_http_handshake = lambda do |request| - ::Constraints::ProjectUrlConstrainer.new.matches?(request) && + ::Constraints::ProjectUrlConstrainer.new.matches?(request, existence_check: false) && (request.query_string.blank? || request.query_string.match(/\Aservice=git-(upload|receive)-pack\z/)) end diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb index eadfbf7bc01..d41490d2ebd 100644 --- a/lib/constraints/project_url_constrainer.rb +++ b/lib/constraints/project_url_constrainer.rb @@ -2,12 +2,13 @@ module Constraints class ProjectUrlConstrainer - def matches?(request) + def matches?(request, existence_check: true) namespace_path = request.params[:namespace_id] project_path = request.params[:project_id] || request.params[:id] full_path = [namespace_path, project_path].join('/') return false unless ProjectPathValidator.valid_path?(full_path) + return true unless existence_check # We intentionally allow SELECT(*) here so result of this query can be used # as cache for further Project.find_by_full_path calls within request diff --git a/spec/lib/constraints/project_url_constrainer_spec.rb b/spec/lib/constraints/project_url_constrainer_spec.rb index c96e7ab8495..3496b01ebcc 100644 --- a/spec/lib/constraints/project_url_constrainer_spec.rb +++ b/spec/lib/constraints/project_url_constrainer_spec.rb @@ -16,6 +16,10 @@ describe Constraints::ProjectUrlConstrainer do let(:request) { build_request('foo', 'bar') } it { expect(subject.matches?(request)).to be_falsey } + + context 'existence_check is false' do + it { expect(subject.matches?(request, existence_check: false)).to be_truthy } + end end context "project id ending with .git" do diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 5b625fd47be..bfa178f5cae 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -104,6 +104,70 @@ describe 'Git HTTP requests' do end end + shared_examples_for 'project path without .git suffix' do + context "GET info/refs" do + let(:path) { "/#{project_path}/info/refs" } + + context "when no params are added" do + before do + get path + end + + it "redirects to the .git suffix version" do + expect(response).to redirect_to("/#{project_path}.git/info/refs") + end + end + + context "when the upload-pack service is requested" do + let(:params) { { service: 'git-upload-pack' } } + + before do + get path, params: params + end + + it "redirects to the .git suffix version" do + expect(response).to redirect_to("/#{project_path}.git/info/refs?service=#{params[:service]}") + end + end + + context "when the receive-pack service is requested" do + let(:params) { { service: 'git-receive-pack' } } + + before do + get path, params: params + end + + it "redirects to the .git suffix version" do + expect(response).to redirect_to("/#{project_path}.git/info/refs?service=#{params[:service]}") + end + end + + context "when the params are anything else" do + let(:params) { { service: 'git-implode-pack' } } + + before do + get path, params: params + end + + it "redirects to the sign-in page" do + expect(response).to redirect_to(new_user_session_path) + end + end + end + + context "POST git-upload-pack" do + it "fails to find a route" do + expect { clone_post(project_path) }.to raise_error(ActionController::RoutingError) + end + end + + context "POST git-receive-pack" do + it "fails to find a route" do + expect { push_post(project_path) }.to raise_error(ActionController::RoutingError) + end + end + end + describe "User with no identities" do let(:user) { create(:user) } @@ -143,6 +207,10 @@ describe 'Git HTTP requests' do expect(response).to have_gitlab_http_status(:unprocessable_entity) end end + + it_behaves_like 'project path without .git suffix' do + let(:project_path) { "#{user.namespace.path}/project.git-project" } + end end end @@ -706,70 +774,8 @@ describe 'Git HTTP requests' do end end - context "when the project path doesn't end in .git" do - let(:project) { create(:project, :repository, :public, path: 'project.git-project') } - - context "GET info/refs" do - let(:path) { "/#{project.full_path}/info/refs" } - - context "when no params are added" do - before do - get path - end - - it "redirects to the .git suffix version" do - expect(response).to redirect_to("/#{project.full_path}.git/info/refs") - end - end - - context "when the upload-pack service is requested" do - let(:params) { { service: 'git-upload-pack' } } - - before do - get path, params: params - end - - it "redirects to the .git suffix version" do - expect(response).to redirect_to("/#{project.full_path}.git/info/refs?service=#{params[:service]}") - end - end - - context "when the receive-pack service is requested" do - let(:params) { { service: 'git-receive-pack' } } - - before do - get path, params: params - end - - it "redirects to the .git suffix version" do - expect(response).to redirect_to("/#{project.full_path}.git/info/refs?service=#{params[:service]}") - end - end - - context "when the params are anything else" do - let(:params) { { service: 'git-implode-pack' } } - - before do - get path, params: params - end - - it "redirects to the sign-in page" do - expect(response).to redirect_to(new_user_session_path) - end - end - end - - context "POST git-upload-pack" do - it "fails to find a route" do - expect { clone_post(project.full_path) }.to raise_error(ActionController::RoutingError) - end - end - - context "POST git-receive-pack" do - it "fails to find a route" do - expect { push_post(project.full_path) }.to raise_error(ActionController::RoutingError) - end - end + it_behaves_like 'project path without .git suffix' do + let(:project_path) { create(:project, :repository, :public, path: 'project.git-project').full_path } end context "retrieving an info/refs file" do |