diff --git a/config/feature_flags/development/rate_limit_frontend_requests.yml b/config/feature_flags/development/rate_limit_frontend_requests.yml new file mode 100644 index 0000000000000000000000000000000000000000..624dc303568529be242db0e1a1c08e77c7a9699e --- /dev/null +++ b/config/feature_flags/development/rate_limit_frontend_requests.yml @@ -0,0 +1,8 @@ +--- +name: rate_limit_frontend_requests +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78082 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/350623 +milestone: '14.8' +type: development +group: group::integrations +default_enabled: false diff --git a/doc/user/admin_area/settings/user_and_ip_rate_limits.md b/doc/user/admin_area/settings/user_and_ip_rate_limits.md index d713ef4b4e0d6289e1862bd19dfa86da9a0afd98..ad61f18343c7910f4ad43fda6172ab36a2fde3a0 100644 --- a/doc/user/admin_area/settings/user_and_ip_rate_limits.md +++ b/doc/user/admin_area/settings/user_and_ip_rate_limits.md @@ -22,6 +22,10 @@ NOTE: By default, all Git operations are first tried unauthenticated. Because of this, HTTP Git operations may trigger the rate limits configured for unauthenticated requests. +NOTE: +The rate limits for API requests don't affect requests made by the frontend, as these are always +counted as web traffic. + ## Enable unauthenticated API request rate limit To enable the unauthenticated request rate limit: diff --git a/lib/gitlab/rack_attack/request.rb b/lib/gitlab/rack_attack/request.rb index 94ae29af3d0f2635866ddd3cd102d595b33b5d17..d4a65227a20308801e10cbc92de29c80796b5a56 100644 --- a/lib/gitlab/rack_attack/request.rb +++ b/lib/gitlab/rack_attack/request.rb @@ -3,6 +3,8 @@ module Gitlab module RackAttack module Request + include ::Gitlab::Utils::StrongMemoize + FILES_PATH_REGEX = %r{^/api/v\d+/projects/[^/]+/repository/files/.+}.freeze GROUP_PATH_REGEX = %r{^/api/v\d+/groups/[^/]+/?$}.freeze @@ -30,15 +32,15 @@ def api_request? end def api_internal_request? - path =~ %r{^/api/v\d+/internal/} + path.match?(%r{^/api/v\d+/internal/}) end def health_check_request? - path =~ %r{^/-/(health|liveness|readiness|metrics)} + path.match?(%r{^/-/(health|liveness|readiness|metrics)}) end def container_registry_event? - path =~ %r{^/api/v\d+/container_registry_event/} + path.match?(%r{^/api/v\d+/container_registry_event/}) end def product_analytics_collector_request? @@ -58,7 +60,7 @@ def protected_path? end def protected_path_regex - path =~ protected_paths_regex + path.match?(protected_paths_regex) end def throttle?(throttle, authenticated:) @@ -70,6 +72,7 @@ def throttle?(throttle, authenticated:) def throttle_unauthenticated_api? api_request? && !should_be_skipped? && + !frontend_request? && !throttle_unauthenticated_packages_api? && !throttle_unauthenticated_files_api? && !throttle_unauthenticated_deprecated_api? && @@ -78,7 +81,7 @@ def throttle_unauthenticated_api? end def throttle_unauthenticated_web? - web_request? && + (web_request? || frontend_request?) && !should_be_skipped? && # TODO: Column will be renamed in https://gitlab.com/gitlab-org/gitlab/-/issues/340031 Gitlab::Throttle.settings.throttle_unauthenticated_enabled && @@ -87,6 +90,7 @@ def throttle_unauthenticated_web? def throttle_authenticated_api? api_request? && + !frontend_request? && !throttle_authenticated_packages_api? && !throttle_authenticated_files_api? && !throttle_authenticated_deprecated_api? && @@ -94,7 +98,7 @@ def throttle_authenticated_api? end def throttle_authenticated_web? - web_request? && + (web_request? || frontend_request?) && !throttle_authenticated_git_lfs? && Gitlab::Throttle.settings.throttle_authenticated_web_enabled end @@ -178,15 +182,26 @@ def protected_paths_regex end def packages_api_path? - path =~ ::Gitlab::Regex::Packages::API_PATH_REGEX + path.match?(::Gitlab::Regex::Packages::API_PATH_REGEX) end def git_lfs_path? - path =~ Gitlab::PathRegex.repository_git_lfs_route_regex + path.match?(Gitlab::PathRegex.repository_git_lfs_route_regex) end def files_api_path? - path =~ FILES_PATH_REGEX + path.match?(FILES_PATH_REGEX) + end + + def frontend_request? + return false unless Feature.enabled?(:rate_limit_frontend_requests, default_enabled: :yaml) + + strong_memoize(:frontend_request) do + next false unless env.include?('HTTP_X_CSRF_TOKEN') && session.include?(:_csrf_token) + + # CSRF tokens are not verified for GET/HEAD requests, so we pretend that we always have a POST request. + Gitlab::RequestForgeryProtection.verified?(env.merge('REQUEST_METHOD' => 'POST')) + end end def deprecated_api_request? @@ -195,7 +210,7 @@ def deprecated_api_request? with_projects = params['with_projects'] with_projects = true if with_projects.blank? - path =~ GROUP_PATH_REGEX && Gitlab::Utils.to_boolean(with_projects) + path.match?(GROUP_PATH_REGEX) && Gitlab::Utils.to_boolean(with_projects) end end end diff --git a/spec/channels/application_cable/connection_spec.rb b/spec/channels/application_cable/connection_spec.rb index c10e0c0cab451105edd972dab67355cbbb8ddefe..affde0095cff014fa7441e6838ab4802b8c934cc 100644 --- a/spec/channels/application_cable/connection_spec.rb +++ b/spec/channels/application_cable/connection_spec.rb @@ -3,15 +3,11 @@ require 'spec_helper' RSpec.describe ApplicationCable::Connection, :clean_gitlab_redis_sessions do - let(:session_id) { Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') } + include SessionHelpers context 'when session cookie is set' do before do - Gitlab::Redis::Sessions.with do |redis| - redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash)) - end - - cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id + stub_session(session_hash) end context 'when user is logged in' do diff --git a/spec/lib/gitlab/rack_attack/request_spec.rb b/spec/lib/gitlab/rack_attack/request_spec.rb index ecdcc23e5881d3ef0fb4883933efd1e06da55661..72592832a8966c49c3179ac8d46b7f11700f30a1 100644 --- a/spec/lib/gitlab/rack_attack/request_spec.rb +++ b/spec/lib/gitlab/rack_attack/request_spec.rb @@ -5,6 +5,19 @@ RSpec.describe Gitlab::RackAttack::Request do using RSpec::Parameterized::TableSyntax + let(:env) { {} } + let(:session) { {} } + let(:request) do + ::Rack::Attack::Request.new( + env.reverse_merge( + 'REQUEST_METHOD' => 'GET', + 'PATH_INFO' => path, + 'rack.input' => StringIO.new, + 'rack.session' => session + ) + ) + end + describe 'FILES_PATH_REGEX' do subject { described_class::FILES_PATH_REGEX } @@ -16,11 +29,80 @@ it { is_expected.not_to match('/api/v4/projects/some/nested/repo/repository/files/README') } end + describe '#api_request?' do + subject { request.api_request? } + + where(:path, :expected) do + '/' | false + '/groups' | false + + '/api' | true + '/api/v4/groups/1' | true + end + + with_them do + it { is_expected.to eq(expected) } + end + end + + describe '#web_request?' do + subject { request.web_request? } + + where(:path, :expected) do + '/' | true + '/groups' | true + + '/api' | false + '/api/v4/groups/1' | false + end + + with_them do + it { is_expected.to eq(expected) } + end + end + + describe '#frontend_request?', :allow_forgery_protection do + subject { request.send(:frontend_request?) } + + let(:path) { '/' } + + # Define these as local variables so we can use them in the `where` block. + valid_token = SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) + other_token = SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) + + where(:session, :env, :expected) do + {} | {} | false # rubocop:disable Lint/BinaryOperatorWithIdenticalOperands + {} | { 'HTTP_X_CSRF_TOKEN' => valid_token } | false + { _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => other_token } | false + { _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => valid_token } | true + end + + with_them do + it { is_expected.to eq(expected) } + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(rate_limit_frontend_requests: false) + end + + where(:session, :env) do + {} | {} # rubocop:disable Lint/BinaryOperatorWithIdenticalOperands + {} | { 'HTTP_X_CSRF_TOKEN' => valid_token } + { _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => other_token } + { _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => valid_token } + end + + with_them do + it { is_expected.to be(false) } + end + end + end + describe '#deprecated_api_request?' do - let(:env) { { 'REQUEST_METHOD' => 'GET', 'rack.input' => StringIO.new, 'PATH_INFO' => path, 'QUERY_STRING' => query } } - let(:request) { ::Rack::Attack::Request.new(env) } + subject { request.send(:deprecated_api_request?) } - subject { !!request.__send__(:deprecated_api_request?) } + let(:env) { { 'QUERY_STRING' => query } } where(:path, :query, :expected) do '/' | '' | false diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 2bc642f8b14806b307331592f904a86ad3827a1b..07c8f76cbbf392d8cc7f10619dc4653ac0ff1ffa 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -5,6 +5,7 @@ RSpec.describe API::Commits do include ProjectForksHelper + include SessionHelpers let(:user) { create(:user) } let(:guest) { create(:user).tap { |u| project.add_guest(u) } } @@ -378,14 +379,7 @@ context 'when using warden' do it 'increments usage counters', :clean_gitlab_redis_sessions do - session_id = Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') - session_hash = { 'warden.user.user.key' => [[user.id], user.encrypted_password[0, 29]] } - - Gitlab::Redis::Sessions.with do |redis| - redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash)) - end - - cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id + stub_session('warden.user.user.key' => [[user.id], user.encrypted_password[0, 29]]) expect(::Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_commits_count) expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).to receive(:track_web_ide_edit_action) diff --git a/spec/requests/rack_attack_global_spec.rb b/spec/requests/rack_attack_global_spec.rb index 793438808a5932622e1e51e57f486ede13d12228..f8c8351e1b30277bc56d1afd219ac0a76e0caa2f 100644 --- a/spec/requests/rack_attack_global_spec.rb +++ b/spec/requests/rack_attack_global_spec.rb @@ -4,6 +4,7 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_caching do include RackAttackSpecHelpers + include SessionHelpers let(:settings) { Gitlab::CurrentSettings.current_application_settings } @@ -63,6 +64,22 @@ end end + describe 'API requests from the frontend', :api, :clean_gitlab_redis_sessions do + context 'when unauthenticated' do + it_behaves_like 'rate-limited frontend API requests' do + let(:throttle_setting_prefix) { 'throttle_unauthenticated' } + end + end + + context 'when authenticated' do + it_behaves_like 'rate-limited frontend API requests' do + let_it_be(:personal_access_token) { create(:personal_access_token) } + + let(:throttle_setting_prefix) { 'throttle_authenticated' } + end + end + end + describe 'API requests authenticated with personal access token', :api do let_it_be(:user) { create(:user) } let_it_be(:token) { create(:personal_access_token, user: user) } diff --git a/spec/support/helpers/rack_attack_spec_helpers.rb b/spec/support/helpers/rack_attack_spec_helpers.rb index d50a6382a4066822a7d32a76b3593c7119f570a4..c82a87dc58ec25d2a29234bb22c69515ecab3e62 100644 --- a/spec/support/helpers/rack_attack_spec_helpers.rb +++ b/spec/support/helpers/rack_attack_spec_helpers.rb @@ -26,14 +26,14 @@ def basic_auth_headers(user, personal_access_token) { 'AUTHORIZATION' => "Basic #{encoded_login}" } end - def expect_rejection(&block) + def expect_rejection(name = nil, &block) yield expect(response).to have_gitlab_http_status(:too_many_requests) expect(response.headers.to_h).to include( 'RateLimit-Limit' => a_string_matching(/^\d+$/), - 'RateLimit-Name' => a_string_matching(/^throttle_.*$/), + 'RateLimit-Name' => name || a_string_matching(/^throttle_.*$/), 'RateLimit-Observed' => a_string_matching(/^\d+$/), 'RateLimit-Remaining' => a_string_matching(/^\d+$/), 'Retry-After' => a_string_matching(/^\d+$/) diff --git a/spec/support/helpers/session_helpers.rb b/spec/support/helpers/session_helpers.rb index 236585296e571305fc1e1352e2f3e77b545f0298..394a401afcaab4e8876b572d14b9ef4ef0ac6f08 100644 --- a/spec/support/helpers/session_helpers.rb +++ b/spec/support/helpers/session_helpers.rb @@ -1,6 +1,22 @@ # frozen_string_literal: true module SessionHelpers + # Stub a session in Redis, for use in request specs where we can't mock the session directly. + # This also needs the :clean_gitlab_redis_sessions tag on the spec. + def stub_session(session_hash) + unless RSpec.current_example.metadata[:clean_gitlab_redis_sessions] + raise 'Add :clean_gitlab_redis_sessions to your spec!' + end + + session_id = Rack::Session::SessionId.new(SecureRandom.hex) + + Gitlab::Redis::Sessions.with do |redis| + redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash)) + end + + cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id + end + def expect_single_session_with_authenticated_ttl expect_single_session_with_expiration(Settings.gitlab['session_expire_delay'] * 60) end diff --git a/spec/support/shared_examples/requests/api/graphql/mutations/snippets_shared_examples.rb b/spec/support/shared_examples/requests/api/graphql/mutations/snippets_shared_examples.rb index 8bffd1f71e9d429807421ca39720490d936bfa14..a42a1fda62e3f89af78184b4c4d4bf119d82a4e3 100644 --- a/spec/support/shared_examples/requests/api/graphql/mutations/snippets_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/graphql/mutations/snippets_shared_examples.rb @@ -10,6 +10,8 @@ end RSpec.shared_examples 'snippet edit usage data counters' do + include SessionHelpers + context 'when user is sessionless' do it 'does not track usage data actions' do expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).not_to receive(:track_snippet_editor_edit_action) @@ -20,14 +22,7 @@ context 'when user is not sessionless', :clean_gitlab_redis_sessions do before do - session_id = Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') - session_hash = { 'warden.user.user.key' => [[current_user.id], current_user.encrypted_password[0, 29]] } - - Gitlab::Redis::Sessions.with do |redis| - redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash)) - end - - cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id + stub_session('warden.user.user.key' => [[current_user.id], current_user.encrypted_password[0, 29]]) end it 'tracks usage data actions', :clean_gitlab_redis_sessions do diff --git a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb b/spec/support/shared_examples/requests/rack_attack_shared_examples.rb index b294467d48231176bb232c07ba2f29f2a8327274..c6c6c44dce893cb7454352bad2efb3e593ac3d6f 100644 --- a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb +++ b/spec/support/shared_examples/requests/rack_attack_shared_examples.rb @@ -580,3 +580,88 @@ def do_request end end end + +# Requires let variables: +# * throttle_setting_prefix: "throttle_authenticated", "throttle_unauthenticated" +RSpec.shared_examples 'rate-limited frontend API requests' do + let(:requests_per_period) { 1 } + let(:csrf_token) { SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) } + let(:csrf_session) { { _csrf_token: csrf_token } } + let(:personal_access_token) { nil } + + let(:api_path) { '/projects' } + + # These don't actually exist, so a 404 is the expected response. + let(:files_api_path) { '/projects/1/repository/files/ref/path' } + let(:packages_api_path) { '/projects/1/packages/foo' } + let(:deprecated_api_path) { '/groups/1?with_projects=true' } + + def get_api(path: api_path, csrf: false) + headers = csrf ? { 'X-CSRF-Token' => csrf_token } : nil + get api(path, personal_access_token: personal_access_token), headers: headers + end + + def expect_not_found(&block) + yield + + expect(response).to have_gitlab_http_status(:not_found) + end + + before do + stub_application_setting( + "#{throttle_setting_prefix}_enabled" => true, + "#{throttle_setting_prefix}_requests_per_period" => requests_per_period, + "#{throttle_setting_prefix}_api_enabled" => true, + "#{throttle_setting_prefix}_api_requests_per_period" => requests_per_period, + "#{throttle_setting_prefix}_web_enabled" => true, + "#{throttle_setting_prefix}_web_requests_per_period" => requests_per_period, + "#{throttle_setting_prefix}_files_api_enabled" => true, + "#{throttle_setting_prefix}_packages_api_enabled" => true, + "#{throttle_setting_prefix}_deprecated_api_enabled" => true + ) + + stub_session(csrf_session) + end + + context 'with a CSRF token' do + it 'uses the rate limit for web requests' do + requests_per_period.times { get_api csrf: true } + + expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true } + expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true, path: files_api_path } + expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true, path: packages_api_path } + expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true, path: deprecated_api_path } + + # API rate limit is not triggered yet + expect_ok { get_api } + expect_not_found { get_api path: files_api_path } + expect_not_found { get_api path: packages_api_path } + expect_not_found { get_api path: deprecated_api_path } + end + + context 'without a CSRF session' do + let(:csrf_session) { nil } + + it 'always uses the rate limit for API requests' do + requests_per_period.times { get_api csrf: true } + + expect_rejection("#{throttle_setting_prefix}_api") { get_api csrf: true } + expect_rejection("#{throttle_setting_prefix}_api") { get_api } + end + end + end + + context 'without a CSRF token' do + it 'uses the rate limit for API requests' do + requests_per_period.times { get_api } + + expect_rejection("#{throttle_setting_prefix}_api") { get_api } + + # Web and custom API rate limits are not triggered yet + expect_ok { get_api csrf: true } + expect_not_found { get_api path: files_api_path } + expect_not_found { get_api path: packages_api_path } + expect_not_found { get_api path: deprecated_api_path } + end + end +end