diff options
| -rw-r--r-- | app/controllers/projects/git_http_controller.rb | 39 | ||||
| -rw-r--r-- | lib/gitlab/git_access.rb | 2 | ||||
| -rw-r--r-- | spec/lib/gitlab/git_access_spec.rb | 12 | ||||
| -rw-r--r-- | spec/requests/git_http_spec.rb | 10 |
4 files changed, 34 insertions, 29 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/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 } diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 022781d8696..8537c252b58 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 |