Fix: remove broken OAuth Application vacuuming & throttle OAuth Application registrations (#30316)
Co-authored-by: Claire <claire.github-309c@sitedethib.com>
This commit is contained in:
		
							parent
							
								
									6eea83211c
								
							
						
					
					
						commit
						d20a5c3ec9
					
				| @ -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 |  | ||||||
| @ -22,7 +22,6 @@ class Scheduler::VacuumScheduler | |||||||
|       preview_cards_vacuum, |       preview_cards_vacuum, | ||||||
|       backups_vacuum, |       backups_vacuum, | ||||||
|       access_tokens_vacuum, |       access_tokens_vacuum, | ||||||
|       applications_vacuum, |  | ||||||
|       feeds_vacuum, |       feeds_vacuum, | ||||||
|       imports_vacuum, |       imports_vacuum, | ||||||
|     ] |     ] | ||||||
| @ -56,10 +55,6 @@ class Scheduler::VacuumScheduler | |||||||
|     Vacuum::ImportsVacuum.new |     Vacuum::ImportsVacuum.new | ||||||
|   end |   end | ||||||
| 
 | 
 | ||||||
|   def applications_vacuum |  | ||||||
|     Vacuum::ApplicationsVacuum.new |  | ||||||
|   end |  | ||||||
| 
 |  | ||||||
|   def content_retention_policy |   def content_retention_policy | ||||||
|     ContentRetentionPolicy.current |     ContentRetentionPolicy.current | ||||||
|   end |   end | ||||||
|  | |||||||
| @ -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)) |     req.authenticated_user_id if (req.post? && req.path.match?(API_DELETE_REBLOG_REGEX)) || (req.delete? && req.path.match?(API_DELETE_STATUS_REGEX)) | ||||||
|   end |   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| |   throttle('throttle_sign_up_attempts/ip', limit: 25, period: 5.minutes) do |req| | ||||||
|     req.throttleable_remote_ip if req.post? && req.path_matches?('/auth') |     req.throttleable_remote_ip if req.post? && req.path_matches?('/auth') | ||||||
|   end |   end | ||||||
|  | |||||||
| @ -131,4 +131,22 @@ describe Rack::Attack, type: :request do | |||||||
|       it_behaves_like 'throttled endpoint' |       it_behaves_like 'throttled endpoint' | ||||||
|     end |     end | ||||||
|   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 | end | ||||||
|  | |||||||
| @ -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 |  | ||||||
		Loading…
	
	
			
			x
			
			
		
	
		Reference in New Issue
	
	Block a user