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:
Emelia Smith 2024-05-29 16:00:05 +02:00 committed by Claire
parent f9c41ae43b
commit 186f916192
5 changed files with 22 additions and 63 deletions

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -103,4 +103,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

View file

@ -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) }
let!(:active_access_token) { Fabricate(:access_token, application: app_with_token) }
let!(:active_access_grant) { Fabricate(:access_grant, application: app_with_grant) }
let!(:user) { Fabricate(:user, created_by_application: app_with_signup) }
before do
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