diff options
-rw-r--r-- | app/validators/git_environment_variables_validator.rb | 13 | ||||
-rw-r--r-- | lib/gitlab/git/rev_list.rb | 16 | ||||
-rw-r--r-- | spec/lib/gitlab/git/rev_list_spec.rb | 36 | ||||
-rw-r--r-- | spec/validators/git_environment_variables_validator_spec.rb | 64 |
4 files changed, 126 insertions, 3 deletions
diff --git a/app/validators/git_environment_variables_validator.rb b/app/validators/git_environment_variables_validator.rb new file mode 100644 index 00000000000..92041e0a773 --- /dev/null +++ b/app/validators/git_environment_variables_validator.rb @@ -0,0 +1,13 @@ +class GitEnvironmentVariablesValidator < ActiveModel::EachValidator + def validate_each(record, attribute, env) + variables_to_validate = %w(GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES) + + variables_to_validate.each do |variable_name| + variable_value = env[variable_name] + + if variable_value.present? && !(variable_value =~ /^#{record.project.repository.path_to_repo}/) + record.errors.add(attribute, "The #{variable_name} variable must start with the project repo path") + end + end + end +end diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb index ecdb7f07744..d8c78d806ea 100644 --- a/lib/gitlab/git/rev_list.rb +++ b/lib/gitlab/git/rev_list.rb @@ -3,19 +3,29 @@ module Gitlab module Git class RevList + include ActiveModel::Validations + + validates :env, git_environment_variables: true + + attr_reader :project, :env + def initialize(oldrev, newrev, project:, env: nil) + @project = project + @env = env.presence || {} @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)) + if self.valid? + Gitlab::Popen.popen(@args, nil, @env.slice(*allowed_environment_variables)) + else + Gitlab::Popen.popen(@args) + end end private diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb new file mode 100644 index 00000000000..cdfbff5658c --- /dev/null +++ b/spec/lib/gitlab/git/rev_list_spec.rb @@ -0,0 +1,36 @@ +require 'spec_helper' +require 'validators/git_environment_variables_validator_spec' + +describe Gitlab::Git::RevList, lib: true do + let(:project) { create(:project) } + + context "validations" do + it_behaves_like( + "validated git environment variables", + ->(env, project) { Gitlab::Git::RevList.new('oldrev', 'newrev', project: project, env: env) } + ) + end + + context "#execute" do + let(:env) { { "GIT_OBJECT_DIRECTORY" => project.repository.path_to_repo } } + let(:rev_list) { Gitlab::Git::RevList.new('oldrev', 'newrev', project: project, env: env) } + + it "calls out to `popen` without environment variables if the record is invalid" do + allow(rev_list).to receive(:valid?).and_return(false) + allow(Open3).to receive(:popen3) + + rev_list.execute + + expect(Open3).to have_received(:popen3).with(hash_excluding(env), any_args) + end + + it "calls out to `popen` with environment variables if the record is valid" do + allow(rev_list).to receive(:valid?).and_return(true) + allow(Open3).to receive(:popen3) + + rev_list.execute + + expect(Open3).to have_received(:popen3).with(hash_including(env), any_args) + end + end +end diff --git a/spec/validators/git_environment_variables_validator_spec.rb b/spec/validators/git_environment_variables_validator_spec.rb new file mode 100644 index 00000000000..81b028b6572 --- /dev/null +++ b/spec/validators/git_environment_variables_validator_spec.rb @@ -0,0 +1,64 @@ +require 'spec_helper' + +shared_examples_for "validated git environment variables" do |record_fn| + subject { GitEnvironmentVariablesValidator.new(attributes: ['env']) } + let(:project) { create(:project) } + + context "GIT_OBJECT_DIRECTORY" do + it "accepts values starting with the project repo path" do + env = { "GIT_OBJECT_DIRECTORY" => "#{project.repository.path_to_repo}/objects" } + record = record_fn[env, project] + + subject.validate_each(record, 'env', env) + + expect(record).to be_valid, "expected #{project.repository.path_to_repo}" + end + + it "rejects values starting not with the project repo path" do + env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path" } + record = record_fn[env, project] + + subject.validate_each(record, 'env', env) + + expect(record).to be_invalid + end + + it "rejects values containing the project repo path but not starting with it" do + env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path/#{project.repository.path_to_repo}" } + record = record_fn[env, project] + + subject.validate_each(record, 'env', env) + + expect(record).to be_invalid + end + end + + context "GIT_ALTERNATE_OBJECT_DIRECTORIES" do + it "accepts values starting with the project repo path" do + env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => project.repository.path_to_repo } + record = record_fn[env, project] + + subject.validate_each(record, 'env', env) + + expect(record).to be_valid, "expected #{project.repository.path_to_repo}" + end + + it "rejects values starting not with the project repo path" do + env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/some/other/path" } + record = record_fn[env, project] + + subject.validate_each(record, 'env', env) + + expect(record).to be_invalid + end + + it "rejects values containing the project repo path but not starting with it" do + env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/some/other/path/#{project.repository.path_to_repo}" } + record = record_fn[env, project] + + subject.validate_each(record, 'env', env) + + expect(record).to be_invalid + end + end +end |