Commit 9d046c87 authored by Mark Chao's avatar Mark Chao

Fix git clone revealing private repo's presence

Ensure redirection to path with .git suffix regardless whether project
exists or not.
parent 701303a5
---
title: Fix git clone revealing private repo's presence
merge_request:
author:
type: security
...@@ -40,7 +40,7 @@ scope(path: '*namespace_id/:project_id', ...@@ -40,7 +40,7 @@ scope(path: '*namespace_id/:project_id',
# /info/refs?service=git-receive-pack, but nothing else. # /info/refs?service=git-receive-pack, but nothing else.
# #
git_http_handshake = lambda do |request| 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.blank? ||
request.query_string.match(/\Aservice=git-(upload|receive)-pack\z/)) request.query_string.match(/\Aservice=git-(upload|receive)-pack\z/))
end end
......
...@@ -2,12 +2,13 @@ ...@@ -2,12 +2,13 @@
module Constraints module Constraints
class ProjectUrlConstrainer class ProjectUrlConstrainer
def matches?(request) def matches?(request, existence_check: true)
namespace_path = request.params[:namespace_id] namespace_path = request.params[:namespace_id]
project_path = request.params[:project_id] || request.params[:id] project_path = request.params[:project_id] || request.params[:id]
full_path = [namespace_path, project_path].join('/') full_path = [namespace_path, project_path].join('/')
return false unless ProjectPathValidator.valid_path?(full_path) 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 # 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 # as cache for further Project.find_by_full_path calls within request
......
...@@ -16,6 +16,10 @@ describe Constraints::ProjectUrlConstrainer do ...@@ -16,6 +16,10 @@ describe Constraints::ProjectUrlConstrainer do
let(:request) { build_request('foo', 'bar') } let(:request) { build_request('foo', 'bar') }
it { expect(subject.matches?(request)).to be_falsey } 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 end
context "project id ending with .git" do context "project id ending with .git" do
......
...@@ -104,6 +104,70 @@ describe 'Git HTTP requests' do ...@@ -104,6 +104,70 @@ describe 'Git HTTP requests' do
end end
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 describe "User with no identities" do
let(:user) { create(:user) } let(:user) { create(:user) }
...@@ -143,6 +207,10 @@ describe 'Git HTTP requests' do ...@@ -143,6 +207,10 @@ describe 'Git HTTP requests' do
expect(response).to have_gitlab_http_status(:unprocessable_entity) expect(response).to have_gitlab_http_status(:unprocessable_entity)
end end
end end
it_behaves_like 'project path without .git suffix' do
let(:project_path) { "#{user.namespace.path}/project.git-project" }
end
end end
end end
...@@ -706,70 +774,8 @@ describe 'Git HTTP requests' do ...@@ -706,70 +774,8 @@ describe 'Git HTTP requests' do
end end
end end
context "when the project path doesn't end in .git" do it_behaves_like 'project path without .git suffix' do
let(:project) { create(:project, :repository, :public, path: 'project.git-project') } let(:project_path) { create(:project, :repository, :public, path: 'project.git-project').full_path }
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
end end
context "retrieving an info/refs file" do context "retrieving an info/refs file" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment