diff --git a/app/models/repository.rb b/app/models/repository.rb
index 4a8c097528298e995ccd037e558074e0bb8a1ede..55e723c8b97df944d7d2a6db300e881dca95a05d 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -885,6 +885,11 @@ def commit_files(user, **options)
options[:start_repository] = start_project.repository.raw_repository
end
+ skip_target_sha = options.delete(:skip_target_sha)
+ unless skip_target_sha || Feature.disabled?(:validate_target_sha_in_user_commit_files, project)
+ options[:target_sha] = self.commit(options[:branch_name])&.sha
+ end
+
with_cache_hooks { raw.commit_files(user, **options) }
end
diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb
index cd554f1055b0702b9e822d256a30fdcd9b41e2dd..58da5195c44477a166a1c45e159ee30f120e5d89 100644
--- a/app/services/snippets/create_service.rb
+++ b/app/services/snippets/create_service.rb
@@ -124,5 +124,9 @@ def build_actions_from_params(_snippet)
def restricted_files_actions
:create
end
+
+ def commit_attrs(snippet, msg)
+ super.merge(skip_target_sha: true)
+ end
end
end
diff --git a/config/feature_flags/gitlab_com_derisk/validate_target_sha_in_user_commit_files.yml b/config/feature_flags/gitlab_com_derisk/validate_target_sha_in_user_commit_files.yml
new file mode 100644
index 0000000000000000000000000000000000000000..32d49121e884fa44e2e55d4e9ad69ab59ba57e22
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/validate_target_sha_in_user_commit_files.yml
@@ -0,0 +1,9 @@
+---
+name: validate_target_sha_in_user_commit_files
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/384017
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/161981
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/479424
+milestone: '17.4'
+group: group::source code
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/ee/spec/models/remote_mirror_spec.rb b/ee/spec/models/remote_mirror_spec.rb
index 2af44015e55c33cfd862f4dc0192a29c107a76f1..ae8c8acc1553b650a2aafdd2778dd8b09a0cc366 100644
--- a/ee/spec/models/remote_mirror_spec.rb
+++ b/ee/spec/models/remote_mirror_spec.rb
@@ -68,8 +68,8 @@
end
before do
- project.repository.create_branch('matched', create_commit)
- project.repository.create_branch('mismatched', create_commit)
+ project.repository.create_branch(branch_name = 'matched', create_commit(branch_name))
+ project.repository.create_branch(branch_name = 'mismatched', create_commit(branch_name))
end
it 'only sync matched and recently updated branch' do
@@ -80,10 +80,10 @@
end
end
- def create_commit
+ def create_commit(branch_name)
project.repository.commit_files(
user,
- branch_name: 'HEAD',
+ branch_name: branch_name,
message: 'commit message',
actions: []
)
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 064368baf1f081965de67c4ae9d3f96711282bd7..092698785591551e910432a0fd97637691bce324 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1014,6 +1014,7 @@ def bundle_to_disk(save_path)
# @param [String] start_sha: The sha to be used as the parent of the commit.
# @param [Gitlab::Git::Repository] start_repository: The repository that contains the start branch or sha. Defaults to use this repository.
# @param [Boolean] force: Force update the branch.
+ # @param [String] target_sha: The latest sha of the target branch (optional). Used to prevent races in updates between different clients.
# @return [Gitlab::Git::OperationService::BranchUpdate]
#
# rubocop:disable Metrics/ParameterLists
@@ -1021,12 +1022,12 @@ def commit_files(
user, branch_name:, message:, actions:,
author_email: nil, author_name: nil,
start_branch_name: nil, start_sha: nil, start_repository: nil,
- force: false, sign: true)
+ force: false, sign: true, target_sha: nil)
wrapped_gitaly_errors do
gitaly_operation_client.user_commit_files(user, branch_name,
- message, actions, author_email, author_name,
- start_branch_name, start_repository, force, start_sha, sign)
+ message, actions, author_email, author_name, start_branch_name,
+ start_repository, force, start_sha, sign, target_sha)
end
end
# rubocop:enable Metrics/ParameterLists
diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb
index 6797890f8d0ffbed212aae9775fe13decb9f7f5b..fe352ade7978cffd49603bb8b0c0cc3f601ace5a 100644
--- a/lib/gitlab/gitaly_client/operation_service.rb
+++ b/lib/gitlab/gitaly_client/operation_service.rb
@@ -507,12 +507,12 @@ def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:)
# rubocop:disable Metrics/ParameterLists
def user_commit_files(
- user, branch_name, commit_message, actions, author_email, author_name,
- start_branch_name, start_repository, force = false, start_sha = nil, sign = true)
+ user, branch_name, commit_message, actions, author_email, author_name, start_branch_name,
+ start_repository, force = false, start_sha = nil, sign = true, target_sha = nil)
req_enum = Enumerator.new do |y|
header = user_commit_files_request_header(user, branch_name,
- commit_message, actions, author_email, author_name,
- start_branch_name, start_repository, force, start_sha, sign)
+ commit_message, actions, author_email, author_name, start_branch_name,
+ start_repository, force, start_sha, sign, target_sha)
y.yield Gitaly::UserCommitFilesRequest.new(header: header)
@@ -561,13 +561,7 @@ def user_commit_files(
when :index_update
raise Gitlab::Git::Index::IndexError, index_error_message(detailed_error.index_update)
else
- # Some invalid path errors are caught by Gitaly directly and returned
- # as an :index_update error, while others are found by libgit2 and
- # come as generic errors. We need to convert the latter as IndexErrors
- # as well.
- if e.to_status.details.start_with?('invalid path')
- raise Gitlab::Git::Index::IndexError, e.to_status.details
- end
+ handle_undetailed_bad_status_errors(e)
raise e
end
@@ -614,7 +608,7 @@ def consume_final_message(response_enum)
# rubocop:disable Metrics/ParameterLists
def user_commit_files_request_header(
user, branch_name, commit_message, actions, author_email, author_name,
- start_branch_name, start_repository, force, start_sha, sign)
+ start_branch_name, start_repository, force, start_sha, sign, target_sha)
Gitaly::UserCommitFilesRequestHeader.new(
repository: @gitaly_repo,
@@ -628,6 +622,7 @@ def user_commit_files_request_header(
force: force,
start_sha: encode_binary(start_sha),
sign: sign,
+ expected_old_oid: target_sha,
timestamp: Google::Protobuf::Timestamp.new(seconds: Time.now.utc.to_i)
)
end
@@ -673,6 +668,18 @@ def index_error_message(index_error)
"Unknown error performing git operation"
end
end
+
+ def handle_undetailed_bad_status_errors(error)
+ # Some invalid path errors are caught by Gitaly directly and returned
+ # as an :index_update error, while others are found by libgit2 and
+ # come as generic errors. We need to convert the latter as IndexErrors
+ # as well.
+ if error.to_status.details.start_with?('invalid path')
+ raise Gitlab::Git::Index::IndexError, error.to_status.details
+ elsif error.is_a?(GRPC::InvalidArgument) && error.to_status.details.include?('expected old object ID')
+ raise Gitlab::Git::CommandError, error
+ end
+ end
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 a84a3294465ccea2558dfb14c5ce06a443d90a49..ee921bf175f9b87b59cf0d24ddce9043c9f21a43 100644
--- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
@@ -1180,17 +1180,19 @@
let(:force) { false }
let(:start_sha) { nil }
let(:sign) { true }
+ let(:target_sha) { nil }
subject do
client.user_commit_files(
user, 'my-branch', 'Commit files message', [], 'janedoe@example.com', 'Jane Doe',
- 'master', repository, force, start_sha, sign)
+ 'master', repository, force, start_sha, sign, target_sha)
end
context 'when UserCommitFiles RPC is called' do
let(:force) { true }
let(:start_sha) { project.commit.id }
let(:sign) { false }
+ let(:target_sha) { 'target_sha' }
it 'successfully builds the header' do
expect_any_instance_of(Gitaly::OperationService::Stub).to receive(:user_commit_files) do |_, req_enum|
@@ -1199,6 +1201,7 @@
expect(header.force).to eq(force)
expect(header.start_sha).to eq(start_sha)
expect(header.sign).to eq(sign)
+ expect(header.expected_old_oid).to eq(target_sha)
end.and_return(Gitaly::UserCommitFilesResponse.new)
subject
@@ -1441,6 +1444,32 @@
end
end
end
+
+ context 'with an invalid target_sha' do
+ context 'when the target_sha is not in a valid format' do
+ let(:target_sha) { 'asdf' }
+
+ it 'raises CommandError' do
+ expect { subject }.to raise_error(Gitlab::Git::CommandError)
+ end
+ end
+
+ context 'when the target_sha is valid but not present in the repo' do
+ let(:target_sha) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff0' }
+
+ it 'raises CommandError' do
+ expect { subject }.to raise_error(Gitlab::Git::CommandError)
+ end
+ end
+
+ context 'when the target_sha is present in the repo but is not the latest' do
+ let(:target_sha) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' }
+
+ it 'raises FailedPrecondition' do
+ expect { subject }.to raise_error(GRPC::FailedPrecondition)
+ end
+ end
+ end
end
end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 58ed4a56a007868fddd04b05b46bd2481d357923..bc912f185a23a9f28772fec083410e96efdb2f1c 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -4307,19 +4307,16 @@ def update_storages(storage_hash)
end
describe '#commit_files' do
- let(:project) { create(:project, :empty_repo) }
-
- it 'calls UserCommitFiles RPC' do
- expect_next_instance_of(Gitlab::GitalyClient::OperationService) do |client|
- expect(client).to receive(:user_commit_files).with(
- user, 'extra-branch', 'commit message', [],
- 'author email', 'author name', nil, nil, true, nil, false
- )
- end
+ let_it_be(:project) { create(:project, :repository) }
+ let(:target_sha) { repository.commit('master').sha }
+ let(:expected_params) do
+ [user, 'master', 'commit message', [], 'author email', 'author name', nil, nil, true, nil, false, target_sha]
+ end
+ subject do
repository.commit_files(
user,
- branch_name: 'extra-branch',
+ branch_name: 'master',
message: 'commit message',
author_name: 'author name',
author_email: 'author email',
@@ -4328,5 +4325,44 @@ def update_storages(storage_hash)
sign: false
)
end
+
+ it 'finds and passes the branches target_sha' do
+ expect_next_instance_of(Gitlab::GitalyClient::OperationService) do |client|
+ expect(client).to receive(:user_commit_files).with(*expected_params)
+ end
+
+ subject
+ end
+
+ context 'when validate_target_sha_in_user_commit_files feature flag is disabled' do
+ let_it_be(:project) { create(:project, :repository) }
+ let(:target_sha) { nil }
+
+ before do
+ stub_feature_flags(validate_target_sha_in_user_commit_files: false)
+ end
+
+ it 'does not find or pass the branches target_sha' do
+ expect_next_instance_of(Gitlab::GitalyClient::OperationService) do |client|
+ expect(client).to receive(:user_commit_files).with(*expected_params)
+ end
+ expect(repository).not_to receive(:commit)
+
+ subject
+ end
+ end
+
+ context 'with an empty branch' do
+ let_it_be(:project) { create(:project, :empty_repo) }
+ let(:target_sha) { nil }
+
+ it 'calls UserCommitFiles RPC' do
+ expect_next_instance_of(Gitlab::GitalyClient::OperationService) do |client|
+ expect(client).to receive(:user_commit_files).with(*expected_params)
+ end
+
+ subject
+ end
+ end
end
end
diff --git a/spec/services/design_management/save_designs_service_spec.rb b/spec/services/design_management/save_designs_service_spec.rb
index bbde629e345ea824a7959f036d6ea32c86143a7f..c8ea024282cdd28e6815b3bed7a6a42c45c330e3 100644
--- a/spec/services/design_management/save_designs_service_spec.rb
+++ b/spec/services/design_management/save_designs_service_spec.rb
@@ -366,7 +366,7 @@ def repository_exists
expect { service.execute }
.to change { issue.designs.count }.from(0).to(2)
.and change { DesignManagement::Version.count }.by(1)
- .and change { Gitlab::GitalyClient.get_request_count }.by(3)
+ .and change { Gitlab::GitalyClient.get_request_count }.by(4)
.and change { commit_count }.by(1)
.and trigger_internal_events(Gitlab::UsageDataCounters::IssueActivityUniqueCounter::ISSUE_DESIGNS_ADDED)
.twice.with(user: user, project: project, category: 'InternalEventTracking')
diff --git a/spec/services/repositories/changelog_service_spec.rb b/spec/services/repositories/changelog_service_spec.rb
index d7546ac08f69fbf2bcf7e1e283ce5a89e17c2043..77df0b5921d6d41ac65020254d5de33195661c22 100644
--- a/spec/services/repositories/changelog_service_spec.rb
+++ b/spec/services/repositories/changelog_service_spec.rb
@@ -265,6 +265,8 @@
end
def create_commit(project, user, params)
+ RequestStore.clear!
+
params = { start_branch: 'master', branch_name: 'master' }.merge(params)
Files::MultiService.new(project, user, params).execute.fetch(:result)
end
diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb
index 8c288e94f73ca01c194e6ddbd6dfb79ecb896665..5254a318221f5ed8d2f3dcb1f8527d32b67c1a11 100644
--- a/spec/services/snippets/create_service_spec.rb
+++ b/spec/services/snippets/create_service_spec.rb
@@ -118,6 +118,14 @@
expect(blob.data).to eq base_opts[:content]
end
+ it 'passes along correct commit attributes' do
+ expect_next_instance_of(Repository) do |repository|
+ expect(repository).to receive(:commit_files).with(anything, a_hash_including(skip_target_sha: true))
+ end
+
+ subject
+ end
+
context 'when repository creation action fails' do
before do
allow_next_instance_of(Snippet) do |instance|
diff --git a/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb b/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb
index 25a72d46a7b1ef105491cea3d5a4f400d9f6c81f..31811527259bd89b7515f5845270b84d38eb99b8 100644
--- a/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb
+++ b/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb
@@ -154,7 +154,7 @@
let_it_be(:repository) { project.repository }
it 'is successful' do
- expect(repository).to receive(:commit).and_return(nil)
+ expect(repository).to receive(:commit).twice.and_return(nil)
expect(result.status).to eq(:success)
end
end