From e89e0c79f03c41c4b703fefe637b9a194acf9875 Mon Sep 17 00:00:00 2001 From: John Mason Date: Mon, 24 Apr 2023 11:58:33 -0400 Subject: [PATCH 1/6] Apply search rate limits to search scope --- .../concerns/search_rate_limitable.rb | 15 ++- .../search_rate_limited_scopes.yml | 8 ++ spec/controllers/search_controller_spec.rb | 100 ++++++++++++++++++ 3 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 config/feature_flags/development/search_rate_limited_scopes.yml diff --git a/app/controllers/concerns/search_rate_limitable.rb b/app/controllers/concerns/search_rate_limitable.rb index a77ebd276b6f02..2b5b01619ace5c 100644 --- a/app/controllers/concerns/search_rate_limitable.rb +++ b/app/controllers/concerns/search_rate_limitable.rb @@ -7,9 +7,22 @@ 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. + 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 + # Use stricter rate limits if abuse is detected + 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 00000000000000..2bfbf3216d6700 --- /dev/null +++ b/config/feature_flags/development/search_rate_limited_scopes.yml @@ -0,0 +1,8 @@ +--- +name: search_rate_limited_scopes +introduced_by_url: +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/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index ff24b754d7a092..497e2d84f4fcde 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 -- GitLab From bdd7492b4544b2c02a69058993b6d46e1eebae79 Mon Sep 17 00:00:00 2001 From: John Mason Date: Mon, 24 Apr 2023 12:15:02 -0400 Subject: [PATCH 2/6] Update documentation --- app/controllers/concerns/search_rate_limitable.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/controllers/concerns/search_rate_limitable.rb b/app/controllers/concerns/search_rate_limitable.rb index 2b5b01619ace5c..7cce30dbb3ca8e 100644 --- a/app/controllers/concerns/search_rate_limitable.rb +++ b/app/controllers/concerns/search_rate_limitable.rb @@ -9,6 +9,8 @@ def check_search_rate_limit! if 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]) @@ -16,7 +18,8 @@ def check_search_rate_limit! end def safe_search_scope - # Use stricter rate limits if abuse is detected + # 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] -- GitLab From 0b939a02e653d84772a6c5f6d19883e6c536179c Mon Sep 17 00:00:00 2001 From: John Mason Date: Mon, 24 Apr 2023 12:15:24 -0400 Subject: [PATCH 3/6] Add MR to feature flag --- config/feature_flags/development/search_rate_limited_scopes.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/feature_flags/development/search_rate_limited_scopes.yml b/config/feature_flags/development/search_rate_limited_scopes.yml index 2bfbf3216d6700..499a51feca5492 100644 --- a/config/feature_flags/development/search_rate_limited_scopes.yml +++ b/config/feature_flags/development/search_rate_limited_scopes.yml @@ -1,6 +1,6 @@ --- name: search_rate_limited_scopes -introduced_by_url: +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 -- GitLab From 6234b4c5cb7369b007bb3ca301b22334c8634afd Mon Sep 17 00:00:00 2001 From: John Mason Date: Tue, 25 Apr 2023 15:26:11 -0400 Subject: [PATCH 4/6] Update documentation for search rate limiting --- doc/administration/instance_limits.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index f0702288da597b..f08f000b5ff20f 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -158,6 +158,7 @@ Set the limit to `0` to disable it. > - [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/118525) to apply limits to search [scopes](../user/search/index.md#global-search-scopes) for authenticated requests in GitLab 16. This setting limits search requests as follows: @@ -166,7 +167,7 @@ This setting limits search requests as follows: | Authenticated user | 30 | | Unauthenticated user | 10 | -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. -- GitLab From cf5a5b68576bf07fac4670e5877f906cd919e4cf Mon Sep 17 00:00:00 2001 From: Ashraf Khamis Date: Wed, 26 Apr 2023 23:43:54 +0000 Subject: [PATCH 5/6] Apply 2 suggestion(s) to 1 file(s) --- doc/administration/instance_limits.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index f08f000b5ff20f..543933be2ec125 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -157,8 +157,8 @@ 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/118525) to apply limits to search [scopes](../user/search/index.md#global-search-scopes) for authenticated requests in GitLab 16. +> - [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: -- GitLab From 1b3caf518a20a0622b33714c22f7a178f67fb929 Mon Sep 17 00:00:00 2001 From: Terri Chu Date: Mon, 1 May 2023 15:52:36 +0000 Subject: [PATCH 6/6] Apply 1 suggestion(s) to 1 file(s) --- doc/administration/instance_limits.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index 543933be2ec125..68d8f3f04b8f63 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -164,8 +164,8 @@ This setting limits search requests as follows: | Limit | Default (requests per minute) | |-------------------------|-------------------------------| -| Authenticated user | 30 | -| Unauthenticated user | 10 | +| Authenticated user | 300 | +| Unauthenticated user | 100 | Search requests that exceed the search rate limit per minute return the following error: -- GitLab