From b8f754dd0abdf437669e17a820a8e6c230afa73e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 3 Aug 2016 14:54:12 +0200 Subject: Stop 'git push' over HTTP early Before this change we always let users push Git data over HTTP before deciding whether to accept to push. This was different from pushing over SSH where we terminate a 'git push' early if we already know the user is not allowed to push. This change let Git over HTTP follow the same behavior as Git over SSH. We also distinguish between HTTP 404 and 403 responses when denying Git requests, depending on whether the user is allowed to know the project exists. --- app/controllers/projects/git_http_controller.rb | 39 ++++++++++++++----------- lib/gitlab/git_access.rb | 2 +- spec/requests/git_http_spec.rb | 10 +++---- 3 files changed, 28 insertions(+), 23 deletions(-) diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 40a8b7940d9..e2f93e239bd 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -20,9 +20,9 @@ class Projects::GitHttpController < Projects::ApplicationController elsif receive_pack? && receive_pack_allowed? render_ok elsif http_blocked? - render_not_allowed + render_http_not_allowed else - render_not_found + render_denied end end @@ -31,7 +31,7 @@ class Projects::GitHttpController < Projects::ApplicationController if upload_pack? && upload_pack_allowed? render_ok else - render_not_found + render_denied end end @@ -40,7 +40,7 @@ class Projects::GitHttpController < Projects::ApplicationController if receive_pack? && receive_pack_allowed? render_ok else - render_not_found + render_denied end end @@ -156,8 +156,17 @@ class Projects::GitHttpController < Projects::ApplicationController render plain: 'Not Found', status: :not_found end - def render_not_allowed - render plain: download_access.message, status: :forbidden + def render_http_not_allowed + render plain: access_check.message, status: :forbidden + end + + def render_denied + if user && user.can?(:read_project, project) + render plain: 'Access denied', status: :forbidden + else + # Do not leak information about project existence + render_not_found + end end def ci? @@ -168,22 +177,20 @@ class Projects::GitHttpController < Projects::ApplicationController return false unless Gitlab.config.gitlab_shell.upload_pack if user - download_access.allowed? + access_check.allowed? else ci? || project.public? end end def access - return @access if defined?(@access) - - @access = Gitlab::GitAccess.new(user, project, 'http') + @access ||= Gitlab::GitAccess.new(user, project, 'http') end - def download_access - return @download_access if defined?(@download_access) - - @download_access = access.check('git-upload-pack') + def access_check + # Use the magic string '_any' to indicate we do not know what the + # changes are. This is also what gitlab-shell does. + @access_check ||= access.check(git_command, '_any') end def http_blocked? @@ -193,8 +200,6 @@ class Projects::GitHttpController < Projects::ApplicationController def receive_pack_allowed? return false unless Gitlab.config.gitlab_shell.receive_pack - # Skip user authorization on upload request. - # It will be done by the pre-receive hook in the repository. - user.present? + access_check.allowed? end end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 8e8f39d9cb2..69943e22353 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -14,7 +14,7 @@ module Gitlab @user_access = UserAccess.new(user, project: project) end - def check(cmd, changes = nil) + def check(cmd, changes) return build_status_object(false, "Git access over #{protocol.upcase} is not allowed") unless protocol_allowed? unless actor diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 82ab582beac..febfdf48c7e 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -75,9 +75,9 @@ describe 'Git HTTP requests', lib: true do context "with correct credentials" do let(:env) { { user: user.username, password: user.password } } - it "uploads get status 200 (because Git hooks do the real check)" do + it "uploads get status 403" do upload(path, env) do |response| - expect(response).to have_http_status(200) + expect(response).to have_http_status(403) end end @@ -86,7 +86,7 @@ describe 'Git HTTP requests', lib: true do allow(Gitlab.config.gitlab_shell).to receive(:receive_pack).and_return(false) upload(path, env) do |response| - expect(response).to have_http_status(404) + expect(response).to have_http_status(403) end end end @@ -236,9 +236,9 @@ describe 'Git HTTP requests', lib: true do end end - it "uploads get status 200 (because Git hooks do the real check)" do + it "uploads get status 404" do upload(path, user: user.username, password: user.password) do |response| - expect(response).to have_http_status(200) + expect(response).to have_http_status(404) end end end -- cgit v1.2.1 From e55e224cd9d5dfb3fc351374a0cd4620f8e845b9 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 4 Aug 2016 15:22:34 +0200 Subject: Fix ArgumentError in GitAccess specs --- spec/lib/gitlab/git_access_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 8447305a316..f12c9a370f7 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -19,11 +19,11 @@ describe Gitlab::GitAccess, lib: true do end it 'blocks ssh git push' do - expect(@acc.check('git-receive-pack').allowed?).to be_falsey + expect(@acc.check('git-receive-pack', '_any').allowed?).to be_falsey end it 'blocks ssh git pull' do - expect(@acc.check('git-upload-pack').allowed?).to be_falsey + expect(@acc.check('git-upload-pack', '_any').allowed?).to be_falsey end end @@ -34,17 +34,17 @@ describe Gitlab::GitAccess, lib: true do end it 'blocks http push' do - expect(@acc.check('git-receive-pack').allowed?).to be_falsey + expect(@acc.check('git-receive-pack', '_any').allowed?).to be_falsey end it 'blocks http git pull' do - expect(@acc.check('git-upload-pack').allowed?).to be_falsey + expect(@acc.check('git-upload-pack', '_any').allowed?).to be_falsey end end end describe 'download_access_check' do - subject { access.check('git-upload-pack') } + subject { access.check('git-upload-pack', '_any') } describe 'master permissions' do before { project.team << [user, :master] } @@ -288,7 +288,7 @@ describe Gitlab::GitAccess, lib: true do let(:actor) { key } context 'push code' do - subject { access.check('git-receive-pack') } + subject { access.check('git-receive-pack', '_any') } context 'when project is authorized' do before { key.projects << project } -- cgit v1.2.1