diff --git a/app/controllers/device_authorizations_controller.rb b/app/controllers/device_authorizations_controller.rb index ce93428..3d84a61 100644 --- a/app/controllers/device_authorizations_controller.rb +++ b/app/controllers/device_authorizations_controller.rb @@ -9,6 +9,14 @@ class DeviceAuthorizationsController < ApplicationController # Browser form endpoint — keep CSRF protection on (do NOT skip it). + # RFC 8628 §5.1: rate-limit user_code entry so a signed-in user cannot brute + # force the short code space to deny or hijack another user's pending + # authorization during its ~10 minute window. Covers both the lookup (show) and + # the state-changing submit (verify). + rate_limit to: 10, within: 1.minute, only: [:show, :verify], with: -> { + render plain: "Too many attempts. Try again later.", status: :too_many_requests + } + # GET /device?user_code=WDJB-MJHT def show @user_code = params[:user_code].to_s diff --git a/app/controllers/oidc_controller.rb b/app/controllers/oidc_controller.rb index eacd3ca..f7c07f9 100644 --- a/app/controllers/oidc_controller.rb +++ b/app/controllers/oidc_controller.rb @@ -15,11 +15,26 @@ class OidcController < ApplicationController before_action :set_application, only: :authorize before_action :validate_redirect_uri, only: :authorize - # Rate limiting to prevent brute force and abuse - rate_limit to: 60, within: 1.minute, only: [:token, :revoke, :introspect, :device_authorization], with: -> { + # Rate limiting to prevent brute force and abuse. + # + # Each rate_limit MUST pass a distinct name: — without it actionpack derives the + # same cache key for every call on the controller, collapsing these into one + # shared counter. That let high-frequency RFC 8628 device polling (on the token + # endpoint) exhaust the budget and 429 unrelated token/refresh/revoke/introspect + # requests, including the poll that completes a just-approved login. + # + # The token endpoint also carries device-code polling, which is legitimately + # frequent and can come from several devices behind one NAT, so its bucket has + # generous headroom. Per-device poll abuse is separately bounded by the slow_down + # interval, and none of these endpoints exposes a brute-forceable secret (tokens, + # codes, and client secrets are opaque high-entropy values), so the limit is a + # DoS guard rather than a credential guard. + rate_limit to: 120, within: 1.minute, name: "oidc_token", only: [:token, :revoke, :introspect, :device_authorization], with: -> { render json: {error: "too_many_requests", error_description: "Rate limit exceeded. Try again later."}, status: :too_many_requests } - rate_limit to: 30, within: 1.minute, only: [:authorize, :consent], with: -> { + # Browser-facing authorization flow — kept on its own counter so token-endpoint + # traffic can never consume the interactive login budget (or vice versa). + rate_limit to: 30, within: 1.minute, name: "oidc_authorize", only: [:authorize, :consent], with: -> { render plain: "Too many authorization attempts. Try again later.", status: :too_many_requests } @@ -599,6 +614,19 @@ class OidcController < ApplicationController # Lock so concurrent polls / a poll racing with approval can't double-issue. device_code.lock! + # Replay: an already-redeemed code must never mint tokens again. Mirror the + # authorization-code reuse semantics (RFC 6749 §4.1.2) — revoke every token + # descended from it and report the reuse distinguishably, rather than the + # generic "Invalid device_code" returned for an unknown code. + if device_code.redeemed? + Rails.logger.warn "OIDC Security: Device code reuse detected for code #{device_code.id}" + now = Time.current + device_code.oidc_access_tokens.where(revoked_at: nil).update_all(revoked_at: now) + device_code.oidc_refresh_tokens.where(revoked_at: nil).update_all(revoked_at: now) + render json: {error: "invalid_grant", error_description: "Device code has already been used"}, status: :bad_request + return + end + if device_code.expired? render json: {error: "expired_token", error_description: "The device_code has expired"}, status: :bad_request return @@ -667,6 +695,7 @@ class OidcController < ApplicationController application: application, user: user, scope: granted_scope, + oidc_device_code: device_code, resource: device_code.resource ) @@ -674,6 +703,7 @@ class OidcController < ApplicationController application: application, user: user, oidc_access_token: access_token_record, + oidc_device_code: device_code, scope: granted_scope, auth_time: device_code.auth_time, acr: device_code.acr, @@ -692,8 +722,10 @@ class OidcController < ApplicationController claims_requests: {} ) - # Single-use: destroy the code so an approved device_code can't be replayed. - device_code.destroy! + # Single-use: mark the code redeemed (don't destroy it) so a replay is + # detected as reuse — see the redeemed? check at the top of this block. The + # cleanup job reaps redeemed codes after they expire. + device_code.update!(redeemed_at: Time.current) response.headers["Cache-Control"] = "no-store" response.headers["Pragma"] = "no-cache" @@ -963,15 +995,18 @@ class OidcController < ApplicationController refresh_token_record.revoke! # Generate new access token record (opaque token with BCrypt hashing) - # Carry the authorization-code FK forward across rotations so replay - # revocation reaches every descendant token in the chain. + # Carry the issuing-code FK forward across rotations so replay revocation + # reaches every descendant token in the chain — for both the authorization-code + # and device-code grants (a token descends from exactly one of them). issuing_auth_code = refresh_token_record.oidc_authorization_code + issuing_device_code = refresh_token_record.oidc_device_code new_access_token = OidcAccessToken.create!( application: application, user: user, scope: refresh_token_record.scope, oidc_authorization_code: issuing_auth_code, + oidc_device_code: issuing_device_code, resource: refresh_token_record.resource ) @@ -981,6 +1016,7 @@ class OidcController < ApplicationController user: user, oidc_access_token: new_access_token, oidc_authorization_code: issuing_auth_code, + oidc_device_code: issuing_device_code, scope: refresh_token_record.scope, token_family_id: refresh_token_record.token_family_id, # Keep same family for rotation tracking auth_time: refresh_token_record.auth_time, # Carry over original auth_time diff --git a/app/models/oidc_access_token.rb b/app/models/oidc_access_token.rb index b4b796e..7394aef 100644 --- a/app/models/oidc_access_token.rb +++ b/app/models/oidc_access_token.rb @@ -2,6 +2,7 @@ class OidcAccessToken < ApplicationRecord belongs_to :application belongs_to :user belongs_to :oidc_authorization_code, optional: true + belongs_to :oidc_device_code, optional: true has_many :oidc_refresh_tokens, dependent: :destroy before_validation :generate_token, on: :create diff --git a/app/models/oidc_device_code.rb b/app/models/oidc_device_code.rb index b1f63b4..9d9cdd3 100644 --- a/app/models/oidc_device_code.rb +++ b/app/models/oidc_device_code.rb @@ -9,6 +9,11 @@ class OidcDeviceCode < ApplicationRecord belongs_to :application belongs_to :user, optional: true # nil until the request is approved + # Tokens minted from this code, so a replayed (already-redeemed) code can revoke + # every token descended from it — mirrors OidcAuthorizationCode. + has_many :oidc_access_tokens, dependent: :nullify + has_many :oidc_refresh_tokens, dependent: :nullify + # Alphabet for the user_code: uppercase letters + digits, minus visually # ambiguous characters (0/O, 1/I, etc.) so it is easy to read and type. USER_CODE_ALPHABET = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789".chars.freeze @@ -83,6 +88,12 @@ class OidcDeviceCode < ApplicationRecord code_challenge.present? end + # True once the approved code has been exchanged for tokens. The row is kept + # (not destroyed) so a replay is detectable and its tokens can be revoked. + def redeemed? + redeemed_at.present? + end + # Grant the request: attach the approving user and capture their auth context. def approve!(user:, acr:, auth_time:) update!(status: "approved", user: user, acr: acr, auth_time: auth_time) @@ -100,8 +111,10 @@ class OidcDeviceCode < ApplicationRecord end def generate_user_code + # The user_code is a security credential (typing it + Approve grants tokens), + # so draw from a CSPRNG rather than Ruby's global Mersenne Twister PRNG. self.user_code ||= USER_CODE_GROUPS.times.map do - USER_CODE_GROUP_SIZE.times.map { USER_CODE_ALPHABET.sample }.join + USER_CODE_GROUP_SIZE.times.map { USER_CODE_ALPHABET.sample(random: SecureRandom) }.join end.join end diff --git a/app/models/oidc_refresh_token.rb b/app/models/oidc_refresh_token.rb index c881980..d1806ca 100644 --- a/app/models/oidc_refresh_token.rb +++ b/app/models/oidc_refresh_token.rb @@ -3,6 +3,7 @@ class OidcRefreshToken < ApplicationRecord belongs_to :user belongs_to :oidc_access_token belongs_to :oidc_authorization_code, optional: true + belongs_to :oidc_device_code, optional: true before_validation :generate_token, on: :create before_validation :set_expiry, on: :create diff --git a/db/migrate/20260719000005_add_device_code_replay_tracking.rb b/db/migrate/20260719000005_add_device_code_replay_tracking.rb new file mode 100644 index 0000000..fe0a194 --- /dev/null +++ b/db/migrate/20260719000005_add_device_code_replay_tracking.rb @@ -0,0 +1,20 @@ +class AddDeviceCodeReplayTracking < ActiveRecord::Migration[8.1] + # Replay-revocation for the device grant, mirroring the authorization-code path. + # + # The device flow previously destroy!ed the code on redemption, so a replayed + # redeemed code was indistinguishable from an unknown one and the tokens it + # minted could not be revoked. Instead we now mark the code redeemed_at and link + # the issued tokens back to it, so a second redemption is detected as reuse and + # every descended token is revoked (RFC 6749 §4.1.2 reuse semantics). + # + # on_delete: :nullify matches the oidc_authorization_code FK: the cleanup job can + # still delete expired device codes without orphaning or destroying live tokens. + def change + add_column :oidc_device_codes, :redeemed_at, :datetime + + add_reference :oidc_access_tokens, :oidc_device_code, null: true, + foreign_key: {on_delete: :nullify} + add_reference :oidc_refresh_tokens, :oidc_device_code, null: true, + foreign_key: {on_delete: :nullify} + end +end diff --git a/db/schema.rb b/db/schema.rb index e11c52f..245a0dc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_07_19_000004) do +ActiveRecord::Schema[8.1].define(version: 2026_07_19_000005) do create_table "active_storage_attachments", force: :cascade do |t| t.bigint "blob_id", null: false t.datetime "created_at", null: false @@ -124,6 +124,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_07_19_000004) do t.datetime "created_at", null: false t.datetime "expires_at", null: false t.integer "oidc_authorization_code_id" + t.integer "oidc_device_code_id" t.string "resource" t.datetime "revoked_at" t.string "scope" @@ -134,6 +135,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_07_19_000004) do t.index ["application_id"], name: "index_oidc_access_tokens_on_application_id" t.index ["expires_at"], name: "index_oidc_access_tokens_on_expires_at" t.index ["oidc_authorization_code_id"], name: "index_oidc_access_tokens_on_oidc_authorization_code_id" + t.index ["oidc_device_code_id"], name: "index_oidc_access_tokens_on_oidc_device_code_id" t.index ["revoked_at"], name: "index_oidc_access_tokens_on_revoked_at" t.index ["token_hmac"], name: "index_oidc_access_tokens_on_token_hmac", unique: true t.index ["user_id"], name: "index_oidc_access_tokens_on_user_id" @@ -176,6 +178,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_07_19_000004) do t.integer "interval", default: 5, null: false t.datetime "last_polled_at" t.string "nonce" + t.datetime "redeemed_at" t.string "resource" t.string "scope" t.string "status", default: "pending", null: false @@ -197,6 +200,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_07_19_000004) do t.datetime "expires_at", null: false t.integer "oidc_access_token_id", null: false t.integer "oidc_authorization_code_id" + t.integer "oidc_device_code_id" t.string "resource" t.datetime "revoked_at" t.string "scope" @@ -209,6 +213,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_07_19_000004) do t.index ["expires_at"], name: "index_oidc_refresh_tokens_on_expires_at" t.index ["oidc_access_token_id"], name: "index_oidc_refresh_tokens_on_oidc_access_token_id" t.index ["oidc_authorization_code_id"], name: "index_oidc_refresh_tokens_on_oidc_authorization_code_id" + t.index ["oidc_device_code_id"], name: "index_oidc_refresh_tokens_on_oidc_device_code_id" t.index ["revoked_at"], name: "index_oidc_refresh_tokens_on_revoked_at" t.index ["token_family_id"], name: "index_oidc_refresh_tokens_on_token_family_id" t.index ["token_hmac"], name: "index_oidc_refresh_tokens_on_token_hmac", unique: true @@ -320,6 +325,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_07_19_000004) do add_foreign_key "application_user_claims", "users", on_delete: :cascade add_foreign_key "oidc_access_tokens", "applications" add_foreign_key "oidc_access_tokens", "oidc_authorization_codes", on_delete: :nullify + add_foreign_key "oidc_access_tokens", "oidc_device_codes", on_delete: :nullify add_foreign_key "oidc_access_tokens", "users" add_foreign_key "oidc_authorization_codes", "applications" add_foreign_key "oidc_authorization_codes", "users" @@ -328,6 +334,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_07_19_000004) do add_foreign_key "oidc_refresh_tokens", "applications" add_foreign_key "oidc_refresh_tokens", "oidc_access_tokens" add_foreign_key "oidc_refresh_tokens", "oidc_authorization_codes", on_delete: :nullify + add_foreign_key "oidc_refresh_tokens", "oidc_device_codes", on_delete: :nullify add_foreign_key "oidc_refresh_tokens", "users" add_foreign_key "oidc_user_consents", "applications" add_foreign_key "oidc_user_consents", "users" diff --git a/test/controllers/oidc_device_flow_controller_test.rb b/test/controllers/oidc_device_flow_controller_test.rb index cef7090..db2d6b4 100644 --- a/test/controllers/oidc_device_flow_controller_test.rb +++ b/test/controllers/oidc_device_flow_controller_test.rb @@ -159,10 +159,37 @@ class OidcDeviceFlowControllerTest < ActionDispatch::IntegrationTest assert_equal "Bearer", body["token_type"] assert_equal "openid groups", body["scope"] - # Replaying the (now consumed) device_code fails. + # Replaying the (now consumed) device_code fails, and is reported as reuse — + # distinguishable from the generic "Invalid device_code" for an unknown code. poll(dc, code_verifier: CODE_VERIFIER) assert_response :bad_request - assert_equal "invalid_grant", JSON.parse(@response.body)["error"] + replay = JSON.parse(@response.body) + assert_equal "invalid_grant", replay["error"] + assert_match(/already been used/i, replay["error_description"]) + end + + test "replaying a redeemed device_code revokes the tokens it issued" do + OidcUserConsent.create!(user: @user, application: @cli, scopes_granted: "openid", granted_at: Time.current) + dc = OidcDeviceCode.create!( + application: @cli, scope: "openid", + code_challenge: code_challenge_for(CODE_VERIFIER), code_challenge_method: "S256" + ) + dc.approve!(user: @user, acr: "1", auth_time: Time.current.to_i) + + poll(dc, code_verifier: CODE_VERIFIER) + assert_response :success + access = OidcAccessToken.find_by_token(JSON.parse(@response.body)["access_token"]) + refresh = OidcRefreshToken.where(oidc_device_code: dc).first + assert access.active?, "token should be live before the replay" + + # The code is kept (not destroyed) so the replay is detectable... + assert dc.reload.redeemed? + poll(dc, code_verifier: CODE_VERIFIER) + assert_response :bad_request + + # ...and every token descended from the replayed code is revoked. + assert access.reload.revoked? + assert refresh.reload.revoked? end test "token endpoint refuses a PKCE-required client whose device_code lacks a challenge" do