Client-side fix for Gitaly TreeEntry buggitaly-tree-entry-dot-dot

3 files changed, 30 insertions, 49 deletions

diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index 341768752dc..74240cedc9d 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -493,13 +493,18 @@ module Gitlab
       def tree_entry(path)
         return unless path.present?
 
-        @repository.gitaly_migrate(:commit_tree_entry) do |is_migrated|
-          if is_migrated
-            gitaly_tree_entry(path)
-          else
-            rugged_tree_entry(path)
-          end
-        end
+        # We're only interested in metadata, so limit actual data to 1 byte
+        # since Gitaly doesn't support "send no data" option.
+        entry = @repository.gitaly_commit_client.tree_entry(id, path, 1)
+        return unless entry
+
+        # To be compatible with the rugged format
+        entry = entry.to_h
+        entry.delete(:data)
+        entry[:name] = File.basename(path)
+        entry[:type] = entry[:type].downcase
+
+        entry
       end
 
       def to_gitaly_commit
@@ -562,28 +567,6 @@ module Gitlab
         SERIALIZE_KEYS
       end
 
-      def gitaly_tree_entry(path)
-        # We're only interested in metadata, so limit actual data to 1 byte
-        # since Gitaly doesn't support "send no data" option.
-        entry = @repository.gitaly_commit_client.tree_entry(id, path, 1)
-        return unless entry
-
-        # To be compatible with the rugged format
-        entry = entry.to_h
-        entry.delete(:data)
-        entry[:name] = File.basename(path)
-        entry[:type] = entry[:type].downcase
-
-        entry
-      end
-
-      # Is this the same as Blob.find_entry_by_path ?
-      def rugged_tree_entry(path)
-        rugged_commit.tree.path(path)
-      rescue Rugged::TreeError
-        nil
-      end
-
       def gitaly_commit_author_from_rugged(author_or_committer)
         Gitaly::CommitAuthor.new(
           name: author_or_committer[:name].b,
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 7f2e6441f16..077297b9d6d 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -76,6 +76,13 @@ module Gitlab
       end
 
       def tree_entry(ref, path, limit = nil)
+        if Pathname.new(path).cleanpath.to_s.start_with?('../')
+          # The TreeEntry RPC should return an empty reponse in this case but in
+          # Gitaly 0.107.0 and earlier we get an exception instead. This early return
+          # saves us a Gitaly roundtrip while also avoiding the exception.
+          return
+        end
+
         request = Gitaly::TreeEntryRequest.new(
           repository: @gitaly_repo,
           revision: encode_binary(ref),
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index 090f91168ad..5157d8fc645 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -514,30 +514,21 @@ eos
   end
 
   describe '#uri_type' do
-    shared_examples 'URI type' do
-      it 'returns the URI type at the given path' do
-        expect(commit.uri_type('files/html')).to be(:tree)
-        expect(commit.uri_type('files/images/logo-black.png')).to be(:raw)
-        expect(project.commit('video').uri_type('files/videos/intro.mp4')).to be(:raw)
-        expect(commit.uri_type('files/js/application.js')).to be(:blob)
-      end
-
-      it "returns nil if the path doesn't exists" do
-        expect(commit.uri_type('this/path/doesnt/exist')).to be_nil
-      end
-
-      it 'is nil if the path is nil or empty' do
-        expect(commit.uri_type(nil)).to be_nil
-        expect(commit.uri_type("")).to be_nil
-      end
+    it 'returns the URI type at the given path' do
+      expect(commit.uri_type('files/html')).to be(:tree)
+      expect(commit.uri_type('files/images/logo-black.png')).to be(:raw)
+      expect(project.commit('video').uri_type('files/videos/intro.mp4')).to be(:raw)
+      expect(commit.uri_type('files/js/application.js')).to be(:blob)
     end
 
-    context 'when Gitaly commit_tree_entry feature is enabled' do
-      it_behaves_like 'URI type'
+    it "returns nil if the path doesn't exists" do
+      expect(commit.uri_type('this/path/doesnt/exist')).to be_nil
+      expect(commit.uri_type('../path/doesnt/exist')).to be_nil
     end
 
-    context 'when Gitaly commit_tree_entry feature is disabled', :disable_gitaly do
-      it_behaves_like 'URI type'
+    it 'is nil if the path is nil or empty' do
+      expect(commit.uri_type(nil)).to be_nil
+      expect(commit.uri_type("")).to be_nil
     end
   end