Cap device-code poll interval and sweep expired device codes
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Q4ATZHoMCWqvSpE2yYoie
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
017dfdff0e
commit
64410a0c50
@@ -611,9 +611,12 @@ class OidcController < ApplicationController
|
|||||||
|
|
||||||
if device_code.pending?
|
if device_code.pending?
|
||||||
# Enforce the polling interval; too-frequent polls get slow_down, and the
|
# 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
|
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
|
render json: {error: "slow_down"}, status: :bad_request
|
||||||
else
|
else
|
||||||
device_code.update!(last_polled_at: Time.current)
|
device_code.update!(last_polled_at: Time.current)
|
||||||
|
|||||||
@@ -25,5 +25,15 @@ class OidcTokenCleanupJob < ApplicationJob
|
|||||||
old_auth_codes = OidcAuthorizationCode.where("created_at < ?", 7.days.ago)
|
old_auth_codes = OidcAuthorizationCode.where("created_at < ?", 7.days.ago)
|
||||||
deleted_count = old_auth_codes.delete_all
|
deleted_count = old_auth_codes.delete_all
|
||||||
Rails.logger.info "OIDC Token Cleanup: Deleted #{deleted_count} old authorization codes"
|
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
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -17,6 +17,14 @@ class OidcDeviceCode < ApplicationRecord
|
|||||||
|
|
||||||
STATUSES = %w[pending approved denied].freeze
|
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
|
attr_accessor :plaintext_device_code
|
||||||
|
|
||||||
before_validation :generate_device_code, on: :create
|
before_validation :generate_device_code, on: :create
|
||||||
|
|||||||
@@ -119,6 +119,14 @@ class OidcDeviceFlowControllerTest < ActionDispatch::IntegrationTest
|
|||||||
assert_equal "slow_down", JSON.parse(@response.body)["error"]
|
assert_equal "slow_down", JSON.parse(@response.body)["error"]
|
||||||
end
|
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
|
test "token endpoint returns expired_token for an expired code" do
|
||||||
dc = OidcDeviceCode.create!(application: @cli, scope: "openid", expires_at: 1.minute.ago)
|
dc = OidcDeviceCode.create!(application: @cli, scope: "openid", expires_at: 1.minute.ago)
|
||||||
poll(dc)
|
poll(dc)
|
||||||
|
|||||||
@@ -48,4 +48,35 @@ class OidcTokenCleanupJobTest < ActiveJob::TestCase
|
|||||||
user&.destroy
|
user&.destroy
|
||||||
application&.destroy
|
application&.destroy
|
||||||
end
|
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
|
end
|
||||||
|
|||||||
Reference in New Issue
Block a user