Merge branch 'feature/ps/add-commit_id-validation' into 'master'

Add verification of commit id before calling remote

See merge request gitlab-org/gitlab!31609

lib/gitlab/git/commit.rb

@@ -57,11 +57,8 @@ module Gitlab
           # Already a commit?
           return commit_id if commit_id.is_a?(Gitlab::Git::Commit)
 
-          # Some weird thing?
-          return unless commit_id.is_a?(String)
-
           # This saves us an RPC round trip.
-          return if commit_id.include?(':')
+          return unless valid?(commit_id)
 
           commit = find_commit(repo, commit_id)
 
@@ -431,6 +428,15 @@ module Gitlab
       def fetch_body_from_gitaly
         self.class.get_message(@repository, id)
       end
+
+      def self.valid?(commit_id)
+        commit_id.is_a?(String) && !(
+          commit_id.start_with?('-') ||
+            commit_id.include?(':') ||
+            commit_id.include?("\x00") ||
+            commit_id.match?(/\s/)
+        )
+      end
     end
   end
 end

spec/lib/gitlab/git/commit_spec.rb

@@ -161,6 +161,26 @@ describe Gitlab::Git::Commit, :seed_helper do
         expect(described_class.find(repository, "+123_4532530XYZ")).to be_nil
       end
 
+      it "returns nil for id started with dash" do
+        expect(described_class.find(repository, "-HEAD")).to be_nil
+      end
+
+      it "returns nil for id containing colon" do
+        expect(described_class.find(repository, "HEAD:")).to be_nil
+      end
+
+      it "returns nil for id containing space" do
+        expect(described_class.find(repository, "HE AD")).to be_nil
+      end
+
+      it "returns nil for id containing tab" do
+        expect(described_class.find(repository, "HE\tAD")).to be_nil
+      end
+
+      it "returns nil for id containing NULL" do
+        expect(described_class.find(repository, "HE\x00AD")).to be_nil
+      end
+
       context 'with broken repo' do
         let(:repository) { Gitlab::Git::Repository.new('default', TEST_BROKEN_REPO_PATH, '', 'group/project') }