diff --git a/config/feature_flags/development/verify_push_rules_for_first_commit.yml b/config/feature_flags/development/verify_push_rules_for_first_commit.yml
new file mode 100644
index 0000000000000000000000000000000000000000..a09142b06d3a18bce57f49b58ab09fc8596dc162
--- /dev/null
+++ b/config/feature_flags/development/verify_push_rules_for_first_commit.yml
@@ -0,0 +1,8 @@
+---
+name: verify_push_rules_for_first_commit
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123950
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/419128
+milestone: '16.3'
+type: development
+group: group::source code
+default_enabled: false
diff --git a/ee/spec/lib/gitlab/checks/diff_check_spec.rb b/ee/spec/lib/gitlab/checks/diff_check_spec.rb
index d6c50715a0fc5ce27b9afd1d4c88a6622829abde..9bf9e4ca29541ebcb7ac1284b10f59fc6c5ab672 100644
--- a/ee/spec/lib/gitlab/checks/diff_check_spec.rb
+++ b/ee/spec/lib/gitlab/checks/diff_check_spec.rb
@@ -239,6 +239,42 @@
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, /File name #{file_path} was prohibited by the pattern/)
end
end
+
+ context 'when the repository was empty' do
+ let(:project) { create(:project, :empty_repo, push_rule: push_rule) }
+ let(:new_rev) { project.repository.create_file(user, file_path, "commit #{file_path}", message: "commit #{file_path}", branch_name: "master") }
+ let(:new_commits) { [project.repository.commit(new_rev)] }
+
+ before do
+ allow(project.repository).to receive(:empty?).and_return(true)
+ end
+
+ context 'when file contains secrets' do
+ let(:file_path) { 'aws/credentials' }
+
+ it "returns an error if a new or renamed filed doesn't match the file name regex" do
+ expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, /File name #{file_path} was prohibited by the pattern/)
+ end
+
+ context 'when feature flag "verify_push_rules_for_first_commit" is disabled' do
+ before do
+ stub_feature_flags(verify_push_rules_for_first_commit: false)
+ end
+
+ it 'does not raise an error' do
+ expect(subject.validate!).to be_truthy
+ end
+ end
+ end
+
+ context 'when file is permitted' do
+ let(:file_path) { 'aws/not_credentials' }
+
+ it 'does not raise an error' do
+ expect(subject.validate!).to be_truthy
+ end
+ end
+ end
end
end
diff --git a/ee/spec/support/shared_contexts/push_rules_checks_shared_context.rb b/ee/spec/support/shared_contexts/push_rules_checks_shared_context.rb
index f2f5232c1cb126bbe96d6dbf08750eed08482589..bc0ed863976baf9ee4b5306b4149500e7ca9850b 100644
--- a/ee/spec/support/shared_contexts/push_rules_checks_shared_context.rb
+++ b/ee/spec/support/shared_contexts/push_rules_checks_shared_context.rb
@@ -5,9 +5,9 @@
let(:project) { create(:project, :public, :repository, push_rule: push_rule) }
+ let(:new_commits) { project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51') }
+
before do
- allow(project.repository).to receive(:new_commits).and_return(
- project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51')
- )
+ allow(project.repository).to receive(:new_commits).and_return(new_commits)
end
end
diff --git a/lib/gitlab/checks/diff_check.rb b/lib/gitlab/checks/diff_check.rb
index bce4f969284a86b517f2b5ea0b1dc4d9150735cd..faf5a764725ae55e4f261831980df5187463938c 100644
--- a/lib/gitlab/checks/diff_check.rb
+++ b/lib/gitlab/checks/diff_check.rb
@@ -18,10 +18,7 @@ def validate!
return unless should_run_validations?
return if commits.empty?
- paths = project.repository.find_changed_paths(
- commits.map(&:sha), merge_commit_diff_mode: :all_parents
- )
-
+ paths = project.repository.find_changed_paths(treeish_objects, merge_commit_diff_mode: :all_parents)
paths.each do |path|
validate_path(path)
end
@@ -31,6 +28,29 @@ def validate!
private
+ def treeish_objects
+ objects = commits
+
+ return objects unless project.repository.empty? &&
+ Feature.enabled?(:verify_push_rules_for_first_commit, project)
+
+ # It's a special case for the push to the empty repository
+ #
+ # Git doesn't display a diff of the initial commit of the repository
+ # if we just provide a commit sha.
+ #
+ # To fix that we can use TreeRequest to check the difference
+ # between empty tree sha and the tree sha of the initial commit
+ #
+ # `commits` are sorted in reverse order, the initial commit is the last one.
+ init_commit = objects.last
+
+ diff_tree = Gitlab::Git::DiffTree.from_commit(init_commit)
+ objects.prepend(diff_tree) if diff_tree
+
+ objects
+ end
+
def validate_lfs_file_locks?
strong_memoize(:validate_lfs_file_locks) do
project.lfs_enabled? && project.any_lfs_file_locks?
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index c0601c7795c881304f20e2d8ecf7e3b3373beec9..b104610038cabea9c7da28a59d2ca03c5b6526b9 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -226,6 +226,12 @@ def short_id(length = 10)
id.to_s[0..length]
end
+ def tree_id
+ return unless raw_commit
+
+ raw_commit.tree_id
+ end
+
def safe_message
@safe_message ||= message
end
diff --git a/lib/gitlab/git/diff_tree.rb b/lib/gitlab/git/diff_tree.rb
new file mode 100644
index 0000000000000000000000000000000000000000..df48baeb1c3f8d7c124dbdf9a5257f005daed8ba
--- /dev/null
+++ b/lib/gitlab/git/diff_tree.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Git
+ # Represents a tree-ish object for git diff-tree command
+ # See: https://git-scm.com/docs/git-diff-tree
+ class DiffTree
+ attr_reader :left_tree_id, :right_tree_id
+
+ def initialize(left_tree_id, right_tree_id)
+ @left_tree_id = left_tree_id
+ @right_tree_id = right_tree_id
+ end
+
+ def self.from_commit(commit)
+ return unless commit.tree_id
+
+ parent_tree_id =
+ if commit.parent_ids.blank?
+ Gitlab::Git::EMPTY_TREE_ID
+ else
+ parent_id = commit.parent_ids.first
+ commit.repository.commit(parent_id).tree_id
+ end
+
+ new(parent_tree_id, commit.tree_id)
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 71be986882ce36caadb3a539bfe44b2d22e637bc..7ea0c0ad697344e9ebfc2fac4e58376079330341 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -525,15 +525,13 @@ def diff_stats(left_id, right_id)
empty_diff_stats
end
- def find_changed_paths(commits, merge_commit_diff_mode: nil)
- processed_commits = commits.reject { |ref| ref.blank? || Gitlab::Git.blank_ref?(ref) }
+ def find_changed_paths(treeish_objects, merge_commit_diff_mode: nil)
+ processed_objects = treeish_objects.compact
- return [] if processed_commits.empty?
+ return [] if processed_objects.empty?
wrapped_gitaly_errors do
- gitaly_commit_client.find_changed_paths(
- processed_commits, merge_commit_diff_mode: merge_commit_diff_mode
- )
+ gitaly_commit_client.find_changed_paths(processed_objects, merge_commit_diff_mode: merge_commit_diff_mode)
end
rescue CommandError, TypeError, NoRepository
[]
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index c10f780665cf277949a6d790d0866734643d3a39..0b0cf19752f12509171843970badf645b6e16434 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -274,8 +274,10 @@ def diff_stats(left_commit_sha, right_commit_sha)
# else # defaults to :include_merges behavior
# ['foo_bar.rb', 'bar_baz.rb'],
#
- def find_changed_paths(commits, merge_commit_diff_mode: nil)
- request = find_changed_paths_request(commits, merge_commit_diff_mode)
+ def find_changed_paths(objects, merge_commit_diff_mode: nil)
+ request = find_changed_paths_request(objects, merge_commit_diff_mode)
+
+ return [] if request.nil?
response = gitaly_client_call(@repository.storage, :diff_service, :find_changed_paths, request, timeout: GitalyClient.medium_timeout)
response.flat_map do |msg|
@@ -646,16 +648,27 @@ def call_find_commit(revision)
response.commit
end
- def find_changed_paths_request(commits, merge_commit_diff_mode)
+ def find_changed_paths_request(objects, merge_commit_diff_mode)
diff_mode = MERGE_COMMIT_DIFF_MODES[merge_commit_diff_mode] if Feature.enabled?(:merge_commit_diff_modes)
- commit_requests = commits.map do |commit|
- Gitaly::FindChangedPathsRequest::Request.new(
- commit_request: Gitaly::FindChangedPathsRequest::Request::CommitRequest.new(commit_revision: commit)
- )
+ requests = objects.filter_map do |object|
+ case object
+ when Gitlab::Git::DiffTree
+ Gitaly::FindChangedPathsRequest::Request.new(
+ tree_request: Gitaly::FindChangedPathsRequest::Request::TreeRequest.new(left_tree_revision: object.left_tree_id, right_tree_revision: object.right_tree_id)
+ )
+ when Commit, Gitlab::Git::Commit
+ next if object.sha.blank? || Gitlab::Git.blank_ref?(object.sha)
+
+ Gitaly::FindChangedPathsRequest::Request.new(
+ commit_request: Gitaly::FindChangedPathsRequest::Request::CommitRequest.new(commit_revision: object.sha)
+ )
+ end
end
- Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, requests: commit_requests, merge_commit_diff_mode: diff_mode)
+ return if requests.blank?
+
+ Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, requests: requests, merge_commit_diff_mode: diff_mode)
end
def path_error_message(path_error)
diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index dd9f77f0211dcbbf1a8834395a9499887660848a..3b1dbf7d602618e04a669ad540d3c422b223bfb9 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -3,15 +3,16 @@
require "spec_helper"
RSpec.describe Gitlab::Git::Commit, feature_category: :source_code_management do
- let(:repository) { create(:project, :repository).repository.raw }
+ let_it_be(:repository) { create(:project, :repository).repository.raw }
let(:commit) { described_class.find(repository, SeedRepo::Commit::ID) }
describe "Commit info from gitaly commit" do
let(:subject) { (+"My commit").force_encoding('ASCII-8BIT') }
let(:body) { subject + (+"My body").force_encoding('ASCII-8BIT') }
let(:body_size) { body.length }
- let(:gitaly_commit) { build(:gitaly_commit, subject: subject, body: body, body_size: body_size) }
+ let(:gitaly_commit) { build(:gitaly_commit, subject: subject, body: body, body_size: body_size, tree_id: tree_id) }
let(:id) { gitaly_commit.id }
+ let(:tree_id) { 'd7f32d821c9cc7b1a9166ca7c4ba95b5c2d0d000' }
let(:committer) { gitaly_commit.committer }
let(:author) { gitaly_commit.author }
let(:commit) { described_class.new(repository, gitaly_commit) }
@@ -26,6 +27,7 @@
it { expect(commit.committer_name).to eq(committer.name) }
it { expect(commit.committer_email).to eq(committer.email) }
it { expect(commit.parent_ids).to eq(gitaly_commit.parent_ids) }
+ it { expect(commit.tree_id).to eq(tree_id) }
context 'non-UTC dates' do
let(:seconds) { Time.now.to_i }
@@ -577,6 +579,14 @@ def create_commit_with_large_message
it { is_expected.to eq(sample_commit_hash[:message]) }
end
+
+ describe '#tree_id' do
+ subject { super().tree_id }
+
+ it "doesn't return tree id for non-Gitaly commits" do
+ is_expected.to be_nil
+ end
+ end
end
describe '#stats' do
diff --git a/spec/lib/gitlab/git/diff_tree_spec.rb b/spec/lib/gitlab/git/diff_tree_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..614a8f03dd8d6e8e5968c6c4daccd37657c95ca1
--- /dev/null
+++ b/spec/lib/gitlab/git/diff_tree_spec.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe Gitlab::Git::DiffTree, feature_category: :source_code_management do
+ let_it_be(:project) { create(:project, :repository) }
+ let_it_be(:repository) { project.repository }
+
+ describe '.from_commit' do
+ subject(:diff_tree) { described_class.from_commit(commit) }
+
+ context 'when commit is an initial commit' do
+ let(:commit) { repository.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') }
+
+ it 'returns the expected diff tree object' do
+ expect(diff_tree.left_tree_id).to eq(Gitlab::Git::EMPTY_TREE_ID)
+ expect(diff_tree.right_tree_id).to eq(commit.tree_id)
+ end
+ end
+
+ context 'when commit is a regular commit' do
+ let(:commit) { repository.commit('60ecb67744cb56576c30214ff52294f8ce2def98') }
+
+ it 'returns the expected diff tree object' do
+ expect(diff_tree.left_tree_id).to eq(commit.parent.tree_id)
+ expect(diff_tree.right_tree_id).to eq(commit.tree_id)
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 9ce8a674146738b1d66a7f5c786160cfe96b12db..a97c3c8597140107997053226deae64150103996 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1675,9 +1675,13 @@ def create_commit(blobs)
end
describe '#find_changed_paths' do
- let(:commit_1) { TestEnv::BRANCH_SHA['with-executables'] }
- let(:commit_2) { TestEnv::BRANCH_SHA['master'] }
- let(:commit_3) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' }
+ let_it_be(:commit_1) { repository.commit(TestEnv::BRANCH_SHA['with-executables']) }
+ let_it_be(:commit_2) { repository.commit(TestEnv::BRANCH_SHA['master']) }
+ let_it_be(:commit_3) { repository.commit('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') }
+
+ let_it_be(:initial_commit) { repository.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') }
+ let_it_be(:diff_tree) { Gitlab::Git::DiffTree.from_commit(initial_commit) }
+
let(:commit_1_files) do
[Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/executables/ls")]
end
@@ -1693,18 +1697,26 @@ def create_commit(blobs)
]
end
+ let(:diff_tree_files) do
+ [
+ Gitlab::Git::ChangedPath.new(status: :ADDED, path: ".gitignore"),
+ Gitlab::Git::ChangedPath.new(status: :ADDED, path: "LICENSE"),
+ Gitlab::Git::ChangedPath.new(status: :ADDED, path: "README.md")
+ ]
+ end
+
it 'returns a list of paths' do
- collection = repository.find_changed_paths([commit_1, commit_2, commit_3])
+ collection = repository.find_changed_paths([commit_1, commit_2, commit_3, diff_tree])
expect(collection).to be_a(Enumerable)
- expect(collection.as_json).to eq((commit_1_files + commit_2_files + commit_3_files).as_json)
+ expect(collection.as_json).to eq((commit_1_files + commit_2_files + commit_3_files + diff_tree_files).as_json)
end
- it 'returns no paths when SHAs are invalid' do
+ it 'returns only paths with valid SHAs' do
collection = repository.find_changed_paths(['invalid', commit_1])
expect(collection).to be_a(Enumerable)
- expect(collection.to_a).to be_empty
+ expect(collection.as_json).to eq(commit_1_files.as_json)
end
it 'returns a list of paths even when containing a blank ref' do
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index fd66efe12c840224183e2d23e5c4a3918bd8d18c..740073d5e4e26ea8df50e88135710c3e2ee6069a 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -192,7 +192,9 @@
Gitaly::FindChangedPathsRequest.new(repository: repository_message, requests: requests, merge_commit_diff_mode: merge_commit_diff_mode)
end
- subject { described_class.new(repository).find_changed_paths(commits, merge_commit_diff_mode: merge_commit_diff_mode).as_json }
+ let(:treeish_objects) { repository.commits_by(oids: commits) }
+
+ subject { described_class.new(repository).find_changed_paths(treeish_objects, merge_commit_diff_mode: merge_commit_diff_mode).as_json }
before do
allow(Gitaly::FindChangedPathsRequest).to receive(:new).and_call_original
@@ -334,6 +336,40 @@
include_examples 'uses requests format'
end
end
+
+ context 'when all requested objects are invalid' do
+ it 'does not send RPC request' do
+ expect_any_instance_of(Gitaly::DiffService::Stub).not_to receive(:find_changed_paths)
+
+ returned_value = described_class.new(repository).find_changed_paths(%w[wrong values])
+
+ expect(returned_value).to eq([])
+ end
+ end
+
+ context 'when commit has an empty SHA' do
+ let(:empty_commit) { build(:commit, project: project, sha: '0000000000000000000000000000000000000000') }
+
+ it 'does not send RPC request' do
+ expect_any_instance_of(Gitaly::DiffService::Stub).not_to receive(:find_changed_paths)
+
+ returned_value = described_class.new(repository).find_changed_paths([empty_commit])
+
+ expect(returned_value).to eq([])
+ end
+ end
+
+ context 'when commit sha is not set' do
+ let(:empty_commit) { build(:commit, project: project, sha: nil) }
+
+ it 'does not send RPC request' do
+ expect_any_instance_of(Gitaly::DiffService::Stub).not_to receive(:find_changed_paths)
+
+ returned_value = described_class.new(repository).find_changed_paths([empty_commit])
+
+ expect(returned_value).to eq([])
+ end
+ end
end
describe '#tree_entries' do