From 6c87529a730c60e9f125934ad90f493d1922b448 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 28 Oct 2026 21:41:43 -0500 Subject: [PATCH] Sweep dynamic registrations nobody authorized after a day, not thirty Registration is unauthenticated and limited only per address, so its rows were bounded by ten a minute per address over thirty days. A registration whose row nothing has touched since it was written, no consent approved and no grant issued or ended, now lapses after 24 hours, and the sweep runs hourly so that holds. Anything that was ever authorized, metadata document clients and operator-provisioned clients keep the rules they had. --- app/models/oauth/client.rb | 25 +++++++++++++--- config/recurring.yml | 2 +- test/integration/oauth_flow_test.rb | 11 +++++++ test/models/oauth/client_test.rb | 46 ++++++++++++++++++++++++++++- 4 files changed, 78 insertions(+), 6 deletions(-) diff --git a/app/models/oauth/client.rb b/app/models/oauth/client.rb index 5c766d0deb..d0b1f2e99e 100644 --- a/app/models/oauth/client.rb +++ b/app/models/oauth/client.rb @@ -9,6 +9,13 @@ class Oauth::Client < ApplicationRecord # this long is abandoned (RFC 7591 ยง5). UNUSED_RETENTION = 30.days + # Registration is unauthenticated and limited only per address, so a + # registration nobody ever authorized is kept just long enough for its user + # to finish consent. Its rows are then bounded by the registration rate over + # this period rather than over UNUSED_RETENTION. A user still on the consent + # screen when it lapses gets "Unknown client" and their app registers again. + UNAUTHORIZED_REGISTRATION_RETENTION = 24.hours + # The grants a client may obtain at the authorization and token endpoints. # An allowlist, so a client is refused any grant type it doesn't list. GRANT_TYPES = %w[ authorization_code refresh_token ] @@ -76,10 +83,20 @@ class Oauth::Client < ApplicationRecord # metadata document. Either comes back on its own when it's used again. scope :self_registered, -> { dynamically_registered.or(metadata_documents) } - # A grant touches its client when it is created or destroyed, so updated_at - # is the last grant activity: an app the user disconnected yesterday is not - # swept for having registered long ago. - scope :stale, -> { self_registered.where.missing(:access_tokens).where(updated_at: ...UNUSED_RETENTION.ago) } + # A grant touches its client when it is created or destroyed, and so does + # consent, so updated_at is the last grant activity: an app the user + # disconnected yesterday is not swept for having registered long ago. + scope :stale, -> { self_registered.where.missing(:access_tokens).merge(unused_beyond_retention) } + + scope :unused_beyond_retention, -> do + where(updated_at: ...UNUSED_RETENTION.ago).or(never_authorized.where(created_at: ...UNAUTHORIZED_REGISTRATION_RETENTION.ago)) + end + + # A registration whose row nothing has touched since it was written: no + # consent approved, no grant issued or ended. Only dynamic registration + # takes the short retention; a metadata document client keeps + # UNUSED_RETENTION. A grant it still holds keeps it regardless (stale). + scope :never_authorized, -> { dynamically_registered.where(arel_table[:updated_at].eq(arel_table[:created_at])) } scope :personal_access, -> { where client_id: PERSONAL_ACCESS_CLIENT_ID } diff --git a/config/recurring.yml b/config/recurring.yml index a4a97edd40..962d6764f4 100644 --- a/config/recurring.yml +++ b/config/recurring.yml @@ -50,7 +50,7 @@ production: &production schedule: every hour at minute 25 cleanup_oauth_clients: command: "Oauth::Client.cleanup" - schedule: every day at 04:12 + schedule: every hour at minute 14 incineration: class: "Account::IncinerateDueJob" schedule: every 8 hours at minute 16 diff --git a/test/integration/oauth_flow_test.rb b/test/integration/oauth_flow_test.rb index ab4fbeab50..e6e54801e6 100644 --- a/test/integration/oauth_flow_test.rb +++ b/test/integration/oauth_flow_test.rb @@ -187,6 +187,17 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_not_includes Oauth::Client.stale, client end + test "consent takes a dynamic registration off the one-day retention for registrations nobody authorized" do + sign_in_as :david + client = travel_to(25.hours.ago) { Oauth::Client.create!(name: "Fresh", redirect_uris: %w[ http://127.0.0.1:8888/callback ], dynamically_registered: true) } + assert_includes Oauth::Client.stale, client + + post_consent client: client + + assert_response :redirect + assert_not_includes Oauth::Client.stale, client + end + test "consent issues no code for a client swept while consent was submitted" do sign_in_as :david client = Oauth::Client.create!(name: "Swept", redirect_uris: %w[ http://127.0.0.1:8888/callback ], dynamically_registered: true) diff --git a/test/models/oauth/client_test.rb b/test/models/oauth/client_test.rb index f10d6b7c20..5b882c0896 100644 --- a/test/models/oauth/client_test.rb +++ b/test/models/oauth/client_test.rb @@ -403,7 +403,8 @@ class Oauth::ClientTest < ActiveSupport::TestCase end test "cleanup keeps recent, granted and operator-provisioned clients" do - recent = travel_to(29.days.ago) { register_client } + recent = travel_to(30.days.ago) { register_client } + travel_to(29.days.ago) { recent.touch } granted = travel_to(31.days.ago) { register_client.tap { identities(:david).access_tokens.create!(oauth_client: it) } } provisioned = travel_to(31.days.ago) { register_client(dynamically_registered: false, redirect_uris: %w[ https://app.example.com/cb ]) } @@ -414,6 +415,49 @@ class Oauth::ClientTest < ActiveSupport::TestCase assert Oauth::Client.exists?(provisioned.id) end + test "cleanup destroys a dynamic registration nobody authorized within a day" do + lapsed = travel_to(25.hours.ago) { register_client } + pending = travel_to(23.hours.ago) { register_client } + + Oauth::Client.cleanup + + assert_not Oauth::Client.exists?(lapsed.id) + assert Oauth::Client.exists?(pending.id) + end + + test "cleanup keeps a dynamic registration past a day once it has been authorized" do + granted = travel_to(2.days.ago) { register_client } + travel_to(47.hours.ago) { identities(:david).access_tokens.create!(oauth_client: granted) } + + disconnected = travel_to(2.days.ago) { register_client } + travel_to(47.hours.ago) { identities(:david).access_tokens.create!(oauth_client: disconnected) } + travel_to(46.hours.ago) { disconnected.access_tokens.each(&:destroy) } + + consented = travel_to(2.days.ago) { register_client } + travel_to(47.hours.ago) { consented.touch } + + Oauth::Client.cleanup + + assert Oauth::Client.exists?(granted.id) + assert Oauth::Client.exists?(disconnected.id), "a client that ever held a grant keeps the full retention period" + assert Oauth::Client.exists?(consented.id), "a client whose code is out keeps the full retention period" + end + + test "cleanup keeps unauthorized clients that weren't dynamically registered past a day" do + trusted = travel_to(2.days.ago) { register_client(dynamically_registered: false, trusted: true, redirect_uris: %w[ https://app.example.com/cb ]) } + provisioned = travel_to(2.days.ago) { register_client(dynamically_registered: false, redirect_uris: %w[ https://app.example.com/cb ]) } + metadata_document = travel_to(2.days.ago) { register_metadata_document_client } + oauth_clients(:trusted_client, :personal_access).each { it.update_columns created_at: 2.days.ago, updated_at: 2.days.ago } + + Oauth::Client.cleanup + + assert Oauth::Client.exists?(trusted.id) + assert Oauth::Client.exists?(provisioned.id) + assert Oauth::Client.exists?(metadata_document.id) + assert Oauth::Client.exists?(oauth_clients(:trusted_client).id) + assert Oauth::Client.exists?(oauth_clients(:personal_access).id) + end + test "cleanup counts the retention period from the client's last grant, not its registration" do client = travel_to(60.days.ago) { register_client } token = travel_to(40.days.ago) { identities(:david).access_tokens.create!(oauth_client: client) }