From 21fd25a269cca742af431f0d13299e139f267346 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 14 Nov 2022 20:26:31 +0100 Subject: [PATCH] Fix rate limiting for paths with formats (#20675) --- Gemfile | 3 +- Gemfile.lock | 1 + config/initializers/rack_attack.rb | 49 +++++++----- config/routes.rb | 6 +- spec/config/initializers/rack_attack_spec.rb | 82 ++++++++++++++++++++ 5 files changed, 117 insertions(+), 24 deletions(-) create mode 100644 spec/config/initializers/rack_attack_spec.rb diff --git a/Gemfile b/Gemfile index 07c300510c..355b7e43f8 100644 --- a/Gemfile +++ b/Gemfile @@ -122,6 +122,7 @@ group :test do gem 'simplecov', '~> 0.21', require: false gem 'webmock', '~> 3.18' gem 'rspec_junit_formatter', '~> 0.6' + gem 'rack-test', '~> 2.0' end group :development do @@ -152,7 +153,5 @@ end gem 'concurrent-ruby', require: false gem 'connection_pool', require: false - gem 'xorcist', '~> 1.1' - gem 'cocoon', '~> 1.2' diff --git a/Gemfile.lock b/Gemfile.lock index 28c6e73481..87d07b6312 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -815,6 +815,7 @@ DEPENDENCIES rack (~> 2.2.4) rack-attack (~> 6.6) rack-cors (~> 1.1) + rack-test (~> 2.0) rails (~> 6.1.7) rails-controller-testing (~> 1.0) rails-i18n (~> 6.0) diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 745eb5d3bf..72ef7ba801 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -17,6 +17,18 @@ class Rack::Attack @remote_ip ||= (@env["action_dispatch.remote_ip"] || ip).to_s end + def throttleable_remote_ip + @throttleable_remote_ip ||= begin + ip = IPAddr.new(remote_ip) + + if ip.ipv6? + ip.mask(64) + else + ip + end + end.to_s + end + def authenticated_user_id authenticated_token&.resource_owner_id end @@ -29,6 +41,10 @@ class Rack::Attack path.start_with?('/api') end + def path_matches?(other_path) + /\A#{Regexp.escape(other_path)}(\..*)?\z/ =~ path + end + def web_request? !api_request? end @@ -51,19 +67,19 @@ class Rack::Attack end throttle('throttle_unauthenticated_api', limit: 300, period: 5.minutes) do |req| - req.remote_ip if req.api_request? && req.unauthenticated? + req.throttleable_remote_ip if req.api_request? && req.unauthenticated? end throttle('throttle_api_media', limit: 30, period: 30.minutes) do |req| - req.authenticated_user_id if req.post? && req.path.match?('^/api/v\d+/media') + req.authenticated_user_id if req.post? && req.path.match?(/\A\/api\/v\d+\/media\z/i) end throttle('throttle_media_proxy', limit: 30, period: 10.minutes) do |req| - req.remote_ip if req.path.start_with?('/media_proxy') + req.throttleable_remote_ip if req.path.start_with?('/media_proxy') end throttle('throttle_api_sign_up', limit: 5, period: 30.minutes) do |req| - req.remote_ip if req.post? && req.path == '/api/v1/accounts' + req.throttleable_remote_ip if req.post? && req.path == '/api/v1/accounts' end throttle('throttle_authenticated_paging', limit: 300, period: 15.minutes) do |req| @@ -71,39 +87,34 @@ class Rack::Attack end throttle('throttle_unauthenticated_paging', limit: 300, period: 15.minutes) do |req| - req.remote_ip if req.paging_request? && req.unauthenticated? + req.throttleable_remote_ip if req.paging_request? && req.unauthenticated? end - API_DELETE_REBLOG_REGEX = /\A\/api\/v1\/statuses\/[\d]+\/unreblog/.freeze - API_DELETE_STATUS_REGEX = /\A\/api\/v1\/statuses\/[\d]+/.freeze + API_DELETE_REBLOG_REGEX = /\A\/api\/v1\/statuses\/[\d]+\/unreblog\z/.freeze + API_DELETE_STATUS_REGEX = /\A\/api\/v1\/statuses\/[\d]+\z/.freeze throttle('throttle_api_delete', limit: 30, period: 30.minutes) do |req| req.authenticated_user_id if (req.post? && req.path.match?(API_DELETE_REBLOG_REGEX)) || (req.delete? && req.path.match?(API_DELETE_STATUS_REGEX)) end throttle('throttle_sign_up_attempts/ip', limit: 25, period: 5.minutes) do |req| - if req.post? && req.path == '/auth' - addr = req.remote_ip - addr = IPAddr.new(addr) if addr.is_a?(String) - addr = addr.mask(64) if addr.ipv6? - addr.to_s - end + req.throttleable_remote_ip if req.post? && req.path_matches?('/auth') end throttle('throttle_password_resets/ip', limit: 25, period: 5.minutes) do |req| - req.remote_ip if req.post? && req.path == '/auth/password' + req.throttleable_remote_ip if req.post? && req.path_matches?('/auth/password') end throttle('throttle_password_resets/email', limit: 5, period: 30.minutes) do |req| - req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/password' + req.params.dig('user', 'email').presence if req.post? && req.path_matches?('/auth/password') end throttle('throttle_email_confirmations/ip', limit: 25, period: 5.minutes) do |req| - req.remote_ip if req.post? && %w(/auth/confirmation /api/v1/emails/confirmations).include?(req.path) + req.throttleable_remote_ip if req.post? && (req.path_matches?('/auth/confirmation') || req.path == '/api/v1/emails/confirmations') end throttle('throttle_email_confirmations/email', limit: 5, period: 30.minutes) do |req| - if req.post? && req.path == '/auth/password' + if req.post? && req.path_matches?('/auth/password') req.params.dig('user', 'email').presence elsif req.post? && req.path == '/api/v1/emails/confirmations' req.authenticated_user_id @@ -111,11 +122,11 @@ class Rack::Attack end throttle('throttle_login_attempts/ip', limit: 25, period: 5.minutes) do |req| - req.remote_ip if req.post? && req.path == '/auth/sign_in' + req.throttleable_remote_ip if req.post? && req.path_matches?('/auth/sign_in') end throttle('throttle_login_attempts/email', limit: 25, period: 1.hour) do |req| - req.session[:attempt_user_id] || req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/sign_in' + req.session[:attempt_user_id] || req.params.dig('user', 'email').presence if req.post? && req.path_matches?('/auth/sign_in') end self.throttled_responder = lambda do |request| diff --git a/config/routes.rb b/config/routes.rb index f095ff6551..98aa5a0330 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -73,7 +73,7 @@ Rails.application.routes.draw do end end - devise_for :users, path: 'auth', controllers: { + devise_for :users, path: 'auth', format: false, controllers: { omniauth_callbacks: 'auth/omniauth_callbacks', sessions: 'auth/sessions', registrations: 'auth/registrations', @@ -216,7 +216,7 @@ Rails.application.routes.draw do resource :relationships, only: [:show, :update] resource :statuses_cleanup, controller: :statuses_cleanup, only: [:show, :update] - get '/media_proxy/:id/(*any)', to: 'media_proxy#show', as: :media_proxy + get '/media_proxy/:id/(*any)', to: 'media_proxy#show', as: :media_proxy, format: false resource :authorize_interaction, only: [:show, :create] resource :share, only: [:show, :create] @@ -404,7 +404,7 @@ Rails.application.routes.draw do get '/admin', to: redirect('/admin/dashboard', status: 302) - namespace :api do + namespace :api, format: false do # OEmbed get '/oembed', to: 'oembed#show', as: :oembed diff --git a/spec/config/initializers/rack_attack_spec.rb b/spec/config/initializers/rack_attack_spec.rb new file mode 100644 index 0000000000..581021cb90 --- /dev/null +++ b/spec/config/initializers/rack_attack_spec.rb @@ -0,0 +1,82 @@ +require 'rails_helper' + +describe Rack::Attack do + include Rack::Test::Methods + + def app + Rails.application + end + + shared_examples 'throttled endpoint' do + context 'when the number of requests is lower than the limit' do + it 'does not change the request status' do + limit.times do + request.call + expect(last_response.status).to_not eq(429) + end + end + end + + context 'when the number of requests is higher than the limit' do + it 'returns http too many requests' do + (limit * 2).times do |i| + request.call + expect(last_response.status).to eq(429) if i > limit + end + end + end + end + + let(:remote_ip) { '1.2.3.5' } + + describe 'throttle excessive sign-up requests by IP address' do + context 'through the website' do + let(:limit) { 25 } + let(:request) { ->() { post path, {}, 'REMOTE_ADDR' => remote_ip } } + + context 'for exact path' do + let(:path) { '/auth' } + it_behaves_like 'throttled endpoint' + end + + context 'for path with format' do + let(:path) { '/auth.html' } + it_behaves_like 'throttled endpoint' + end + end + + context 'through the API' do + let(:limit) { 5 } + let(:request) { ->() { post path, {}, 'REMOTE_ADDR' => remote_ip } } + + context 'for exact path' do + let(:path) { '/api/v1/accounts' } + it_behaves_like 'throttled endpoint' + end + + context 'for path with format' do + let(:path) { '/api/v1/accounts.json' } + + it 'returns http not found' do + request.call + expect(last_response.status).to eq(404) + end + end + end + end + + describe 'throttle excessive sign-in requests by IP address' do + let(:limit) { 25 } + let(:request) { ->() { post path, {}, 'REMOTE_ADDR' => remote_ip } } + + context 'for exact path' do + let(:path) { '/auth/sign_in' } + it_behaves_like 'throttled endpoint' + end + + context 'for path with format' do + let(:path) { '/auth/sign_in.html' } + it_behaves_like 'throttled endpoint' + end + end +end