From f82d549d26af89cba00005e1a1c9b721c076f7a0 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 7 Dec 2016 13:25:49 +0530
Subject: [PATCH] Accept environment variables from the `pre-receive` script.
1. Starting version 2.11, git changed the way the pre-receive flow works.
- Previously, the new potential objects would be added to the main repo. If the
pre-receive passes, the new objects stay in the repo but are linked up. If
the pre-receive fails, the new objects stay orphaned in the repo, and are
cleaned up during the next `git gc`.
- In 2.11, the new potential objects are added to a temporary "alternate object
directory", that git creates for this purpose. If the pre-receive passes, the
objects from the alternate object directory are migrated to the main repo. If
the pre-receive fails the alternate object directory is simply deleted.
2. In our workflow, the pre-recieve script (in `gitlab-shell) calls the
`/allowed` endpoint, which calls out directly to git to perform
various checks. These direct calls to git do _not_ have the necessary
environment variables set which allow access to the "alternate object
directory" (explained above). Therefore these calls to git are not able to
access any of the new potential objects to be added during this push.
3. We fix this by accepting the relevant environment variables
(GIT_ALTERNATE_OBJECT_DIRECTORIES, GIT_OBJECT_DIRECTORY) on the
`/allowed` endpoint, and then include these environment variables while
calling out to git.
4. This commit includes (whitelisted) these environment variables while making
the "force push" check. A `Gitlab::Git::RevList` module is extracted to
prevent `ForcePush` from being littered with these checks.
---
lib/api/helpers/internal_helpers.rb | 8 ++++++++
lib/api/internal.rb | 6 +++++-
lib/gitlab/checks/change_access.rb | 5 +++--
lib/gitlab/checks/force_push.rb | 4 ++--
lib/gitlab/git/rev_list.rb | 28 ++++++++++++++++++++++++++++
lib/gitlab/git_access.rb | 5 +++--
lib/gitlab/popen.rb | 4 ++--
7 files changed, 51 insertions(+), 9 deletions(-)
create mode 100644 lib/gitlab/git/rev_list.rb
diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb
index eb223c1101d..e8975eb57e0 100644
--- a/lib/api/helpers/internal_helpers.rb
+++ b/lib/api/helpers/internal_helpers.rb
@@ -52,6 +52,14 @@ module API
:push_code
]
end
+
+ def parse_allowed_environment_variables
+ return if params[:env].blank?
+
+ JSON.parse(params[:env])
+
+ rescue JSON::ParserError
+ end
end
end
end
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 7087ce11401..db2d18f935d 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -32,7 +32,11 @@ module API
if wiki?
Gitlab::GitAccessWiki.new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
else
- Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
+ Gitlab::GitAccess.new(actor,
+ project,
+ protocol,
+ authentication_abilities: ssh_authentication_abilities,
+ env: parse_allowed_environment_variables)
end
access_status = access.check(params[:action], params[:changes])
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index cb1065223d4..3d203017d9f 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -3,11 +3,12 @@ module Gitlab
class ChangeAccess
attr_reader :user_access, :project
- def initialize(change, user_access:, project:)
+ def initialize(change, user_access:, project:, env: {})
@oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@branch_name = Gitlab::Git.branch_name(@ref)
@user_access = user_access
@project = project
+ @env = env
end
def exec
@@ -68,7 +69,7 @@ module Gitlab
end
def forced_push?
- Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev)
+ Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev, env: @env)
end
def matching_merge_request?
diff --git a/lib/gitlab/checks/force_push.rb b/lib/gitlab/checks/force_push.rb
index 5fe86553bd0..589525e40ad 100644
--- a/lib/gitlab/checks/force_push.rb
+++ b/lib/gitlab/checks/force_push.rb
@@ -1,14 +1,14 @@
module Gitlab
module Checks
class ForcePush
- def self.force_push?(project, oldrev, newrev)
+ def self.force_push?(project, oldrev, newrev, env: {})
return false if project.empty_repo?
# Created or deleted branch
if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
false
else
- missed_ref, _ = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --git-dir=#{project.repository.path_to_repo} rev-list --max-count=1 #{oldrev} ^#{newrev}))
+ missed_ref, _ = Gitlab::Git::RevList.new(oldrev, newrev, project: project, env: env).execute
missed_ref.present?
end
end
diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb
new file mode 100644
index 00000000000..ecdb7f07744
--- /dev/null
+++ b/lib/gitlab/git/rev_list.rb
@@ -0,0 +1,28 @@
+# Call out to the `git rev-list` command
+
+module Gitlab
+ module Git
+ class RevList
+ def initialize(oldrev, newrev, project:, env: nil)
+ @args = [Gitlab.config.git.bin_path,
+ "--git-dir=#{project.repository.path_to_repo}",
+ "rev-list",
+ "--max-count=1",
+ oldrev,
+ "^#{newrev}"]
+
+ @env = env.slice(*allowed_environment_variables)
+ end
+
+ def execute
+ Gitlab::Popen.popen(@args, nil, @env.slice(*allowed_environment_variables))
+ end
+
+ private
+
+ def allowed_environment_variables
+ %w(GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_OBJECT_DIRECTORY)
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index db07b7c5fcc..c6b6efda360 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -17,12 +17,13 @@ module Gitlab
attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
- def initialize(actor, project, protocol, authentication_abilities:)
+ def initialize(actor, project, protocol, authentication_abilities:, env: {})
@actor = actor
@project = project
@protocol = protocol
@authentication_abilities = authentication_abilities
@user_access = UserAccess.new(user, project: project)
+ @env = env
end
def check(cmd, changes)
@@ -103,7 +104,7 @@ module Gitlab
end
def change_access_check(change)
- Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec
+ Checks::ChangeAccess.new(change, user_access: user_access, project: project, env: @env).exec
end
def protocol_allowed?
diff --git a/lib/gitlab/popen.rb b/lib/gitlab/popen.rb
index cc74bb29087..4bc5cda8cb5 100644
--- a/lib/gitlab/popen.rb
+++ b/lib/gitlab/popen.rb
@@ -5,13 +5,13 @@ module Gitlab
module Popen
extend self
- def popen(cmd, path = nil)
+ def popen(cmd, path = nil, vars = {})
unless cmd.is_a?(Array)
raise "System commands must be given as an array of strings"
end
path ||= Dir.pwd
- vars = { "PWD" => path }
+ vars['PWD'] = path
options = { chdir: path }
unless File.directory?(path)
--
GitLab