Merge branch 'sh-fix-gitaly-bug-2857' into 'master'

Fix 500 errors with filenames that contain glob characters

Closes gitaly#2857

See merge request gitlab-org/gitlab!34864

Gemfile

@@ -453,7 +453,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 13.1.0.pre.rc1'
+gem 'gitaly', '~> 13.2.0.pre.rc1'
 
 gem 'grpc', '~> 1.24.0'
 

Gemfile.lock

@@ -377,7 +377,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     git (1.5.0)
-    gitaly (13.1.0.pre.rc1)
+    gitaly (13.2.0.pre.rc1)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-chronic (0.10.5)
@@ -1237,7 +1237,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 13.1.0.pre.rc1)
+  gitaly (~> 13.2.0.pre.rc1)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-labkit (= 0.12.0)

app/controllers/projects/blob_controller.rb

@@ -214,7 +214,7 @@ class Projects::BlobController < Projects::ApplicationController
     environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit }
     environment_params[:find_latest] = true
     @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
-    @last_commit = @repository.last_commit_for_path(@commit.id, @blob.path)
+    @last_commit = @repository.last_commit_for_path(@commit.id, @blob.path, literal_pathspec: true)
     @code_navigation_path = Gitlab::CodeNavigationPath.new(@project, @blob.commit_id).full_json_path_for(@blob.path)
 
     render 'show'

app/controllers/projects/tree_controller.rb

@@ -34,7 +34,7 @@ class Projects::TreeController < Projects::ApplicationController
       format.html do
         lfs_blob_ids if Feature.disabled?(:vue_file_list, @project, default_enabled: true)
 
-        @last_commit = @repository.last_commit_for_path(@commit.id, @tree.path) || @commit
+        @last_commit = @repository.last_commit_for_path(@commit.id, @tree.path, literal_pathspec: true) || @commit
       end
     end
   end

app/models/repository.rb

@@ -684,16 +684,16 @@ class Repository
     end
   end
 
-  def last_commit_for_path(sha, path)
-    commit = raw_repository.last_commit_for_path(sha, path)
+  def last_commit_for_path(sha, path, literal_pathspec: false)
+    commit = raw_repository.last_commit_for_path(sha, path, literal_pathspec: literal_pathspec)
     ::Commit.new(commit, container) if commit
   end
 
-  def last_commit_id_for_path(sha, path)
+  def last_commit_id_for_path(sha, path, literal_pathspec: false)
     key = path.blank? ? "last_commit_id_for_path:#{sha}" : "last_commit_id_for_path:#{sha}:#{Digest::SHA1.hexdigest(path)}"
 
     cache.fetch(key) do
-      last_commit_for_path(sha, path)&.id
+      last_commit_for_path(sha, path, literal_pathspec: literal_pathspec)&.id
     end
   end
 

changelogs/unreleased/sh-fix-gitaly-bug-2857.yml

+---
+title: Fix 500 errors with filenames that contain glob characters
+merge_request: 34864
+author:
+type: fixed

lib/api/files.rb

@@ -56,7 +56,7 @@ module API
           ref: params[:ref],
           blob_id: @blob.id,
           commit_id: @commit.id,
-          last_commit_id: @repo.last_commit_id_for_path(@commit.sha, params[:file_path])
+          last_commit_id: @repo.last_commit_id_for_path(@commit.sha, params[:file_path], literal_pathspec: true)
         }
       end
 

lib/gitlab/git/repository.rb

@@ -1008,9 +1008,9 @@ module Gitlab
         end
       end
 
-      def last_commit_for_path(sha, path)
+      def last_commit_for_path(sha, path, literal_pathspec: false)
         wrapped_gitaly_errors do
-          gitaly_commit_client.last_commit_for_path(sha, path)
+          gitaly_commit_client.last_commit_for_path(sha, path, literal_pathspec: literal_pathspec)
         end
       end
 

lib/gitlab/gitaly_client/commit_service.rb

@@ -180,11 +180,12 @@ module Gitlab
         end
       end
 
-      def last_commit_for_path(revision, path)
+      def last_commit_for_path(revision, path, literal_pathspec: false)
         request = Gitaly::LastCommitForPathRequest.new(
           repository: @gitaly_repo,
           revision: encode_binary(revision),
-          path: encode_binary(path.to_s)
+          path: encode_binary(path.to_s),
+          literal_pathspec: literal_pathspec
         )
 
         gitaly_commit = GitalyClient.call(@repository.storage, :commit_service, :last_commit_for_path, request, timeout: GitalyClient.fast_timeout).commit

spec/features/projects/files/user_browses_files_spec.rb

@@ -3,6 +3,8 @@
 require "spec_helper"
 
 RSpec.describe "User browses files" do
+  include RepoHelpers
+
   let(:fork_message) do
     "You're not allowed to make changes to this project directly. "\
     "A fork of this project has been created that you can make changes in, so you can submit a merge request."
@@ -339,6 +341,24 @@ RSpec.describe "User browses files" do
     end
   end
 
+  context "when browsing a file with glob characters" do
+    let(:filename) { ':wq' }
+    let(:newrev) { project.repository.commit('master').sha }
+
+    before do
+      create_file_in_repo(project, 'master', 'master', filename, 'Test file')
+      path = File.join('master', filename)
+
+      visit(project_blob_path(project, path))
+    end
+
+    it "shows a raw file content" do
+      click_link("Open raw")
+
+      expect(source).to eq("") # Body is filled in by gitlab-workhorse
+    end
+  end
+
   context "when browsing a raw file" do
     before do
       path = File.join(RepoHelpers.sample_commit.id, RepoHelpers.sample_blob.path)

spec/features/projects/tree/tree_show_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe 'Projects tree', :js do
+  include RepoHelpers
+
   let(:user) { create(:user) }
   let(:project) { create(:project, :repository) }
   let(:gravatar_enabled) { true }
@@ -47,6 +49,26 @@ RSpec.describe 'Projects tree', :js do
     expect(page).not_to have_selector('.flash-alert')
   end
 
+  context "with a tree that contains glob characters" do
+    let(:path) { ':wq' }
+    let(:filename) { File.join(path, 'test.txt') }
+    let(:newrev) { project.repository.commit('master').sha }
+    let(:message) { 'Glob characters'}
+
+    before do
+      create_file_in_repo(project, 'master', 'master', filename, 'Test file', commit_message: message)
+      visit project_tree_path(project, File.join('master', path))
+      wait_for_requests
+    end
+
+    # Disabled until https://gitlab.com/gitlab-org/gitaly/-/issues/2888 is resolved
+    xit "renders tree table without errors" do
+      expect(page).to have_selector('.tree-item')
+      expect(page).to have_content('test.txt')
+      expect(page).to have_content(message)
+    end
+  end
+
   context 'gravatar disabled' do
     let(:gravatar_enabled) { false }
 

spec/models/repository_spec.rb

@@ -252,6 +252,21 @@ describe Repository do
         end
       end
     end
+
+    context 'with filename with glob characters' do
+      let(:filename) { ':wq' }
+      let(:newrev) { project.repository.commit('master').sha }
+
+      before do
+        create_file_in_repo(project, 'master', 'master', filename, 'Test file')
+      end
+
+      subject { repository.last_commit_for_path('master', filename, literal_pathspec: true).id }
+
+      it 'returns a commit SHA' do
+        expect(subject).to eq(newrev)
+      end
+    end
   end
 
   describe '#last_commit_id_for_path' do
@@ -276,6 +291,21 @@ describe Repository do
         end
       end
     end
+
+    context 'with filename with glob characters' do
+      let(:filename) { ':wq' }
+      let(:newrev) { project.repository.commit('master').sha }
+
+      before do
+        create_file_in_repo(project, 'master', 'master', filename, 'Test file')
+      end
+
+      subject { repository.last_commit_id_for_path('master', filename, literal_pathspec: true) }
+
+      it 'returns a commit SHA' do
+        expect(subject).to eq(newrev)
+      end
+    end
   end
 
   describe '#commits' do

spec/requests/api/files_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe API::Files do
+  include RepoHelpers
+
   let(:user) { create(:user) }
   let!(:project) { create(:project, :repository, namespace: user.namespace ) }
   let(:guest) { create(:user) { |u| project.add_guest(u) } }
@@ -183,6 +185,26 @@ describe API::Files do
         expect(response.content_type).to eq('application/json')
       end
 
+      context 'with filename with glob characters' do
+        let(:file_path) { ':wq' }
+        let(:newrev) { project.repository.commit('master').sha }
+
+        before do
+          create_file_in_repo(project, 'master', 'master', file_path, 'Test file')
+        end
+
+        it 'returns JSON wth commit SHA' do
+          params[:ref] = 'master'
+
+          get api(route(file_path), api_user), params: params
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['file_path']).to eq(file_path)
+          expect(json_response['file_name']).to eq(file_path)
+          expect(json_response['last_commit_id']).to eq(newrev)
+        end
+      end
+
       it 'returns file by commit sha' do
         # This file is deleted on HEAD
         file_path = "files%2Fjs%2Fcommit%2Ejs%2Ecoffee"