From 0851e2f024dfb983596d601f417ff485f33f39f8 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Sun, 13 Nov 2022 21:07:55 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- lib/gitlab/gitaly_client/operation_service.rb | 41 +++- .../package_registry/helm_registry_spec.rb | 2 +- .../parsers/sbom/cyclonedx_properties_spec.rb | 8 +- .../gitaly_client/operation_service_spec.rb | 227 ++++++++++++++++-- 4 files changed, 254 insertions(+), 24 deletions(-) diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index 7835fb32f59..298162a5e2c 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -435,9 +435,25 @@ module Gitlab end Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update) - end - # rubocop:enable Metrics/ParameterLists + rescue GRPC::BadStatus => e + detailed_error = GitalyClient.decode_detailed_error(e) + case detailed_error&.error + when :access_check + access_check_error = detailed_error.access_check + # These messages were returned from internal/allowed API calls + raise Gitlab::Git::PreReceiveError.new(fallback_message: access_check_error.error_message) + when :custom_hook + raise Gitlab::Git::PreReceiveError.new(custom_hook_error_message(detailed_error.custom_hook), + fallback_message: e.details) + when :index_update + raise Gitlab::Git::Index::IndexError, index_error_message(detailed_error.index_update) + else + raise e + end + end + + # rubocop:enable Metrics/ParameterLists def user_commit_patches(user, branch_name, patches) header = Gitaly::UserApplyPatchRequest::Header.new( repository: @gitaly_repo, @@ -575,6 +591,27 @@ module Gitlab custom_hook_output = custom_hook_error.stderr.presence || custom_hook_error.stdout EncodingHelper.encode!(custom_hook_output) end + + def index_error_message(index_error) + encoded_path = EncodingHelper.encode!(index_error.path) + + case index_error.error_type + when :ERROR_TYPE_EMPTY_PATH + "Received empty path" + when :ERROR_TYPE_INVALID_PATH + "Invalid path: #{encoded_path}" + when :ERROR_TYPE_DIRECTORY_EXISTS + "Directory already exists: #{encoded_path}" + when :ERROR_TYPE_DIRECTORY_TRAVERSAL + "Directory traversal in path escapes repository: #{encoded_path}" + when :ERROR_TYPE_FILE_EXISTS + "File already exists: #{encoded_path}" + when :ERROR_TYPE_FILE_NOT_FOUND + "File not found: #{encoded_path}" + else + "Unknown error performing git operation" + end + end end end end diff --git a/qa/qa/specs/features/browser_ui/5_package/package_registry/helm_registry_spec.rb b/qa/qa/specs/features/browser_ui/5_package/package_registry/helm_registry_spec.rb index 4c15b7c7f99..2cefb95db98 100644 --- a/qa/qa/specs/features/browser_ui/5_package/package_registry/helm_registry_spec.rb +++ b/qa/qa/specs/features/browser_ui/5_package/package_registry/helm_registry_spec.rb @@ -2,7 +2,7 @@ module QA RSpec.describe 'Package', :skip_live_env, :orchestrated, :packages, :object_storage, product_group: :package_registry do - describe 'Helm Registry' do + describe 'Helm Registry', quarantine: { type: :broken, issue: "https://gitlab.com/gitlab-org/gitlab/-/issues/382262" } do using RSpec::Parameterized::TableSyntax include Runtime::Fixtures include Support::Helpers::MaskToken diff --git a/spec/lib/gitlab/ci/parsers/sbom/cyclonedx_properties_spec.rb b/spec/lib/gitlab/ci/parsers/sbom/cyclonedx_properties_spec.rb index 38b229e0dd8..f09b85aa2c7 100644 --- a/spec/lib/gitlab/ci/parsers/sbom/cyclonedx_properties_spec.rb +++ b/spec/lib/gitlab/ci/parsers/sbom/cyclonedx_properties_spec.rb @@ -3,7 +3,7 @@ require 'fast_spec_helper' RSpec.describe Gitlab::Ci::Parsers::Sbom::CyclonedxProperties do - subject(:parse_source) { described_class.parse_source(properties) } + subject(:parse_source_from_properties) { described_class.parse_source(properties) } context 'when properties are nil' do let(:properties) { nil } @@ -50,9 +50,9 @@ RSpec.describe Gitlab::Ci::Parsers::Sbom::CyclonedxProperties do end it 'does not call dependency_scanning parser' do - expect(Gitlab::Ci::Parsers::Sbom::Source::DependencyScanning).not_to receive(:parse_source) + expect(Gitlab::Ci::Parsers::Sbom::Source::DependencyScanning).not_to receive(:source) - parse_source + parse_source_from_properties end end @@ -82,7 +82,7 @@ RSpec.describe Gitlab::Ci::Parsers::Sbom::CyclonedxProperties do it 'passes only supported properties to the dependency scanning parser' do expect(Gitlab::Ci::Parsers::Sbom::Source::DependencyScanning).to receive(:source).with(expected_input) - parse_source + parse_source_from_properties end end end diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb index 7e8aaa3cdf4..604feeea325 100644 --- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb @@ -830,32 +830,225 @@ RSpec.describe Gitlab::GitalyClient::OperationService do 'master', repository) end - before do - expect_any_instance_of(Gitaly::OperationService::Stub) - .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash)) - .and_return(response) - end + context 'with unstructured errors' do + before do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash)) + .and_return(response) + end - context 'when a pre_receive_error is present' do - let(:response) { Gitaly::UserCommitFilesResponse.new(pre_receive_error: "GitLab: something failed") } + context 'when a pre_receive_error is present' do + let(:response) { Gitaly::UserCommitFilesResponse.new(pre_receive_error: "GitLab: something failed") } - it 'raises a PreReceiveError' do - expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed") + it 'raises a PreReceiveError' do + expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed") + end + end + + context 'when an index_error is present' do + let(:response) { Gitaly::UserCommitFilesResponse.new(index_error: "something failed") } + + it 'raises an IndexError' do + expect { subject }.to raise_error(Gitlab::Git::Index::IndexError, "something failed") + end + end + + context 'when branch_update is nil' do + let(:response) { Gitaly::UserCommitFilesResponse.new } + + it { expect(subject).to be_nil } end end - context 'when an index_error is present' do - let(:response) { Gitaly::UserCommitFilesResponse.new(index_error: "something failed") } + context 'with structured errors' do + context 'with AccessCheckError' do + before do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash)) + .and_raise(raised_error) + end - it 'raises a PreReceiveError' do - expect { subject }.to raise_error(Gitlab::Git::Index::IndexError, "something failed") + let(:raised_error) do + new_detailed_error( + GRPC::Core::StatusCodes::PERMISSION_DENIED, + "error updating file", + Gitaly::UserCommitFilesError.new( + access_check: Gitaly::AccessCheckError.new( + error_message: "something went wrong" + ))) + end + + it 'raises a PreReceiveError' do + expect { subject }.to raise_error do |error| + expect(error).to be_a(Gitlab::Git::PreReceiveError) + expect(error.message).to eq("something went wrong") + end + end end - end - context 'when branch_update is nil' do - let(:response) { Gitaly::UserCommitFilesResponse.new } + context 'with IndexError' do + let(:status_code) { nil } + let(:expected_error) { nil } - it { expect(subject).to be_nil } + let(:structured_error) do + new_detailed_error( + status_code, + "unused error message", + expected_error) + end + + shared_examples '#user_commit_files failure' do + it 'raises a PreReceiveError' do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash)) + .and_raise(structured_error) + + expect { subject }.to raise_error do |error| + expect(error).to be_a(Gitlab::Git::Index::IndexError) + expect(error.message).to eq(expected_message) + end + end + end + + context 'with missing file' do + let(:status_code) { GRPC::Core::StatusCodes::NOT_FOUND } + let(:expected_message) { "File not found: README.md" } + let(:expected_error) do + Gitaly::UserCommitFilesError.new( + index_update: Gitaly::IndexError.new( + path: "README.md", + error_type: Gitaly::IndexError::ErrorType::ERROR_TYPE_FILE_NOT_FOUND + )) + end + + it_behaves_like '#user_commit_files failure' + end + + context 'with existing directory' do + let(:status_code) { GRPC::Core::StatusCodes::ALREADY_EXISTS } + let(:expected_message) { "Directory already exists: dir1" } + let(:expected_error) do + Gitaly::UserCommitFilesError.new( + index_update: Gitaly::IndexError.new( + path: "dir1", + error_type: Gitaly::IndexError::ErrorType::ERROR_TYPE_DIRECTORY_EXISTS + )) + end + + it_behaves_like '#user_commit_files failure' + end + + context 'with existing file' do + let(:status_code) { GRPC::Core::StatusCodes::ALREADY_EXISTS } + let(:expected_message) { "File already exists: README.md" } + let(:expected_error) do + Gitaly::UserCommitFilesError.new( + index_update: Gitaly::IndexError.new( + path: "README.md", + error_type: Gitaly::IndexError::ErrorType::ERROR_TYPE_FILE_EXISTS + )) + end + + it_behaves_like '#user_commit_files failure' + end + + context 'with invalid path' do + let(:status_code) { GRPC::Core::StatusCodes::INVALID_ARGUMENT } + let(:expected_message) { "Invalid path: invalid://file/name" } + let(:expected_error) do + Gitaly::UserCommitFilesError.new( + index_update: Gitaly::IndexError.new( + path: "invalid://file/name", + error_type: Gitaly::IndexError::ErrorType::ERROR_TYPE_INVALID_PATH + )) + end + + it_behaves_like '#user_commit_files failure' + end + + context 'with directory traversal' do + let(:status_code) { GRPC::Core::StatusCodes::INVALID_ARGUMENT } + let(:expected_message) { "Directory traversal in path escapes repository: ../../../../etc/shadow" } + let(:expected_error) do + Gitaly::UserCommitFilesError.new( + index_update: Gitaly::IndexError.new( + path: "../../../../etc/shadow", + error_type: Gitaly::IndexError::ErrorType::ERROR_TYPE_DIRECTORY_TRAVERSAL + )) + end + + it_behaves_like '#user_commit_files failure' + end + + context 'with empty path' do + let(:status_code) { GRPC::Core::StatusCodes::INVALID_ARGUMENT } + let(:expected_message) { "Received empty path" } + let(:expected_error) do + Gitaly::UserCommitFilesError.new( + index_update: Gitaly::IndexError.new( + path: "", + error_type: Gitaly::IndexError::ErrorType::ERROR_TYPE_EMPTY_PATH + )) + end + + it_behaves_like '#user_commit_files failure' + end + + context 'with unspecified error' do + let(:status_code) { GRPC::Core::StatusCodes::INVALID_ARGUMENT } + let(:expected_message) { "Unknown error performing git operation" } + let(:expected_error) do + Gitaly::UserCommitFilesError.new( + index_update: Gitaly::IndexError.new( + path: "", + error_type: Gitaly::IndexError::ErrorType::ERROR_TYPE_UNSPECIFIED + )) + end + + it_behaves_like '#user_commit_files failure' + end + + context 'with an exception without the detailed error' do + let(:permission_error) do + GRPC::PermissionDenied.new + end + + it 'raises PermissionDenied' do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash)) + .and_raise(permission_error) + + expect { subject }.to raise_error(GRPC::PermissionDenied) + end + end + end + + context 'with CustomHookError' do + before do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash)) + .and_raise(raised_error) + end + + let(:raised_error) do + new_detailed_error( + GRPC::Core::StatusCodes::PERMISSION_DENIED, + "error updating file", + Gitaly::UserCommitFilesError.new( + custom_hook: Gitaly::CustomHookError.new( + stdout: "some stdout", + stderr: "GitLab: some custom hook error message", + hook_type: Gitaly::CustomHookError::HookType::HOOK_TYPE_PRERECEIVE + ))) + end + + it 'raises a PreReceiveError' do + expect { subject }.to raise_error do |error| + expect(error).to be_a(Gitlab::Git::PreReceiveError) + expect(error.message).to eq("some custom hook error message") + end + end + end end end