From bac26b731a3bf99392abe7376c957e0c06a27825 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Tue, 25 Nov 2025 16:26:37 +0100 Subject: [PATCH 1/2] Migrate Git HTTP traffic to Git HTTP rate limiter Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/581142 **Problem** This change removes the feature flag and permanently excludes Git HTTP requests from the web rate limiter, preventing them from being throttled twice (once by the web limiter and once by the Git HTTP limiter). **Changes** - Remove exclude_git_http_from_web_rate_limiter feature flag - Update throttle_authenticated_web? to always exclude Git HTTP paths - Add migration to backfill Git HTTP throttle settings from web throttle settings for instances with web throttling enabled The migration copies existing web throttle configuration to the Git HTTP throttle settings when: - Web throttling is enabled - Git HTTP throttle is not already configured or is disabled This ensures a smooth transition without disrupting existing rate limiting behavior. Changelog: changed --- ...exclude_git_http_from_web_rate_limiter.yml | 10 -- ...hrottle_authenticated_git_http_settings.rb | 26 ++++ db/schema_migrations/20251125102233 | 1 + lib/gitlab/rack_attack/request.rb | 2 +- spec/lib/gitlab/rack_attack/request_spec.rb | 33 ----- ...le_authenticated_git_http_settings_spec.rb | 129 ++++++++++++++++++ 6 files changed, 157 insertions(+), 44 deletions(-) delete mode 100644 config/feature_flags/gitlab_com_derisk/exclude_git_http_from_web_rate_limiter.yml create mode 100644 db/post_migrate/20251125102233_backfill_throttle_authenticated_git_http_settings.rb create mode 100644 db/schema_migrations/20251125102233 create mode 100644 spec/migrations/db/post_migrate/20251125102233_backfill_throttle_authenticated_git_http_settings_spec.rb 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 32fdae51d02492..00000000000000 --- 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 00000000000000..5e0a8e49f6756a --- /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 00000000000000..7c1261557019cf --- /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 283772948e0a22..51fa35d7454f94 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 diff --git a/spec/lib/gitlab/rack_attack/request_spec.rb b/spec/lib/gitlab/rack_attack/request_spec.rb index 0de42dd42c0f56..2e56f5bbc22928 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 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 00000000000000..01173dbcd24143 --- /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 -- GitLab From 588732c0bb9d1d37c6e2114c60dc0434e17443df Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Wed, 10 Dec 2025 19:05:15 +0100 Subject: [PATCH 2/2] Add fallback to Web rate limits when Git HTTP limits are disabled For compatibility with backports. --- lib/gitlab/rack_attack/request.rb | 5 +++- lib/gitlab/throttle.rb | 4 +++ spec/lib/gitlab/rack_attack/request_spec.rb | 24 ++++++++++++++++ spec/lib/gitlab/throttle_spec.rb | 32 +++++++++++++++++++++ 4 files changed, 64 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/rack_attack/request.rb b/lib/gitlab/rack_attack/request.rb index 51fa35d7454f94..97c8b84faf4790 100644 --- a/lib/gitlab/rack_attack/request.rb +++ b/lib/gitlab/rack_attack/request.rb @@ -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 b945c765edd0ac..365d087f5d73dc 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 2e56f5bbc22928..e3078af320f194 100644 --- a/spec/lib/gitlab/rack_attack/request_spec.rb +++ b/spec/lib/gitlab/rack_attack/request_spec.rb @@ -288,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 50d723193ac907..67c9fff6e6ee98 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 -- GitLab