Validate environment variables in `Gitlab::Git::RevList`

The list of environment variables in `Gitlab::Git::RevList` need to be validate
to make sure that they don't reference any other project on disk.

This commit mixes in `ActiveModel::Validations` into `Gitlab::Git::RevList`, and
validates that the environment variables are on the level (using a custom
validator class). If the validations fail, the force push is still executed
without any environment variables set.

Add specs for the validation using shared examples.

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