Commit a47b3d6b authored by Stan Hu's avatar Stan Hu

Fix 500 errors with filenames that contain glob characters

By default, Git allows the pathspec to have glob patterns
(https://css-tricks.com/git-pathspecs-and-how-to-use-them), but the
LastCommitForPath RPC now supports a `literal_pathspec` to disable this
(https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2285). We now use
this whenever we look up specific files in the repository.

Closes https://gitlab.com/gitlab-org/gitaly/-/issues/2857
parent cd953871
......@@ -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'
......
......@@ -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)
......
......@@ -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'
......
......@@ -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
......
......@@ -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
......
---
title: Fix 500 errors with filenames that contain glob characters
merge_request: 34864
author:
type: fixed
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -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 }
......
......@@ -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
......
......@@ -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"
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment