diff options
| -rw-r--r-- | Gemfile | 2 | ||||
| -rw-r--r-- | Gemfile.lock | 4 | ||||
| -rw-r--r-- | app/controllers/projects/blob_controller.rb | 2 | ||||
| -rw-r--r-- | app/controllers/projects/tree_controller.rb | 2 | ||||
| -rw-r--r-- | app/models/repository.rb | 8 | ||||
| -rw-r--r-- | app/services/spam/spam_verdict_service.rb | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-fix-gitaly-bug-2857.yml | 5 | ||||
| -rw-r--r-- | lib/api/files.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/git/repository.rb | 4 | ||||
| -rw-r--r-- | lib/gitlab/gitaly_client/commit_service.rb | 5 | ||||
| -rw-r--r-- | spec/features/projects/files/user_browses_files_spec.rb | 20 | ||||
| -rw-r--r-- | spec/features/projects/tree/tree_show_spec.rb | 22 | ||||
| -rw-r--r-- | spec/models/repository_spec.rb | 30 | ||||
| -rw-r--r-- | spec/requests/api/files_spec.rb | 22 | ||||
| -rw-r--r-- | spec/services/spam/spam_verdict_service_spec.rb | 6 | ||||
| -rw-r--r-- | spec/simplecov_env.rb | 46 |
16 files changed, 143 insertions, 42 deletions
@@ -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' diff --git a/Gemfile.lock b/Gemfile.lock index cfccca87bf8..6eb3fc1b8dd 100644 --- a/Gemfile.lock +++ b/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) diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index abc1d58cbf1..804f1db4721 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/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' diff --git a/app/controllers/projects/tree_controller.rb b/app/controllers/projects/tree_controller.rb index 9cb345724cc..b4925cd2774 100644 --- a/app/controllers/projects/tree_controller.rb +++ b/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 diff --git a/app/models/repository.rb b/app/models/repository.rb index 911cfc7db38..838b850ddcb 100644 --- a/app/models/repository.rb +++ b/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 diff --git a/app/services/spam/spam_verdict_service.rb b/app/services/spam/spam_verdict_service.rb index 68f1135ae28..be156a0ebeb 100644 --- a/app/services/spam/spam_verdict_service.rb +++ b/app/services/spam/spam_verdict_service.rb @@ -50,10 +50,7 @@ module Spam # @TODO metrics/logging # Expecting: # error: (string or nil) - # result: (string or nil) - verdict = json_result[:verdict] - return unless SUPPORTED_VERDICTS.include?(verdict) - + # verdict: (string or nil) # @TODO log if json_result[:error] json_result[:verdict] diff --git a/changelogs/unreleased/sh-fix-gitaly-bug-2857.yml b/changelogs/unreleased/sh-fix-gitaly-bug-2857.yml new file mode 100644 index 00000000000..111b8d42da1 --- /dev/null +++ b/changelogs/unreleased/sh-fix-gitaly-bug-2857.yml @@ -0,0 +1,5 @@ +--- +title: Fix 500 errors with filenames that contain glob characters +merge_request: 34864 +author: +type: fixed diff --git a/lib/api/files.rb b/lib/api/files.rb index 76ab9a2190b..c525897b8fe 100644 --- a/lib/api/files.rb +++ b/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 diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index ed746163748..f38bc898cc0 100644 --- a/lib/gitlab/git/repository.rb +++ b/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 diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index aed132aaca0..45c1faf9146 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/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 diff --git a/spec/features/projects/files/user_browses_files_spec.rb b/spec/features/projects/files/user_browses_files_spec.rb index 44b5833a8c8..968cfbd17b9 100644 --- a/spec/features/projects/files/user_browses_files_spec.rb +++ b/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) diff --git a/spec/features/projects/tree/tree_show_spec.rb b/spec/features/projects/tree/tree_show_spec.rb index 388fa39874d..7e2a41ad6e6 100644 --- a/spec/features/projects/tree/tree_show_spec.rb +++ b/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 } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index c698b40a4c0..e2a148165ab 100644 --- a/spec/models/repository_spec.rb +++ b/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 diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 198e4f64bcc..a54213a8556 100644 --- a/spec/requests/api/files_spec.rb +++ b/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" diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb index f6d9cd96da5..fe70da7a894 100644 --- a/spec/services/spam/spam_verdict_service_spec.rb +++ b/spec/services/spam/spam_verdict_service_spec.rb @@ -192,8 +192,8 @@ describe Spam::SpamVerdictService do context 'the verdict is an unexpected string' do let(:verdict) { 'this is fine' } - it 'returns nil' do - expect(subject).to be_nil + it 'returns the string' do + expect(subject).to eq verdict end end @@ -209,7 +209,7 @@ describe Spam::SpamVerdictService do let(:verdict) { '' } it 'returns nil' do - expect(subject).to be_nil + expect(subject).to eq verdict end end diff --git a/spec/simplecov_env.rb b/spec/simplecov_env.rb index 92f7eb211d6..d618c83d78e 100644 --- a/spec/simplecov_env.rb +++ b/spec/simplecov_env.rb @@ -37,30 +37,34 @@ module SimpleCovEnv def configure_profile SimpleCov.configure do load_profile 'test_frameworks' - track_files '{app,lib}/**/*.rb' + track_files '{app,config,danger,db,haml_lint,lib,qa,rubocop,scripts,tooling}/**/*.rb' add_filter '/vendor/ruby/' - add_filter 'app/controllers/sherlock/' - add_filter 'config/initializers/' - add_filter 'db/fixtures/' - add_filter 'lib/gitlab/sidekiq_middleware/' - add_filter 'lib/system_check/' + add_filter '/app/controllers/sherlock/' + add_filter '/bin/' + add_filter 'db/fixtures/' # Matches EE files as well + add_filter '/lib/gitlab/sidekiq_middleware/' + add_filter '/lib/system_check/' - add_group 'Channels', 'app/channels' - add_group 'Controllers', 'app/controllers' - add_group 'Finders', 'app/finders' - add_group 'GraphQL', 'app/graphql' - add_group 'Helpers', 'app/helpers' - add_group 'Libraries', 'lib' - add_group 'Mailers', 'app/mailers' - add_group 'Models', 'app/models' - add_group 'Policies', 'app/policies' - add_group 'Presenters', 'app/presenters' - add_group 'Serializers', 'app/serializers' - add_group 'Services', 'app/services' - add_group 'Uploaders', 'app/uploaders' - add_group 'Validators', 'app/validators' - add_group 'Workers', %w(app/jobs app/workers) + add_group 'Channels', 'app/channels' # Matches EE files as well + add_group 'Controllers', 'app/controllers' # Matches EE files as well + add_group 'Finders', 'app/finders' # Matches EE files as well + add_group 'GraphQL', 'app/graphql' # Matches EE files as well + add_group 'Helpers', 'app/helpers' # Matches EE files as well + add_group 'Mailers', 'app/mailers' # Matches EE files as well + add_group 'Models', 'app/models' # Matches EE files as well + add_group 'Policies', 'app/policies' # Matches EE files as well + add_group 'Presenters', 'app/presenters' # Matches EE files as well + add_group 'Serializers', 'app/serializers' # Matches EE files as well + add_group 'Services', 'app/services' # Matches EE files as well + add_group 'Uploaders', 'app/uploaders' # Matches EE files as well + add_group 'Validators', 'app/validators' # Matches EE files as well + add_group 'Workers', %w[app/jobs app/workers] # Matches EE files as well + add_group 'Initializers', %w[config/initializers config/initializers_before_autoloader] # Matches EE files as well + add_group 'Migrations', %w[db/migrate db/optional_migrations db/post_migrate] # Matches EE files as well + add_group 'Libraries', %w[/lib /ee/lib] + add_group 'Tooling', %w[/danger /haml_lint /rubocop /scripts /tooling] + add_group 'QA', '/qa' merge_timeout 365.days end |