From d20a5c3ec9ed40a991245fe32d0acb6187dd48c4 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Wed, 29 May 2024 16:00:05 +0200 Subject: [PATCH] Fix: remove broken OAuth Application vacuuming & throttle OAuth Application registrations (#30316) Co-authored-by: Claire --- app/lib/vacuum/applications_vacuum.rb | 10 ---- app/workers/scheduler/vacuum_scheduler.rb | 5 -- config/initializers/rack_attack.rb | 4 ++ spec/config/initializers/rack/attack_spec.rb | 18 ++++++++ spec/lib/vacuum/applications_vacuum_spec.rb | 48 -------------------- 5 files changed, 22 insertions(+), 63 deletions(-) delete mode 100644 app/lib/vacuum/applications_vacuum.rb delete mode 100644 spec/lib/vacuum/applications_vacuum_spec.rb diff --git a/app/lib/vacuum/applications_vacuum.rb b/app/lib/vacuum/applications_vacuum.rb deleted file mode 100644 index ba88655f16..0000000000 --- a/app/lib/vacuum/applications_vacuum.rb +++ /dev/null @@ -1,10 +0,0 @@ -# frozen_string_literal: true - -class Vacuum::ApplicationsVacuum - def perform - Doorkeeper::Application.where(owner_id: nil) - .where.missing(:created_users, :access_tokens, :access_grants) - .where(created_at: ...1.day.ago) - .in_batches.delete_all - end -end diff --git a/app/workers/scheduler/vacuum_scheduler.rb b/app/workers/scheduler/vacuum_scheduler.rb index 1c9a2aabe3..c22d6f5f80 100644 --- a/app/workers/scheduler/vacuum_scheduler.rb +++ b/app/workers/scheduler/vacuum_scheduler.rb @@ -22,7 +22,6 @@ class Scheduler::VacuumScheduler preview_cards_vacuum, backups_vacuum, access_tokens_vacuum, - applications_vacuum, feeds_vacuum, imports_vacuum, ] @@ -56,10 +55,6 @@ class Scheduler::VacuumScheduler Vacuum::ImportsVacuum.new end - def applications_vacuum - Vacuum::ApplicationsVacuum.new - end - def content_retention_policy ContentRetentionPolicy.current end diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index fa1bdca544..1757ce5df1 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -105,6 +105,10 @@ class Rack::Attack 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_oauth_application_registrations/ip', limit: 5, period: 10.minutes) do |req| + req.throttleable_remote_ip if req.post? && req.path == '/api/v1/apps' + end + throttle('throttle_sign_up_attempts/ip', limit: 25, period: 5.minutes) do |req| req.throttleable_remote_ip if req.post? && req.path_matches?('/auth') end diff --git a/spec/config/initializers/rack/attack_spec.rb b/spec/config/initializers/rack/attack_spec.rb index e25b7dfde9..0a388c2f41 100644 --- a/spec/config/initializers/rack/attack_spec.rb +++ b/spec/config/initializers/rack/attack_spec.rb @@ -131,4 +131,22 @@ describe Rack::Attack, type: :request do it_behaves_like 'throttled endpoint' end end + + describe 'throttle excessive oauth application registration requests by IP address' do + let(:throttle) { 'throttle_oauth_application_registrations/ip' } + let(:limit) { 5 } + let(:period) { 10.minutes } + let(:path) { '/api/v1/apps' } + let(:params) do + { + client_name: 'Throttle Test', + redirect_uris: 'urn:ietf:wg:oauth:2.0:oob', + scopes: 'read', + } + end + + let(:request) { -> { post path, params: params, headers: { 'REMOTE_ADDR' => remote_ip } } } + + it_behaves_like 'throttled endpoint' + end end diff --git a/spec/lib/vacuum/applications_vacuum_spec.rb b/spec/lib/vacuum/applications_vacuum_spec.rb deleted file mode 100644 index df5c860602..0000000000 --- a/spec/lib/vacuum/applications_vacuum_spec.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.describe Vacuum::ApplicationsVacuum do - subject { described_class.new } - - describe '#perform' do - let!(:app_with_token) { Fabricate(:application, created_at: 1.month.ago) } - let!(:app_with_grant) { Fabricate(:application, created_at: 1.month.ago) } - let!(:app_with_signup) { Fabricate(:application, created_at: 1.month.ago) } - let!(:app_with_owner) { Fabricate(:application, created_at: 1.month.ago, owner: Fabricate(:user)) } - let!(:unused_app) { Fabricate(:application, created_at: 1.month.ago) } - let!(:recent_app) { Fabricate(:application, created_at: 1.hour.ago) } - - before do - Fabricate(:access_token, application: app_with_token) - Fabricate(:access_grant, application: app_with_grant) - Fabricate(:user, created_by_application: app_with_signup) - - subject.perform - end - - it 'does not delete applications with valid access tokens' do - expect { app_with_token.reload }.to_not raise_error - end - - it 'does not delete applications with valid access grants' do - expect { app_with_grant.reload }.to_not raise_error - end - - it 'does not delete applications that were used to create users' do - expect { app_with_signup.reload }.to_not raise_error - end - - it 'does not delete owned applications' do - expect { app_with_owner.reload }.to_not raise_error - end - - it 'does not delete applications registered less than a day ago' do - expect { recent_app.reload }.to_not raise_error - end - - it 'deletes unused applications' do - expect { unused_app.reload }.to raise_error ActiveRecord::RecordNotFound - end - end -end