From 6251f6deb3004c88e23c3721f11a152ef20e07d4 Mon Sep 17 00:00:00 2001 From: hustewart Date: Thu, 22 May 2025 20:14:12 -0400 Subject: [PATCH 1/3] Add new rate limit for authenticated Git HTTP This commit adds three new fields to the rate_limits jsonb column that are related to rate limiting authenticated Git HTTP requests. It adds the new rate limits for authenticated Git HTTP requests to the throttle definitions and makes use of them in Rackt Attack. It checks if the setting is enabled and if the path is a git path. It leaves out the check against `unauthenticated` since this rate limit is for authenticated requests. This commit makes the authenticated Git HTTP rate limits setting visible in the network settings page available to admins. UI changes and backend checks have been placed behind a new feature flag named git_authenticated_http_limit that is disabled by default. --- app/helpers/application_settings_helper.rb | 3 + app/models/application_setting.rb | 10 ++ .../application_setting_implementation.rb | 5 + .../application_setting_rate_limits.json | 14 +++ .../_git_http_limits.html.haml | 14 +++ .../application_settings/network.html.haml | 1 + .../beta/git_authenticated_http_limit.yml | 10 ++ lib/gitlab/rack_attack.rb | 14 ++- lib/gitlab/rack_attack/request.rb | 6 ++ lib/gitlab/throttle.rb | 7 ++ locale/gitlab.pot | 15 +++ spec/features/admin/admin_settings_spec.rb | 61 ++++++++++++ spec/lib/gitlab/rack_attack/request_spec.rb | 50 ++++++++++ spec/lib/gitlab/rack_attack_spec.rb | 33 ++++++- spec/models/application_setting_spec.rb | 7 ++ spec/requests/rack_attack_global_spec.rb | 99 +++++++++++++++++++ .../network.html.haml_spec.rb | 30 +++++- 17 files changed, 376 insertions(+), 3 deletions(-) create mode 100644 config/feature_flags/beta/git_authenticated_http_limit.yml diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 0a2e2a1e77dd4a..2af7bb0c0203b7 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -444,6 +444,9 @@ def visible_attributes :throttle_authenticated_api_enabled, :throttle_authenticated_api_period_in_seconds, :throttle_authenticated_api_requests_per_period, + :throttle_authenticated_git_http_enabled, + :throttle_authenticated_git_http_period_in_seconds, + :throttle_authenticated_git_http_requests_per_period, :throttle_authenticated_git_lfs_enabled, :throttle_authenticated_git_lfs_period_in_seconds, :throttle_authenticated_git_lfs_requests_per_period, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 0c82f5b07de3e5..98df7e10bd339e 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -44,6 +44,9 @@ class ApplicationSetting < ApplicationRecord DEFAULT_HELM_MAX_PACKAGES_COUNT = 1000 + DEFAULT_AUTHENTICATED_GIT_HTTP_LIMIT = 3600 + DEFAULT_AUTHENTICATED_GIT_HTTP_PERIOD = 3600 + enum :whats_new_variant, { all_tiers: 0, current_tier: 1, disabled: 2 }, prefix: true enum :email_confirmation_setting, { off: 0, soft: 1, hard: 2 }, prefix: true @@ -599,6 +602,8 @@ def self.kroki_formats_attributes :throttle_authenticated_deprecated_api_requests_per_period, :throttle_authenticated_files_api_period_in_seconds, :throttle_authenticated_files_api_requests_per_period, + :throttle_authenticated_git_http_period_in_seconds, + :throttle_authenticated_git_http_requests_per_period, :throttle_authenticated_git_lfs_period_in_seconds, :throttle_authenticated_git_lfs_requests_per_period, :throttle_authenticated_packages_api_period_in_seconds, @@ -1115,6 +1120,11 @@ def self.rate_limits_definition group_archive_unarchive_api_limit: [:integer, { default: 60 }], project_invited_groups_api_limit: [:integer, { default: 60 }], projects_api_limit: [:integer, { default: 2000 }], + throttle_authenticated_git_http_enabled: [:boolean, { default: false }], + throttle_authenticated_git_http_requests_per_period: + [:integer, { default: DEFAULT_AUTHENTICATED_GIT_HTTP_LIMIT }], + throttle_authenticated_git_http_period_in_seconds: + [:integer, { default: DEFAULT_AUTHENTICATED_GIT_HTTP_PERIOD }], user_contributed_projects_api_limit: [:integer, { default: 100 }], user_projects_api_limit: [:integer, { default: 300 }], user_starred_projects_api_limit: [:integer, { default: 100 }], diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index 39344491cb090a..e4de7f61569547 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -212,6 +212,11 @@ def defaults # rubocop:disable Metrics/AbcSize throttle_authenticated_api_enabled: false, throttle_authenticated_api_period_in_seconds: 3600, throttle_authenticated_api_requests_per_period: 7200, + throttle_authenticated_git_http_enabled: false, + throttle_authenticated_git_http_requests_per_period: + ApplicationSetting::DEFAULT_AUTHENTICATED_GIT_HTTP_LIMIT, + throttle_authenticated_git_http_period_in_seconds: + ApplicationSetting::DEFAULT_AUTHENTICATED_GIT_HTTP_PERIOD, throttle_authenticated_git_lfs_enabled: false, throttle_authenticated_git_lfs_period_in_seconds: 60, throttle_authenticated_git_lfs_requests_per_period: 1000, diff --git a/app/validators/json_schemas/application_setting_rate_limits.json b/app/validators/json_schemas/application_setting_rate_limits.json index b7264ab69517ed..d6303858300c9b 100644 --- a/app/validators/json_schemas/application_setting_rate_limits.json +++ b/app/validators/json_schemas/application_setting_rate_limits.json @@ -84,6 +84,20 @@ "minimum": 0, "description": "Number of requests allowed to the GET /api/v4/projects endpoint." }, + "throttle_authenticated_git_http_enabled": { + "type": "boolean", + "description": "Enable authenticated Git HTTP request rate limit" + }, + "throttle_authenticated_git_http_requests_per_period": { + "type": "integer", + "minimum": 1, + "description": "Authenticated Git HTTP request rate limit" + }, + "throttle_authenticated_git_http_period_in_seconds": { + "type": "integer", + "minimum": 1, + "description": "Authenticated Git HTTP rate limit period in seconds" + }, "user_contributed_projects_api_limit": { "type": "integer", "minimum": 0, diff --git a/app/views/admin/application_settings/_git_http_limits.html.haml b/app/views/admin/application_settings/_git_http_limits.html.haml index e624667875f62d..754453b95bd455 100644 --- a/app/views/admin/application_settings/_git_http_limits.html.haml +++ b/app/views/admin/application_settings/_git_http_limits.html.haml @@ -14,5 +14,19 @@ .form-group = f.label :throttle_unauthenticated_git_http_period_in_seconds, _('Unauthenticated Git HTTP rate limit period in seconds'), class: 'gl-font-bold' = f.number_field :throttle_unauthenticated_git_http_period_in_seconds, class: 'form-control gl-form-input' + - if Feature.enabled?(:git_authenticated_http_limit, :instance) + %fieldset + %h5 + = _('Authenticated Git HTTP request rate limit') + .form-group + = f.gitlab_ui_checkbox_component :throttle_authenticated_git_http_enabled, + _('Enable authenticated Git HTTP request rate limit'), + help_text: _('Helps reduce authenticated Git HTTP request volume for git paths.') + .form-group + = f.label :throttle_authenticated_git_http_requests_per_period, _('Maximum authenticated Git HTTP requests per period per user'), class: 'gl-font-weight-bold' + = f.number_field :throttle_authenticated_git_http_requests_per_period, class: 'form-control gl-form-input' + .form-group + = f.label :throttle_authenticated_git_http_period_in_seconds, _('Authenticated Git HTTP rate limit period in seconds'), class: 'gl-font-weight-bold' + = f.number_field :throttle_authenticated_git_http_period_in_seconds, class: 'form-control gl-form-input' = f.submit _('Save changes'), pajamas_button: true diff --git a/app/views/admin/application_settings/network.html.haml b/app/views/admin/application_settings/network.html.haml index 81b9d7c8093f29..dbaae4c754d404 100644 --- a/app/views/admin/application_settings/network.html.haml +++ b/app/views/admin/application_settings/network.html.haml @@ -63,6 +63,7 @@ = render ::Layouts::SettingsBlockComponent.new(_('Git HTTP rate limits'), id: 'js-git-http-limits-settings', + testid: 'git-http-limits-settings', expanded: expanded_by_default?) do |c| - c.with_description do = _('Configure specific limits for Git HTTP requests.') diff --git a/config/feature_flags/beta/git_authenticated_http_limit.yml b/config/feature_flags/beta/git_authenticated_http_limit.yml new file mode 100644 index 00000000000000..b99e42ddc6a70c --- /dev/null +++ b/config/feature_flags/beta/git_authenticated_http_limit.yml @@ -0,0 +1,10 @@ +--- +name: git_authenticated_http_limit +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/543784 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/191552 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/543768 +milestone: '18.1' +group: group::source code +type: beta +default_enabled: false diff --git a/lib/gitlab/rack_attack.rb b/lib/gitlab/rack_attack.rb index 89d7da2641d4ea..054b6b5f0e2ad9 100644 --- a/lib/gitlab/rack_attack.rb +++ b/lib/gitlab/rack_attack.rb @@ -122,7 +122,8 @@ def self.throttle_definitions Gitlab::Throttle.throttle_authenticated_git_lfs_options, ->(req) { req.throttled_identifer([:api]) if req.throttle_authenticated_git_lfs? } ), - **throttle_definitions_unauthenticated_git_http + **throttle_definitions_unauthenticated_git_http, + **throttle_definitions_authenticated_git_http } end @@ -135,6 +136,17 @@ def self.throttle_definitions_unauthenticated_git_http } end + def self.throttle_definitions_authenticated_git_http + return {} unless Feature.enabled?(:git_authenticated_http_limit, :instance) + + { + 'throttle_authenticated_git_http' => ThrottleDefinition.new( + Gitlab::Throttle.throttle_authenticated_git_http_options, + ->(req) { req.throttled_identifer([:api]) if req.throttle_authenticated_git_http? } + ) + } + end + def self.configure_throttles(rack_attack) # Each of these settings follows the same pattern of specifying separate # authenticated and unauthenticated rates via settings diff --git a/lib/gitlab/rack_attack/request.rb b/lib/gitlab/rack_attack/request.rb index 4a6902594cccb2..12de588cec30c5 100644 --- a/lib/gitlab/rack_attack/request.rb +++ b/lib/gitlab/rack_attack/request.rb @@ -178,6 +178,12 @@ def throttle_unauthenticated_git_http? unauthenticated? end + def throttle_authenticated_git_http? + git_path? && + Feature.enabled?(:git_authenticated_http_limit, :instance) && + Gitlab::Throttle.settings.throttle_authenticated_git_http_enabled + end + def throttle_authenticated_git_lfs? git_lfs_path? && Gitlab::Throttle.settings.throttle_authenticated_git_lfs_enabled diff --git a/lib/gitlab/throttle.rb b/lib/gitlab/throttle.rb index 8158b7afa6b2c4..b945c765edd0ac 100644 --- a/lib/gitlab/throttle.rb +++ b/lib/gitlab/throttle.rb @@ -78,6 +78,13 @@ def self.throttle_unauthenticated_git_http_options { limit: limit_proc, period: period_proc } end + def self.throttle_authenticated_git_http_options + 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 } + + { limit: limit_proc, period: period_proc } + end + def self.throttle_authenticated_git_lfs_options limit_proc = proc { |req| settings.throttle_authenticated_git_lfs_requests_per_period } period_proc = proc { |req| settings.throttle_authenticated_git_lfs_period_in_seconds.seconds } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4cfc61ba2c00e3..4acaa0c1d709d4 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -9212,6 +9212,12 @@ msgstr "" msgid "Authenticated API requests" msgstr "" +msgid "Authenticated Git HTTP rate limit period in seconds" +msgstr "" + +msgid "Authenticated Git HTTP request rate limit" +msgstr "" + msgid "Authenticated Git LFS rate limit period in seconds" msgstr "" @@ -24168,6 +24174,9 @@ msgstr "" msgid "Enable authenticated API request rate limit" msgstr "" +msgid "Enable authenticated Git HTTP request rate limit" +msgstr "" + msgid "Enable authenticated Git LFS request rate limit" msgstr "" @@ -31096,6 +31105,9 @@ msgstr "" msgid "Helps prevent malicious users hide their activity." msgstr "" +msgid "Helps reduce authenticated Git HTTP request volume for git paths." +msgstr "" + msgid "Helps reduce request volume (for example, from crawlers or abusive bots)" msgstr "" @@ -37359,6 +37371,9 @@ msgstr "" msgid "Maximum authenticated API requests per rate limit period per user" msgstr "" +msgid "Maximum authenticated Git HTTP requests per period per user" +msgstr "" + msgid "Maximum authenticated requests to %{open}project/:id/jobs%{close} per minute" msgstr "" diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index de0c26610725dc..cb9fbee9945f06 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -862,6 +862,67 @@ ) end + context 'when git_authenticated_http_limit feature flag is enabled' do + before do + stub_feature_flags(git_authenticated_http_limit: true) + end + + it 'changes authenticated Git HTTP rate limits settings' do + visit network_admin_application_settings_path + + # Default settings + expect(current_settings.throttle_authenticated_git_http_enabled) + .to eq(false) + expect(current_settings.throttle_authenticated_git_http_requests_per_period) + .to eq(ApplicationSetting::DEFAULT_AUTHENTICATED_GIT_HTTP_PERIOD) + expect(current_settings.throttle_authenticated_git_http_period_in_seconds) + .to eq(ApplicationSetting::DEFAULT_AUTHENTICATED_GIT_HTTP_LIMIT) + + within_testid('git-http-limits-settings') do + check 'Enable unauthenticated Git HTTP request rate limit' + + fill_in( + 'Maximum authenticated Git HTTP requests per period per user', + with: ApplicationSetting::DEFAULT_AUTHENTICATED_GIT_HTTP_LIMIT + 1 + ) + + fill_in( + 'Authenticated Git HTTP rate limit period in seconds', + with: ApplicationSetting::DEFAULT_AUTHENTICATED_GIT_HTTP_PERIOD + 2 + ) + + click_button 'Save changes' + end + + expect(page).to have_content 'Application settings saved successfully' + + expect(current_settings.throttle_authenticated_git_http_enabled) + .to eq(false) + + expect(current_settings.throttle_authenticated_git_http_requests_per_period) + .to eq(ApplicationSetting::DEFAULT_AUTHENTICATED_GIT_HTTP_PERIOD + 1) + + expect(current_settings.throttle_authenticated_git_http_period_in_seconds) + .to eq(ApplicationSetting::DEFAULT_AUTHENTICATED_GIT_HTTP_LIMIT + 2) + end + end + + context 'when git_authenticated_http_limit feature flag is disabled' do + before do + stub_feature_flags(git_authenticated_http_limit: false) + end + + it 'does not show authenticated Git HTTP rate limit settings' do + stub_feature_flags(git_authenticated_http_limit: false) + visit network_admin_application_settings_path + + within_testid('git-http-limits-settings') do + expect(page).not_to have_content('Authenticated Git HTTP request rate limit') + expect(page).not_to have_field('Enable authenticated Git HTTP request rate limit') + end + end + end + it 'changes Issues rate limits settings' do visit network_admin_application_settings_path diff --git a/spec/lib/gitlab/rack_attack/request_spec.rb b/spec/lib/gitlab/rack_attack/request_spec.rb index 09142fbc6fef0d..fd06a2eb220beb 100644 --- a/spec/lib/gitlab/rack_attack/request_spec.rb +++ b/spec/lib/gitlab/rack_attack/request_spec.rb @@ -250,6 +250,56 @@ end end + describe '#throttle_authenticated_git_http?' do + let_it_be(:project) { create(:project) } + + let(:git_info_refs_path) { "/#{project.full_path}.git/info/refs?service=git-upload-pack" } + let(:git_receive_pack_path) { "/#{project.full_path}.git/git-receive-pack" } + let(:git_upload_pack_path) { "/#{project.full_path}.git/git-upload-pack" } + + subject { request.throttle_authenticated_git_http? } + + where(:path, :request_authenticated?, :application_setting_throttle_authenticated_git_http_enabled, :feature_flag_enabled, :expected) do + ref(:git_info_refs_path) | true | true | true | true + ref(:git_info_refs_path) | false | true | true | true + ref(:git_info_refs_path) | true | false | true | false + ref(:git_info_refs_path) | false | false | true | false + ref(:git_info_refs_path) | true | true | false | false + ref(:git_info_refs_path) | false | true | false | false + + ref(:git_receive_pack_path) | true | true | true | true + ref(:git_receive_pack_path) | false | true | true | true + ref(:git_receive_pack_path) | true | false | true | false + ref(:git_receive_pack_path) | false | false | true | false + ref(:git_receive_pack_path) | true | true | false | false + ref(:git_receive_pack_path) | false | true | false | false + + ref(:git_upload_pack_path) | true | true | true | true + ref(:git_upload_pack_path) | false | true | true | true + ref(:git_upload_pack_path) | true | false | true | false + ref(:git_upload_pack_path) | false | false | true | false + ref(:git_upload_pack_path) | true | true | false | false + ref(:git_upload_pack_path) | false | true | false | false + + '/users/sign_in' | true | true | true | false + '/users/sign_in' | false | true | true | false + '/users/sign_in' | true | false | true | false + '/users/sign_in' | false | false | true | false + '/users/sign_in' | true | true | false | false + '/users/sign_in' | false | true | false | false + end + + with_them do + before do + stub_application_setting(throttle_authenticated_git_http_enabled: application_setting_throttle_authenticated_git_http_enabled) + stub_feature_flags(git_authenticated_http_limit: feature_flag_enabled) + allow(request).to receive(:authenticated?).and_return(request_authenticated?) + end + + it { is_expected.to eq expected } + end + end + describe '#protected_path?' do subject { request.protected_path? } diff --git a/spec/lib/gitlab/rack_attack_spec.rb b/spec/lib/gitlab/rack_attack_spec.rb index a01daed8bab82f..686060ed8fe9dc 100644 --- a/spec/lib/gitlab/rack_attack_spec.rb +++ b/spec/lib/gitlab/rack_attack_spec.rb @@ -22,7 +22,8 @@ throttle_authenticated_packages_api: Gitlab::Throttle.options(:packages_api, authenticated: true), throttle_authenticated_git_lfs: Gitlab::Throttle.throttle_authenticated_git_lfs_options, throttle_unauthenticated_files_api: Gitlab::Throttle.options(:files_api, authenticated: false), - throttle_authenticated_files_api: Gitlab::Throttle.options(:files_api, authenticated: true) + throttle_authenticated_files_api: Gitlab::Throttle.options(:files_api, authenticated: true), + throttle_authenticated_git_http: Gitlab::Throttle.throttle_authenticated_git_http_options } end @@ -118,6 +119,36 @@ end end + describe '.throttle_definitions' do + let(:throttle_definitions) { described_class.throttle_definitions } + + context 'with git_authenticated_http_limit feature flag enabled' do + before do + stub_feature_flags(git_authenticated_http_limit: true) + end + + it 'includes authenticated git http throttle' do + expect(throttle_definitions.keys).to include('throttle_authenticated_git_http') + end + + it 'properly configures the authenticated git http throttle' do + throttle_def = throttle_definitions['throttle_authenticated_git_http'] + expect(throttle_def).to be_a(Gitlab::RackAttack::ThrottleDefinition) + expect(throttle_def.options).to include(:limit, :period) + end + end + + context 'with git_authenticated_http_limit feature flag disabled' do + before do + stub_feature_flags(git_authenticated_http_limit: false) + end + + it 'excludes authenticated git http throttle' do + expect(throttle_definitions.keys).not_to include('throttle_authenticated_git_http') + end + end + end + describe '.throttled_response_headers' do where(:matched, :match_data, :headers) do [ diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 7481999a153b2d..41b36562bcbcb6 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -252,6 +252,11 @@ spam_check_endpoint_enabled: false, suggest_pipeline_enabled: true, terminal_max_session_time: 0, + throttle_authenticated_git_http_enabled: false, + throttle_authenticated_git_http_requests_per_period: + ApplicationSetting::DEFAULT_AUTHENTICATED_GIT_HTTP_LIMIT, + throttle_authenticated_git_http_period_in_seconds: + ApplicationSetting::DEFAULT_AUTHENTICATED_GIT_HTTP_PERIOD, throttle_unauthenticated_git_http_enabled: false, throttle_unauthenticated_git_http_period_in_seconds: 3600, throttle_unauthenticated_git_http_requests_per_period: 3600, @@ -598,6 +603,8 @@ def many_usernames(num = 100) throttle_authenticated_deprecated_api_requests_per_period throttle_authenticated_files_api_period_in_seconds throttle_authenticated_files_api_requests_per_period + throttle_authenticated_git_http_requests_per_period + throttle_authenticated_git_http_period_in_seconds throttle_authenticated_git_lfs_period_in_seconds throttle_authenticated_git_lfs_requests_per_period throttle_authenticated_packages_api_period_in_seconds diff --git a/spec/requests/rack_attack_global_spec.rb b/spec/requests/rack_attack_global_spec.rb index 73cb4aa47818f3..b32af8d581e8cd 100644 --- a/spec/requests/rack_attack_global_spec.rb +++ b/spec/requests/rack_attack_global_spec.rb @@ -28,6 +28,8 @@ throttle_unauthenticated_packages_api_period_in_seconds: 1, throttle_authenticated_packages_api_requests_per_period: 100, throttle_authenticated_packages_api_period_in_seconds: 1, + throttle_authenticated_git_http_requests_per_period: 100, + throttle_authenticated_git_http_period_in_seconds: 1, throttle_unauthenticated_git_http_requests_per_period: 100, throttle_unauthenticated_git_http_period_in_seconds: 1, throttle_authenticated_git_lfs_requests_per_period: 100, @@ -700,6 +702,103 @@ def do_request end end + describe 'authenticated git http requests' do + let_it_be(:project) { create(:project, :repository, :public) } + let_it_be(:user) { create(:user) } + let_it_be(:token) { create(:personal_access_token, user: user) } + + let(:git_info_refs_path) { "/#{project.full_path}.git/info/refs?service=git-upload-pack" } + let(:headers) do + WorkhorseHelpers + .workhorse_internal_api_request_header + .merge(basic_auth_headers(user, token)) + end + + def do_request + get git_info_refs_path, headers: headers + end + + context 'with git_authenticated_http_limit feature flag disabled' do + before do + stub_feature_flags(git_authenticated_http_limit: false) + settings_to_set[:throttle_authenticated_git_http_requests_per_period] = requests_per_period + settings_to_set[:throttle_authenticated_git_http_period_in_seconds] = period_in_seconds + settings_to_set[:throttle_authenticated_git_http_enabled] = true + stub_application_setting(settings_to_set) + end + + it 'does not apply rate limiting' do + # Multiple requests should succeed even though they exceed the limit + (requests_per_period + 2).times do + do_request + expect(response).to have_gitlab_http_status(:ok) + end + end + end + + context 'with git_authenticated_http_limit feature flag enabled' do + before do + stub_feature_flags(git_authenticated_http_limit: true) + end + + context 'when rate limit is enabled' do + before do + settings_to_set[:throttle_authenticated_git_http_requests_per_period] = requests_per_period + settings_to_set[:throttle_authenticated_git_http_period_in_seconds] = period_in_seconds + settings_to_set[:throttle_authenticated_git_http_enabled] = true + stub_application_setting(settings_to_set) + end + + it 'rejects requests over the rate limit' do + # First requests should succeed + requests_per_period.times do + do_request + expect(response).to have_gitlab_http_status(:ok) + end + + # Request over the limit should be throttled + do_request + expect(response).to have_gitlab_http_status(:too_many_requests) + end + + it 'allows different users to make requests independently' do + # First user makes requests up to the limit + requests_per_period.times do + do_request + expect(response).to have_gitlab_http_status(:ok) + end + + # Different user should not be throttled + different_user = create(:user) + different_token = create(:personal_access_token, user: different_user) + different_headers = WorkhorseHelpers + .workhorse_internal_api_request_header + .merge(basic_auth_headers(different_user, different_token)) + + get git_info_refs_path, headers: different_headers + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when rate limit is disabled' do + before do + settings_to_set[:throttle_authenticated_git_http_requests_per_period] = requests_per_period + settings_to_set[:throttle_authenticated_git_http_period_in_seconds] = period_in_seconds + settings_to_set[:throttle_authenticated_git_http_enabled] = false + stub_application_setting(settings_to_set) + end + + it 'does not reject requests over the rate limit' do + # Multiple requests should succeed even though they exceed the limit + (requests_per_period + 2).times do + do_request + expect(response).to have_gitlab_http_status(:ok) + end + end + end + end + end + describe 'unauthenticated git http requests' do let_it_be(:project) { create(:project, :repository, :public) } diff --git a/spec/views/admin/application_settings/network.html.haml_spec.rb b/spec/views/admin/application_settings/network.html.haml_spec.rb index be298123b480ad..6ebdc75971e462 100644 --- a/spec/views/admin/application_settings/network.html.haml_spec.rb +++ b/spec/views/admin/application_settings/network.html.haml_spec.rb @@ -11,7 +11,7 @@ allow(view).to receive(:current_user) { admin } end - context 'for Git HTTP rate limit' do + context 'for Git HTTP rate limits' do it 'renders the `git_http_rate_limit_unauthenticated` field' do render @@ -19,6 +19,34 @@ expect(rendered).to have_field('application_setting_throttle_unauthenticated_git_http_requests_per_period') expect(rendered).to have_field('application_setting_throttle_unauthenticated_git_http_period_in_seconds') end + + context 'with git_authenticated_http_limit feature flag enabled' do + before do + stub_feature_flags(git_authenticated_http_limit: true) + end + + it 'renders the `git_http_rate_limit_authenticated` field' do + render + + expect(rendered).to have_field('application_setting_throttle_authenticated_git_http_enabled') + expect(rendered).to have_field('application_setting_throttle_authenticated_git_http_requests_per_period') + expect(rendered).to have_field('application_setting_throttle_authenticated_git_http_period_in_seconds') + end + end + + context 'with git_authenticated_http_limit feature flag disabled' do + before do + stub_feature_flags(git_authenticated_http_limit: false) + end + + it 'does not render the `git_http_rate_limit_authenticated` field' do + render + + expect(rendered).not_to have_field('application_setting_throttle_authenticated_git_http_enabled') + expect(rendered).not_to have_field('application_setting_throttle_authenticated_git_http_requests_per_period') + expect(rendered).not_to have_field('application_setting_throttle_authenticated_git_http_period_in_seconds') + end + end end context 'for Users API rate limits' do -- GitLab From 8a401342da9026670238c48ef637136489bfbe1f Mon Sep 17 00:00:00 2001 From: hustewart Date: Wed, 4 Jun 2025 10:29:02 -0400 Subject: [PATCH 2/3] Use correct heading level and class --- .../admin/application_settings/_git_http_limits.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/application_settings/_git_http_limits.html.haml b/app/views/admin/application_settings/_git_http_limits.html.haml index 754453b95bd455..99c85b60b948bb 100644 --- a/app/views/admin/application_settings/_git_http_limits.html.haml +++ b/app/views/admin/application_settings/_git_http_limits.html.haml @@ -2,7 +2,7 @@ = form_errors(@application_setting) %fieldset - %h5 + %h3.gl-heading-5 = _('Unauthenticated Git HTTP request rate limit') .form-group = f.gitlab_ui_checkbox_component :throttle_unauthenticated_git_http_enabled, @@ -16,7 +16,7 @@ = f.number_field :throttle_unauthenticated_git_http_period_in_seconds, class: 'form-control gl-form-input' - if Feature.enabled?(:git_authenticated_http_limit, :instance) %fieldset - %h5 + %h3.gl-heading-5 = _('Authenticated Git HTTP request rate limit') .form-group = f.gitlab_ui_checkbox_component :throttle_authenticated_git_http_enabled, -- GitLab From 6ad9a9169ff0feab00ccc8425c9e6fcd992c6dcb Mon Sep 17 00:00:00 2001 From: hustewart Date: Mon, 9 Jun 2025 13:50:06 -0400 Subject: [PATCH 3/3] Remove feature flag call from rack attack config The QA tests are failing setting up the application in the CNG instances. It appears to be application related containers that fail, and I am wondering if having a Feature Flag call in the Rack Attack config initializer is causing a problem. There is no problem in GDK or RSpec. The Feature Flag should still work as expected in terms of keeping the feature off. This change removes the Feature Flag condition around including the throttle configuration at all. So the configuration will be added, but it's usage will still be behind `git_authenticated_http_limit`. --- lib/gitlab/rack_attack.rb | 2 -- spec/lib/gitlab/rack_attack_spec.rb | 30 ----------------------------- 2 files changed, 32 deletions(-) diff --git a/lib/gitlab/rack_attack.rb b/lib/gitlab/rack_attack.rb index 054b6b5f0e2ad9..d902237dd5069c 100644 --- a/lib/gitlab/rack_attack.rb +++ b/lib/gitlab/rack_attack.rb @@ -137,8 +137,6 @@ def self.throttle_definitions_unauthenticated_git_http end def self.throttle_definitions_authenticated_git_http - return {} unless Feature.enabled?(:git_authenticated_http_limit, :instance) - { 'throttle_authenticated_git_http' => ThrottleDefinition.new( Gitlab::Throttle.throttle_authenticated_git_http_options, diff --git a/spec/lib/gitlab/rack_attack_spec.rb b/spec/lib/gitlab/rack_attack_spec.rb index 686060ed8fe9dc..976d267cf19bd8 100644 --- a/spec/lib/gitlab/rack_attack_spec.rb +++ b/spec/lib/gitlab/rack_attack_spec.rb @@ -119,36 +119,6 @@ end end - describe '.throttle_definitions' do - let(:throttle_definitions) { described_class.throttle_definitions } - - context 'with git_authenticated_http_limit feature flag enabled' do - before do - stub_feature_flags(git_authenticated_http_limit: true) - end - - it 'includes authenticated git http throttle' do - expect(throttle_definitions.keys).to include('throttle_authenticated_git_http') - end - - it 'properly configures the authenticated git http throttle' do - throttle_def = throttle_definitions['throttle_authenticated_git_http'] - expect(throttle_def).to be_a(Gitlab::RackAttack::ThrottleDefinition) - expect(throttle_def.options).to include(:limit, :period) - end - end - - context 'with git_authenticated_http_limit feature flag disabled' do - before do - stub_feature_flags(git_authenticated_http_limit: false) - end - - it 'excludes authenticated git http throttle' do - expect(throttle_definitions.keys).not_to include('throttle_authenticated_git_http') - end - end - end - describe '.throttled_response_headers' do where(:matched, :match_data, :headers) do [ -- GitLab