From 64410a0c505fea30c1bef9f7cd0b81aeeb488959 Mon Sep 17 00:00:00 2001 From: Dan Milne Date: Sun, 19 Jul 2026 13:28:55 +1000 Subject: [PATCH] Cap device-code poll interval and sweep expired device codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more fixes from the device-flow security review: slow_down interval grew without bound (OidcController): - Each too-fast poll bumped the persisted interval by 5s with no ceiling, so a client polling slightly fast — or an attacker spamming a known device_code — could balloon it (5→10→15→…) past the 10-minute expiry and starve a legitimate client of its token. Clamp the bump to OidcDeviceCode::MAX_INTERVAL (30s); a client polling at the advertised interval is never throttled. Expired device codes accumulated forever (OidcTokenCleanupJob): - Anonymous callers can create device codes via /oauth/device_authorization, and expired/denied/abandoned rows were never reaped. Sweep rows past expiry (with a 1-hour grace to stay clear of in-flight redemption); redeemed codes are already destroyed at token issuance. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_016Q4ATZHoMCWqvSpE2yYoie --- app/controllers/oidc_controller.rb | 7 +++-- app/jobs/oidc_token_cleanup_job.rb | 10 ++++++ app/models/oidc_device_code.rb | 8 +++++ .../oidc_device_flow_controller_test.rb | 8 +++++ test/jobs/oidc_token_cleanup_job_test.rb | 31 +++++++++++++++++++ 5 files changed, 62 insertions(+), 2 deletions(-) diff --git a/app/controllers/oidc_controller.rb b/app/controllers/oidc_controller.rb index b837afb..eacd3ca 100644 --- a/app/controllers/oidc_controller.rb +++ b/app/controllers/oidc_controller.rb @@ -611,9 +611,12 @@ class OidcController < ApplicationController if device_code.pending? # Enforce the polling interval; too-frequent polls get slow_down, and the - # client is expected to add 5s to its interval (RFC 8628 §3.5). + # client is expected to add 5s to its interval (RFC 8628 §3.5). The bump is + # capped at MAX_INTERVAL so a persistently fast poller can't grow it without + # bound and starve a legitimate client before the code expires. if device_code.last_polled_at && (Time.current - device_code.last_polled_at) < device_code.interval - device_code.update!(interval: device_code.interval + 5, last_polled_at: Time.current) + bumped_interval = [device_code.interval + OidcDeviceCode::INTERVAL_INCREMENT, OidcDeviceCode::MAX_INTERVAL].min + device_code.update!(interval: bumped_interval, last_polled_at: Time.current) render json: {error: "slow_down"}, status: :bad_request else device_code.update!(last_polled_at: Time.current) diff --git a/app/jobs/oidc_token_cleanup_job.rb b/app/jobs/oidc_token_cleanup_job.rb index 47c72ef..11d00fb 100644 --- a/app/jobs/oidc_token_cleanup_job.rb +++ b/app/jobs/oidc_token_cleanup_job.rb @@ -25,5 +25,15 @@ class OidcTokenCleanupJob < ApplicationJob old_auth_codes = OidcAuthorizationCode.where("created_at < ?", 7.days.ago) deleted_count = old_auth_codes.delete_all Rails.logger.info "OIDC Token Cleanup: Deleted #{deleted_count} old authorization codes" + + # Delete expired device codes (RFC 8628). They have a ~10 minute TTL and no + # audit value; a redeemed code is already destroyed at token issuance. Once + # expired a code can never be redeemed, so this single expiry sweep clears the + # expired, denied, and abandoned rows that would otherwise accumulate forever + # (anonymous callers can create them via /oauth/device_authorization). The + # short grace keeps this clear of any in-flight redemption near expiry. + expired_device_codes = OidcDeviceCode.where("expires_at < ?", 1.hour.ago) + deleted_count = expired_device_codes.delete_all + Rails.logger.info "OIDC Token Cleanup: Deleted #{deleted_count} expired device codes" end end diff --git a/app/models/oidc_device_code.rb b/app/models/oidc_device_code.rb index 1caa7ce..b1f63b4 100644 --- a/app/models/oidc_device_code.rb +++ b/app/models/oidc_device_code.rb @@ -17,6 +17,14 @@ class OidcDeviceCode < ApplicationRecord STATUSES = %w[pending approved denied].freeze + # Polling interval, in seconds. The token endpoint bumps the persisted interval + # by INTERVAL_INCREMENT on each too-fast poll (RFC 8628 §3.5 slow_down), but + # clamps it to MAX_INTERVAL so a client polling slightly fast — or an attacker + # spamming a known device_code — can't balloon it past the expiry window and + # starve a well-behaved client of its token. + INTERVAL_INCREMENT = 5 + MAX_INTERVAL = 30 + attr_accessor :plaintext_device_code before_validation :generate_device_code, on: :create diff --git a/test/controllers/oidc_device_flow_controller_test.rb b/test/controllers/oidc_device_flow_controller_test.rb index ad77be1..cef7090 100644 --- a/test/controllers/oidc_device_flow_controller_test.rb +++ b/test/controllers/oidc_device_flow_controller_test.rb @@ -119,6 +119,14 @@ class OidcDeviceFlowControllerTest < ActionDispatch::IntegrationTest assert_equal "slow_down", JSON.parse(@response.body)["error"] end + test "slow_down interval is capped and does not grow without bound" do + dc = OidcDeviceCode.create!(application: @cli, scope: "openid") + # Hammer the code far more times than it would take to exceed the cap if the + # interval grew by 5 unbounded (20 * 5 = 100s >> MAX_INTERVAL). + 20.times { poll(dc) } + assert_operator dc.reload.interval, :<=, OidcDeviceCode::MAX_INTERVAL + end + test "token endpoint returns expired_token for an expired code" do dc = OidcDeviceCode.create!(application: @cli, scope: "openid", expires_at: 1.minute.ago) poll(dc) diff --git a/test/jobs/oidc_token_cleanup_job_test.rb b/test/jobs/oidc_token_cleanup_job_test.rb index ef25c36..9395b52 100644 --- a/test/jobs/oidc_token_cleanup_job_test.rb +++ b/test/jobs/oidc_token_cleanup_job_test.rb @@ -48,4 +48,35 @@ class OidcTokenCleanupJobTest < ActiveJob::TestCase user&.destroy application&.destroy end + + # Device codes (RFC 8628) are created by anonymous callers and would grow the + # table without bound; the cleanup job must purge expired ones (pending, denied, + # or abandoned) while leaving live codes alone. + test "deletes expired device codes and keeps live ones" do + application = Application.create!( + name: "Device Cleanup App", + slug: "device-cleanup-app", + app_type: "oidc", + is_public_client: true, + active: true + ) + + expired_pending = nil + expired_denied = nil + travel_to(2.hours.ago) do + expired_pending = OidcDeviceCode.create!(application: application, scope: "openid") + expired_denied = OidcDeviceCode.create!(application: application, scope: "openid") + expired_denied.deny! + end + live = OidcDeviceCode.create!(application: application, scope: "openid") + + OidcTokenCleanupJob.new.perform + + assert_not OidcDeviceCode.exists?(expired_pending.id), "expired pending code should be deleted" + assert_not OidcDeviceCode.exists?(expired_denied.id), "expired denied code should be deleted" + assert OidcDeviceCode.exists?(live.id), "live code should be kept" + ensure + OidcDeviceCode.where(application: application).delete_all if application + application&.destroy + end end