diff --git a/config/feature_flags/gitlab_com_derisk/push_rule_file_size_limit.yml b/config/feature_flags/gitlab_com_derisk/push_rule_file_size_limit.yml new file mode 100644 index 0000000000000000000000000000000000000000..42f129d7470da6cfc60932888999769c3f62f263 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/push_rule_file_size_limit.yml @@ -0,0 +1,9 @@ +--- +name: push_rule_file_size_limit +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/505364 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183600 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/523497 +milestone: '17.10' +type: gitlab_com_derisk +group: group::source code +default_enabled: false diff --git a/ee/lib/ee/gitlab/checks/file_size_limit_check.rb b/ee/lib/ee/gitlab/checks/file_size_limit_check.rb new file mode 100644 index 0000000000000000000000000000000000000000..d5e2836d936629fc1897ce5e810f951f62123bb4 --- /dev/null +++ b/ee/lib/ee/gitlab/checks/file_size_limit_check.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module EE + module Gitlab + module Checks + module FileSizeLimitCheck + extend ActiveSupport::Concern + extend ::Gitlab::Utils::Override + + private + + override :file_size_limit + def file_size_limit + if ::Feature.enabled?(:push_rule_file_size_limit, project) + global_limit = super + return global_limit if push_rule.nil? + + push_rule_limit = push_rule.max_file_size + return push_rule_limit if global_limit.nil? + return global_limit if push_rule_limit == 0 + + [push_rule_limit, global_limit].min + else + super + end + end + end + end + end +end diff --git a/ee/lib/ee/gitlab/checks/push_rule_check.rb b/ee/lib/ee/gitlab/checks/push_rule_check.rb index 80da4b3cd48b42627012526bc10fabba0a4efea8..213f3faae5fe9382495640f46f740cabdcb0626c 100644 --- a/ee/lib/ee/gitlab/checks/push_rule_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rule_check.rb @@ -44,6 +44,9 @@ def check_file_size! # @raise [Gitlab::GitAccess::ForbiddenError] if any check fails def run_checks_in_sequence! check_tag_or_branch! + + return if ::Feature.enabled?(:push_rule_file_size_limit, project) + check_file_size! end @@ -63,8 +66,10 @@ def run_checks_in_parallel! check_tag_or_branch! end - parallelize(git_env) do - check_file_size! + unless ::Feature.enabled?(:push_rule_file_size_limit, project) + parallelize(git_env) do + check_file_size! + end end # Block whilst waiting for threads, however if one errors diff --git a/ee/spec/lib/ee/gitlab/checks/file_size_limit_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/file_size_limit_check_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..cfa755d5a3a0c8e2decf8811f78781185fe31406 --- /dev/null +++ b/ee/spec/lib/ee/gitlab/checks/file_size_limit_check_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Checks::FileSizeLimitCheck, feature_category: :source_code_management do + include_context 'changes access checks context' + include_context 'push rules checks context' + + let(:changes) do + [ + # Update of existing branch + { oldrev: oldrev, newrev: newrev, ref: ref }, + # Creation of new branch + { newrev: newrev, ref: 'refs/heads/something' }, + # Deletion of branch + { oldrev: oldrev, ref: 'refs/heads/deleteme' } + ] + end + + let(:changes_access) do + Gitlab::Checks::ChangesAccess.new( + changes, + project: project, + user_access: user_access, + protocol: protocol, + logger: logger, + push_options: push_options, + gitaly_context: gitaly_context + ) + end + + let(:push_rule_limit) { 50 } + let(:push_rule) { create(:push_rule, max_file_size: push_rule_limit) } + let(:global_limit) { 10 } + let(:plan_limits) { instance_double(PlanLimits, file_size_limit_mb: global_limit) } + + before do + allow(project).to receive_messages( + predefined_push_rule: push_rule, + actual_limits: plan_limits + ) + end + + subject(:file_size_check) { described_class.new(changes_access) } + + RSpec.shared_examples 'checks file size limit' do |expected_limit| + it "passes the correct file size limit to HookEnvironmentAwareAnyOversizedBlobs" do + expect_next_instance_of(Gitlab::Checks::FileSizeCheck::HookEnvironmentAwareAnyOversizedBlobs, + project: project, + changes: changes, + file_size_limit_megabytes: expected_limit + ) do |check| + expect(check).to receive(:find).and_call_original + end + file_size_check.validate! + end + end + + describe '#validate!' do + context 'when the global file limit is smaller than the push rule file size limit' do + let(:push_rule_limit) { 50 } + let(:global_limit) { 20 } + + it_behaves_like 'checks file size limit', 20 + end + + context 'when the push rule file limit is smaller than the global file size limit' do + let(:push_rule_limit) { 10 } + let(:global_limit) { 50 } + + it_behaves_like 'checks file size limit', 10 + end + + context 'when the push rule does not exist' do + let(:push_rule) { nil } + let(:global_limit) { 50 } + + it_behaves_like 'checks file size limit', 50 + end + + context 'when the global file limit is nil' do + let(:push_rule_limit) { 30 } + let(:global_limit) { nil } + + it_behaves_like 'checks file size limit', 30 + end + + context 'when the push rule limit is 0' do + let(:push_rule_limit) { 0 } + let(:global_limit) { 20 } + + it_behaves_like 'checks file size limit', 20 + end + + context 'when feature flag "push_rule_file_size_limit" is disabled' do + let(:push_rule_limit) { 30 } + let(:global_limit) { 10 } + + before do + stub_feature_flags(push_rule_file_size_limit: false) + end + + it_behaves_like 'checks file size limit', 10 + end + end +end diff --git a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb index 8f6791bde31ad105199c9eef6e1ec376b1212cbe..242cd50ed7689789898391bd879a95665e866536 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb @@ -24,18 +24,10 @@ .to receive(:validate!).and_return(nil) end - it_behaves_like 'use predefined push rules' - it "returns nil on success" do expect(subject.validate!).to be_nil end - it "raises an error on failure" do - expect_any_instance_of(EE::Gitlab::Checks::PushRules::FileSizeCheck).to receive(:validate!).and_raise(Gitlab::GitAccess::ForbiddenError) - - expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError) - end - context 'when tag name exists' do let(:changes) do [ @@ -102,6 +94,36 @@ subject.validate! end end + + context "when :push_rule_file_size_limit is enabled" do + before do + stub_feature_flags(push_rule_file_size_limit: true) + end + + it "does not run check_file_size!" do + expect(subject).not_to receive(:check_file_size!) + subject.validate! + end + end + + context "when :push_rule_file_size_limit is disabled" do + before do + stub_feature_flags(push_rule_file_size_limit: false) + end + + it_behaves_like 'use predefined push rules' + + it "runs check_file_size!" do + expect(subject).to receive(:check_file_size!).and_call_original + subject.validate! + end + + it "raises an error on failure" do + expect_any_instance_of(EE::Gitlab::Checks::PushRules::FileSizeCheck).to receive(:validate!).and_raise(Gitlab::GitAccess::ForbiddenError) + + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError) + end + end end describe '#validate!' do diff --git a/lib/gitlab/checks/changes_access.rb b/lib/gitlab/checks/changes_access.rb index 34146e3ccf82dd99c93b1d75340ce71a35cc5804..4b0ba12de511981ea01d39faaab98d3fe845f8a8 100644 --- a/lib/gitlab/checks/changes_access.rb +++ b/lib/gitlab/checks/changes_access.rb @@ -120,7 +120,7 @@ def single_access_checks! def bulk_access_checks! Gitlab::Checks::LfsCheck.new(self).validate! - Gitlab::Checks::GlobalFileSizeCheck.new(self).validate! + Gitlab::Checks::FileSizeLimitCheck.new(self).validate! Gitlab::Checks::IntegrationsCheck.new(self).validate! end diff --git a/lib/gitlab/checks/global_file_size_check.rb b/lib/gitlab/checks/file_size_limit_check.rb similarity index 95% rename from lib/gitlab/checks/global_file_size_check.rb rename to lib/gitlab/checks/file_size_limit_check.rb index 2acc7e371e5df5fdcc379b5c33aecdb0ea1266e8..04518901e4bcae53e94e8c02377ed17179ec2b1f 100644 --- a/lib/gitlab/checks/global_file_size_check.rb +++ b/lib/gitlab/checks/file_size_limit_check.rb @@ -2,7 +2,7 @@ module Gitlab module Checks - class GlobalFileSizeCheck < BaseBulkChecker + class FileSizeLimitCheck < BaseBulkChecker include ActionView::Helpers::NumberHelper LOG_MESSAGE = 'Checking for blobs over the file size limit' @@ -52,3 +52,5 @@ def file_size_limit end end end + +Gitlab::Checks::FileSizeLimitCheck.prepend_mod diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/push_over_ssh_file_size_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/push_over_ssh_file_size_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..69e509741336414f659d2922304abe99e9b7cead --- /dev/null +++ b/qa/qa/specs/features/browser_ui/3_create/repository/push_over_ssh_file_size_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +module QA + # This test modifies an instance-level setting, + # so skipping on live environments to avoid transient issues. + RSpec.describe 'Create', :requires_admin, :skip_live_env, product_group: :source_code do + describe 'push after setting the file size limit via admin/application_settings using SSH' do + include Support::API + + let!(:project) { create(:project, name: 'project-test-push-limit') } + let(:key) { create(:ssh_key, title: "key for ssh tests #{Time.now.to_f}") } + let(:commit_message) { 'Adding a new file' } + + after(:context) do + set_file_size_limit(nil) + end + + it( + 'push is successful when the file size is under the limit', + testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/523543' + ) do + set_file_size_limit(5) # Set a 5MB file size limit + + retry_on_fail do + push = push_new_file('oversize_file_1.bin') + + push.project.visit! + Page::Project::Show.perform do |project| + expect(project).to have_commit_message(commit_message) + end + end + end + + it( + 'push fails when the file size is above the limit', + testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/523544' + ) do + set_file_size_limit(2) # Set a 2MB file size limit + + retry_on_fail do + expect { push_new_file('oversize_file_2.bin') } + .to raise_error(QA::Support::Run::CommandError, /remote: fatal: pack exceeds maximum allowed size/) + end + end + + def set_file_size_limit(limit) + request = Runtime::API::Request.new(Runtime::API::Client.as_admin, '/application/settings') + response = put request.url, receive_max_input_size: limit + + expect(response.code).to eq(QA::Support::API::HTTP_STATUS_OK) + expect(parse_body(response)[:receive_max_input_size]).to eq(limit) + end + + def push_new_file(file_name) + Resource::Repository::ProjectPush.fabricate! do |p| + p.project = project + p.ssh_key = key + p.file_name = file_name + p.file_content = SecureRandom.random_bytes(3_000_000) # 3MB file + p.commit_message = commit_message + end + end + + # Application settings are cached for up to a minute. + # Instead of waiting, retry the attempt to push if it fails. + def retry_on_fail(&block) + Support::Retrier.retry_on_exception(max_attempts: 6, reload_page: false, sleep_interval: 10, &block) + end + end + end +end diff --git a/qa/qa/specs/features/ee/browser_ui/3_create/repository/push_rules_spec.rb b/qa/qa/specs/features/ee/browser_ui/3_create/repository/push_rules_spec.rb index c3688ce5a0834ebe5b8647aad6bddeb26ecf626e..3252e6d4942c8eec3e2c45bc04aac6871e7a2ed2 100644 --- a/qa/qa/specs/features/ee/browser_ui/3_create/repository/push_rules_spec.rb +++ b/qa/qa/specs/features/ee/browser_ui/3_create/repository/push_rules_spec.rb @@ -32,30 +32,40 @@ module QA end end - context 'when using non signed commits' do + context 'when using non signed commits', + feature_flag: { name: :push_rule_file_size_limit } do it 'allows an unrestricted push', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347790' do expect_no_error_on_push(file: standard_file) end - it 'restricts files by name and size', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347784', + it 'restricts files by name and size', + testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347784', quarantine: { only: { job: 'ee-qa-browser_ui-3_create' }, type: :investigating, issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/455927' } do - # Note: The file size limits in this test should be lower than the limits in - # browser_ui/3_create/repository/push_over_http_file_size_spec to prevent - # the limit set in that test from triggering in this test (which can happen - # on Staging where the tests are run in parallel). - # See: https://gitlab.com/gitlab-org/gitlab/-/issues/218620#note_361634705 + # # Note: The file size limits in this test should be lower than the limits in + # # browser_ui/3_create/repository/push_over_http_file_size_spec to prevent + # # the limit set in that test from triggering in this test (which can happen + # # on Staging where the tests are run in parallel). + # # See: https://gitlab.com/gitlab-org/gitlab/-/issues/218620#note_361634705 + + ff_enabled = Runtime::Feature.enabled?(:push_rule_file_size_limit) + + expected_error = if ff_enabled + 'You are attempting to check in one or more blobs which exceed the 1MiB limit' + else + 'File "file" is larger than the allowed size of 1 MiB' + end expect_error_on_push( file: { name: 'file', content: SecureRandom.hex(1000000) }, - error: 'File "file" is larger than the allowed size of 1 MiB') + error: expected_error) expect_error_on_push( file: { name: file_name_limitation, diff --git a/spec/lib/gitlab/checks/changes_access_spec.rb b/spec/lib/gitlab/checks/changes_access_spec.rb index 523a96f48ff4c55dba02c58bc9d3538a76820097..5ce0f1ec44d49c095f767780ea28559806325f7a 100644 --- a/spec/lib/gitlab/checks/changes_access_spec.rb +++ b/spec/lib/gitlab/checks/changes_access_spec.rb @@ -26,7 +26,7 @@ end it 'calls file size check' do - expect_next_instance_of(Gitlab::Checks::GlobalFileSizeCheck) do |instance| + expect_next_instance_of(Gitlab::Checks::FileSizeLimitCheck) do |instance| expect(instance).to receive(:validate!) end diff --git a/spec/lib/gitlab/checks/global_file_size_check_spec.rb b/spec/lib/gitlab/checks/file_size_limit_check_spec.rb similarity index 83% rename from spec/lib/gitlab/checks/global_file_size_check_spec.rb rename to spec/lib/gitlab/checks/file_size_limit_check_spec.rb index 7efb8e0cadb5a498bb326871e852554b86904ffb..08f596fef6bc771791f45467a33bf274eb1dce58 100644 --- a/spec/lib/gitlab/checks/global_file_size_check_spec.rb +++ b/spec/lib/gitlab/checks/file_size_limit_check_spec.rb @@ -2,9 +2,11 @@ require 'spec_helper' -RSpec.describe Gitlab::Checks::GlobalFileSizeCheck, feature_category: :source_code_management do +RSpec.describe Gitlab::Checks::FileSizeLimitCheck, feature_category: :source_code_management do include_context 'changes access checks context' + subject(:file_size_check) { described_class.new(changes_access) } + describe '#validate!' do it 'checks for file sizes' do expect_next_instance_of(Gitlab::Checks::FileSizeCheck::HookEnvironmentAwareAnyOversizedBlobs, @@ -14,10 +16,10 @@ ) do |check| expect(check).to receive(:find).and_call_original end - expect(subject.logger).to receive(:log_timed).with('Checking for blobs over the file size limit') + expect(file_size_check.logger).to receive(:log_timed).with('Checking for blobs over the file size limit') .and_call_original expect(Gitlab::AppJsonLogger).to receive(:info).with('Checking for blobs over the file size limit') - subject.validate! + file_size_check.validate! end context 'when there are oversized blobs' do @@ -43,7 +45,7 @@ blob_details: [{ "id" => mock_blob_id, "size" => mock_blob_size }] ) expect do - subject.validate! + file_size_check.validate! end.to raise_exception(Gitlab::GitAccess::ForbiddenError, /- #{mock_blob_id} \(#{size_msg} MiB\)/) end