BigW Consortium Gitlab

Commit b8f754dd by Jacob Vosmaer

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.
parent 132a81f4
...@@ -20,9 +20,9 @@ class Projects::GitHttpController < Projects::ApplicationController ...@@ -20,9 +20,9 @@ class Projects::GitHttpController < Projects::ApplicationController
elsif receive_pack? && receive_pack_allowed? elsif receive_pack? && receive_pack_allowed?
render_ok render_ok
elsif http_blocked? elsif http_blocked?
render_not_allowed render_http_not_allowed
else else
render_not_found render_denied
end end
end end
...@@ -31,7 +31,7 @@ class Projects::GitHttpController < Projects::ApplicationController ...@@ -31,7 +31,7 @@ class Projects::GitHttpController < Projects::ApplicationController
if upload_pack? && upload_pack_allowed? if upload_pack? && upload_pack_allowed?
render_ok render_ok
else else
render_not_found render_denied
end end
end end
...@@ -40,7 +40,7 @@ class Projects::GitHttpController < Projects::ApplicationController ...@@ -40,7 +40,7 @@ class Projects::GitHttpController < Projects::ApplicationController
if receive_pack? && receive_pack_allowed? if receive_pack? && receive_pack_allowed?
render_ok render_ok
else else
render_not_found render_denied
end end
end end
...@@ -156,8 +156,17 @@ class Projects::GitHttpController < Projects::ApplicationController ...@@ -156,8 +156,17 @@ class Projects::GitHttpController < Projects::ApplicationController
render plain: 'Not Found', status: :not_found render plain: 'Not Found', status: :not_found
end end
def render_not_allowed def render_http_not_allowed
render plain: download_access.message, status: :forbidden 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 end
def ci? def ci?
...@@ -168,22 +177,20 @@ class Projects::GitHttpController < Projects::ApplicationController ...@@ -168,22 +177,20 @@ class Projects::GitHttpController < Projects::ApplicationController
return false unless Gitlab.config.gitlab_shell.upload_pack return false unless Gitlab.config.gitlab_shell.upload_pack
if user if user
download_access.allowed? access_check.allowed?
else else
ci? || project.public? ci? || project.public?
end end
end end
def access def access
return @access if defined?(@access) @access ||= Gitlab::GitAccess.new(user, project, 'http')
@access = Gitlab::GitAccess.new(user, project, 'http')
end end
def download_access def access_check
return @download_access if defined?(@download_access) # Use the magic string '_any' to indicate we do not know what the
# changes are. This is also what gitlab-shell does.
@download_access = access.check('git-upload-pack') @access_check ||= access.check(git_command, '_any')
end end
def http_blocked? def http_blocked?
...@@ -193,8 +200,6 @@ class Projects::GitHttpController < Projects::ApplicationController ...@@ -193,8 +200,6 @@ class Projects::GitHttpController < Projects::ApplicationController
def receive_pack_allowed? def receive_pack_allowed?
return false unless Gitlab.config.gitlab_shell.receive_pack return false unless Gitlab.config.gitlab_shell.receive_pack
# Skip user authorization on upload request. access_check.allowed?
# It will be done by the pre-receive hook in the repository.
user.present?
end end
end end
...@@ -14,7 +14,7 @@ module Gitlab ...@@ -14,7 +14,7 @@ module Gitlab
@user_access = UserAccess.new(user, project: project) @user_access = UserAccess.new(user, project: project)
end 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? return build_status_object(false, "Git access over #{protocol.upcase} is not allowed") unless protocol_allowed?
unless actor unless actor
......
...@@ -75,9 +75,9 @@ describe 'Git HTTP requests', lib: true do ...@@ -75,9 +75,9 @@ describe 'Git HTTP requests', lib: true do
context "with correct credentials" do context "with correct credentials" do
let(:env) { { user: user.username, password: user.password } } 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| upload(path, env) do |response|
expect(response).to have_http_status(200) expect(response).to have_http_status(403)
end end
end end
...@@ -86,7 +86,7 @@ describe 'Git HTTP requests', lib: true do ...@@ -86,7 +86,7 @@ describe 'Git HTTP requests', lib: true do
allow(Gitlab.config.gitlab_shell).to receive(:receive_pack).and_return(false) allow(Gitlab.config.gitlab_shell).to receive(:receive_pack).and_return(false)
upload(path, env) do |response| upload(path, env) do |response|
expect(response).to have_http_status(404) expect(response).to have_http_status(403)
end end
end end
end end
...@@ -236,9 +236,9 @@ describe 'Git HTTP requests', lib: true do ...@@ -236,9 +236,9 @@ describe 'Git HTTP requests', lib: true do
end end
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| 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 end
end end
......
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