Commit 0b5a8a34 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'backport-ee-1525' into 'master'

Backport Git-env code from EE "Fix push rules on Git 2.11" (gitlab-org/gitlab-ee!1525)

See merge request !10547
parents 008e135f 3de11e1a
...@@ -53,12 +53,12 @@ module API ...@@ -53,12 +53,12 @@ module API
] ]
end end
def parse_allowed_environment_variables def parse_env
return if params[:env].blank? return {} if params[:env].blank?
JSON.parse(params[:env]) JSON.parse(params[:env])
rescue JSON::ParserError rescue JSON::ParserError
{}
end end
end end
end end
......
...@@ -11,14 +11,16 @@ module API ...@@ -11,14 +11,16 @@ module API
# Params: # Params:
# key_id - ssh key id for Git over SSH # key_id - ssh key id for Git over SSH
# user_id - user id for Git over HTTP # user_id - user id for Git over HTTP
# protocol - Git access protocol being used, e.g. HTTP or SSH
# project - project path with namespace # project - project path with namespace
# action - git action (git-upload-pack or git-receive-pack) # action - git action (git-upload-pack or git-receive-pack)
# ref - branch name # changes - changes as "oldrev newrev ref", see Gitlab::ChangesList
# forced_push - forced_push
# protocol - Git access protocol being used, e.g. HTTP or SSH
post "/allowed" do post "/allowed" do
status 200 status 200
# Stores some Git-specific env thread-safely
Gitlab::Git::Env.set(parse_env)
actor = actor =
if params[:key_id] if params[:key_id]
Key.find_by(id: params[:key_id]) Key.find_by(id: params[:key_id])
...@@ -30,18 +32,10 @@ module API ...@@ -30,18 +32,10 @@ module API
actor.update_last_used_at if actor.is_a?(Key) actor.update_last_used_at if actor.is_a?(Key)
access = access_checker = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess
if wiki? access_status = access_checker
Gitlab::GitAccessWiki.new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities) .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
else .check(params[:action], params[:changes])
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])
response = { status: access_status.status, message: access_status.message } response = { status: access_status.status, message: access_status.message }
......
...@@ -5,7 +5,7 @@ module Gitlab ...@@ -5,7 +5,7 @@ module Gitlab
attr_reader :user_access, :project, :skip_authorization, :protocol attr_reader :user_access, :project, :skip_authorization, :protocol
def initialize( def initialize(
change, user_access:, project:, env: {}, skip_authorization: false, change, user_access:, project:, skip_authorization: false,
protocol: protocol:
) )
@oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref) @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
...@@ -13,7 +13,6 @@ module Gitlab ...@@ -13,7 +13,6 @@ module Gitlab
@tag_name = Gitlab::Git.tag_name(@ref) @tag_name = Gitlab::Git.tag_name(@ref)
@user_access = user_access @user_access = user_access
@project = project @project = project
@env = env
@skip_authorization = skip_authorization @skip_authorization = skip_authorization
@protocol = protocol @protocol = protocol
end end
...@@ -97,7 +96,7 @@ module Gitlab ...@@ -97,7 +96,7 @@ module Gitlab
end end
def forced_push? def forced_push?
Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev, env: @env) Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev)
end end
def update? def update?
......
module Gitlab module Gitlab
module Checks module Checks
class ForcePush class ForcePush
def self.force_push?(project, oldrev, newrev, env: {}) def self.force_push?(project, oldrev, newrev)
return false if project.empty_repo? return false if project.empty_repo?
# Created or deleted branch # Created or deleted branch
if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev) if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
false false
else else
missed_ref, exit_status = Gitlab::Git::RevList.new(oldrev, newrev, project: project, env: env).execute Gitlab::Git::RevList.new(
path_to_repo: project.repository.path_to_repo,
if exit_status == 0 oldrev: oldrev, newrev: newrev).missed_ref.present?
missed_ref.present?
else
raise "Got a non-zero exit code while calling out to `git rev-list` in the force-push check."
end
end end
end end
end end
......
module Gitlab
module Git
# Ephemeral (per request) storage for environment variables that some Git
# commands may need.
#
# For example, in pre-receive hooks, new objects are put in a temporary
# $GIT_OBJECT_DIRECTORY. Without it set, the new objects cannot be retrieved
# (this would break push rules for instance).
#
# This class is thread-safe via RequestStore.
class Env
WHITELISTED_GIT_VARIABLES = %w[
GIT_OBJECT_DIRECTORY
GIT_ALTERNATE_OBJECT_DIRECTORIES
].freeze
def self.set(env)
return unless RequestStore.active?
RequestStore.store[:gitlab_git_env] = whitelist_git_env(env)
end
def self.all
return {} unless RequestStore.active?
RequestStore.fetch(:gitlab_git_env) { {} }
end
def self.[](key)
all[key]
end
def self.whitelist_git_env(env)
env.select { |key, _| WHITELISTED_GIT_VARIABLES.include?(key.to_s) }.with_indifferent_access
end
end
end
end
...@@ -8,6 +8,10 @@ module Gitlab ...@@ -8,6 +8,10 @@ module Gitlab
class Repository class Repository
include Gitlab::Git::Popen include Gitlab::Git::Popen
ALLOWED_OBJECT_DIRECTORIES_VARIABLES = %w[
GIT_OBJECT_DIRECTORY
GIT_ALTERNATE_OBJECT_DIRECTORIES
].freeze
SEARCH_CONTEXT_LINES = 3 SEARCH_CONTEXT_LINES = 3
NoRepository = Class.new(StandardError) NoRepository = Class.new(StandardError)
...@@ -58,7 +62,7 @@ module Gitlab ...@@ -58,7 +62,7 @@ module Gitlab
end end
def rugged def rugged
@rugged ||= Rugged::Repository.new(path) @rugged ||= Rugged::Repository.new(path, alternates: alternate_object_directories)
rescue Rugged::RepositoryError, Rugged::OSError rescue Rugged::RepositoryError, Rugged::OSError
raise NoRepository.new('no repository for such path') raise NoRepository.new('no repository for such path')
end end
...@@ -978,6 +982,10 @@ module Gitlab ...@@ -978,6 +982,10 @@ module Gitlab
private private
def alternate_object_directories
Gitlab::Git::Env.all.values_at(*ALLOWED_OBJECT_DIRECTORIES_VARIABLES).compact
end
# Get the content of a blob for a given commit. If the blob is a commit # Get the content of a blob for a given commit. If the blob is a commit
# (for submodules) then return the blob's OID. # (for submodules) then return the blob's OID.
def blob_content(commit, blob_name) def blob_content(commit, blob_name)
......
module Gitlab module Gitlab
module Git module Git
class RevList class RevList
attr_reader :project, :env attr_reader :oldrev, :newrev, :path_to_repo
ALLOWED_VARIABLES = %w[GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES].freeze def initialize(path_to_repo:, newrev:, oldrev: nil)
@oldrev = oldrev
def initialize(oldrev, newrev, project:, env: nil) @newrev = newrev
@project = project @path_to_repo = path_to_repo
@env = env.presence || {}
@args = [Gitlab.config.git.bin_path,
"--git-dir=#{project.repository.path_to_repo}",
"rev-list",
"--max-count=1",
oldrev,
"^#{newrev}"]
end end
def execute # This method returns an array of new references
Gitlab::Popen.popen(@args, nil, parse_environment_variables) def new_refs
execute([*base_args, newrev, '--not', '--all'])
end end
def valid? # This methods returns an array of missed references
environment_variables.all? do |(name, value)| def missed_ref
value.to_s.start_with?(project.repository.path_to_repo) execute([*base_args, '--max-count=1', oldrev, "^#{newrev}"])
end
end end
private private
def parse_environment_variables def execute(args)
return {} unless valid? output, status = Gitlab::Popen.popen(args, nil, Gitlab::Git::Env.all.stringify_keys)
unless status.zero?
raise "Got a non-zero exit code while calling out `#{args.join(' ')}`."
end
environment_variables output.split("\n")
end end
def environment_variables def base_args
@environment_variables ||= env.slice(*ALLOWED_VARIABLES).compact [
Gitlab.config.git.bin_path,
"--git-dir=#{path_to_repo}",
'rev-list'
]
end end
end end
end end
......
...@@ -18,13 +18,12 @@ module Gitlab ...@@ -18,13 +18,12 @@ module Gitlab
attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
def initialize(actor, project, protocol, authentication_abilities:, env: {}) def initialize(actor, project, protocol, authentication_abilities:)
@actor = actor @actor = actor
@project = project @project = project
@protocol = protocol @protocol = protocol
@authentication_abilities = authentication_abilities @authentication_abilities = authentication_abilities
@user_access = UserAccess.new(user, project: project) @user_access = UserAccess.new(user, project: project)
@env = env
end end
def check(cmd, changes) def check(cmd, changes)
...@@ -152,7 +151,6 @@ module Gitlab ...@@ -152,7 +151,6 @@ module Gitlab
change, change,
user_access: user_access, user_access: user_access,
project: project, project: project,
env: @env,
skip_authorization: deploy_key?, skip_authorization: deploy_key?,
protocol: protocol protocol: protocol
).exec ).exec
......
require 'spec_helper'
describe Gitlab::Git::Env do
describe "#set" do
context 'with RequestStore.store disabled' do
before do
allow(RequestStore).to receive(:active?).and_return(false)
end
it 'does not store anything' do
described_class.set(GIT_OBJECT_DIRECTORY: 'foo')
expect(described_class.all).to be_empty
end
end
context 'with RequestStore.store enabled' do
before do
allow(RequestStore).to receive(:active?).and_return(true)
end
it 'whitelist some `GIT_*` variables and stores them using RequestStore' do
described_class.set(
GIT_OBJECT_DIRECTORY: 'foo',
GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar',
GIT_EXEC_PATH: 'baz',
PATH: '~/.bin:/bin')
expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo')
expect(described_class[:GIT_ALTERNATE_OBJECT_DIRECTORIES]).to eq('bar')
expect(described_class[:GIT_EXEC_PATH]).to be_nil
expect(described_class[:bar]).to be_nil
end
end
end
describe "#all" do
context 'with RequestStore.store enabled' do
before do
allow(RequestStore).to receive(:active?).and_return(true)
described_class.set(
GIT_OBJECT_DIRECTORY: 'foo',
GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar')
end
it 'returns an env hash' do
expect(described_class.all).to eq({
'GIT_OBJECT_DIRECTORY' => 'foo',
'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
})
end
end
end
describe "#[]" do
context 'with RequestStore.store enabled' do
before do
allow(RequestStore).to receive(:active?).and_return(true)
end
before do
described_class.set(
GIT_OBJECT_DIRECTORY: 'foo',
GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar')
end
it 'returns a stored value for an existing key' do
expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo')
end
it 'returns nil for an non-existing key' do
expect(described_class[:foo]).to be_nil
end
end
end
describe 'thread-safety' do
context 'with RequestStore.store enabled' do
before do
allow(RequestStore).to receive(:active?).and_return(true)
described_class.set(GIT_OBJECT_DIRECTORY: 'foo')
end
it 'is thread-safe' do
another_thread = Thread.new do
described_class.set(GIT_OBJECT_DIRECTORY: 'bar')
Thread.stop
described_class[:GIT_OBJECT_DIRECTORY]
end
# Ensure another_thread runs first
sleep 0.1 until another_thread.stop?
expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo')
another_thread.run
expect(another_thread.value).to eq('bar')
end
end
end
end
...@@ -40,6 +40,36 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -40,6 +40,36 @@ describe Gitlab::Git::Repository, seed_helper: true do
end end
end end
describe "#rugged" do
context 'with no Git env stored' do
before do
expect(Gitlab::Git::Env).to receive(:all).and_return({})
end
it "whitelist some variables and pass them via the alternates keyword argument" do
expect(Rugged::Repository).to receive(:new).with(repository.path, alternates: [])
repository.rugged
end
end
context 'with some Git env stored' do
before do
expect(Gitlab::Git::Env).to receive(:all).and_return({
'GIT_OBJECT_DIRECTORY' => 'foo',
'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar',
'GIT_OTHER' => 'another_env'
})
end
it "whitelist some variables and pass them via the alternates keyword argument" do
expect(Rugged::Repository).to receive(:new).with(repository.path, alternates: %w[foo bar])
repository.rugged
end
end
end
describe "#discover_default_branch" do describe "#discover_default_branch" do
let(:master) { 'master' } let(:master) { 'master' }
let(:feature) { 'feature' } let(:feature) { 'feature' }
......
...@@ -3,58 +3,54 @@ require 'spec_helper' ...@@ -3,58 +3,54 @@ require 'spec_helper'
describe Gitlab::Git::RevList, lib: true do describe Gitlab::Git::RevList, lib: true do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
context "validations" do before do
described_class::ALLOWED_VARIABLES.each do |var| expect(Gitlab::Git::Env).to receive(:all).and_return({
context var do GIT_OBJECT_DIRECTORY: 'foo',
it "accepts values starting with the project repo path" do GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar'
env = { var => "#{project.repository.path_to_repo}/objects" } })
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) end
expect(rev_list).to be_valid context "#new_refs" do
end let(:rev_list) { Gitlab::Git::RevList.new(newrev: 'newrev', path_to_repo: project.repository.path_to_repo) }
it "rejects values starting not with the project repo path" do it 'calls out to `popen`' do
env = { var => "/some/other/path" } expect(Gitlab::Popen).to receive(:popen).with([
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) Gitlab.config.git.bin_path,
"--git-dir=#{project.repository.path_to_repo}",
expect(rev_list).not_to be_valid 'rev-list',
end 'newrev',
'--not',
it "rejects values containing the project repo path but not starting with it" do '--all'
env = { var => "/some/other/path/#{project.repository.path_to_repo}" } ],
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) nil,
{
expect(rev_list).not_to be_valid 'GIT_OBJECT_DIRECTORY' => 'foo',
end 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
}).and_return(["sha1\nsha2", 0])
it "ignores nil values" do
env = { var => nil } expect(rev_list.new_refs).to eq(%w[sha1 sha2])
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) end
end
expect(rev_list).to be_valid
end context "#missed_ref" do
end let(:rev_list) { Gitlab::Git::RevList.new(oldrev: 'oldrev', newrev: 'newrev', path_to_repo: project.repository.path_to_repo) }
end
end it 'calls out to `popen`' do
expect(Gitlab::Popen).to receive(:popen).with([
context "#execute" do Gitlab.config.git.bin_path,
let(:env) { { "GIT_OBJECT_DIRECTORY" => project.repository.path_to_repo } } "--git-dir=#{project.repository.path_to_repo}",
let(:rev_list) { Gitlab::Git::RevList.new('oldrev', 'newrev', project: project, env: env) } 'rev-list',
'--max-count=1',
it "calls out to `popen` without environment variables if the record is invalid" do 'oldrev',
allow(rev_list).to receive(:valid?).and_return(false) '^newrev'
],
expect(Open3).to receive(:popen3).with(hash_excluding(env), any_args) nil,
{
rev_list.execute 'GIT_OBJECT_DIRECTORY' => 'foo',
end 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
}).and_return(["sha1\nsha2", 0])
it "calls out to `popen` with environment variables if the record is valid" do
allow(rev_list).to receive(:valid?).and_return(true) expect(rev_list.missed_ref).to eq(%w[sha1 sha2])
expect(Open3).to receive(:popen3).with(hash_including(env), any_args)
rev_list.execute
end end
end end
end end
...@@ -153,6 +153,22 @@ describe API::Internal, api: true do ...@@ -153,6 +153,22 @@ describe API::Internal, api: true do
project.team << [user, :developer] project.team << [user, :developer]
end end
context 'with env passed as a JSON' do
it 'sets env in RequestStore' do
expect(Gitlab::Git::Env).to receive(:set).with({
'GIT_OBJECT_DIRECTORY' => 'foo',
'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
})
push(key, project.wiki, env: {
GIT_OBJECT_DIRECTORY: 'foo',
GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar'
}.to_json)
expect(response).to have_http_status(200)
end
end
context "git push with project.wiki" do context "git push with project.wiki" do
it 'responds with success' do it 'responds with success' do
push(key, project.wiki) push(key, project.wiki)
...@@ -463,7 +479,7 @@ describe API::Internal, api: true do ...@@ -463,7 +479,7 @@ describe API::Internal, api: true do
) )
end end
def push(key, project, protocol = 'ssh') def push(key, project, protocol = 'ssh', env: nil)
post( post(
api("/internal/allowed"), api("/internal/allowed"),
changes: 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master', changes: 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master',
...@@ -471,7 +487,8 @@ describe API::Internal, api: true do ...@@ -471,7 +487,8 @@ describe API::Internal, api: true do
project: project.repository.path_to_repo, project: project.repository.path_to_repo,
action: 'git-receive-pack', action: 'git-receive-pack',
secret_token: secret_token, secret_token: secret_token,
protocol: protocol protocol: protocol,
env: env
) )
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