Skip to content
Snippets Groups Projects
Verified Commit 47d7d954 authored by Sean McGivern's avatar Sean McGivern :red_circle:
Browse files

Revert frontend API rate limits change 

This reverts merge request
!78082 because of a
production incident.

Changelog: other
parent dd3c2f30
No related branches found
No related tags found
1 merge request!78893Revert "Merge branch '344807-frontend-api-requests-rate-limit-take2' into 'master'"
Hide whitespace changes
Inline Side-by-side
Showing
with 37 additions and 249 deletions
config/feature_flags/development/rate_limit_frontend_requests.yml deleted 100644 → 0
+ 0
− 8
View file @ dd3c2f30
---
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
doc/user/admin_area/settings/user_and_ip_rate_limits.md
+ 0
− 4
View file @ 47d7d954
......@@ -22,10 +22,6 @@ 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:
......
lib/gitlab/rack_attack/request.rb
+ 10
− 25
View file @ 47d7d954
......@@ -3,8 +3,6 @@
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
......@@ -32,15 +30,15 @@ def api_request?
end
def api_internal_request?
path.match?(%r{^/api/v\d+/internal/})
path =~ %r{^/api/v\d+/internal/}
end
def health_check_request?
path.match?(%r{^/-/(health|liveness|readiness|metrics)})
path =~ %r{^/-/(health|liveness|readiness|metrics)}
end
def container_registry_event?
path.match?(%r{^/api/v\d+/container_registry_event/})
path =~ %r{^/api/v\d+/container_registry_event/}
end
def product_analytics_collector_request?
......@@ -60,7 +58,7 @@ def protected_path?
end
def protected_path_regex
path.match?(protected_paths_regex)
path =~ protected_paths_regex
end
def throttle?(throttle, authenticated:)
......@@ -72,7 +70,6 @@ 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? &&
......@@ -81,7 +78,7 @@ def throttle_unauthenticated_api?
end
def throttle_unauthenticated_web?
(web_request? || frontend_request?) &&
web_request? &&
!should_be_skipped? &&
# TODO: Column will be renamed in https://gitlab.com/gitlab-org/gitlab/-/issues/340031
Gitlab::Throttle.settings.throttle_unauthenticated_enabled &&
......@@ -90,7 +87,6 @@ def throttle_unauthenticated_web?
def throttle_authenticated_api?
api_request? &&
!frontend_request? &&
!throttle_authenticated_packages_api? &&
!throttle_authenticated_files_api? &&
!throttle_authenticated_deprecated_api? &&
......@@ -98,7 +94,7 @@ def throttle_authenticated_api?
end
def throttle_authenticated_web?
(web_request? || frontend_request?) &&
web_request? &&
!throttle_authenticated_git_lfs? &&
Gitlab::Throttle.settings.throttle_authenticated_web_enabled
end
......@@ -182,26 +178,15 @@ def protected_paths_regex
end
def packages_api_path?
path.match?(::Gitlab::Regex::Packages::API_PATH_REGEX)
path =~ ::Gitlab::Regex::Packages::API_PATH_REGEX
end
def git_lfs_path?
path.match?(Gitlab::PathRegex.repository_git_lfs_route_regex)
path =~ Gitlab::PathRegex.repository_git_lfs_route_regex
end
def files_api_path?
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
path =~ FILES_PATH_REGEX
end
def deprecated_api_request?
......@@ -210,7 +195,7 @@ def deprecated_api_request?
with_projects = params['with_projects']
with_projects = true if with_projects.blank?
path.match?(GROUP_PATH_REGEX) && Gitlab::Utils.to_boolean(with_projects)
path =~ GROUP_PATH_REGEX && Gitlab::Utils.to_boolean(with_projects)
end
end
end
......
spec/channels/application_cable/connection_spec.rb
+ 6
− 2
View file @ 47d7d954
......@@ -3,11 +3,15 @@
require 'spec_helper'
RSpec.describe ApplicationCable::Connection, :clean_gitlab_redis_sessions do
include SessionHelpers
let(:session_id) { Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') }
context 'when session cookie is set' do
before do
stub_session(session_hash)
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
context 'when user is logged in' do
......
spec/lib/gitlab/rack_attack/request_spec.rb
+ 3
− 85
View file @ 47d7d954
......@@ -5,19 +5,6 @@
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 }
......@@ -29,80 +16,11 @@
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
subject { request.send(:deprecated_api_request?) }
let(:env) { { 'REQUEST_METHOD' => 'GET', 'rack.input' => StringIO.new, 'PATH_INFO' => path, 'QUERY_STRING' => query } }
let(:request) { ::Rack::Attack::Request.new(env) }
let(:env) { { 'QUERY_STRING' => query } }
subject { !!request.__send__(:deprecated_api_request?) }
where(:path, :query, :expected) do
'/' | '' | false
......
spec/requests/api/commits_spec.rb
+ 8
− 2
View file @ 47d7d954
......@@ -5,7 +5,6 @@
RSpec.describe API::Commits do
include ProjectForksHelper
include SessionHelpers
let(:user) { create(:user) }
let(:guest) { create(:user).tap { |u| project.add_guest(u) } }
......@@ -379,7 +378,14 @@
context 'when using warden' do
it 'increments usage counters', :clean_gitlab_redis_sessions do
stub_session('warden.user.user.key' => [[user.id], user.encrypted_password[0, 29]])
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
expect(::Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_commits_count)
expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).to receive(:track_web_ide_edit_action)
......
spec/requests/rack_attack_global_spec.rb
+ 0
− 17
View file @ 47d7d954
......@@ -4,7 +4,6 @@
RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_caching do
include RackAttackSpecHelpers
include SessionHelpers
let(:settings) { Gitlab::CurrentSettings.current_application_settings }
......@@ -64,22 +63,6 @@
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) }
......
spec/support/helpers/rack_attack_spec_helpers.rb
+ 2
− 2
View file @ 47d7d954
......@@ -26,14 +26,14 @@ def basic_auth_headers(user, personal_access_token)
{ 'AUTHORIZATION' => "Basic #{encoded_login}" }
end
def expect_rejection(name = nil, &block)
def expect_rejection(&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' => name || a_string_matching(/^throttle_.*$/),
'RateLimit-Name' => a_string_matching(/^throttle_.*$/),
'RateLimit-Observed' => a_string_matching(/^\d+$/),
'RateLimit-Remaining' => a_string_matching(/^\d+$/),
'Retry-After' => a_string_matching(/^\d+$/)
......
spec/support/helpers/session_helpers.rb
+ 0
− 16
View file @ 47d7d954
# 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
......
spec/support/shared_examples/requests/api/graphql/mutations/snippets_shared_examples.rb
+ 8
− 3
View file @ 47d7d954
......@@ -10,8 +10,6 @@
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)
......@@ -22,7 +20,14 @@
context 'when user is not sessionless', :clean_gitlab_redis_sessions do
before do
stub_session('warden.user.user.key' => [[current_user.id], current_user.encrypted_password[0, 29]])
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
end
it 'tracks usage data actions', :clean_gitlab_redis_sessions do
......
spec/support/shared_examples/requests/rack_attack_shared_examples.rb
+ 0
− 85
View file @ 47d7d954
......@@ -580,88 +580,3 @@ 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
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment