diff --git a/app/controllers/concerns/search_rate_limitable.rb b/app/controllers/concerns/search_rate_limitable.rb index a77ebd276b6f0273c3024d75c5d4bb189f7a6fd0..7cce30dbb3ca8e72e7b3b64419a72942f02c509b 100644 --- a/app/controllers/concerns/search_rate_limitable.rb +++ b/app/controllers/concerns/search_rate_limitable.rb @@ -7,9 +7,25 @@ module SearchRateLimitable def check_search_rate_limit! if current_user - check_rate_limit!(:search_rate_limit, scope: [current_user]) + # Because every search in the UI typically runs concurrent searches with different + # scopes to get counts, we apply rate limits on the search scope if it is present. + # + # If abusive search is detected, we have stricter limits and ignore the search scope. + check_rate_limit!(:search_rate_limit, scope: [current_user, safe_search_scope].compact) else check_rate_limit!(:search_rate_limit_unauthenticated, scope: [request.ip]) end end + + def safe_search_scope + # Sometimes search scope can have abusive length or invalid keyword. We don't want + # to send those to redis for rate limit checks, so we guard against that here. + return if Feature.disabled?(:search_rate_limited_scopes) || abuse_detected? + + params[:scope] + end + + def abuse_detected? + Gitlab::Search::Params.new(params, detect_abuse: true).abusive? + end end diff --git a/config/feature_flags/development/search_rate_limited_scopes.yml b/config/feature_flags/development/search_rate_limited_scopes.yml new file mode 100644 index 0000000000000000000000000000000000000000..499a51feca5492312d24a88ee255704f8836a35c --- /dev/null +++ b/config/feature_flags/development/search_rate_limited_scopes.yml @@ -0,0 +1,8 @@ +--- +name: search_rate_limited_scopes +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118525 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/408521 +milestone: '16.0' +type: development +group: group::global search +default_enabled: false diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index f0702288da597bfd482ddcc831c30d7c92b14863..68d8f3f04b8f6310fceea34eadbcfdbe39e220e1 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -157,16 +157,17 @@ Set the limit to `0` to disable it. ### Search rate limit > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80631) in GitLab 14.9. -> - [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104208) to include issue, merge request, and epic searches to the rate limit in GitLab 15.9. +> - [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104208) in GitLab 15.9 to include issue, merge request, and epic searches in the rate limit. +> - [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118525) in GitLab 16.0 to apply rate limits to [search scopes](../user/search/index.md#global-search-scopes) for authenticated requests. This setting limits search requests as follows: | Limit | Default (requests per minute) | |-------------------------|-------------------------------| -| Authenticated user | 30 | -| Unauthenticated user | 10 | +| Authenticated user | 300 | +| Unauthenticated user | 100 | -Depending on the number of enabled [scopes](../user/search/index.md#global-search-scopes), a global search request can consume two to seven requests per minute. You may want to disable one or more scopes to use fewer requests. Search requests that exceed the search rate limit per minute return the following error: +Search requests that exceed the search rate limit per minute return the following error: ```plaintext This endpoint has been requested too many times. Try again later. diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index ff24b754d7a092dd55d09d5891a4e8dcc4e95fec..497e2d84f4fcde5c6be012c1a40f8c73de44d490 100644 --- a/spec/controllers/search_controller_spec.rb +++ b/spec/controllers/search_controller_spec.rb @@ -38,6 +38,41 @@ it_behaves_like 'with external authorization service enabled', :show, { search: 'hello' } it_behaves_like 'support for active record query timeouts', :show, { search: 'hello' }, :search_objects, :html + describe 'rate limit scope' do + it 'uses current_user and search scope' do + %w[projects blobs users issues merge_requests].each do |scope| + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user, scope]) + get :show, params: { search: 'hello', scope: scope } + end + end + + it 'uses just current_user when no search scope is used' do + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user]) + get :show, params: { search: 'hello' } + end + + it 'uses just current_user when search scope is abusive' do + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user]) + get(:show, params: { search: 'hello', scope: 'hack-the-mainframe' }) + + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user]) + get :show, params: { search: 'hello', scope: 'blobs' * 1000 } + end + + context 'when search_rate_limited_scopes feature flag is disabled' do + before do + stub_feature_flags(search_rate_limited_scopes: false) + end + + it 'uses just current_user' do + %w[projects blobs users issues merge_requests].each do |scope| + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user]) + get :show, params: { search: 'hello', scope: scope } + end + end + end + end + context 'uses the right partials depending on scope' do using RSpec::Parameterized::TableSyntax render_views @@ -345,6 +380,36 @@ def request expect(json_response).to eq({ 'count' => '1' }) end + describe 'rate limit scope' do + it 'uses current_user and search scope' do + %w[projects blobs users issues merge_requests].each do |scope| + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user, scope]) + get :count, params: { search: 'hello', scope: scope } + end + end + + it 'uses just current_user when search scope is abusive' do + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user]) + get :count, params: { search: 'hello', scope: 'hack-the-mainframe' } + + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user]) + get :count, params: { search: 'hello', scope: 'blobs' * 1000 } + end + + context 'when search_rate_limited_scopes feature flag is disabled' do + before do + stub_feature_flags(search_rate_limited_scopes: false) + end + + it 'uses just current_user' do + %w[projects blobs users issues merge_requests].each do |scope| + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user]) + get :count, params: { search: 'hello', scope: scope } + end + end + end + end + it 'raises an error if search term is missing' do expect do get :count, params: { scope: 'projects' } @@ -406,6 +471,36 @@ def request expect(json_response).to match_array([]) end + describe 'rate limit scope' do + it 'uses current_user and search scope' do + %w[projects blobs users issues merge_requests].each do |scope| + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user, scope]) + get :autocomplete, params: { term: 'hello', scope: scope } + end + end + + it 'uses just current_user when search scope is abusive' do + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user]) + get :autocomplete, params: { term: 'hello', scope: 'hack-the-mainframe' } + + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user]) + get :autocomplete, params: { term: 'hello', scope: 'blobs' * 1000 } + end + + context 'when search_rate_limited_scopes feature flag is disabled' do + before do + stub_feature_flags(search_rate_limited_scopes: false) + end + + it 'uses just current_user' do + %w[projects blobs users issues merge_requests].each do |scope| + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit, scope: [user]) + get :autocomplete, params: { term: 'hello', scope: scope } + end + end + end + end + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do let(:current_user) { user } @@ -525,6 +620,11 @@ def request get endpoint, params: params.merge(project_id: project.id) end end + + it 'uses request IP as rate limiting scope' do + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:search_rate_limit_unauthenticated, scope: [request.ip]) + get endpoint, params: params.merge(project_id: project.id) + end end end