diff --git a/config/feature_flags/gitlab_com_derisk/exclude_git_http_from_web_rate_limiter.yml b/config/feature_flags/gitlab_com_derisk/exclude_git_http_from_web_rate_limiter.yml deleted file mode 100644 index 32fdae51d02492e08c1e8567982998c5f4933d1d..0000000000000000000000000000000000000000 --- a/config/feature_flags/gitlab_com_derisk/exclude_git_http_from_web_rate_limiter.yml +++ /dev/null @@ -1,10 +0,0 @@ ---- -name: exclude_git_http_from_web_rate_limiter -description: -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/581142 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/213141 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/581527 -milestone: '18.6' -group: group::source code -type: gitlab_com_derisk -default_enabled: false diff --git a/db/post_migrate/20251125102233_backfill_throttle_authenticated_git_http_settings.rb b/db/post_migrate/20251125102233_backfill_throttle_authenticated_git_http_settings.rb new file mode 100644 index 0000000000000000000000000000000000000000..5e0a8e49f6756a3f7a7a61d0316d890098353eaa --- /dev/null +++ b/db/post_migrate/20251125102233_backfill_throttle_authenticated_git_http_settings.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class BackfillThrottleAuthenticatedGitHttpSettings < Gitlab::Database::Migration[2.3] + milestone '18.7' + + restrict_gitlab_migration gitlab_schema: :gitlab_main + + def up + execute <<~SQL + UPDATE application_settings + SET rate_limits = COALESCE(rate_limits, '{}'::jsonb) || + jsonb_build_object( + 'throttle_authenticated_git_http_enabled', true, + 'throttle_authenticated_git_http_requests_per_period', throttle_authenticated_web_requests_per_period, + 'throttle_authenticated_git_http_period_in_seconds', throttle_authenticated_web_period_in_seconds + ) + WHERE throttle_authenticated_web_enabled = true + AND (rate_limits->>'throttle_authenticated_git_http_enabled' IS NULL + OR (rate_limits->>'throttle_authenticated_git_http_enabled')::boolean = false) + SQL + end + + def down + # No-op: We don't want to remove settings that may have been manually configured + end +end diff --git a/db/schema_migrations/20251125102233 b/db/schema_migrations/20251125102233 new file mode 100644 index 0000000000000000000000000000000000000000..7c1261557019cf6def9dc5c975a2db8e73d172a8 --- /dev/null +++ b/db/schema_migrations/20251125102233 @@ -0,0 +1 @@ +5d1b13906dece5d3a97127f3a1e101081bf020f7349886912b330a65d5b14620 \ No newline at end of file diff --git a/lib/gitlab/rack_attack/request.rb b/lib/gitlab/rack_attack/request.rb index 283772948e0a22d8fd3e25603ed3ae0409fe1fd0..97c8b84faf4790ab8c56919dde9b05f46d04787b 100644 --- a/lib/gitlab/rack_attack/request.rb +++ b/lib/gitlab/rack_attack/request.rb @@ -116,7 +116,7 @@ def throttle_authenticated_api? def throttle_authenticated_web? (web_request? || frontend_request?) && !throttle_authenticated_git_lfs? && - !(git_path? && !git_lfs_path? && Feature.enabled?(:exclude_git_http_from_web_rate_limiter, :instance)) && + !(git_path? && !git_lfs_path?) && Gitlab::Throttle.settings.throttle_authenticated_web_enabled end @@ -183,7 +183,10 @@ def throttle_unauthenticated_git_http? def throttle_authenticated_git_http? git_path? && - Gitlab::Throttle.settings.throttle_authenticated_git_http_enabled + ( + Gitlab::Throttle.settings.throttle_authenticated_git_http_enabled || + Gitlab::Throttle.settings.throttle_authenticated_web_enabled + ) end def throttle_authenticated_git_lfs? diff --git a/lib/gitlab/throttle.rb b/lib/gitlab/throttle.rb index b945c765edd0ac2d005ff4389745964b29a14363..365d087f5d73dc8a26c593670c4e3d8d24bd213e 100644 --- a/lib/gitlab/throttle.rb +++ b/lib/gitlab/throttle.rb @@ -79,6 +79,10 @@ def self.throttle_unauthenticated_git_http_options end def self.throttle_authenticated_git_http_options + if !settings.throttle_authenticated_git_http_enabled && settings.throttle_authenticated_web_enabled + return authenticated_web_options + end + limit_proc = proc { |req| settings.throttle_authenticated_git_http_requests_per_period } period_proc = proc { |req| settings.throttle_authenticated_git_http_period_in_seconds.seconds } diff --git a/spec/lib/gitlab/rack_attack/request_spec.rb b/spec/lib/gitlab/rack_attack/request_spec.rb index 0de42dd42c0f56cd5794392e0f080e26ab7a43af..e3078af320f1943eeee281d8c5ac74f28c80f00d 100644 --- a/spec/lib/gitlab/rack_attack/request_spec.rb +++ b/spec/lib/gitlab/rack_attack/request_spec.rb @@ -256,39 +256,6 @@ it { is_expected.to eq expected } end - - context 'when exclude_git_http_from_web_rate_limiter is disabled' do - where(:path, :throttle_authenticated_web_enabled, :throttle_authenticated_git_lfs_enabled, :expected) do - ref(:web_path) | true | false | true - ref(:web_path) | false | false | false - ref(:web_path) | true | true | true - ref(:web_path) | false | true | false - - # Git HTTP paths are not excluded from web rate limiter - ref(:git_info_refs_path) | true | false | true - ref(:git_info_refs_path) | false | false | false - ref(:git_info_refs_path) | true | true | true - ref(:git_info_refs_path) | false | true | false - - # Git LFS paths are excluded when LFS throttle is enabled - ref(:git_lfs_path) | true | true | false - ref(:git_lfs_path) | false | true | false - ref(:git_lfs_path) | true | false | true - ref(:git_lfs_path) | false | false | false - end - - with_them do - before do - stub_feature_flags(exclude_git_http_from_web_rate_limiter: false) - stub_application_setting( - throttle_authenticated_web_enabled: throttle_authenticated_web_enabled, - throttle_authenticated_git_lfs_enabled: throttle_authenticated_git_lfs_enabled - ) - end - - it { is_expected.to eq expected } - end - end end describe '#throttle_unauthenticated_git_http?' do @@ -321,6 +288,30 @@ it { is_expected.to eq expected } end + + context 'when falls back for web requests' do + where(:git_http_enabled, :web_enabled, :expected) do + false | false | false + true | false | true + false | true | true + true | true | true + end + + with_them do + before do + stub_application_setting( + throttle_authenticated_git_http_enabled: git_http_enabled, + throttle_authenticated_web_enabled: web_enabled + ) + end + + it 'enables Git HTTP setttings if either web or Git HTTP rate limiter is enabled' do + expect(request).to receive(:git_path?).and_return(true) + + expect(request.throttle_authenticated_git_http?).to eq(expected) + end + end + end end describe '#throttle_authenticated_git_http?' do diff --git a/spec/lib/gitlab/throttle_spec.rb b/spec/lib/gitlab/throttle_spec.rb index 50d723193ac9073db29076a1e7548d63af20ffc2..67c9fff6e6ee985efa3a98b6545d311c181a31ef 100644 --- a/spec/lib/gitlab/throttle_spec.rb +++ b/spec/lib/gitlab/throttle_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Gitlab::Throttle do + using RSpec::Parameterized::TableSyntax + describe '.protected_paths_enabled?' do subject { described_class.protected_paths_enabled? } @@ -58,4 +60,34 @@ end end end + + describe '.throttle_authenticated_git_http_options' do + where(:git_http_enabled, :web_enabled, :expected_requests, :expected_period) do + false | false | 50 | 30 + false | true | 100 | 60 + true | false | 50 | 30 + true | true | 50 | 30 + end + + with_them do + before do + stub_application_setting( + throttle_authenticated_web_requests_per_period: 100, + throttle_authenticated_web_period_in_seconds: 60, + throttle_authenticated_git_http_requests_per_period: 50, + throttle_authenticated_git_http_period_in_seconds: 30, + + throttle_authenticated_git_http_enabled: git_http_enabled, + throttle_authenticated_web_enabled: web_enabled + ) + end + + it 'returns correct options' do + options = described_class.throttle_authenticated_git_http_options + + expect(options[:limit].call).to eq(expected_requests) + expect(options[:period].call).to eq(expected_period) + end + end + end end diff --git a/spec/migrations/db/post_migrate/20251125102233_backfill_throttle_authenticated_git_http_settings_spec.rb b/spec/migrations/db/post_migrate/20251125102233_backfill_throttle_authenticated_git_http_settings_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..01173dbcd241436108d5566241e809378ca21682 --- /dev/null +++ b/spec/migrations/db/post_migrate/20251125102233_backfill_throttle_authenticated_git_http_settings_spec.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe BackfillThrottleAuthenticatedGitHttpSettings, feature_category: :source_code_management do + let(:application_settings) { table(:application_settings) } + + describe '#up' do + context 'when throttle_authenticated_web is enabled' do + it 'copies settings to rate_limits when git_http throttle is not set' do + setting = application_settings.create!( + throttle_authenticated_web_enabled: true, + throttle_authenticated_web_requests_per_period: 300, + throttle_authenticated_web_period_in_seconds: 60, + rate_limits: {} + ) + + migrate! + + setting.reload + expect(setting.rate_limits['throttle_authenticated_git_http_enabled']).to be(true) + expect(setting.rate_limits['throttle_authenticated_git_http_requests_per_period']).to eq(300) + expect(setting.rate_limits['throttle_authenticated_git_http_period_in_seconds']).to eq(60) + end + + it 'does not override existing git_http throttle settings' do + setting = application_settings.create!( + throttle_authenticated_web_enabled: true, + throttle_authenticated_web_requests_per_period: 300, + throttle_authenticated_web_period_in_seconds: 60, + rate_limits: { + 'throttle_authenticated_git_http_enabled' => true, + 'throttle_authenticated_git_http_requests_per_period' => 500, + 'throttle_authenticated_git_http_period_in_seconds' => 120 + } + ) + + migrate! + + setting.reload + expect(setting.rate_limits['throttle_authenticated_git_http_enabled']).to be(true) + expect(setting.rate_limits['throttle_authenticated_git_http_requests_per_period']).to eq(500) + expect(setting.rate_limits['throttle_authenticated_git_http_period_in_seconds']).to eq(120) + end + + it 'copies settings when git_http throttle is explicitly disabled' do + setting = application_settings.create!( + throttle_authenticated_web_enabled: true, + throttle_authenticated_web_requests_per_period: 300, + throttle_authenticated_web_period_in_seconds: 60, + rate_limits: { + 'other_setting' => true, + 'throttle_authenticated_git_http_enabled' => false, + 'throttle_authenticated_git_http_requests_per_period' => 35, + 'throttle_authenticated_git_http_period_in_seconds' => 35 + } + ) + + migrate! + + setting.reload + expect(setting.rate_limits['throttle_authenticated_git_http_enabled']).to be(true) + expect(setting.rate_limits['throttle_authenticated_git_http_requests_per_period']).to eq(300) + expect(setting.rate_limits['throttle_authenticated_git_http_period_in_seconds']).to eq(60) + + expect(setting.rate_limits['other_setting']).to be(true) + end + + it 'copies settings when git_http throttle is disabled but partially set' do + setting = application_settings.create!( + throttle_authenticated_web_enabled: true, + throttle_authenticated_web_requests_per_period: 300, + throttle_authenticated_web_period_in_seconds: 60, + rate_limits: { + 'throttle_authenticated_git_http_requests_per_period' => 35, + 'throttle_authenticated_git_http_period_in_seconds' => 35 + } + ) + + migrate! + + setting.reload + expect(setting.rate_limits['throttle_authenticated_git_http_enabled']).to be(true) + expect(setting.rate_limits['throttle_authenticated_git_http_requests_per_period']).to eq(300) + expect(setting.rate_limits['throttle_authenticated_git_http_period_in_seconds']).to eq(60) + end + end + + context 'when throttle_authenticated_web is disabled' do + it 'does not copy settings' do + setting = application_settings.create!( + throttle_authenticated_web_enabled: false, + throttle_authenticated_web_requests_per_period: 300, + throttle_authenticated_web_period_in_seconds: 60, + rate_limits: {} + ) + + migrate! + + setting.reload + expect(setting.rate_limits['throttle_authenticated_git_http_enabled']).to be_nil + end + end + end + + describe '#down' do + it 'is a no-op' do + setting = application_settings.create!( + throttle_authenticated_web_enabled: true, + throttle_authenticated_web_requests_per_period: 300, + throttle_authenticated_web_period_in_seconds: 60, + rate_limits: {} + ) + + migrate! + + setting.reload + expect(setting.rate_limits['throttle_authenticated_git_http_enabled']).to be(true) + + schema_migrate_down! + + setting.reload + expect(setting.rate_limits['throttle_authenticated_git_http_enabled']).to be(true) + expect(setting.rate_limits['throttle_authenticated_git_http_requests_per_period']).to eq(300) + expect(setting.rate_limits['throttle_authenticated_git_http_period_in_seconds']).to eq(60) + end + end +end