diff --git a/app/controllers/my/access_tokens_controller.rb b/app/controllers/my/access_tokens_controller.rb index 4e836141b3..b32d11846a 100644 --- a/app/controllers/my/access_tokens_controller.rb +++ b/app/controllers/my/access_tokens_controller.rb @@ -8,29 +8,24 @@ def index @access_tokens = my_access_tokens.order(created_at: :desc) end - def show - @access_token = my_access_tokens.find(verifier.verify(params[:id])) - rescue ActiveSupport::MessageVerifier::InvalidSignature - redirect_to my_access_tokens_path, alert: "Token is no longer visible" - end - def new @access_token = my_access_tokens.new end + # The raw token is shown in this response and on no page after it, so the + # response must not be cached. def create - access_token = my_access_tokens.create!(access_token_params) + @access_token = my_access_tokens.create!(access_token_params) + no_store respond_to do |format| - format.html do - expiring_id = verifier.generate access_token.id, expires_in: 10.seconds - redirect_to my_access_token_path(expiring_id) - end + format.turbo_stream + format.html { render :create, status: :created } format.json do render status: :created, json: \ - { id: access_token.id, token: access_token.token, description: access_token.description, - permission: access_token.permission, created_at: access_token.created_at.utc } + { id: @access_token.id, token: @access_token.raw_token, description: @access_token.description, + permission: @access_token.permission, created_at: @access_token.created_at.utc } end end end @@ -52,8 +47,4 @@ def my_access_tokens def access_token_params params.expect(access_token: %i[ description permission ]) end - - def verifier - Rails.application.message_verifier(:access_tokens) - end end diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 7aec59607b..e7866b663f 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -5,6 +5,8 @@ class Oauth::AuthorizationsController < Oauth::BaseController # An S256 challenge is the unpadded base64url SHA-256 of the verifier: 43 characters (RFC 7636 §4.2). S256_CODE_CHALLENGE = /\A[A-Za-z0-9_-]{43}\z/ RETURN_URL_MAXIMUM_LENGTH = 2000 + # Read to find the client and the redirect target, so refused before either. + LOOKUP_PARAMETERS = %w[ client_id redirect_uri ] # Browsers apply form-action to every hop of a form submission, redirects # included, so the app-wide form-action 'self' would block the consent POST's @@ -16,12 +18,14 @@ class Oauth::AuthorizationsController < Oauth::BaseController # Gated ahead of sign-in so a dark server never asks anyone to sign in for # it. A denial is never gated: it only tells the client no. before_action :require_issuance_enabled, unless: :denial? + before_action :require_single_valued_parameters before_action :save_oauth_return_url before_action :require_authentication disallow_oauth_grants before_action :set_client before_action :validate_redirect_uri + before_action :validate_single_valued_parameters before_action :validate_response_type before_action :validate_pkce before_action :validate_scope @@ -128,12 +132,16 @@ def redirect_with_error(error, description) if params[:redirect_uri].present? && @client&.allows_redirect?(params[:redirect_uri]) && @client.vetted_redirect?(params[:redirect_uri]) redirect_to error_redirect_uri(error, description), allow_other_host: true else - @error = error - @error_description = description - render :error, status: :bad_request + render_error error, description end end + def render_error(error, description) + @error = error + @error_description = description + render :error, status: :bad_request + end + def success_redirect_uri(code) build_redirect_uri params[:redirect_uri], code: code, @@ -169,6 +177,25 @@ def validated_redirect_form_action_source nil end + # client_id and redirect_uri are refused before the client is looked up, + # and shown here, never redirected: a malformed redirect_uri is exactly the + # one that can't be validated. + def require_single_valued_parameters + if (malformed = malformed_parameter_names & LOOKUP_PARAMETERS).any? + render_error "invalid_request", malformed_parameters_description(malformed) + end + end + + # The rest are refused once the redirect target is known, so they go back + # to a vetted client like any other error. + def validate_single_valued_parameters + malformed = malformed_parameter_names + + if !performed? && malformed.any? + redirect_with_error "invalid_request", malformed_parameters_description(malformed) + end + end + def build_redirect_uri(base, **query_params) uri = URI.parse(base) query = URI.decode_www_form(uri.query || "") diff --git a/app/controllers/oauth/base_controller.rb b/app/controllers/oauth/base_controller.rb index f293ac3274..8e1734f792 100644 --- a/app/controllers/oauth/base_controller.rb +++ b/app/controllers/oauth/base_controller.rb @@ -9,7 +9,56 @@ class Oauth::BaseController < ApplicationController # a halted chain skips after_actions (RFC 6749 §5.1, §5.2). prepend_before_action :prevent_caching, :prevent_referrer + # OAuth parameters are top-level names. Rails' JSON parameter wrapping would + # also copy a JSON body under the controller's name, and for the token + # endpoint that's `token`, an OAuth parameter of its own. + wrap_parameters false + + # OAuth parameters that name one value (RFC 6749 §3.1: "Request and + # response parameters MUST NOT be included more than once"). Rails turns a + # name[]= form field or a JSON array or object into an Array or a nested + # Parameters, and handed to a lookup such as find_by(client_id: ...), an + # Array becomes an SQL IN list. Each endpoint refuses that shape right after + # its availability gate, so a dark server still answers 404, and before it + # reads anything. The authorization endpoint refuses client_id and + # redirect_uri that early, and the rest once its redirect target is known. client_secret isn't here: Oauth::ClientAuthentication reads + # it as a credential, and one that isn't a string fails authentication + # (invalid_client) like any wrong secret. + SINGLE_VALUED_PARAMETERS = %w[ + client_id redirect_uri scope state + code code_verifier code_challenge code_challenge_method response_type + grant_type refresh_token token token_type_hint + resource subject_token subject_token_type requested_token_type audience account_id + ].freeze + private + def require_single_valued_parameters + if (malformed = malformed_parameter_names).any? + refuse_malformed_parameters malformed + end + end + + # A JSON null is left to each endpoint, which treats it as absent or, for a + # refresh's scope, as naming no scope. + def malformed_parameter_names + (SINGLE_VALUED_PARAMETERS - list_valued_parameter_names).select do |name| + params.key?(name) && !params[name].nil? && !params[name].is_a?(String) + end + end + + # Parameters an endpoint takes as a list, exempt from the check above. + def list_valued_parameter_names + [] + end + + def refuse_malformed_parameters(names) + oauth_error "invalid_request", malformed_parameters_description(names) + end + + def malformed_parameters_description(names) + "#{names.to_sentence} must #{names.one? ? "be" : "each be"} a single string" + end + # Discovery follows acceptance (see Oauth::Availability): a dark server # answers as if it had no authorization server at all. def require_acceptance_enabled @@ -35,9 +84,12 @@ def piloting_client_id end # The metadata issuer and the iss authorization response parameter must be - # identical (RFC 9207 §2). script_name: nil keeps an account prefix out of it. + # identical (RFC 9207 §2), and clients compare them to the issuer they were + # configured with as exact strings (RFC 8414 §3.3). Like bc3's + # canonical_issuer_url, it's the origin with no trailing slash: + # https://app.fizzy.do. script_name: nil keeps an account prefix out of it. def oauth_issuer - root_url(script_name: nil) + root_url(script_name: nil).delete_suffix("/") end def prevent_caching diff --git a/app/controllers/oauth/clients_controller.rb b/app/controllers/oauth/clients_controller.rb index 2642b39936..cd9cad8814 100644 --- a/app/controllers/oauth/clients_controller.rb +++ b/app/controllers/oauth/clients_controller.rb @@ -6,6 +6,7 @@ class Oauth::ClientsController < Oauth::BaseController # that counted requests first would answer 429 to the eleventh, not 404. before_action :require_issuance_enabled rate_limit to: 10, within: 1.minute, only: :create, with: :oauth_rate_limit_exceeded + before_action :require_single_valued_parameters before_action :validate_redirect_uris before_action :validate_redirect_uri_origins before_action :validate_auth_method @@ -30,6 +31,16 @@ def piloting_client_id nil end + # Registration metadata may give scope as a list (validated_scopes), and + # its other lists, redirect_uris and grant_types, aren't single-valued names. + def list_valued_parameter_names + %w[ scope ] + end + + def refuse_malformed_parameters(names) + oauth_error "invalid_client_metadata", malformed_parameters_description(names) + end + # Bounded before any URI is parsed, so an oversized list costs nothing more # than reading the request. def validate_redirect_uris @@ -100,8 +111,8 @@ def validated_scopes def dynamic_client_registration_response(client) { client_id: client.client_id, - client_secret: client.client_secret, - client_secret_expires_at: (0 if client.client_secret), + client_secret: client.raw_client_secret, + client_secret_expires_at: (0 if client.raw_client_secret), client_name: client.name, redirect_uris: client.redirect_uris, token_endpoint_auth_method: client.token_endpoint_auth_method, diff --git a/app/controllers/oauth/revocations_controller.rb b/app/controllers/oauth/revocations_controller.rb index 147835f61b..b3a666aa9d 100644 --- a/app/controllers/oauth/revocations_controller.rb +++ b/app/controllers/oauth/revocations_controller.rb @@ -21,6 +21,7 @@ class Oauth::RevocationsController < Oauth::BaseController # throttled like the token endpoint (RFC 6749 §2.3.1, RFC 7009 §5). rate_limit to: 20, within: 1.minute, only: :create, with: :oauth_rate_limit_exceeded + before_action :require_single_valued_parameters before_action :reject_ambiguous_client_credentials before_action :require_token @@ -42,7 +43,7 @@ def require_token end def revocable_token - Identity::AccessToken.personal.find_by(token: params[:token]) || client_token + Identity::AccessToken.personal.find_by_raw_token(params[:token]) || client_token end # Either token of a grant revokes the whole grant, and so does a refresh @@ -51,8 +52,8 @@ def client_token if client = authenticated_client tokens = Identity::AccessToken.oauth.where(oauth_client: client) - tokens.find_by(token: params[:token]) || tokens.find_by(refresh_token: params[:token]) || - tokens.find_by(id: Oauth::RetiredRefreshToken.where(refresh_token: params[:token]).select(:access_token_id)) + tokens.find_by_raw_token(params[:token]) || tokens.find_by_refresh_token(params[:token]) || + tokens.find_by(id: Oauth::RetiredRefreshToken.find_by_raw_token(params[:token])&.access_token_id) end end end diff --git a/app/controllers/oauth/tokens_controller.rb b/app/controllers/oauth/tokens_controller.rb index 6756e65235..6deaf7c607 100644 --- a/app/controllers/oauth/tokens_controller.rb +++ b/app/controllers/oauth/tokens_controller.rb @@ -8,6 +8,7 @@ class Oauth::TokensController < Oauth::BaseController # that counted requests first would answer 429 to the eleventh, not 404. before_action :require_issuance_enabled rate_limit to: 20, within: 1.minute, only: :create, with: :oauth_rate_limit_exceeded + before_action :require_single_valued_parameters before_action :validate_grant_type # Authenticate the client the request names before looking anything up about @@ -54,7 +55,7 @@ def create answer_refresh_replay @retired_refresh_token elsif @access_token.refresh(permission: @refresh_permission) render json: refresh_response(@access_token) - elsif retired_refresh_token = Oauth::RetiredRefreshToken.find_by(refresh_token: params[:refresh_token]) + elsif retired_refresh_token = Oauth::RetiredRefreshToken.find_by_raw_token(params[:refresh_token]) # A concurrent refresh won the rotation: this is the same retry. answer_refresh_replay retired_refresh_token else @@ -132,7 +133,7 @@ def set_identity # known to be the grant's own. def set_refreshable_access_token @access_token = Identity::AccessToken.find_by_refresh_token(params[:refresh_token]) || - (@retired_refresh_token = Oauth::RetiredRefreshToken.find_by(refresh_token: params[:refresh_token]))&.access_token + (@retired_refresh_token = Oauth::RetiredRefreshToken.find_by_raw_token(params[:refresh_token]))&.access_token unless @access_token oauth_error "invalid_grant", "Invalid refresh token" @@ -214,7 +215,7 @@ def authenticate_client def answer_refresh_replay(retired_refresh_token) if grant = Identity::AccessToken.uncached { Identity::AccessToken.find_by(id: retired_refresh_token.access_token_id) } if retired_refresh_token.retryable?(grant) - answer_refresh_retry grant + answer_refresh_retry grant, retired_refresh_token.successor elsif retired_refresh_token.within_grace? oauth_error "invalid_grant", "Refresh token superseded" else @@ -228,12 +229,18 @@ def answer_refresh_replay(retired_refresh_token) # A retry repeats the rotation it lost the response to, so it must ask for # what that rotation produced. One asking for less can't be handed the - # wider successor that already exists. - def answer_refresh_retry(grant) - if @refresh_permission == grant.permission + # wider successor that already exists. The answer is the very pair that + # rotation issued, kept encrypted for the grace window, since only its + # digests are stored. Once that's gone there's nothing to repeat, and the + # client, which holds no live token, is refused without revoking. + def answer_refresh_retry(grant, successor) + if @refresh_permission != grant.permission + oauth_error "invalid_scope", "Requested scope differs from the rotation being retried" + elsif successor + grant.assign_attributes raw_token: successor[:access_token], raw_refresh_token: successor[:refresh_token] render json: refresh_response(grant) else - oauth_error "invalid_scope", "Requested scope differs from the rotation being retried" + oauth_error "invalid_grant", "Refresh token no longer retryable" end end @@ -243,10 +250,10 @@ def refresh_response(access_token) def token_response(access_token, scope: nil) { - access_token: access_token.token, + access_token: access_token.raw_token, token_type: "Bearer", expires_in: access_token.expires_in, - refresh_token: access_token.refresh_token, + refresh_token: access_token.raw_refresh_token, scope: scope }.compact end diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 777bcf6d9e..8d4199f213 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -48,6 +48,21 @@ class Identity::AccessToken < ApplicationRecord enum :permission, %w[ read write ].index_by(&:itself), default: :read + # Tokens are stored as digests (TokenDigester). The raw values exist only in + # memory, on the record that issued or rotated them, to be handed to their + # holder once. Personal access tokens also keep their plaintext in `token` + # for now, because production code that reads only `token` shares this + # database until this code is deployed there. OAuth access tokens never do. + # + # The plaintext `refresh_token` column is ignored until it's dropped, beside + # `token`. Without partial_inserts, a model that knows a column names it on + # every INSERT, so only code that ignores it can keep creating tokens once + # it's gone. + self.ignored_columns += %w[ refresh_token ] + + attribute :raw_token, :string + attribute :raw_refresh_token, :string + before_create :set_token before_create :set_expiry_and_refresh_token, if: :oauth_client_id? @@ -74,11 +89,28 @@ class << self # The token a bearer request may authenticate with: active and honored. # Whether it permits the request's method is the caller's question. def find_honored(token) - if (access_token = active.find_by(token: token)) && access_token.honored? + if (access_token = active.find_by_raw_token(token)) && access_token.honored? access_token end end + # Finds the token within the current scope by its digest. A personal + # access token written by code that stores only plaintext has no digest + # yet, so it's found by plaintext and given one: no lookup but the first + # ever matches on plaintext. Code that stores only plaintext issues + # prefixed tokens too, so a prefix doesn't exempt a token from this. + def find_by_raw_token(raw) + if raw.is_a?(String) && raw.present? + find_by(token_digest: TokenDigester.digests(raw)) || find_and_digest_undigested(raw) + end + end + + def find_by_refresh_token(raw) + if raw.is_a?(String) && raw.present? + oauth.find_by(refresh_token_digest: TokenDigester.digests(raw)) + end + end + def generate_token(kind) PREFIXES.fetch(kind) + SecureRandom.alphanumeric(BODY_LENGTH, chars: BASE58) end @@ -94,10 +126,6 @@ def cleanup end end - def find_by_refresh_token(refresh_token) - oauth.find_by(refresh_token: refresh_token) - end - # An authorization code redeems at most once (RFC 6749 §4.1.2): the grant it # mints is stamped with the code's jti, and the unique index refuses a second # stamp. A replay also revokes the grant the first redemption issued. @@ -107,6 +135,24 @@ def redeem(authorization_code, **attributes) where(authorization_code_jti: authorization_code.jti).destroy_all nil end + + private + # MySQL compares `token` case-insensitively, so a match is confirmed + # byte for byte: a differently cased token neither authenticates nor + # installs a digest that the real token would then never match. + # + # The digest goes to the primary even when the token was found on the + # replica, as GET requests read. update_columns leaves updated_at alone. + # A datacenter that can't reach the primary (Deployment::DatabaseResolver) + # still authenticates the token and leaves the digest to one that can, + # or to the backfill rerun. + def find_and_digest_undigested(raw) + personal.where(token_digest: nil, token: raw).detect { |candidate| candidate.token == raw }&.tap do |access_token| + if Deployment::DatabaseResolver.in_primary_datacenter? + ApplicationRecord.with_writing_role { access_token.update_columns token_digest: TokenDigester.digest(raw) } + end + end + end end # OAuth tokens are honored only while OAuth acceptance is on (see @@ -137,16 +183,22 @@ def lapsed? # lapses mid-request doesn't rotate either, so a sweep that found it lapsed # never deletes a grant that was just renewed. The presented # token is retired, not forgotten, so presenting it again is recognized as - # a retry or a replay (see Oauth::RetiredRefreshToken). + # a retry or a replay (see Oauth::RetiredRefreshToken). The retired row + # carries the new pair, encrypted, so a retry within the grace window can be + # answered with it. def refresh(permission: self.permission) - rotated = { token: self.class.generate_token(:access), - refresh_token: self.class.generate_token(:refresh), + raw_token, raw_refresh_token = self.class.generate_token(:access), self.class.generate_token(:refresh) + rotated = { token: nil, token_digest: TokenDigester.digest(raw_token), + refresh_token_digest: TokenDigester.digest(raw_refresh_token), expires_at: EXPIRES_IN.from_now, permission: permission, updated_at: Time.current } transaction do - if self.class.unlapsed.where(id: id, refresh_token: refresh_token).update_all(rotated) == 1 - retired_refresh_tokens.create! refresh_token: refresh_token, successor_refresh_token: rotated[:refresh_token] - assign_attributes rotated + if self.class.unlapsed.where(id: id, refresh_token_digest: refresh_token_digest).update_all(rotated) == 1 + retired_refresh_tokens.create! \ + refresh_token_digest: refresh_token_digest, + successor_refresh_token_digest: rotated[:refresh_token_digest], + successor: { access_token: raw_token, refresh_token: raw_refresh_token } + assign_attributes rotated.merge(raw_token: raw_token, raw_refresh_token: raw_refresh_token) true end end @@ -167,12 +219,18 @@ def lock_grant self.class.lock.where(id: id).pluck(:id) end + # Personal access tokens are written in plaintext as well as digested, + # for production code that reads only plaintext. An explicitly given + # token is digested like a generated one. def set_token - self.token ||= self.class.generate_token(oauth_client_id? ? :access : :personal) + self.raw_token ||= token.presence || self.class.generate_token(oauth_client_id? ? :access : :personal) + self.token_digest = TokenDigester.digest(raw_token) + self.token = oauth_client_id? ? nil : raw_token end def set_expiry_and_refresh_token self.expires_at ||= EXPIRES_IN.from_now - self.refresh_token ||= self.class.generate_token(:refresh) + self.raw_refresh_token ||= self.class.generate_token(:refresh) + self.refresh_token_digest = TokenDigester.digest(raw_refresh_token) end end diff --git a/app/models/oauth/client.rb b/app/models/oauth/client.rb index 25a2d0cc8b..28d103eb52 100644 --- a/app/models/oauth/client.rb +++ b/app/models/oauth/client.rb @@ -29,6 +29,11 @@ class Oauth::Client < ApplicationRecord validates :token_endpoint_auth_method, inclusion: { in: AUTH_METHODS } validate :redirect_uris_are_valid + # A confidential client's secret is stored as a digest (TokenDigester). The + # raw secret exists only in memory on the record that generated it, for the + # registration response, the one time it's shown. + attribute :raw_client_secret, :string + before_create :generate_client_secret, if: :confidential? attribute :redirect_uris, default: -> { [] } @@ -73,8 +78,7 @@ def confidential? end def authenticate_secret(secret) - confidential? && client_secret.present? && secret.is_a?(String) && secret.present? && - ActiveSupport::SecurityUtils.secure_compare(client_secret, secret) + confidential? && client_secret_digest.present? && TokenDigester.verify(secret, client_secret_digest) end def allows_redirect?(uri) @@ -103,7 +107,8 @@ def lock_client end def generate_client_secret - self.client_secret ||= Identity::AccessToken.generate_token(:client_secret) + self.raw_client_secret ||= Identity::AccessToken.generate_token(:client_secret) + self.client_secret_digest = TokenDigester.digest(raw_client_secret) end def redirect_uris_are_valid diff --git a/app/models/oauth/retired_refresh_token.rb b/app/models/oauth/retired_refresh_token.rb index 76fde92525..a070df7525 100644 --- a/app/models/oauth/retired_refresh_token.rb +++ b/app/models/oauth/retired_refresh_token.rb @@ -12,14 +12,51 @@ class Oauth::RetiredRefreshToken < ApplicationRecord # grant a retired token came from has lapsed unless it rotated since. RETENTION = Identity::AccessToken::REFRESH_IDLE_LIMIT + # A retry is answered with the pair the rotation issued, which is stored + # nowhere else in raw form. It's kept encrypted, sealed to expire with the + # grace window, and blanked by the sweep soon after. The sweep looks back + # this far, so it stays an index range over recent rows rather than a scan + # of every retired token; anything older has long been unreadable anyway. + SUCCESSOR_SWEEP_WINDOW = 1.day + belongs_to :access_token, class_name: "Identity::AccessToken" scope :stale, -> { where(created_at: ...RETENTION.ago) } class << self + def find_by_raw_token(raw) + if raw.is_a?(String) && raw.present? + find_by(refresh_token_digest: TokenDigester.digests(raw)) + end + end + def cleanup stale.in_batches.delete_all end + + def forget_successors + where(created_at: SUCCESSOR_SWEEP_WINDOW.ago...GRACE.ago).where.not(successor_payload: nil).update_all(successor_payload: nil) + end + + def successor_encryptor + @successor_encryptor ||= ActiveSupport::MessageEncryptor.new \ + Rails.application.key_generator.generate_key("oauth_refresh_token_successor", 32) + end + end + + # The access and refresh token the rotation issued: { access_token:, refresh_token: }. + def successor=(pair) + self.successor_payload = self.class.successor_encryptor.encrypt_and_sign(pair.to_json, expires_in: GRACE, purpose: :oauth_refresh_token_successor) + end + + # The pair, or nil once the grace window has passed or the sweep has + # blanked it. + def successor + if successor_payload.present? && (decrypted = self.class.successor_encryptor.decrypt_and_verify(successor_payload, purpose: :oauth_refresh_token_successor)) + JSON.parse(decrypted).symbolize_keys.slice(:access_token, :refresh_token) + end + rescue ActiveSupport::MessageEncryptor::InvalidMessage, JSON::ParserError + nil end # Whether a replay now is a retry the grant can answer with the successor @@ -39,6 +76,6 @@ def within_grace? # grant's current token, never from timestamp order, which skewed host # clocks could invert. def superseded?(grant) - grant.refresh_token != successor_refresh_token + grant.refresh_token_digest != successor_refresh_token_digest end end diff --git a/app/models/token_digester.rb b/app/models/token_digester.rb new file mode 100644 index 0000000000..cf39ffa0aa --- /dev/null +++ b/app/models/token_digester.rb @@ -0,0 +1,94 @@ +# Bearer credentials (access tokens, refresh tokens, client secrets) are +# stored as HMAC-SHA256(pepper, raw) digests, never as the raw value, so a +# copy of the database doesn't hand anyone a working credential. A lookup +# digests what it's given and finds the row by that digest. +# +# Byte-compatible with bc3's Oauth::TokenDigester: lowercase hex of +# HMAC-SHA256 under the pepper, nothing else. +# +# A digest may carry a version prefix, "$", so the pepper can be +# rotated later without a flag day: digests made under a new pepper carry its +# version while older rows keep theirs, and lookups try every version still +# in service. A bare digest is version 1. Only version 1 exists today. +# +# The pepper: +# +# - SaaS deployments read it from OAUTH_TOKEN_PEPPER, and refuse to boot +# without it (config/initializers/token_digester.rb). The name mirrors +# bc3's credentials.oauth.token_pepper; personal access tokens are digested +# under it too. Beta reads production's database, so beta and production +# must share one pepper: saas/.kamal/secrets.beta fetches production's +# field rather than keeping a copy that could drift. +# - Elsewhere OAUTH_TOKEN_PEPPER is used when set, and otherwise a key is +# derived from secret_key_base. On a self-hosted install, then, changing +# SECRET_KEY_BASE invalidates every personal access token and OAuth grant. +# +# Changing the pepper invalidates every stored digest, so it MUST NOT change +# except through a versioned rotation. +module TokenDigester + extend self + + class MissingPepper < StandardError; end + + CURRENT_VERSION = 1 + VERSION_SEPARATOR = "$" + MINIMUM_PEPPER_LENGTH = 32 + + def digest(raw, version: CURRENT_VERSION) + hex = OpenSSL::HMAC.hexdigest("SHA256", peppers.fetch(version), raw) + version == 1 ? hex : "#{version}#{VERSION_SEPARATOR}#{hex}" + end + + # Every digest a stored credential might carry, one per pepper version in + # service, for an indexed lookup: where(token_digest: TokenDigester.digests(raw)). + def digests(raw) + peppers.keys.map { |version| digest(raw, version: version) } + end + + def verify(raw, stored_digest) + if raw.is_a?(String) && raw.present? && stored_digest.is_a?(String) && peppers.key?(version = version_of(stored_digest)) + ActiveSupport::SecurityUtils.secure_compare(digest(raw, version: version), stored_digest) + else + false + end + end + + def version_of(stored_digest) + if stored_digest.include?(VERSION_SEPARATOR) + Integer(stored_digest.split(VERSION_SEPARATOR, 2).first, 10, exception: false) + else + 1 + end + end + + # Raises unless a usable pepper is configured. Run at boot, so a deploy + # missing the secret fails before it serves a request, rather than at the + # first lookup, and never by quietly digesting under another key. + def verify_pepper! + peppers + true + end + + def pepper_required? + Fizzy.saas? && !Rails.env.local? + end + + private + def peppers + @peppers ||= { 1 => configured_pepper(ENV["OAUTH_TOKEN_PEPPER"], required: pepper_required?) } + end + + def configured_pepper(value, required:) + if value.present? + if value.length < MINIMUM_PEPPER_LENGTH + raise MissingPepper, "OAUTH_TOKEN_PEPPER must be at least #{MINIMUM_PEPPER_LENGTH} characters; generate one with bin/rails secret" + else + value + end + elsif required + raise MissingPepper, "OAUTH_TOKEN_PEPPER is required in this environment. Refusing to fall back to a key derived from secret_key_base, which would digest under a different key than every token already stored" + else + Rails.application.key_generator.generate_key("token_digest_pepper", 32) + end + end +end diff --git a/app/views/my/access_tokens/_reveal.html.erb b/app/views/my/access_tokens/_reveal.html.erb new file mode 100644 index 0000000000..7900852076 --- /dev/null +++ b/app/views/my/access_tokens/_reveal.html.erb @@ -0,0 +1,17 @@ +<%# Temporary, so Turbo's snapshot cache doesn't keep the token for Back to restore. %> +
+
+ +

Be sure to save this access token now because you won’t be able to see it again.

+ + <%= tag.button class: "btn btn--link center", data: { + controller: "copy-to-clipboard", action: "copy-to-clipboard#copy", + copy_to_clipboard_success_class: "btn--success", copy_to_clipboard_content_value: access_token.raw_token } do %> + <%= icon_tag "copy-paste" %> + Copy access token + <% end %> +
+
diff --git a/app/views/my/access_tokens/create.html.erb b/app/views/my/access_tokens/create.html.erb new file mode 100644 index 0000000000..173680bf3d --- /dev/null +++ b/app/views/my/access_tokens/create.html.erb @@ -0,0 +1,11 @@ +<% @page_title = "New personal access token" %> + +<% content_for :header do %> +
+ <%= back_link_to "Tokens", my_access_tokens_path, "keydown.left@document->hotkey#click keydown.esc@document->hotkey#click" %> +
+ +

<%= @page_title %>

+<% end %> + +<%= render "my/access_tokens/reveal", access_token: @access_token %> diff --git a/app/views/my/access_tokens/create.turbo_stream.erb b/app/views/my/access_tokens/create.turbo_stream.erb new file mode 100644 index 0000000000..b4e63845bd --- /dev/null +++ b/app/views/my/access_tokens/create.turbo_stream.erb @@ -0,0 +1 @@ +<%= turbo_stream.replace "new_access_token", partial: "my/access_tokens/reveal", locals: { access_token: @access_token } %> diff --git a/app/views/my/access_tokens/new.html.erb b/app/views/my/access_tokens/new.html.erb index 27e7fbffc7..367309d8e7 100644 --- a/app/views/my/access_tokens/new.html.erb +++ b/app/views/my/access_tokens/new.html.erb @@ -1,4 +1,6 @@ <% @page_title = "Generate a personal access token" %> +<%# Never snapshotted: once the form gives way to the revealed token, Back must fetch a fresh form, not restore a page holding neither. %> +<% turbo_exempts_page_from_cache %> <% content_for :header do %>
@@ -8,7 +10,7 @@

<%= @page_title %>

<% end %> -
+
<%= form_with model: @access_token, url: my_access_tokens_path, scope: :access_token, data: { controller: "form" }, html: { class: "flex flex-column gap" } do |form| %>
<%= form.label :description, "Access token description" %> diff --git a/app/views/my/access_tokens/show.html.erb b/app/views/my/access_tokens/show.html.erb deleted file mode 100644 index 7611377107..0000000000 --- a/app/views/my/access_tokens/show.html.erb +++ /dev/null @@ -1,26 +0,0 @@ -<% @page_title = "New personal access token" %> - -<% content_for :header do %> -
- <%= back_link_to "Tokens", my_access_tokens_path, "keydown.left@document->hotkey#click keydown.esc@document->hotkey#click" %> -
- -

<%= @page_title %>

-<% end %> - -
-
- -

Be sure to save this access token now because you won’t be able to see it again.

- - <%= tag.button class: "btn btn--link center", data: { - controller: "copy-to-clipboard", action: "copy-to-clipboard#copy", - copy_to_clipboard_success_class: "btn--success", copy_to_clipboard_content_value: @access_token.token } do %> - <%= icon_tag "copy-paste" %> - Copy access token - <% end %> -
-
diff --git a/config/initializers/token_digester.rb b/config/initializers/token_digester.rb new file mode 100644 index 0000000000..6aac5d3e9f --- /dev/null +++ b/config/initializers/token_digester.rb @@ -0,0 +1,6 @@ +# Fail fast on a SaaS deploy missing OAUTH_TOKEN_PEPPER (see TokenDigester). +# Asset precompilation boots with a dummy secret key base and no runtime +# secrets, and never digests anything. +Rails.application.config.after_initialize do + TokenDigester.verify_pepper! unless ENV["SECRET_KEY_BASE_DUMMY"] +end diff --git a/config/recurring.yml b/config/recurring.yml index 046489c489..3929a4d39b 100644 --- a/config/recurring.yml +++ b/config/recurring.yml @@ -30,6 +30,9 @@ production: &production cleanup_oauth_retired_refresh_tokens: command: "Oauth::RetiredRefreshToken.cleanup" schedule: every day at 04:22 + forget_oauth_refresh_successors: + command: "Oauth::RetiredRefreshToken.forget_successors" + schedule: every 5 minutes cleanup_lapsed_oauth_grants: command: "Identity::AccessToken.cleanup" schedule: every day at 04:32 diff --git a/config/routes.rb b/config/routes.rb index 6776ffa3db..802c723e59 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -198,9 +198,10 @@ namespace :my do resource :passkey_challenge, only: :create resource :identity, only: :show - # show is reached by a signed id that reveals the new token for a few seconds + # show, which revealed a new token by its signed id, is gone; the filter + # stays so an old reveal link in a log or Referer still reads [FILTERED]. filter_path_parameters :id do - resources :access_tokens + resources :access_tokens, except: %i[ show edit update ] end resources :connected_apps, only: %i[ index destroy ] resources :passkeys, except: %i[ show new ] diff --git a/db/migrate/20261015120000_add_token_digests.rb b/db/migrate/20261015120000_add_token_digests.rb new file mode 100644 index 0000000000..2384459282 --- /dev/null +++ b/db/migrate/20261015120000_add_token_digests.rb @@ -0,0 +1,18 @@ +# Credentials are looked up by an HMAC digest of the raw value (TokenDigester) +# rather than by the value itself. Each digest gets a unique index, created +# here, before any digest is written: a duplicate then fails loudly instead +# of indexing as two rows, and no lookup ever scans the table. +# +# One statement per change, each skipped when it's already done, so a run that +# stopped partway (MySQL commits each DDL statement on its own) can simply be +# run again. Apart, the column adds are INSTANT, and the indexes INPLACE. +class AddTokenDigests < ActiveRecord::Migration[8.2] + def change + add_column :identity_access_tokens, :token_digest, :string, limit: 80, if_not_exists: true + add_column :identity_access_tokens, :refresh_token_digest, :string, limit: 80, if_not_exists: true + add_index :identity_access_tokens, :token_digest, unique: true, if_not_exists: true + add_index :identity_access_tokens, :refresh_token_digest, unique: true, if_not_exists: true + + add_column :oauth_clients, :client_secret_digest, :string, limit: 80, if_not_exists: true + end +end diff --git a/db/migrate/20261015120100_backfill_token_digests.rb b/db/migrate/20261015120100_backfill_token_digests.rb new file mode 100644 index 0000000000..483514ddf8 --- /dev/null +++ b/db/migrate/20261015120100_backfill_token_digests.rb @@ -0,0 +1,78 @@ +# Digests every stored credential that has none yet. MySQL and SQLite have no +# HMAC, so this runs in Ruby, a batch at a time, outside one long transaction. +# +# update_all writes only the digest: a grant's updated_at is its idle-lapse +# clock (Identity::AccessToken::REFRESH_IDLE_LIMIT), and backfilling must not +# restart it. +# +# Personal access tokens keep their plaintext: production still reads it until +# this code reaches it. OAuth access tokens are digest-only, so any plaintext +# one is cleared once digested. Refresh tokens and client secrets are moved +# wholesale: a refresh token's plaintext is cleared once digested, and the +# client secret column is dropped by the next migration. The refresh token +# column stays, empty, until it's dropped beside `token`. +# +# Rerunnable: it only touches rows without a digest, and skips columns already +# dropped. Its down is a no-op, so `db:migrate:redo:primary VERSION=20261015120100` +# runs it again, to digest personal access tokens written since by code that +# doesn't write digests. +class BackfillTokenDigests < ActiveRecord::Migration[8.2] + disable_ddl_transaction! + + BATCH_SIZE = 500 + + class AccessToken < ActiveRecord::Base + self.table_name = "identity_access_tokens" + end + + class Client < ActiveRecord::Base + self.table_name = "oauth_clients" + end + + def up + refuse_duplicates AccessToken, :token + + each_undigested AccessToken, :token, :token_digest do |row, digest| + AccessToken.where(id: row.id, token_digest: nil).update_all \ + token_digest: digest, token: (row.oauth_client_id ? nil : row.token) + end + + # Each is gone once dropped, so a rerun skips it. + if column_exists?(:identity_access_tokens, :refresh_token) + each_undigested AccessToken, :refresh_token, :refresh_token_digest do |row, digest| + AccessToken.where(id: row.id, refresh_token_digest: nil).update_all refresh_token_digest: digest, refresh_token: nil + end + end + + if column_exists?(:oauth_clients, :client_secret) + each_undigested Client, :client_secret, :client_secret_digest do |row, digest| + Client.where(id: row.id, client_secret_digest: nil).update_all client_secret_digest: digest + end + end + end + + def down + # Digests are derived from what's still stored: nothing to undo. + end + + private + # Two rows sharing a token would share a digest, which the unique index + # refuses partway through. Stop before writing anything instead, naming + # the rows but never the tokens. + def refuse_duplicates(model, column) + duplicated = model.where.not(column => nil).group(column).having("COUNT(*) > 1").pluck(column) + + if duplicated.any? + ids = model.where(column => duplicated).pluck(:id) + raise "#{model.table_name}.#{column} has #{duplicated.size} duplicated value(s), in rows #{ids.join(", ")}. Resolve them before digesting." + end + end + + def each_undigested(model, column, digest_column) + model.where(digest_column => nil).where.not(column => nil).in_batches(of: BATCH_SIZE) do |batch| + batch.select(:id, column, *(:oauth_client_id if model == AccessToken)).each do |row| + yield row, TokenDigester.digest(row[column]) + end + end + end +end diff --git a/db/migrate/20261015120200_drop_plaintext_oauth_credentials.rb b/db/migrate/20261015120200_drop_plaintext_oauth_credentials.rb new file mode 100644 index 0000000000..9516e84fe8 --- /dev/null +++ b/db/migrate/20261015120200_drop_plaintext_oauth_credentials.rb @@ -0,0 +1,59 @@ +# Client secrets have been digested (BackfillTokenDigests) and are never read +# in plaintext again. Nothing in production holds them: the OAuth tables have +# never served production traffic. +# +# Retired refresh tokens are only kept to recognize a replay, so the table is +# rebuilt keyed by digest rather than converted. A replay of a token rotated +# before this migration is then answered as an unknown token, not revoked. +# +# Only tables that code from before this stack never models are touched here. +# Without partial_inserts, a process names every column it has cached on each +# INSERT, so dropping a column of a table that running code writes would fail +# that code's inserts until it restarts. identity_access_tokens.refresh_token +# is such a column: Identity::AccessToken ignores it, and it is dropped beside +# `token` once every process runs code that ignores both. +# +# Each step is skipped when it's already done, so a run that stopped partway +# (MySQL commits each DDL statement on its own) can simply be run again. The +# retired-token table is told apart by its columns: a rerun drops the old one +# but never the new one it already made. +class DropPlaintextOauthCredentials < ActiveRecord::Migration[8.2] + def up + remove_column :oauth_clients, :client_secret, if_exists: true + + if table_exists?(:oauth_retired_refresh_tokens) && column_exists?(:oauth_retired_refresh_tokens, :refresh_token) + drop_table :oauth_retired_refresh_tokens + end + + create_table :oauth_retired_refresh_tokens, id: :uuid, if_not_exists: true do |t| + t.uuid :access_token_id, null: false + t.string :refresh_token_digest, limit: 80, null: false + t.string :successor_refresh_token_digest, limit: 80, null: false + t.text :successor_payload + t.datetime :created_at, null: false + + t.index :refresh_token_digest, unique: true + t.index :access_token_id + t.index :created_at + end + end + + def down + if table_exists?(:oauth_retired_refresh_tokens) && column_exists?(:oauth_retired_refresh_tokens, :refresh_token_digest) + drop_table :oauth_retired_refresh_tokens + end + + create_table :oauth_retired_refresh_tokens, id: :uuid, if_not_exists: true do |t| + t.uuid :access_token_id, null: false + t.string :refresh_token, null: false + t.string :successor_refresh_token, null: false + t.datetime :created_at, null: false + + t.index :refresh_token, unique: true + t.index :access_token_id + t.index :created_at + end + + add_column :oauth_clients, :client_secret, :string, if_not_exists: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 2870409449..84cc4c91b7 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.2].define(version: 2026_10_06_130100) do +ActiveRecord::Schema[8.2].define(version: 2026_10_15_120200) do create_table "accesses", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.datetime "accessed_at" t.uuid "account_id", null: false @@ -360,12 +360,16 @@ t.uuid "oauth_client_id" t.string "permission" t.string "refresh_token" + t.string "refresh_token_digest", limit: 80 t.string "token" + t.string "token_digest", limit: 80 t.datetime "updated_at", null: false t.index ["authorization_code_jti"], name: "index_identity_access_tokens_on_authorization_code_jti", unique: true t.index ["identity_id"], name: "index_access_token_on_identity_id" t.index ["oauth_client_id"], name: "index_identity_access_tokens_on_oauth_client_id" t.index ["refresh_token"], name: "index_identity_access_tokens_on_refresh_token", unique: true + t.index ["refresh_token_digest"], name: "index_identity_access_tokens_on_refresh_token_digest", unique: true + t.index ["token_digest"], name: "index_identity_access_tokens_on_token_digest", unique: true end create_table "identity_transfers", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| @@ -438,7 +442,7 @@ create_table "oauth_clients", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "client_id", null: false - t.string "client_secret" + t.string "client_secret_digest", limit: 80 t.datetime "created_at", null: false t.boolean "dynamically_registered", default: false t.string "name", null: false @@ -453,11 +457,12 @@ create_table "oauth_retired_refresh_tokens", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.uuid "access_token_id", null: false t.datetime "created_at", null: false - t.string "refresh_token", null: false - t.string "successor_refresh_token", null: false + t.string "refresh_token_digest", limit: 80, null: false + t.text "successor_payload" + t.string "successor_refresh_token_digest", limit: 80, null: false t.index ["access_token_id"], name: "index_oauth_retired_refresh_tokens_on_access_token_id" t.index ["created_at"], name: "index_oauth_retired_refresh_tokens_on_created_at" - t.index ["refresh_token"], name: "index_oauth_retired_refresh_tokens_on_refresh_token", unique: true + t.index ["refresh_token_digest"], name: "index_oauth_retired_refresh_tokens_on_refresh_token_digest", unique: true end create_table "pins", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb index 130ba3dbd2..1033aa3dc9 100644 --- a/db/schema_sqlite.rb +++ b/db/schema_sqlite.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.2].define(version: 2026_10_06_130100) do +ActiveRecord::Schema[8.2].define(version: 2026_10_15_120200) do create_table "accesses", id: :uuid, force: :cascade do |t| t.datetime "accessed_at" t.uuid "account_id", null: false @@ -360,12 +360,16 @@ t.uuid "oauth_client_id" t.string "permission", limit: 255 t.string "refresh_token", limit: 255 + t.string "refresh_token_digest", limit: 80 t.string "token", limit: 255 + t.string "token_digest", limit: 80 t.datetime "updated_at", null: false t.index ["authorization_code_jti"], name: "index_identity_access_tokens_on_authorization_code_jti", unique: true t.index ["identity_id"], name: "index_access_token_on_identity_id" t.index ["oauth_client_id"], name: "index_identity_access_tokens_on_oauth_client_id" t.index ["refresh_token"], name: "index_identity_access_tokens_on_refresh_token", unique: true + t.index ["refresh_token_digest"], name: "index_identity_access_tokens_on_refresh_token_digest", unique: true + t.index ["token_digest"], name: "index_identity_access_tokens_on_token_digest", unique: true end create_table "identity_transfers", id: :uuid, force: :cascade do |t| @@ -438,7 +442,7 @@ create_table "oauth_clients", id: :uuid, force: :cascade do |t| t.string "client_id", limit: 255, null: false - t.string "client_secret", limit: 255 + t.string "client_secret_digest", limit: 80 t.datetime "created_at", null: false t.boolean "dynamically_registered", default: false t.string "name", limit: 255, null: false @@ -452,12 +456,13 @@ create_table "oauth_retired_refresh_tokens", id: :uuid, force: :cascade do |t| t.uuid "access_token_id", null: false - t.string "refresh_token", limit: 255, null: false - t.string "successor_refresh_token", limit: 255, null: false + t.string "refresh_token_digest", limit: 80, null: false + t.string "successor_refresh_token_digest", limit: 80, null: false + t.text "successor_payload", limit: 65535 t.datetime "created_at", null: false t.index ["access_token_id"], name: "index_oauth_retired_refresh_tokens_on_access_token_id" t.index ["created_at"], name: "index_oauth_retired_refresh_tokens_on_created_at" - t.index ["refresh_token"], name: "index_oauth_retired_refresh_tokens_on_refresh_token", unique: true + t.index ["refresh_token_digest"], name: "index_oauth_retired_refresh_tokens_on_refresh_token_digest", unique: true end create_table "pins", id: :uuid, force: :cascade do |t| diff --git a/docs/docker-deployment.md b/docs/docker-deployment.md index c81765bf26..b025084020 100644 --- a/docs/docker-deployment.md +++ b/docs/docker-deployment.md @@ -48,6 +48,11 @@ Once you have one, set it in the `SECRET_KEY_BASE` environment variable: docker run --env SECRET_KEY_BASE=abcdefabcdef ... ``` +Personal access tokens and OAuth credentials are looked up by a keyed digest (HMAC-SHA256) rather than by the token itself. +By default the key is derived from `SECRET_KEY_BASE`, so **changing `SECRET_KEY_BASE` invalidates every personal access token and every connected OAuth app**: people have to generate new tokens and reconnect their apps. +To keep tokens working across a change of `SECRET_KEY_BASE`, set the key separately in `OAUTH_TOKEN_PEPPER` (at least 32 characters, for example from `bin/rails secret`). +Set it before upgrading to a version that digests tokens, or before anyone creates one, and then keep it: changing, adding or removing it later invalidates tokens in the same way. + #### SSL If you want the Fizzy container to handle its own SSL automatically, you just need to specify the domain name that you're running it on. diff --git a/docs/kamal-deployment.md b/docs/kamal-deployment.md index c50c51299b..e481f93b33 100644 --- a/docs/kamal-deployment.md +++ b/docs/kamal-deployment.md @@ -57,7 +57,7 @@ SMTP_PASSWORD=email-provider-password The values you enter here will be specific to you, and you can get or create them as follows: -- `SECRET_KEY_BASE` should be a long, random secret. You can run `bin/rails secret` to create a suitable value for this. +- `SECRET_KEY_BASE` should be a long, random secret. You can run `bin/rails secret` to create a suitable value for this. Keep it: changing it invalidates every personal access token and connected OAuth app, unless you set `OAUTH_TOKEN_PEPPER` separately (see [Docker deployment](docker-deployment.md#secret-key-base)). To set it, add `OAUTH_TOKEN_PEPPER` (at least 32 characters, e.g. from `bin/rails secret`) to `.kamal/secrets` **and** to the `env.secret` list in `config/deploy.yml`: Kamal passes a container only the secrets that list names. Set it before the first deploy that digests tokens, or keep the derived one: changing the pepper later invalidates every token too. - `SMTP_USERNAME` & `SMTP_PASSWORD` should be valid credentials for your SMTP server. If you're using a 3rd-party service here, consult their documentation for what to use. - `VAPID_PUBLIC_KEY` & `VAPID_PRIVATE_KEY` are a pair of credentials that are used for sending notifications. You can create your own keys by starting a development console with: diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index 0e747491e6..856dcffc5d 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -17,6 +17,7 @@ Snapshot: 2026-10-06, at the heads of the OAuth train, rebased onto main: | [#3160](https://github.com/basecamp/fizzy/pull/3160) | `oauth-refresh-replay` | Refresh-token replay detection (on #3082) | | [#3161](https://github.com/basecamp/fizzy/pull/3161) | `oauth-grant-idle-expiry` | 90-day idle expiry of grants (on #3160) | | [#3159](https://github.com/basecamp/fizzy/pull/3159) | `oauth-client-sweep` | Sweep of abandoned DCR clients (on #3161) | +| [#3192](https://github.com/basecamp/fizzy/pull/3192) | `oauth-token-digests` | Tokens and client secrets stored as digests, step 1 of 3 (on #3169) | Commits are cited by subject, with the short SHA as of the snapshot. Rebases change the SHAs of #3080 and above; the subjects stay the same. @@ -81,13 +82,14 @@ Status key: |---|---| | `client_secret_post` | **Conformant** (#3082) | | HTTP Basic (`client_secret_basic`) MUST be supported for clients issued a password; a failed Basic attempt gets 401 + `WWW-Authenticate` (§5.2) | **Fixed** #3082 `d65d0117c` "Accept client_secret_basic at the token endpoint". Either method authenticates any confidential client; both at once is `invalid_request`. DCR and metadata advertise it. bc3 closes the same gap in bc3#13678. | -| Secrets stored in plaintext, as access tokens are | **Deliberate** (#3082 body) | +| Client secrets are stored as digests, never in plaintext, and shown once, in the registration response | **Fixed** #3192 `oauth-token-digests` (was **Deliberate** plaintext, #3082 body). HMAC-SHA256 under a server pepper (`TokenDigester`), checked in constant time. **Deviation from bc3**, which stores client secrets with bcrypt (`has_secure_password :secret`): Fizzy only issues generated secrets, 257 bits of randomness (24-character secrets from before #3163, about 140), so a deliberately slow hash adds no guessing resistance that the entropy doesn't already give, and one keyed digest serves every credential. Without the pepper, a copy of the database gives nothing to test a guess against. | | An OAuth grant can't reach a credential or consent surface: personal access tokens, Connected Apps, passkey registration, transfer links, email changes or OAuth consent. Anything it minted there would outlive disconnecting the app, and anything it removed would belong to another app | **Fixed** #2296 `196b9b0b3` "Keep OAuth grants out of every credential and consent surface" (`Authentication.disallow_oauth_grants`, declared on each such controller). Personal access tokens may still manage personal access tokens (fizzy-cli) | ## Token format | Requirement | Status | |---|---| +| Tokens are stored as HMAC-SHA256 digests under a server pepper, byte-compatible with bc3's `Oauth::TokenDigester`, and looked up by a unique index on the digest. Raw values are shown once: in the token response, the registration response, or the response that creates a personal access token | **Fixed** #3192 `oauth-token-digests`, step 1 of 3. OAuth access and refresh tokens and client secrets are digest-only, and a refresh retry within the grace window is answered from an encrypted copy of the successor pair that expires with the window. Personal access tokens are digested and, until step 3, still also kept in plaintext, because production code that reads only plaintext shares the database while beta runs this. One written without a digest is found by plaintext once and digested. Step 2 stops reading and writing plaintext; step 3 drops the column. | | Newly issued tokens carry a distinctive prefix naming their kind: `fizzy_pat_` (personal access token), `fizzy_at_` (OAuth access token), `fizzy_rt_` (OAuth refresh token). Tokens issued earlier have no prefix and still work | **Fixed** #3163. bc3 prefixes similarly (`bc_at_`, `bc_rt_`, and PATs as `bc_at_`). Client secrets carry `fizzy_cs_`, as bc3's carry `bc_cs_`. New tokens and secrets carry 44 base58 characters after the prefix, 257 bits, at least bc3's 32 bytes (rubric TK-1); earlier ones are 24 characters and keep working. Nobody has registered with GitHub's secret-scanning partner program, either here or in bc3. | ## Refresh tokens (RFC 6749 §6, OAuth 2.1 §4.3, RFC 9700 §4.14) @@ -126,7 +128,7 @@ Status key: | Requirement | Status | |---|---| -| `iss` on success and error redirects, equal to the metadata issuer; `authorization_response_iss_parameter_supported: true` (mix-up defense, RFC 9700 §4.4) | **Fixed** #2296 `d468283d7`. A single `Oauth::BaseController#oauth_issuer` feeds both the redirect and the metadata. | +| `iss` on success and error redirects, equal to the metadata issuer; `authorization_response_iss_parameter_supported: true` (mix-up defense, RFC 9700 §4.4) | **Fixed** #2296 `d468283d7`. A single `Oauth::BaseController#oauth_issuer` feeds the redirect, the metadata and the protected resource's `authorization_servers`. It's the origin with no trailing slash (`https://app.fizzy.do`), as bc3's `canonical_issuer_url`, since clients compare issuers as exact strings (RFC 8414 §3.3); #3192 dropped the slash `root_url` gave it. | ## RFC 9700: open redirector diff --git a/lib/rails_ext/active_record_sql_redaction.rb b/lib/rails_ext/active_record_sql_redaction.rb index e003c05031..e6256974f9 100644 --- a/lib/rails_ext/active_record_sql_redaction.rb +++ b/lib/rails_ext/active_record_sql_redaction.rb @@ -11,8 +11,7 @@ # identity_access_tokens.token API bearer token # identity_transfers.token signs its holder in as the identity # magic_links.code signs its holder in as the identity -# oauth_clients.client_secret authenticates a confidential client -# oauth_retired_refresh_tokens a retired refresh token and the live one that succeeded it +# oauth_retired_refresh_tokens successor_payload holds a live successor token pair, encrypted # webhooks.signing_secret, .url signs payloads; a chat webhook URL is itself a credential # # Filtering by table rather than column keeps INSERT ... VALUES, multi-row inserts and IN lists covered @@ -23,7 +22,7 @@ # test/lib/rails_ext/active_record_sql_redaction_test.rb until its table is listed here or the column is # declared not secret there. A credential in a column named otherwise has to be listed by hand. module ActiveRecordSqlRedaction - TABLES = %w[ account_join_codes identity_access_tokens identity_transfers magic_links oauth_clients oauth_retired_refresh_tokens webhooks ].freeze + TABLES = %w[ account_join_codes identity_access_tokens identity_transfers magic_links oauth_retired_refresh_tokens webhooks ].freeze # Names a table however the statement spells it: `magic_links` on MySQL, "magic_links" on SQLite, or bare # in hand-written SQL, in any case. Over-matching only filters more. diff --git a/saas/.kamal/secrets.beta b/saas/.kamal/secrets.beta index 493d7aac31..67ad3efeaa 100644 --- a/saas/.kamal/secrets.beta +++ b/saas/.kamal/secrets.beta @@ -1,6 +1,6 @@ # Queenbee's API token comes from Production: beta talks to Queenbee beta (beta-billing), which runs on # Queenbee's production credentials and so knows only production's Fizzy token. -SECRETS=$(kamal secrets fetch --adapter 1password --account 23QPQDKZC5BKBIIG7UGT5GR5RM --from Deploy/Fizzy Deployments/BASECAMP_REGISTRY_PASSWORD Deployments/DASH_BASIC_AUTH_SECRET Beta/RAILS_MASTER_KEY Beta/MYSQL_ALTER_PASSWORD Beta/MYSQL_ALTER_USER Beta/MYSQL_APP_PASSWORD Beta/MYSQL_APP_USER Beta/MYSQL_READONLY_PASSWORD Beta/MYSQL_READONLY_USER Beta/SECRET_KEY_BASE Beta/VAPID_PUBLIC_KEY Beta/VAPID_PRIVATE_KEY Beta/ACTIVE_STORAGE_ACCESS_KEY_ID Beta/ACTIVE_STORAGE_SECRET_ACCESS_KEY Beta/ACTIVE_STORAGE_DIRECT_UPLOAD_ACCESS_KEY_ID Beta/ACTIVE_STORAGE_DIRECT_UPLOAD_SECRET_ACCESS_KEY Production/QUEENBEE_API_TOKEN Production/QUEENBEE_API_TOKEN_PREVIOUS Common/queenbee_secret Beta/SIGNAL_ID_SECRET Beta/SENTRY_DSN Beta/ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY Beta/ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY Beta/ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT Beta/STRIPE_MONTHLY_V1_PRICE_ID Beta/STRIPE_MONTHLY_EXTRA_STORAGE_V1_PRICE_ID Beta/STRIPE_SECRET_KEY Beta/STRIPE_WEBHOOK_SECRET Beta/APNS_KEY_ID Beta/APNS_ENCRYPTION_KEY_B64 Beta/FCM_ENCRYPTION_KEY_B64) +SECRETS=$(kamal secrets fetch --adapter 1password --account 23QPQDKZC5BKBIIG7UGT5GR5RM --from Deploy/Fizzy Deployments/BASECAMP_REGISTRY_PASSWORD Deployments/DASH_BASIC_AUTH_SECRET Beta/RAILS_MASTER_KEY Beta/MYSQL_ALTER_PASSWORD Beta/MYSQL_ALTER_USER Beta/MYSQL_APP_PASSWORD Beta/MYSQL_APP_USER Beta/MYSQL_READONLY_PASSWORD Beta/MYSQL_READONLY_USER Beta/SECRET_KEY_BASE Beta/VAPID_PUBLIC_KEY Beta/VAPID_PRIVATE_KEY Beta/ACTIVE_STORAGE_ACCESS_KEY_ID Beta/ACTIVE_STORAGE_SECRET_ACCESS_KEY Beta/ACTIVE_STORAGE_DIRECT_UPLOAD_ACCESS_KEY_ID Beta/ACTIVE_STORAGE_DIRECT_UPLOAD_SECRET_ACCESS_KEY Production/QUEENBEE_API_TOKEN Production/QUEENBEE_API_TOKEN_PREVIOUS Common/queenbee_secret Beta/SIGNAL_ID_SECRET Beta/SENTRY_DSN Beta/ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY Beta/ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY Beta/ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT Beta/STRIPE_MONTHLY_V1_PRICE_ID Beta/STRIPE_MONTHLY_EXTRA_STORAGE_V1_PRICE_ID Beta/STRIPE_SECRET_KEY Beta/STRIPE_WEBHOOK_SECRET Beta/APNS_KEY_ID Beta/APNS_ENCRYPTION_KEY_B64 Beta/FCM_ENCRYPTION_KEY_B64 Production/OAUTH_TOKEN_PEPPER) GITHUB_TOKEN=$(gh config get -h github.com oauth_token) BASECAMP_REGISTRY_PASSWORD=$(kamal secrets extract BASECAMP_REGISTRY_PASSWORD $SECRETS) @@ -13,6 +13,7 @@ MYSQL_APP_USER=$(kamal secrets extract MYSQL_APP_USER $SECRETS) MYSQL_READONLY_PASSWORD=$(kamal secrets extract MYSQL_READONLY_PASSWORD $SECRETS) MYSQL_READONLY_USER=$(kamal secrets extract MYSQL_READONLY_USER $SECRETS) SECRET_KEY_BASE=$(kamal secrets extract SECRET_KEY_BASE $SECRETS) +OAUTH_TOKEN_PEPPER=$(kamal secrets extract OAUTH_TOKEN_PEPPER $SECRETS) VAPID_PUBLIC_KEY=$(kamal secrets extract VAPID_PUBLIC_KEY $SECRETS) VAPID_PRIVATE_KEY=$(kamal secrets extract VAPID_PRIVATE_KEY $SECRETS) ACTIVE_STORAGE_ACCESS_KEY_ID=$(kamal secrets extract ACTIVE_STORAGE_ACCESS_KEY_ID $SECRETS) diff --git a/saas/.kamal/secrets.production b/saas/.kamal/secrets.production index 64745779b6..8a967e9b71 100644 --- a/saas/.kamal/secrets.production +++ b/saas/.kamal/secrets.production @@ -1,4 +1,4 @@ -SECRETS=$(kamal secrets fetch --adapter 1password --account 23QPQDKZC5BKBIIG7UGT5GR5RM --from Deploy/Fizzy Deployments/BASECAMP_REGISTRY_PASSWORD Deployments/DASH_BASIC_AUTH_SECRET Production/RAILS_MASTER_KEY Production/MYSQL_ALTER_PASSWORD Production/MYSQL_ALTER_USER Production/MYSQL_APP_PASSWORD Production/MYSQL_APP_USER Production/MYSQL_READONLY_PASSWORD Production/MYSQL_READONLY_USER Production/SECRET_KEY_BASE Production/VAPID_PUBLIC_KEY Production/VAPID_PRIVATE_KEY Production/ACTIVE_STORAGE_ACCESS_KEY_ID Production/ACTIVE_STORAGE_SECRET_ACCESS_KEY Production/ACTIVE_STORAGE_DIRECT_UPLOAD_ACCESS_KEY_ID Production/ACTIVE_STORAGE_DIRECT_UPLOAD_SECRET_ACCESS_KEY Production/QUEENBEE_API_TOKEN Production/QUEENBEE_API_TOKEN_PREVIOUS Common/queenbee_secret Production/SIGNAL_ID_SECRET Production/SENTRY_DSN Production/ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY Production/ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY Production/ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT Production/STRIPE_MONTHLY_V1_PRICE_ID Production/STRIPE_MONTHLY_EXTRA_STORAGE_V1_PRICE_ID Production/STRIPE_SECRET_KEY Production/STRIPE_WEBHOOK_SECRET Production/APNS_KEY_ID Production/APNS_ENCRYPTION_KEY_B64 Production/FCM_ENCRYPTION_KEY_B64 Production/FIZZY_REVIEWER_SIGN_IN_EMAIL Production/FIZZY_REVIEWER_SIGN_IN_CODE_DIGEST) +SECRETS=$(kamal secrets fetch --adapter 1password --account 23QPQDKZC5BKBIIG7UGT5GR5RM --from Deploy/Fizzy Deployments/BASECAMP_REGISTRY_PASSWORD Deployments/DASH_BASIC_AUTH_SECRET Production/RAILS_MASTER_KEY Production/MYSQL_ALTER_PASSWORD Production/MYSQL_ALTER_USER Production/MYSQL_APP_PASSWORD Production/MYSQL_APP_USER Production/MYSQL_READONLY_PASSWORD Production/MYSQL_READONLY_USER Production/SECRET_KEY_BASE Production/VAPID_PUBLIC_KEY Production/VAPID_PRIVATE_KEY Production/ACTIVE_STORAGE_ACCESS_KEY_ID Production/ACTIVE_STORAGE_SECRET_ACCESS_KEY Production/ACTIVE_STORAGE_DIRECT_UPLOAD_ACCESS_KEY_ID Production/ACTIVE_STORAGE_DIRECT_UPLOAD_SECRET_ACCESS_KEY Production/QUEENBEE_API_TOKEN Production/QUEENBEE_API_TOKEN_PREVIOUS Common/queenbee_secret Production/SIGNAL_ID_SECRET Production/SENTRY_DSN Production/ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY Production/ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY Production/ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT Production/STRIPE_MONTHLY_V1_PRICE_ID Production/STRIPE_MONTHLY_EXTRA_STORAGE_V1_PRICE_ID Production/STRIPE_SECRET_KEY Production/STRIPE_WEBHOOK_SECRET Production/APNS_KEY_ID Production/APNS_ENCRYPTION_KEY_B64 Production/FCM_ENCRYPTION_KEY_B64 Production/FIZZY_REVIEWER_SIGN_IN_EMAIL Production/FIZZY_REVIEWER_SIGN_IN_CODE_DIGEST Production/OAUTH_TOKEN_PEPPER) GITHUB_TOKEN=$(gh config get -h github.com oauth_token) BASECAMP_REGISTRY_PASSWORD=$(kamal secrets extract BASECAMP_REGISTRY_PASSWORD $SECRETS) @@ -11,6 +11,7 @@ MYSQL_APP_USER=$(kamal secrets extract MYSQL_APP_USER $SECRETS) MYSQL_READONLY_PASSWORD=$(kamal secrets extract MYSQL_READONLY_PASSWORD $SECRETS) MYSQL_READONLY_USER=$(kamal secrets extract MYSQL_READONLY_USER $SECRETS) SECRET_KEY_BASE=$(kamal secrets extract SECRET_KEY_BASE $SECRETS) +OAUTH_TOKEN_PEPPER=$(kamal secrets extract OAUTH_TOKEN_PEPPER $SECRETS) VAPID_PUBLIC_KEY=$(kamal secrets extract VAPID_PUBLIC_KEY $SECRETS) VAPID_PRIVATE_KEY=$(kamal secrets extract VAPID_PRIVATE_KEY $SECRETS) ACTIVE_STORAGE_ACCESS_KEY_ID=$(kamal secrets extract ACTIVE_STORAGE_ACCESS_KEY_ID $SECRETS) diff --git a/saas/.kamal/secrets.staging b/saas/.kamal/secrets.staging index 1e8cfca8c0..7dd9e8b312 100644 --- a/saas/.kamal/secrets.staging +++ b/saas/.kamal/secrets.staging @@ -1,4 +1,4 @@ -SECRETS=$(kamal secrets fetch --adapter 1password --account 23QPQDKZC5BKBIIG7UGT5GR5RM --from Deploy/Fizzy Deployments/BASECAMP_REGISTRY_PASSWORD Deployments/DASH_BASIC_AUTH_SECRET Staging/RAILS_MASTER_KEY Staging/MYSQL_ALTER_PASSWORD Staging/MYSQL_ALTER_USER Staging/MYSQL_APP_PASSWORD Staging/MYSQL_APP_USER Staging/MYSQL_READONLY_PASSWORD Staging/MYSQL_READONLY_USER Staging/SECRET_KEY_BASE Staging/VAPID_PUBLIC_KEY Staging/VAPID_PRIVATE_KEY Staging/ACTIVE_STORAGE_ACCESS_KEY_ID Staging/ACTIVE_STORAGE_SECRET_ACCESS_KEY Staging/ACTIVE_STORAGE_DIRECT_UPLOAD_ACCESS_KEY_ID Staging/ACTIVE_STORAGE_DIRECT_UPLOAD_SECRET_ACCESS_KEY Staging/QUEENBEE_API_TOKEN Staging/QUEENBEE_API_TOKEN_PREVIOUS Common/queenbee_secret Staging/SIGNAL_ID_SECRET Staging/SENTRY_DSN Staging/ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY Staging/ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY Staging/ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT Staging/STRIPE_MONTHLY_V1_PRICE_ID Staging/STRIPE_MONTHLY_EXTRA_STORAGE_V1_PRICE_ID Staging/STRIPE_SECRET_KEY Staging/STRIPE_WEBHOOK_SECRET) +SECRETS=$(kamal secrets fetch --adapter 1password --account 23QPQDKZC5BKBIIG7UGT5GR5RM --from Deploy/Fizzy Deployments/BASECAMP_REGISTRY_PASSWORD Deployments/DASH_BASIC_AUTH_SECRET Staging/RAILS_MASTER_KEY Staging/MYSQL_ALTER_PASSWORD Staging/MYSQL_ALTER_USER Staging/MYSQL_APP_PASSWORD Staging/MYSQL_APP_USER Staging/MYSQL_READONLY_PASSWORD Staging/MYSQL_READONLY_USER Staging/SECRET_KEY_BASE Staging/VAPID_PUBLIC_KEY Staging/VAPID_PRIVATE_KEY Staging/ACTIVE_STORAGE_ACCESS_KEY_ID Staging/ACTIVE_STORAGE_SECRET_ACCESS_KEY Staging/ACTIVE_STORAGE_DIRECT_UPLOAD_ACCESS_KEY_ID Staging/ACTIVE_STORAGE_DIRECT_UPLOAD_SECRET_ACCESS_KEY Staging/QUEENBEE_API_TOKEN Staging/QUEENBEE_API_TOKEN_PREVIOUS Common/queenbee_secret Staging/SIGNAL_ID_SECRET Staging/SENTRY_DSN Staging/ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY Staging/ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY Staging/ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT Staging/STRIPE_MONTHLY_V1_PRICE_ID Staging/STRIPE_MONTHLY_EXTRA_STORAGE_V1_PRICE_ID Staging/STRIPE_SECRET_KEY Staging/STRIPE_WEBHOOK_SECRET Staging/OAUTH_TOKEN_PEPPER) GITHUB_TOKEN=$(gh config get -h github.com oauth_token) BASECAMP_REGISTRY_PASSWORD=$(kamal secrets extract BASECAMP_REGISTRY_PASSWORD $SECRETS) @@ -11,6 +11,7 @@ MYSQL_APP_USER=$(kamal secrets extract MYSQL_APP_USER $SECRETS) MYSQL_READONLY_PASSWORD=$(kamal secrets extract MYSQL_READONLY_PASSWORD $SECRETS) MYSQL_READONLY_USER=$(kamal secrets extract MYSQL_READONLY_USER $SECRETS) SECRET_KEY_BASE=$(kamal secrets extract SECRET_KEY_BASE $SECRETS) +OAUTH_TOKEN_PEPPER=$(kamal secrets extract OAUTH_TOKEN_PEPPER $SECRETS) VAPID_PUBLIC_KEY=$(kamal secrets extract VAPID_PUBLIC_KEY $SECRETS) VAPID_PRIVATE_KEY=$(kamal secrets extract VAPID_PRIVATE_KEY $SECRETS) ACTIVE_STORAGE_ACCESS_KEY_ID=$(kamal secrets extract ACTIVE_STORAGE_ACCESS_KEY_ID $SECRETS) diff --git a/saas/config/deploy.beta.yml b/saas/config/deploy.beta.yml index 5ee20aaa02..2019c29e8b 100644 --- a/saas/config/deploy.beta.yml +++ b/saas/config/deploy.beta.yml @@ -65,6 +65,7 @@ env: - MYSQL_READONLY_PASSWORD - MYSQL_READONLY_USER - SECRET_KEY_BASE + - OAUTH_TOKEN_PEPPER - VAPID_PUBLIC_KEY - VAPID_PRIVATE_KEY - ACTIVE_STORAGE_ACCESS_KEY_ID diff --git a/saas/config/deploy.production.yml b/saas/config/deploy.production.yml index 74155fba40..7ad590b180 100644 --- a/saas/config/deploy.production.yml +++ b/saas/config/deploy.production.yml @@ -43,6 +43,7 @@ env: - MYSQL_READONLY_PASSWORD - MYSQL_READONLY_USER - SECRET_KEY_BASE + - OAUTH_TOKEN_PEPPER - VAPID_PUBLIC_KEY - VAPID_PRIVATE_KEY - ACTIVE_STORAGE_ACCESS_KEY_ID diff --git a/saas/config/deploy.staging.yml b/saas/config/deploy.staging.yml index edbd817b2a..6042ac05a9 100644 --- a/saas/config/deploy.staging.yml +++ b/saas/config/deploy.staging.yml @@ -43,6 +43,7 @@ env: - MYSQL_READONLY_PASSWORD - MYSQL_READONLY_USER - SECRET_KEY_BASE + - OAUTH_TOKEN_PEPPER - VAPID_PUBLIC_KEY - VAPID_PRIVATE_KEY - ACTIVE_STORAGE_ACCESS_KEY_ID diff --git a/test/controllers/api_test.rb b/test/controllers/api_test.rb index a0e1d943af..a10954b9b3 100644 --- a/test/controllers/api_test.rb +++ b/test/controllers/api_test.rb @@ -60,7 +60,7 @@ class ApiTest < ActionDispatch::IntegrationTest token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :write) travel Identity::AccessToken::EXPIRES_IN + 1.second do - get boards_path(format: :json), env: bearer_token_env(token.token) + get boards_path(format: :json), env: bearer_token_env(token.raw_token) end assert_response :unauthorized @@ -71,7 +71,7 @@ class ApiTest < ActionDispatch::IntegrationTest token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :write) with_oauth_availability acceptance: false do - get boards_path(format: :json), env: bearer_token_env(token.token) + get boards_path(format: :json), env: bearer_token_env(token.raw_token) end assert_response :unauthorized @@ -92,7 +92,7 @@ class ApiTest < ActionDispatch::IntegrationTest test "a read-only grant changing data is forbidden, so the app asks for write" do token = identities(:jason).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :read) - post boards_path(format: :json), params: { board: { name: "My new board" } }, env: bearer_token_env(token.token) + post boards_path(format: :json), params: { board: { name: "My new board" } }, env: bearer_token_env(token.raw_token) assert_response :forbidden assert_bearer_challenge error: "insufficient_scope" @@ -108,7 +108,7 @@ class ApiTest < ActionDispatch::IntegrationTest token = identities(:jason).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :read) travel Identity::AccessToken::EXPIRES_IN + 1.second do - post boards_path(format: :json), params: { board: { name: "My new board" } }, env: bearer_token_env(token.token) + post boards_path(format: :json), params: { board: { name: "My new board" } }, env: bearer_token_env(token.raw_token) end assert_response :unauthorized diff --git a/test/controllers/my/access_tokens_controller_test.rb b/test/controllers/my/access_tokens_controller_test.rb index 9bec86fad0..02764bf2b2 100644 --- a/test/controllers/my/access_tokens_controller_test.rb +++ b/test/controllers/my/access_tokens_controller_test.rb @@ -24,12 +24,41 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest get new_my_access_token_path assert_response :success + assert_select "meta[name=turbo-cache-control][content=no-cache]" assert_changes -> { identities(:kevin).access_tokens.count }, +1 do post my_access_tokens_path, params: { access_token: { description: "GitHub", permission: "read" } } - follow_redirect! - assert_in_body identities(:kevin).access_tokens.last.token end + + assert_response :created + assert_equal "no-store", response.headers["Cache-Control"] + assert_revealed identities(:kevin).access_tokens.order(:created_at).last + end + + test "create new token from the form reveals it in a turbo stream" do + assert_changes -> { identities(:kevin).access_tokens.count }, +1 do + post my_access_tokens_path, params: { access_token: { description: "GitHub", permission: "write" } }, as: :turbo_stream + end + + assert_response :success + assert_equal "no-store", response.headers["Cache-Control"] + assert_turbo_stream action: :replace, target: "new_access_token" + assert_revealed identities(:kevin).access_tokens.order(:created_at).last + end + + # Only the token's digest is ever looked up, so the response that created it + # is the one place it's shown. No page can show it again. + test "a created token is revealed once, and no page shows it again" do + post my_access_tokens_path, params: { access_token: { description: "GitHub", permission: "read" } } + raw_token = token_in_body + access_token = identities(:kevin).access_tokens.order(:created_at).last + + get "/my/access_tokens/#{access_token.id}" + assert_response :not_found + + get my_access_tokens_path + assert_response :success + assert_not_includes response.body, raw_token end test "create new token via JSON with session" do @@ -37,9 +66,10 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest post my_access_tokens_path, params: { access_token: { description: "Fizzy CLI", permission: "write" } }, as: :json end assert_response :created + assert_equal "no-store", response.headers["Cache-Control"] body = @response.parsed_body assert body["id"].present? - assert body["token"].present? + assert_equal TokenDigester.digest(body["token"]), Identity::AccessToken.find(body["id"]).token_digest assert_equal "Fizzy CLI", body["description"] assert_equal "write", body["permission"] assert body["created_at"].present? @@ -65,7 +95,7 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest test "an OAuth grant can't create, list or revoke personal access tokens" do sign_out grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :write) - bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{grant.token}" } + bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{grant.raw_token}" } assert_no_difference -> { identities(:david).access_tokens.count } do post my_access_tokens_path, params: { access_token: { description: "Escape hatch", permission: "write" } }, env: bearer_token, as: :json @@ -121,12 +151,14 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest assert_response :no_content end - test "accessing new token after reveal window redirects to index" do - assert_changes -> { identities(:kevin).access_tokens.count }, +1 do - post my_access_tokens_path, params: { access_token: { description: "GitHub", permission: "read" } } - travel_to 15.seconds.from_now - follow_redirect! - assert_equal "Token is no longer visible", flash[:alert] + private + def token_in_body + response.body[/value="(fizzy_pat_\w+)"/, 1] + end + + # Temporary, so Turbo's snapshot cache doesn't keep the token for Back to restore. + def assert_revealed(access_token) + assert_equal access_token.token_digest, TokenDigester.digest(token_in_body) + assert_match(/
]* data-turbo-temporary[ >]/, response.body) end - end end diff --git a/test/fixtures/identity/access_tokens.yml b/test/fixtures/identity/access_tokens.yml index cfbbb6248e..2c7cf7aa0d 100644 --- a/test/fixtures/identity/access_tokens.yml +++ b/test/fixtures/identity/access_tokens.yml @@ -1,11 +1,13 @@ jasons_api_token: identity: jason token: 018cf1425682700098f24f0799e3fe20 + token_digest: <%= TokenDigester.digest("018cf1425682700098f24f0799e3fe20") %> description: My Superscript permission: read davids_api_token: identity: david token: x18cf1425682700098f24f0799e3fe20 + token_digest: <%= TokenDigester.digest("x18cf1425682700098f24f0799e3fe20") %> description: My Superscript permission: write diff --git a/test/fixtures/oauth/clients.yml b/test/fixtures/oauth/clients.yml index 581f21494d..f2aaf446d7 100644 --- a/test/fixtures/oauth/clients.yml +++ b/test/fixtures/oauth/clients.yml @@ -22,7 +22,7 @@ trusted_client: confidential_client: client_id: confidential_client_789 - client_secret: confidential_secret_789 + client_secret_digest: <%= TokenDigester.digest("confidential_secret_789") %> name: Confidential Connector redirect_uris: - https://connector.example.com/callback diff --git a/test/integration/oauth_availability_test.rb b/test/integration/oauth_availability_test.rb index ce44af86ae..e77b1ff009 100644 --- a/test/integration/oauth_availability_test.rb +++ b/test/integration/oauth_availability_test.rb @@ -185,18 +185,19 @@ class OauthAvailabilityTest < ActionDispatch::IntegrationTest end assert_response :not_found - assert_equal token.refresh_token, token.reload.refresh_token + assert_equal token.refresh_token_digest, token.reload.refresh_token_digest end test "pausing issuance refuses refresh with 503 and leaves the grant untouched" do token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :read) + raw_token = token.raw_token with_oauth_availability acceptance: true, issuance: false do untenanted { post oauth_token_path, params: refresh_params(token), as: :json } assert_response :service_unavailable - assert_equal token.refresh_token, token.reload.refresh_token + assert_equal token.refresh_token_digest, token.reload.refresh_token_digest - get user_path(users(:david)), env: bearer(token.token), as: :json + get user_path(users(:david)), env: bearer(raw_token), as: :json assert_response :success end end @@ -210,7 +211,7 @@ class OauthAvailabilityTest < ActionDispatch::IntegrationTest end assert_response :success - assert_not_equal token.refresh_token, token.reload.refresh_token + assert_not_equal token.refresh_token_digest, token.reload.refresh_token_digest end test "a dark server refreshes no confidential grants, even with the right secret" do @@ -218,11 +219,11 @@ class OauthAvailabilityTest < ActionDispatch::IntegrationTest token = identities(:david).access_tokens.create!(oauth_client: client, permission: :read) with_oauth_availability acceptance: false do - untenanted { post oauth_token_path, params: refresh_params(token).merge(client_secret: client.client_secret) } + untenanted { post oauth_token_path, params: refresh_params(token).merge(client_secret: "confidential_secret_789") } end assert_response :not_found - assert_equal token.refresh_token, token.reload.refresh_token + assert_equal token.refresh_token_digest, token.reload.refresh_token_digest end test "a confidential pilot client still authenticates against a dark server" do @@ -234,7 +235,7 @@ class OauthAvailabilityTest < ActionDispatch::IntegrationTest end assert_client_authentication_failed basic: false - assert_equal token.refresh_token, token.reload.refresh_token + assert_equal token.refresh_token_digest, token.reload.refresh_token_digest end # Acceptance @@ -243,7 +244,7 @@ class OauthAvailabilityTest < ActionDispatch::IntegrationTest token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :read) with_oauth_availability acceptance: false do - get user_path(users(:david)), env: bearer(token.token), as: :json + get user_path(users(:david)), env: bearer(token.raw_token), as: :json end assert_response :unauthorized @@ -261,7 +262,7 @@ class OauthAvailabilityTest < ActionDispatch::IntegrationTest token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :read) with_oauth_availability acceptance: true, issuance: false do - get user_path(users(:david)), env: bearer(token.token), as: :json + get user_path(users(:david)), env: bearer(token.raw_token), as: :json end assert_response :success @@ -274,7 +275,7 @@ class OauthAvailabilityTest < ActionDispatch::IntegrationTest with_oauth_availability acceptance: false, issuance: false do assert_difference "Identity::AccessToken.count", -1 do - untenanted { post oauth_revocation_path, params: { token: token.token, client_id: token.oauth_client.client_id } } + untenanted { post oauth_revocation_path, params: { token: token.raw_token, client_id: token.oauth_client.client_id } } end end @@ -327,7 +328,7 @@ def token_params end def refresh_params(token) - { grant_type: "refresh_token", refresh_token: token.refresh_token, client_id: token.oauth_client.client_id } + { grant_type: "refresh_token", refresh_token: token.raw_refresh_token, client_id: token.oauth_client.client_id } end def bearer(token) diff --git a/test/integration/oauth_client_secret_basic_test.rb b/test/integration/oauth_client_secret_basic_test.rb index 64d60f8ab8..84fbbd8e72 100644 --- a/test/integration/oauth_client_secret_basic_test.rb +++ b/test/integration/oauth_client_secret_basic_test.rb @@ -19,8 +19,8 @@ class OauthClientSecretBasicTest < ActionDispatch::IntegrationTest refresh token, headers: basic(@client.client_id, @secret) assert_response :success - assert_not_equal token.refresh_token, token.reload.refresh_token - assert_equal token.refresh_token, response.parsed_body["refresh_token"] + assert_not_equal token.refresh_token_digest, token.reload.refresh_token_digest + assert_equal token.refresh_token_digest, TokenDigester.digest(response.parsed_body["refresh_token"]) end test "a confidential client redeems a code with HTTP Basic alone" do @@ -39,7 +39,7 @@ class OauthClientSecretBasicTest < ActionDispatch::IntegrationTest test "Basic credentials are form-urlencoded before they are encoded, so %3A and + decode after the split" do client = Oauth::Client.create! name: "Colon", redirect_uris: %w[ https://connector.example.com/callback ], - token_endpoint_auth_method: "client_secret_basic", client_secret: "pa:ss word+" + token_endpoint_auth_method: "client_secret_basic", raw_client_secret: "pa:ss word+" token = grant_for(client) refresh token, headers: basic(client.client_id, "pa:ss word+") @@ -73,7 +73,7 @@ class OauthClientSecretBasicTest < ActionDispatch::IntegrationTest assert_equal "invalid_request", response.parsed_body["error"] end - assert_equal token.refresh_token, token.reload.refresh_token + assert_equal token.refresh_token_digest, token.reload.refresh_token_digest end test "Basic naming one client while the body names another fails as invalid_client" do @@ -82,7 +82,7 @@ class OauthClientSecretBasicTest < ActionDispatch::IntegrationTest refresh token, params: { client_id: oauth_clients(:mcp_client).client_id }, headers: basic(@client.client_id, @secret) assert_client_authentication_failed - assert_equal token.refresh_token, token.reload.refresh_token + assert_equal token.refresh_token_digest, token.reload.refresh_token_digest end test "a malformed Basic header fails as invalid_client" do @@ -95,25 +95,25 @@ class OauthClientSecretBasicTest < ActionDispatch::IntegrationTest assert_client_authentication_failed header end - assert_equal token.refresh_token, token.reload.refresh_token + assert_equal token.refresh_token_digest, token.reload.refresh_token_digest end test "a bare Basic scheme is a failed attempt, not an absent one" do client = oauth_clients(:mcp_client) token = grant_for(client) - refresh_token = token.refresh_token + refresh_token_digest = token.refresh_token_digest personal = identities(:david).access_tokens.create!(description: "Personal") [ "Basic", "Basic ", "basic" ].each do |header| refresh token, params: { client_id: client.client_id }, headers: { "Authorization" => header } assert_client_authentication_failed header - untenanted { post oauth_revocation_path, params: { token: personal.token }, headers: { "Authorization" => header } } + untenanted { post oauth_revocation_path, params: { token: personal.raw_token }, headers: { "Authorization" => header } } assert_client_authentication_failed header end assert Identity::AccessToken.exists?(personal.id) - assert_equal refresh_token, token.reload.refresh_token + assert_equal refresh_token_digest, token.reload.refresh_token_digest end test "Basic naming a public client fails: a public client holds no password" do @@ -126,7 +126,7 @@ class OauthClientSecretBasicTest < ActionDispatch::IntegrationTest assert_client_authentication_failed end - assert_equal token.refresh_token, token.reload.refresh_token + assert_equal token.refresh_token_digest, token.reload.refresh_token_digest end test "a wrong Basic secret and an unknown Basic client get the same 401 and Basic challenge" do @@ -140,7 +140,7 @@ class OauthClientSecretBasicTest < ActionDispatch::IntegrationTest assert_client_authentication_failed assert_equal wrong_secret, unknown_client - assert_equal token.refresh_token, token.reload.refresh_token + assert_equal token.refresh_token_digest, token.reload.refresh_token_digest end test "a failed client_secret_post authentication is a 400 with no challenge, since the client never tried the header" do @@ -157,7 +157,7 @@ class OauthClientSecretBasicTest < ActionDispatch::IntegrationTest test "a failed client_secret_post beside an Authorization header of another scheme is a 400: Basic was never evaluated" do token = grant_for(@client) - refresh token, params: { client_id: @client.client_id, client_secret: "wrong" }, headers: { "Authorization" => "Bearer #{token.token}" } + refresh token, params: { client_id: @client.client_id, client_secret: "wrong" }, headers: { "Authorization" => "Bearer #{token.raw_token}" } assert_client_authentication_failed basic: false end @@ -206,7 +206,7 @@ def grant_for(client) def refresh(token, params: nil, headers: {}) params ||= headers.key?("Authorization") ? {} : { client_id: token.oauth_client.client_id } untenanted do - post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: token.refresh_token }.merge(params), headers: headers + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: token.raw_refresh_token }.merge(params), headers: headers end end diff --git a/test/integration/oauth_delegate_resources_test.rb b/test/integration/oauth_delegate_resources_test.rb index ee00099afe..48feb992e4 100644 --- a/test/integration/oauth_delegate_resources_test.rb +++ b/test/integration/oauth_delegate_resources_test.rb @@ -49,7 +49,7 @@ class OauthDelegateResourcesTest < ActionDispatch::IntegrationTest test "revoking the grant removes the app's push subscriptions" do subscribe_push bearer(@grant) - untenanted { post oauth_revocation_path, params: { token: @grant.token, client_id: @client.client_id } } + untenanted { post oauth_revocation_path, params: { token: @grant.raw_token, client_id: @client.client_id } } assert_response :success assert_not Identity::AccessToken.exists?(@grant.id) @@ -68,7 +68,7 @@ class OauthDelegateResourcesTest < ActionDispatch::IntegrationTest test "a replayed refresh token revoking the grant removes the app's push subscriptions" do subscribe_push bearer(@grant) - presented = @grant.refresh_token + presented = @grant.raw_refresh_token refresh presented assert_response :success @@ -184,7 +184,7 @@ class OauthDelegateResourcesTest < ActionDispatch::IntegrationTest subscribe_push bearer(@grant) webhook = create_webhook bearer(@grant) - untenanted { post oauth_revocation_path, params: { token: other_grant.token, client_id: other.client_id } } + untenanted { post oauth_revocation_path, params: { token: other_grant.raw_token, client_id: other.client_id } } assert_not Identity::AccessToken.exists?(other_grant.id) assert Push::Subscription.exists?(endpoint: ENDPOINT) @@ -329,7 +329,7 @@ class OauthDelegateResourcesTest < ActionDispatch::IntegrationTest private def bearer(token) - { "HTTP_AUTHORIZATION" => "Bearer #{token.token}" } + { "HTTP_AUTHORIZATION" => "Bearer #{token.raw_token}" } end def another_admins_grant diff --git a/test/integration/oauth_flow_test.rb b/test/integration/oauth_flow_test.rb index 4af1c0a05d..faff4eadbf 100644 --- a/test/integration/oauth_flow_test.rb +++ b/test/integration/oauth_flow_test.rb @@ -419,7 +419,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_response :success assert_equal "read write", response.parsed_body["scope"] - assert_equal "write", Identity::AccessToken.find_by(token: response.parsed_body["access_token"]).permission + assert_equal "write", Identity::AccessToken.find_by_raw_token(response.parsed_body["access_token"]).permission end test "authorization redirects carry the issuer from the metadata (RFC 9207)" do @@ -452,7 +452,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest query = Rack::Utils.parse_query(URI.parse(response.location).query) assert_equal "acme", query["tenant"] - assert_equal "http://www.example.com/", query["iss"] + assert_equal "http://www.example.com", query["iss"] end test "consent screen lets the form redirect to the validated loopback origin" do @@ -530,7 +530,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_equal "Bearer", body["token_type"] assert_equal "read", body["scope"] - token = Identity::AccessToken.find_by(token: body["access_token"]) + token = Identity::AccessToken.find_by_raw_token(body["access_token"]) assert_equal client, token.oauth_client assert_equal identity, token.identity assert_equal "read", token.permission @@ -763,7 +763,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest untenanted { post oauth_token_path, params: exchange, as: :json } assert_response :success first_token = response.parsed_body["access_token"] - assert Identity::AccessToken.exists?(token: first_token) + assert Identity::AccessToken.exists?(token_digest: TokenDigester.digest(first_token)) assert_difference "Identity::AccessToken.count", -1 do untenanted { post oauth_token_path, params: exchange, as: :json } @@ -771,7 +771,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_response :bad_request assert_equal "invalid_grant", response.parsed_body["error"] - assert_not Identity::AccessToken.exists?(token: first_token) + assert_not Identity::AccessToken.exists?(token_digest: TokenDigester.digest(first_token)) fresh_code = authorization_code_for(client, code_verifier: code_verifier) untenanted { post oauth_token_path, params: exchange.merge(code: fresh_code), as: :json } @@ -791,7 +791,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest untenanted { post oauth_token_path, params: exchange.merge(code_verifier: "wrong_verifier"), as: :json } assert_response :bad_request - assert Identity::AccessToken.exists?(token: first_token) + assert Identity::AccessToken.exists?(token_digest: TokenDigester.digest(first_token)) end @@ -998,7 +998,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest untenanted do post oauth_token_path, params: { grant_type: "refresh_token", - refresh_token: token.refresh_token, + refresh_token: token.raw_refresh_token, client_id: client.client_id }, as: :json end @@ -1008,7 +1008,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest untenanted do post oauth_token_path, params: { grant_type: "refresh_token", - refresh_token: token.refresh_token, + refresh_token: token.raw_refresh_token, client_id: client.client_id, client_secret: "confidential_secret_789" }, as: :json @@ -1052,7 +1052,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest token = identities(:david).access_tokens.create!(oauth_client: client) { { grant_type: "password" } => :bad_request, - { grant_type: "refresh_token", refresh_token: token.refresh_token, client_id: client.client_id, client_secret: "wrong" } => :bad_request, + { grant_type: "refresh_token", refresh_token: token.raw_refresh_token, client_id: client.client_id, client_secret: "wrong" } => :bad_request, { grant_type: "refresh_token", refresh_token: "bogus", client_id: oauth_clients(:mcp_client).client_id } => :bad_request }.each do |request, status| untenanted { post oauth_token_path, params: request, as: :json } @@ -1073,7 +1073,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest client = oauth_clients(:confidential_client) token = identities(:david).access_tokens.create!(oauth_client: client) - [ token.refresh_token, "not-a-refresh-token" ].each do |refresh_token| + [ token.raw_refresh_token, "not-a-refresh-token" ].each do |refresh_token| untenanted { post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: refresh_token, client_id: client.client_id, client_secret: "wrong" }, as: :json } assert_equal "invalid_client", response.parsed_body["error"], "wrong secret" @@ -1084,7 +1084,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_equal "invalid_request", response.parsed_body["error"], "no client_id" end - assert_equal token.refresh_token, token.reload.refresh_token + assert_equal token.refresh_token_digest, token.reload.refresh_token_digest end test "confidential client authentication does not reveal whether a code is valid" do @@ -1103,7 +1103,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest test "a grant issued to another client answers exactly as a dead one does" do live = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:confidential_client)) - answers = [ live.refresh_token, "no-such-refresh-token" ].map do |refresh_token| + answers = [ live.raw_refresh_token, "no-such-refresh-token" ].map do |refresh_token| untenanted do post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: refresh_token, client_id: oauth_clients(:mcp_client).client_id } end @@ -1130,7 +1130,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest untenanted do post oauth_token_path, params: { grant_type: "refresh_token", - refresh_token: token.refresh_token, + refresh_token: token.raw_refresh_token, client_secret: "confidential_secret_789" }, as: :json end @@ -1144,7 +1144,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest test "refresh grant rotates access and refresh tokens" do client = oauth_clients(:mcp_client) token = identities(:david).access_tokens.create!(oauth_client: client) - old_access_token, old_refresh_token = token.token, token.refresh_token + old_access_token, old_refresh_token = token.raw_token, token.raw_refresh_token assert_no_difference "Identity::AccessToken.count" do untenanted do @@ -1169,13 +1169,13 @@ class OauthFlowTest < ActionDispatch::IntegrationTest test "a grant issued before prefixes still authenticates, and refreshes into prefixed tokens" do client = oauth_clients(:mcp_client) grant = identities(:david).access_tokens.create!(oauth_client: client, - token: "legacyAccessToken0123456789", refresh_token: "legacyRefreshToken0123456789") + raw_token: "legacyAccessToken0123456789", raw_refresh_token: "legacyRefreshToken0123456789") - get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{grant.token}" }, as: :json + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{grant.raw_token}" }, as: :json assert_response :success untenanted do - post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: grant.refresh_token, client_id: client.client_id }, as: :json + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: grant.raw_refresh_token, client_id: client.client_id }, as: :json end assert_response :success @@ -1197,7 +1197,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest untenanted do post oauth_token_path, params: { grant_type: "refresh_token", - refresh_token: token.refresh_token, + refresh_token: token.raw_refresh_token, client_id: client.client_id }, as: :json end @@ -1209,7 +1209,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest test "refresh grant requires refresh_token and client_id" do client = oauth_clients(:mcp_client) token = identities(:david).access_tokens.create!(oauth_client: client) - complete = { grant_type: "refresh_token", refresh_token: token.refresh_token, client_id: client.client_id } + complete = { grant_type: "refresh_token", refresh_token: token.raw_refresh_token, client_id: client.client_id } %i[ refresh_token client_id ].each do |name| untenanted { post oauth_token_path, params: complete.except(name), as: :json } @@ -1219,13 +1219,13 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_match name.to_s, response.parsed_body["error_description"] end - assert_equal complete[:refresh_token], token.reload.refresh_token + assert_equal TokenDigester.digest(complete[:refresh_token]), token.reload.refresh_token_digest end test "refresh grant rejects non-string refresh_token and client_id as a malformed request" do client = oauth_clients(:mcp_client) token = identities(:david).access_tokens.create!(oauth_client: client) - complete = { grant_type: "refresh_token", refresh_token: token.refresh_token, client_id: client.client_id } + complete = { grant_type: "refresh_token", refresh_token: token.raw_refresh_token, client_id: client.client_id } %i[ refresh_token client_id ].each do |name| [ [ complete[name] ], { "value" => complete[name] }, 1 ].each do |malformed| @@ -1237,7 +1237,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest end end - assert_equal complete[:refresh_token], token.reload.refresh_token + assert_equal TokenDigester.digest(complete[:refresh_token]), token.reload.refresh_token_digest end test "a reused code revokes the grant it issued, refreshed or not" do @@ -1261,7 +1261,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_response :bad_request assert_equal "invalid_grant", response.parsed_body["error"] - assert_not Identity::AccessToken.exists?(token: refreshed["access_token"]) + assert_not Identity::AccessToken.exists?(token_digest: TokenDigester.digest(refreshed["access_token"])) untenanted do post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: refreshed["refresh_token"], client_id: client.client_id }, as: :json @@ -1277,7 +1277,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest untenanted do post oauth_token_path, params: { grant_type: "refresh_token", - refresh_token: token.refresh_token, + refresh_token: token.raw_refresh_token, client_id: client.client_id }, as: :json end @@ -1313,7 +1313,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest untenanted do post oauth_token_path, params: { grant_type: "refresh_token", - refresh_token: token.refresh_token, + refresh_token: token.raw_refresh_token, client_id: client.client_id, scope: "read" }, as: :json @@ -1321,13 +1321,13 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_response :success assert_equal "read", response.parsed_body["scope"] - assert_equal "read", Identity::AccessToken.find_by(token: response.parsed_body["access_token"]).permission + assert_equal "read", Identity::AccessToken.find_by_raw_token(response.parsed_body["access_token"]).permission end test "refresh grant rejects a scope broader than the original grant" do client = oauth_clients(:mcp_client) token = identities(:david).access_tokens.create!(oauth_client: client, permission: :read) - old_refresh_token = token.refresh_token + old_refresh_token = token.raw_refresh_token untenanted do post oauth_token_path, params: { @@ -1340,13 +1340,13 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_response :bad_request assert_equal "invalid_scope", response.parsed_body["error"] - assert_equal old_refresh_token, token.reload.refresh_token + assert_equal TokenDigester.digest(old_refresh_token), token.reload.refresh_token_digest end test "refresh grant rejects a blank scope rather than restoring the full grant" do client = oauth_clients(:mcp_client) token = identities(:david).access_tokens.create!(oauth_client: client, permission: :write) - old_refresh_token = token.refresh_token + old_refresh_token = token.raw_refresh_token untenanted do post oauth_token_path, params: { @@ -1359,13 +1359,13 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_response :bad_request assert_equal "invalid_scope", response.parsed_body["error"] - assert_equal old_refresh_token, token.reload.refresh_token + assert_equal TokenDigester.digest(old_refresh_token), token.reload.refresh_token_digest end test "refresh grant rejects an explicit null scope rather than restoring the full grant" do client = oauth_clients(:mcp_client) token = identities(:david).access_tokens.create!(oauth_client: client, permission: :write) - old_refresh_token = token.refresh_token + old_refresh_token = token.raw_refresh_token untenanted do post oauth_token_path, params: { @@ -1378,7 +1378,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_response :bad_request assert_equal "invalid_scope", response.parsed_body["error"] - assert_equal old_refresh_token, token.reload.refresh_token + assert_equal TokenDigester.digest(old_refresh_token), token.reload.refresh_token_digest end test "refresh grant works after the access token expires" do @@ -1389,20 +1389,20 @@ class OauthFlowTest < ActionDispatch::IntegrationTest untenanted do post oauth_token_path, params: { grant_type: "refresh_token", - refresh_token: token.refresh_token, + refresh_token: token.raw_refresh_token, client_id: client.client_id }, as: :json end assert_response :success - assert_not Identity::AccessToken.find_by(token: response.parsed_body["access_token"]).expired? + assert_not Identity::AccessToken.find_by_raw_token(response.parsed_body["access_token"]).expired? end end test "refresh grant invalidates the previous refresh token" do client = oauth_clients(:mcp_client) token = identities(:david).access_tokens.create!(oauth_client: client) - old_refresh_token = token.refresh_token + old_refresh_token = token.raw_refresh_token untenanted do post oauth_token_path, params: { @@ -1432,7 +1432,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest untenanted do post oauth_token_path, params: { grant_type: "refresh_token", - refresh_token: token.refresh_token, + refresh_token: token.raw_refresh_token, client_id: oauth_clients(:trusted_client).client_id }, as: :json end @@ -1475,7 +1475,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_difference "Identity::AccessToken.count", -1 do untenanted do - post oauth_revocation_path, params: { token: token.token, client_id: token.oauth_client.client_id }, as: :json + post oauth_revocation_path, params: { token: token.raw_token, client_id: token.oauth_client.client_id }, as: :json end end @@ -1487,7 +1487,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_difference "Identity::AccessToken.count", -1 do untenanted do - post oauth_revocation_path, params: { token: token.refresh_token, client_id: token.oauth_client.client_id }, as: :json + post oauth_revocation_path, params: { token: token.raw_refresh_token, client_id: token.oauth_client.client_id }, as: :json end end @@ -1534,7 +1534,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_response :success body = response.parsed_body - assert_equal "http://www.example.com/", body["issuer"] + assert_equal "http://www.example.com", body["issuer"], "the issuer is the origin, with no trailing slash" assert_match %r{/oauth/authorization/new$}, body["authorization_endpoint"] assert_match %r{/oauth/token$}, body["token_endpoint"] assert_match %r{/oauth/clients$}, body["registration_endpoint"] @@ -1558,7 +1558,15 @@ class OauthFlowTest < ActionDispatch::IntegrationTest body = response.parsed_body assert_equal "http://www.example.com/", body["resource"] - assert_includes body["authorization_servers"], "http://www.example.com/" + assert_equal [ "http://www.example.com" ], body["authorization_servers"] + end + + test "the protected resource names exactly the issuer the authorization server advertises" do + untenanted { get "/.well-known/oauth-authorization-server" } + issuer = response.parsed_body["issuer"] + + untenanted { get "/.well-known/oauth-protected-resource" } + assert_equal [ issuer ], response.parsed_body["authorization_servers"] end @@ -1823,6 +1831,34 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert Oauth::Client.find_by(client_id: body["client_id"]).confidential? end + # The registration response is the only place a client secret is shown. It's + # stored as a digest, which verifies it. + test "a registered client secret is stored digested and authenticates its client" do + untenanted do + post oauth_clients_path, params: { + client_name: "Hosted Connector", + redirect_uris: [ "https://connector.example.com/callback" ], + token_endpoint_auth_method: "client_secret_basic" + }, as: :json + end + + secret = response.parsed_body["client_secret"] + client = Oauth::Client.find_by(client_id: response.parsed_body["client_id"]) + + assert_equal TokenDigester.digest(secret), client.client_secret_digest + assert_not_includes client.attributes.except("raw_client_secret").values.compact.map(&:to_s), secret + assert_nil client.raw_client_secret + assert client.authenticate_secret(secret) + assert_not client.authenticate_secret(secret.succ) + + grant = identities(:david).access_tokens.create!(oauth_client: client) + untenanted do + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: grant.raw_refresh_token }, + headers: { "Authorization" => "Basic #{Base64.strict_encode64("#{client.client_id}:#{secret}")}" } + end + assert_response :success + end + test "DCR omits client_secret for public clients" do untenanted do post oauth_clients_path, params: { diff --git a/test/integration/oauth_grant_idle_expiry_test.rb b/test/integration/oauth_grant_idle_expiry_test.rb index 3ab8e96de1..7da101ab4d 100644 --- a/test/integration/oauth_grant_idle_expiry_test.rb +++ b/test/integration/oauth_grant_idle_expiry_test.rb @@ -12,14 +12,14 @@ class OauthGrantIdleExpiryTest < ActionDispatch::IntegrationTest test "a grant refreshes up to the end of the idle window" do later_by Identity::AccessToken::REFRESH_IDLE_LIMIT - refresh @grant.refresh_token + refresh @grant.raw_refresh_token assert_response :success end test "a grant idle past the window lapses" do later_by Identity::AccessToken::REFRESH_IDLE_LIMIT + 1.second - refresh @grant.refresh_token + refresh @grant.raw_refresh_token assert_response :bad_request assert_equal "invalid_grant", response.parsed_body["error"] @@ -27,7 +27,7 @@ class OauthGrantIdleExpiryTest < ActionDispatch::IntegrationTest test "each refresh restarts the idle clock" do later_by Identity::AccessToken::REFRESH_IDLE_LIMIT - 1.day - refresh @grant.refresh_token + refresh @grant.raw_refresh_token assert_response :success later_by Identity::AccessToken::REFRESH_IDLE_LIMIT - 1.day @@ -76,16 +76,16 @@ class OauthGrantIdleExpiryTest < ActionDispatch::IntegrationTest test "rotation itself refuses a lapsed grant, so one racing the deadline can't renew it" do later_by Identity::AccessToken::REFRESH_IDLE_LIMIT + 1.second - presented = @grant.refresh_token + presented = @grant.raw_refresh_token assert_nil @grant.refresh - assert_equal presented, @grant.reload.refresh_token + assert_equal TokenDigester.digest(presented), @grant.reload.refresh_token_digest end test "the idle clock is the grant's last rotation, so a rotation by older code mid-deploy restarts it too" do later_by Identity::AccessToken::REFRESH_IDLE_LIMIT - 1.day rotated_by_older_code = Identity::AccessToken.generate_unique_secure_token - Identity::AccessToken.where(id: @grant.id).update_all(refresh_token: rotated_by_older_code, updated_at: Time.current) + Identity::AccessToken.where(id: @grant.id).update_all(refresh_token_digest: TokenDigester.digest(rotated_by_older_code), updated_at: Time.current) later_by Identity::AccessToken::REFRESH_IDLE_LIMIT - 1.day refresh rotated_by_older_code diff --git a/test/integration/oauth_grant_reach_test.rb b/test/integration/oauth_grant_reach_test.rb index bf60286619..e301c68d99 100644 --- a/test/integration/oauth_grant_reach_test.rb +++ b/test/integration/oauth_grant_reach_test.rb @@ -6,7 +6,7 @@ class OauthGrantReachTest < ActionDispatch::IntegrationTest setup do @grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :write) - @bearer = { "HTTP_AUTHORIZATION" => "Bearer #{@grant.token}" } + @bearer = { "HTTP_AUTHORIZATION" => "Bearer #{@grant.raw_token}" } end test "an OAuth grant can't consent to an app" do @@ -105,7 +105,7 @@ class OauthGrantReachTest < ActionDispatch::IntegrationTest test "a personal access token still reads the join code and webhook credentials" do webhook = webhooks(:active) - pat = { "HTTP_AUTHORIZATION" => "Bearer #{identities(:kevin).access_tokens.create!(permission: :read).token}" } + pat = { "HTTP_AUTHORIZATION" => "Bearer #{identities(:kevin).access_tokens.create!(permission: :read).raw_token}" } get account_join_code_path, env: pat, as: :json assert_response :success @@ -127,7 +127,7 @@ class OauthGrantReachTest < ActionDispatch::IntegrationTest private def admin_grant - token = identities(:kevin).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :write).token + token = identities(:kevin).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :write).raw_token { "HTTP_AUTHORIZATION" => "Bearer #{token}" } end diff --git a/test/integration/oauth_refresh_replay_test.rb b/test/integration/oauth_refresh_replay_test.rb index 66a28ef7ae..d87ab0760f 100644 --- a/test/integration/oauth_refresh_replay_test.rb +++ b/test/integration/oauth_refresh_replay_test.rb @@ -11,7 +11,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "a retry within the grace window gets the same successor, and rotates nothing" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token refresh presented first = response.parsed_body @@ -20,11 +20,26 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest assert_response :success assert_equal first.slice("access_token", "refresh_token", "scope"), response.parsed_body.slice("access_token", "refresh_token", "scope") - assert_equal first["refresh_token"], @grant.reload.refresh_token + assert_equal TokenDigester.digest(first["refresh_token"]), @grant.reload.refresh_token_digest + end + + test "a retry within the grace window whose successor pair is gone is refused, without revoking" do + presented = @grant.raw_refresh_token + refresh presented + successor = response.parsed_body + Oauth::RetiredRefreshToken.update_all successor_payload: nil + + travel 1.second + refresh presented + + assert_response :bad_request + assert_equal "invalid_grant", response.parsed_body["error"] + assert Identity::AccessToken.exists?(@grant.id) + assert_equal TokenDigester.digest(successor["refresh_token"]), @grant.reload.refresh_token_digest end test "a replay at the end of the grace window revokes the grant" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token refresh presented successor = response.parsed_body @@ -37,7 +52,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "a replay long after rotation revokes the grant" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token refresh presented successor = response.parsed_body @@ -50,7 +65,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "a replay of an older ancestor revokes the grant" do - ancestor = @grant.refresh_token + ancestor = @grant.raw_refresh_token refresh ancestor travel 1.hour refresh response.parsed_body["refresh_token"] @@ -64,7 +79,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "a retry within the grace window after the successor has itself rotated is refused, without revoking" do - ancestor = @grant.refresh_token + ancestor = @grant.raw_refresh_token refresh ancestor travel 1.second refresh response.parsed_body["refresh_token"] @@ -76,11 +91,11 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest assert_response :bad_request assert_equal "invalid_grant", response.parsed_body["error"] assert Identity::AccessToken.exists?(@grant.id) - assert_equal successor["refresh_token"], @grant.reload.refresh_token + assert_equal TokenDigester.digest(successor["refresh_token"]), @grant.reload.refresh_token_digest end test "a superseded retry within the grace window is refused as superseded, whatever scope it asks for" do - ancestor = @grant.refresh_token + ancestor = @grant.raw_refresh_token refresh ancestor travel 1.second refresh response.parsed_body["refresh_token"] @@ -99,7 +114,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "a replay asking for a scope beyond the grant still revokes it" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token refresh presented successor = response.parsed_body @@ -114,7 +129,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "a retry within the grace window weighs its scope as a live refresh would" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token refresh presented successor = response.parsed_body @@ -127,12 +142,12 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest assert_equal "invalid_scope", response.parsed_body["error"], scope.inspect end - assert_equal successor["refresh_token"], @grant.reload.refresh_token + assert_equal TokenDigester.digest(successor["refresh_token"]), @grant.reload.refresh_token_digest end test "a retry within the grace window must ask for the scope the rotation produced" do @grant.update! permission: :write - presented = @grant.refresh_token + presented = @grant.raw_refresh_token refresh presented successor = response.parsed_body assert_equal "read write", successor["scope"] @@ -151,7 +166,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "a successor rotated by a host whose clock runs behind still supersedes the retry" do - ancestor = @grant.refresh_token + ancestor = @grant.raw_refresh_token refresh ancestor travel(-5.seconds) refresh response.parsed_body["refresh_token"] @@ -162,11 +177,11 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest assert_response :bad_request assert_equal "invalid_grant", response.parsed_body["error"] - assert_equal successor["refresh_token"], @grant.reload.refresh_token + assert_equal TokenDigester.digest(successor["refresh_token"]), @grant.reload.refresh_token_digest end test "a replay whose grant vanished mid-request is refused, not an error" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token loser = Identity::AccessToken.find(@grant.id) refresh presented Identity::AccessToken.where(id: @grant.id).delete_all @@ -179,7 +194,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "another client replaying a rotated token is refused, without revoking" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token refresh presented travel 1.day @@ -191,7 +206,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "a refresh that loses a concurrent rotation gets the winner's successor" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token loser = Identity::AccessToken.find(@grant.id) refresh presented @@ -205,7 +220,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "revoking a rotated refresh token revokes the grant" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token refresh presented successor = response.parsed_body @@ -216,7 +231,7 @@ class OauthRefreshReplayTest < ActionDispatch::IntegrationTest end test "another client cannot revoke through a rotated refresh token" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token refresh presented untenanted { post oauth_revocation_path, params: { token: presented, client_id: oauth_clients(:trusted_client).client_id } } diff --git a/test/integration/oauth_revocation_test.rb b/test/integration/oauth_revocation_test.rb index ad7ef680dc..a73aba60fa 100644 --- a/test/integration/oauth_revocation_test.rb +++ b/test/integration/oauth_revocation_test.rb @@ -16,7 +16,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest token = grant_for(@client) [ { client_id: @client.client_id }, { client_id: @client.client_id, client_secret: "wrong" } ].each do |credentials| - revoke token.token, params: credentials + revoke token.raw_token, params: credentials assert_client_authentication_failed credentials.inspect, basic: false end @@ -28,7 +28,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest [ [ { client_id: @client.client_id, client_secret: @secret }, {} ], [ {}, basic(@client.client_id, @secret) ] ].each do |params, headers| token = grant_for(@client) - revoke token.refresh_token, params: params, headers: headers + revoke token.raw_refresh_token, params: params, headers: headers assert_response :success assert_not Identity::AccessToken.exists?(token.id) @@ -38,7 +38,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest test "an authenticated confidential client cannot revoke another client's token, and is told nothing" do token = grant_for(oauth_clients(:mcp_client)) - [ token.token, token.refresh_token ].each do |presented| + [ token.raw_token, token.raw_refresh_token ].each do |presented| revoke presented, headers: basic(@client.client_id, @secret) assert_response :success @@ -50,7 +50,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest test "a public client revokes its own token by naming its client_id" do client = oauth_clients(:mcp_client) - [ :token, :refresh_token ].each do |kind| + [ :raw_token, :raw_refresh_token ].each do |kind| token = grant_for(client) revoke token.public_send(kind), params: { client_id: client.client_id } @@ -64,7 +64,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest token = grant_for(oauth_clients(:mcp_client)) [ {}, { client_id: oauth_clients(:trusted_client).client_id }, { client_id: "no-such-client" } ].each do |params| - revoke token.token, params: params + revoke token.raw_token, params: params assert_response :success, params.inspect end @@ -87,7 +87,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest [ {}, { client_id: oauth_clients(:mcp_client).client_id }, { client_id: "no-such-client" } ].each do |params| token = personal_token - revoke token.token, params: params + revoke token.raw_token, params: params assert_response :success, params.inspect assert_not Identity::AccessToken.exists?(token.id), params.inspect @@ -98,7 +98,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest [ [ { client_id: @client.client_id, client_secret: @secret }, {} ], [ {}, basic(@client.client_id, @secret) ] ].each do |params, headers| token = personal_token - revoke token.token, params: params, headers: headers + revoke token.raw_token, params: params, headers: headers assert_response :success assert_not Identity::AccessToken.exists?(token.id) @@ -111,7 +111,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest [ [ { client_id: @client.client_id }, {}, false ], [ { client_id: @client.client_id, client_secret: "wrong" }, {}, false ], [ { client_secret: @secret }, {}, false ], [ { client_id: oauth_clients(:mcp_client).client_id, client_secret: "anything" }, {}, false ], [ {}, basic(@client.client_id, "wrong"), true ], [ {}, basic("no-such-client", "x"), true ] ].each do |params, headers, via_basic| - revoke token.token, params: params, headers: headers + revoke token.raw_token, params: params, headers: headers assert_client_authentication_failed [ params, headers ].inspect, basic: via_basic end @@ -122,7 +122,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest test "a personal access token is not revoked by two client authentication methods at once" do token = personal_token - revoke token.token, params: { client_secret: @secret }, headers: basic(@client.client_id, @secret) + revoke token.raw_token, params: { client_secret: @secret }, headers: basic(@client.client_id, @secret) assert_response :bad_request assert Identity::AccessToken.exists?(token.id) @@ -131,10 +131,10 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest test "tokens issued before prefixes revoke as before" do personal = identities(:david).access_tokens.create!(permission: :read, token: "legacyPersonalToken0123456789") grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), - token: "legacyAccessToken0123456789", refresh_token: "legacyRefreshToken0123456789") + raw_token: "legacyAccessToken0123456789", raw_refresh_token: "legacyRefreshToken0123456789") - revoke personal.token - revoke grant.refresh_token, params: { client_id: oauth_clients(:mcp_client).client_id } + revoke personal.raw_token + revoke grant.raw_refresh_token, params: { client_id: oauth_clients(:mcp_client).client_id } assert_not Identity::AccessToken.exists?(personal.id) assert_not Identity::AccessToken.exists?(grant.id) @@ -142,12 +142,12 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest test "a revoked personal access token no longer authenticates" do token = personal_token - get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{token.token}" }, as: :json + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{token.raw_token}" }, as: :json assert_response :success - revoke token.token + revoke token.raw_token - get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{token.token}" }, as: :json + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{token.raw_token}" }, as: :json assert_response :unauthorized end @@ -155,7 +155,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest token = grant_for(oauth_clients(:mcp_client)) [ basic("no-such-client", "x"), basic(@client.client_id, "wrong"), basic(oauth_clients(:mcp_client).client_id, "") ].each do |headers| - [ token.token, "not-a-token" ].each do |presented| + [ token.raw_token, "not-a-token" ].each do |presented| revoke presented, headers: headers assert_client_authentication_failed presented @@ -170,7 +170,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest [ { client_secret: @secret }, { client_id: "no-such-client", client_secret: @secret }, { client_id: oauth_clients(:mcp_client).client_id, client_secret: "anything" } ].each do |params| - [ token.token, "not-a-token" ].each do |presented| + [ token.raw_token, "not-a-token" ].each do |presented| revoke presented, params: params assert_client_authentication_failed params.inspect, basic: false @@ -183,7 +183,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest test "a failed body authentication beside an Authorization header of another scheme is a 400: Basic was never evaluated" do token = grant_for(@client) - revoke token.token, params: { client_id: @client.client_id, client_secret: "wrong" }, headers: { "Authorization" => "Bearer #{token.token}" } + revoke token.raw_token, params: { client_id: @client.client_id, client_secret: "wrong" }, headers: { "Authorization" => "Bearer #{token.raw_token}" } assert_client_authentication_failed basic: false assert Identity::AccessToken.exists?(token.id) @@ -192,7 +192,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest test "revocation refuses Basic beside a body client_secret" do token = grant_for(@client) - revoke token.token, params: { client_secret: @secret }, headers: basic(@client.client_id, @secret) + revoke token.raw_token, params: { client_secret: @secret }, headers: basic(@client.client_id, @secret) assert_response :bad_request assert_equal "invalid_request", response.parsed_body["error"] diff --git a/test/integration/oauth_single_valued_parameters_test.rb b/test/integration/oauth_single_valued_parameters_test.rb new file mode 100644 index 0000000000..11c2f35071 --- /dev/null +++ b/test/integration/oauth_single_valued_parameters_test.rb @@ -0,0 +1,166 @@ +require "test_helper" + +# RFC 6749 §3.1: request parameters MUST NOT be included more than once. Rails +# parses a name[]= form field or a JSON array or object into an Array or a +# nested Parameters, so each endpoint refuses that shape before any lookup. +class OauthSingleValuedParametersTest < ActionDispatch::IntegrationTest + CHALLENGE = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" + + setup do + @client = oauth_clients(:mcp_client) + end + + # Authorization endpoint + + test "an array client_id is refused before it becomes an IN list" do + sign_in_as :david + + assert_no_queries_match(/oauth_clients/) do + untenanted { get new_oauth_authorization_path, params: authorize_params(client_id: [ @client.client_id ]) } + end + + assert_response :bad_request + assert_select "code", text: "invalid_request" + assert_match "client_id", response.body + end + + test "an array redirect_uri is shown an error, never redirected to" do + sign_in_as :david + + untenanted { get new_oauth_authorization_path, params: authorize_params(redirect_uri: [ "http://127.0.0.1:8888/callback" ]) } + + assert_response :bad_request + assert_select "code", text: "invalid_request" + assert_nil response.location + end + + test "an array or object in any other authorization parameter goes back to a vetted client as invalid_request" do + sign_in_as :david + + { scope: [ "read" ], response_type: [ "code" ], code_challenge: [ CHALLENGE ], code_challenge_method: [ "S256" ], state: { a: "b" } }.each do |name, value| + untenanted { get new_oauth_authorization_path, params: authorize_params(name => value) } + + assert_response :redirect, name.to_s + query = URI.decode_www_form(URI.parse(response.location).query).to_h + assert_equal "invalid_request", query["error"], name.to_s + assert_match name.to_s, query["error_description"] + assert_not query.key?("code") + end + end + + test "an array in another authorization parameter from an unvetted client is shown here" do + sign_in_as :david + @client = Oauth::Client.create!(name: "Hosted", redirect_uris: %w[ https://connector.example.com/callback ], dynamically_registered: true) + + untenanted { get new_oauth_authorization_path, params: authorize_params(redirect_uri: "https://connector.example.com/callback", scope: [ "read" ]) } + + assert_response :bad_request + assert_select "code", text: "invalid_request" + end + + test "the consent POST refuses an array client_id" do + sign_in_as :david + + assert_no_queries_match(/oauth_clients/) do + untenanted { post oauth_authorization_path, params: authorize_params(client_id: [ @client.client_id ]) } + end + + assert_response :bad_request + assert_select "code", text: "invalid_request" + end + + test "a dark server still answers 404 to a malformed request" do + with_oauth_acceptance(false) do + untenanted { get new_oauth_authorization_path, params: authorize_params(client_id: [ @client.client_id ]) } + end + + assert_response :not_found + end + + # Token endpoint + + test "the token endpoint refuses an array client_id before any lookup" do + assert_no_queries_match(/oauth_clients|identity_access_tokens/) do + untenanted do + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: "fizzy_rt_x", client_id: [ @client.client_id ] }, as: :json + end + end + + assert_response :bad_request + assert_equal "invalid_request", response.parsed_body["error"] + end + + test "the token endpoint refuses an array or object in any single-valued parameter" do + [ + { grant_type: [ "refresh_token" ], refresh_token: "fizzy_rt_x", client_id: @client.client_id }, + { grant_type: "refresh_token", refresh_token: [ "fizzy_rt_x" ], client_id: @client.client_id }, + { grant_type: "refresh_token", refresh_token: "fizzy_rt_x", client_id: @client.client_id, scope: [ "read" ] }, + { grant_type: "authorization_code", code: { a: "b" }, code_verifier: "v", redirect_uri: "http://127.0.0.1:8888/callback", client_id: @client.client_id } + ].each do |body| + untenanted { post oauth_token_path, params: body, as: :json } + + assert_response :bad_request, body.inspect + assert_equal "invalid_request", response.parsed_body["error"], body.inspect + end + end + + test "a refresh's JSON null scope still means no scope, not a malformed one" do + untenanted do + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: "fizzy_rt_unknown", client_id: @client.client_id, scope: nil }, as: :json + end + + assert_equal "invalid_grant", response.parsed_body["error"], response.body + end + + # Revocation endpoint + + test "revocation refuses an array client_id or token_type_hint before any lookup" do + [ { token: "x", client_id: [ @client.client_id ] }, { token: "x", token_type_hint: [ "access_token" ] } ].each do |body| + assert_no_queries_match(/oauth_clients|identity_access_tokens/) do + untenanted { post oauth_revocation_path, params: body, as: :json } + end + + assert_response :bad_request, body.inspect + assert_equal "invalid_request", response.parsed_body["error"], body.inspect + end + end + + # Dynamic client registration + + test "registration refuses an array in a single-valued parameter, and still takes scope as a list" do + untenanted do + post oauth_clients_path, params: { redirect_uris: [ "http://127.0.0.1:9999/cb" ], client_id: [ "chosen" ] }, as: :json + end + + assert_response :bad_request + assert_equal "invalid_client_metadata", response.parsed_body["error"] + + untenanted do + post oauth_clients_path, params: { redirect_uris: [ "http://127.0.0.1:9999/cb" ], scope: [ "read" ] }, as: :json + end + + assert_response :created + assert_equal "read", response.parsed_body["scope"] + end + + private + def authorize_params(**overrides) + { + client_id: @client.client_id, + redirect_uri: "http://127.0.0.1:8888/callback", + response_type: "code", + code_challenge: CHALLENGE, + code_challenge_method: "S256", + scope: "read", + state: "xyz" + }.merge(overrides) + end + + def with_oauth_acceptance(enabled) + previous = Rails.configuration.x.oauth.acceptance_enabled + Rails.configuration.x.oauth.acceptance_enabled = enabled + yield + ensure + Rails.configuration.x.oauth.acceptance_enabled = previous + end +end diff --git a/test/lib/rails_ext/active_record_sql_redaction_test.rb b/test/lib/rails_ext/active_record_sql_redaction_test.rb index ed914a5029..530635149e 100644 --- a/test/lib/rails_ext/active_record_sql_redaction_test.rb +++ b/test/lib/rails_ext/active_record_sql_redaction_test.rb @@ -8,6 +8,8 @@ class ActiveRecordSqlRedactionTest < ActiveSupport::TestCase TOKEN = "madeUpAccessToken0000000" TRANSFER_TOKEN = "madeUpTransferToken00000" SECRET = "madeUpSigningSecret00000" + RETIRED_DIGEST = "madeUpRetiredRefreshTokenDigest000000000" + SUCCESSOR_DIGEST = "madeUpSuccessorRefreshTokenDigest0000000" WEBHOOK_URL = "https://hooks.slack.com/services/T0000/B0000/madeUpWebhookPath0000" # Columns whose names look like a credential's but whose values aren't one. A secret-bearing column @@ -21,6 +23,8 @@ class ActiveRecordSqlRedactionTest < ActiveSupport::TestCase "audits1984_auditor_tokens.token_digest" => "a digest; the token itself isn't stored", "board_publications.key" => "a published board's public link, which is in its URL path anyway", "filters.params_digest" => "a digest of the filter's parameters", + "oauth_clients.client_secret_digest" => "a digest; the secret itself isn't stored", + "oauth_clients.token_endpoint_auth_method" => "names how a client authenticates: none, client_secret_basic or client_secret_post", "push_subscriptions.auth_key" => "Web Push accepts a message only when it is signed with our VAPID key", "push_subscriptions.p256dh_key" => "a public key" } @@ -46,6 +50,26 @@ class ActiveRecordSqlRedactionTest < ActiveSupport::TestCase assert_sent_to_database OTHER_CODE, executed end + # Encrypted at rest, but the payload decrypts to a live successor pair, so it stays out of logs too. + test "a retired refresh token's successor payload is absent from its logged insert, lookup and blanking" do + retired = nil + log, executed = capture do + access_token = identities(:kevin).access_tokens.create!(token: TOKEN, permission: "read") + retired = access_token.retired_refresh_tokens.create! \ + refresh_token_digest: RETIRED_DIGEST, successor_refresh_token_digest: SUCCESSOR_DIGEST, + successor: { access_token: "madeUpSuccessorAccess000", refresh_token: "madeUpSuccessorRefresh00" } + assert_equal retired, Oauth::RetiredRefreshToken.find_by(refresh_token_digest: RETIRED_DIGEST) + retired.update!(successor_payload: nil) + end + + assert_match(/INSERT INTO [`"]oauth_retired_refresh_tokens[`"]/, log) + assert_match(/UPDATE [`"]oauth_retired_refresh_tokens[`"]/, log) + [ retired.successor_payload_before_last_save, RETIRED_DIGEST, SUCCESSOR_DIGEST ].each do |secret| + assert_no_secret secret, log + assert_sent_to_database secret, executed + end + end + test "an access token is absent from its logged insert and lookup" do log, executed = capture do access_token = identities(:kevin).access_tokens.create!(token: TOKEN, permission: "write") diff --git a/test/migrations/backfill_token_digests_test.rb b/test/migrations/backfill_token_digests_test.rb new file mode 100644 index 0000000000..f2d83874b1 --- /dev/null +++ b/test/migrations/backfill_token_digests_test.rb @@ -0,0 +1,75 @@ +require "test_helper" +require Rails.root.join("db/migrate/20261015120100_backfill_token_digests") + +class BackfillTokenDigestsTest < ActiveSupport::TestCase + setup do + @personal = identities(:david).access_tokens.create!(description: "Before digests") + @personal.update_columns token: "legacyPersonalToken0123456789", token_digest: nil, updated_at: 3.days.ago.round + + @grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + @grant.update_columns token: "legacyAccessToken0123456789", token_digest: nil, updated_at: 80.days.ago.round + end + + test "digests every undigested token, keeping a personal token's plaintext and clearing a grant's" do + backfill + + @personal.reload + assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), @personal.token_digest + assert_equal "legacyPersonalToken0123456789", @personal.token + + @grant.reload + assert_equal TokenDigester.digest("legacyAccessToken0123456789"), @grant.token_digest + assert_nil @grant.token + end + + # A grant's updated_at is its idle-lapse clock. + test "leaves updated_at alone" do + assert_no_changes -> { [ @personal.reload.updated_at, @grant.reload.updated_at ] } do + backfill + end + end + + test "leaves digested tokens alone, so it can run again" do + digested = identity_access_tokens(:davids_api_token) + + assert_no_changes -> { digested.reload.attributes } do + backfill + backfill + end + end + + test "the backfilled token authenticates by digest" do + backfill + + assert_equal @personal, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") + assert_equal @grant, Identity::AccessToken.find_by_raw_token("legacyAccessToken0123456789") + end + + # The plaintext refresh token column outlives this stack, so it's emptied. + test "digests a grant's refresh token and clears its plaintext" do + BackfillTokenDigests::AccessToken.where(id: @grant.id).update_all refresh_token: "legacyRefreshToken0123456789", refresh_token_digest: nil + + backfill + + assert_equal @grant, Identity::AccessToken.find_by_refresh_token("legacyRefreshToken0123456789") + assert_nil BackfillTokenDigests::AccessToken.find(@grant.id).refresh_token + end + + test "refuses duplicate tokens before digesting anything, without naming them" do + copy = identities(:kevin).access_tokens.create!(description: "Copy") + copy.update_columns token: "legacyPersonalToken0123456789", token_digest: nil + + error = assert_raises(RuntimeError) { backfill } + + assert_match @personal.id, error.message + assert_match copy.id, error.message + assert_no_match(/legacyPersonalToken/, error.message) + assert_nil @personal.reload.token_digest + assert_nil @grant.reload.token_digest + end + + private + def backfill + BackfillTokenDigests.new.tap { |migration| migration.verbose = false }.up + end +end diff --git a/test/migrations/drop_plaintext_oauth_credentials_test.rb b/test/migrations/drop_plaintext_oauth_credentials_test.rb new file mode 100644 index 0000000000..b091a27741 --- /dev/null +++ b/test/migrations/drop_plaintext_oauth_credentials_test.rb @@ -0,0 +1,43 @@ +require "test_helper" +require Rails.root.join("db/migrate/20261015120200_drop_plaintext_oauth_credentials") + +# DropPlaintextOauthCredentials runs against the production database while +# production still serves code from before this stack, and without restarting +# it. That code ignores no column, and with partial_inserts off it names every +# column it has cached on each INSERT. So a process that loaded +# identity_access_tokens' columns before the migration must still create +# tokens after it: the migration must not drop any of that table's columns. +class DropPlaintextOauthCredentialsTest < ActiveSupport::TestCase + # Each step is DDL, which MySQL commits on its own. + self.use_transactional_tests = false + + # identity_access_tokens as code from before this stack models it. + class PreviousAccessToken < ApplicationRecord + self.table_name = "identity_access_tokens" + end + + setup do + migrate :down + [ PreviousAccessToken, Identity::AccessToken ].each { |model| model.reset_column_information.then { model.columns } } + end + + teardown do + migrate :up unless ActiveRecord::Base.connection.column_exists?(:oauth_retired_refresh_tokens, :refresh_token_digest) + [ PreviousAccessToken, Identity::AccessToken, Oauth::Client, Oauth::RetiredRefreshToken ].each(&:reset_column_information) + PreviousAccessToken.where(description: "Cached before the migration").delete_all + end + + test "a process that cached identity_access_tokens' columns before it creates tokens after it" do + migrate :up + + assert_difference -> { PreviousAccessToken.count }, 2 do + PreviousAccessToken.create! identity_id: identities(:david).id, permission: "write", description: "Cached before the migration" + identities(:david).access_tokens.create! description: "Cached before the migration" + end + end + + private + def migrate(direction) + DropPlaintextOauthCredentials.new.tap { |migration| migration.verbose = false }.migrate(direction) + end +end diff --git a/test/models/identity/access_token_test.rb b/test/models/identity/access_token_test.rb index 29c1f2ad8f..1bf37bf66b 100644 --- a/test/models/identity/access_token_test.rb +++ b/test/models/identity/access_token_test.rb @@ -4,14 +4,14 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase test "oauth tokens expire and get refresh tokens on create" do token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) - assert_not_nil token.refresh_token + assert_not_nil token.raw_refresh_token assert_in_delta Identity::AccessToken::EXPIRES_IN.from_now, token.expires_at, 5.seconds end test "personal tokens don't expire and get no refresh token" do token = identities(:david).access_tokens.create!(description: "Personal") - assert_nil token.refresh_token + assert_nil token.raw_refresh_token assert_nil token.expires_at assert_not token.expired? end @@ -20,19 +20,19 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase personal = identities(:david).access_tokens.create!(description: "Personal") grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) - assert_match(/\Afizzy_pat_[1-9A-HJ-NP-Za-km-z]{44}\z/, personal.token) - assert_match(/\Afizzy_at_[1-9A-HJ-NP-Za-km-z]{44}\z/, grant.token) - assert_match(/\Afizzy_rt_[1-9A-HJ-NP-Za-km-z]{44}\z/, grant.refresh_token) + assert_match(/\Afizzy_pat_[1-9A-HJ-NP-Za-km-z]{44}\z/, personal.raw_token) + assert_match(/\Afizzy_at_[1-9A-HJ-NP-Za-km-z]{44}\z/, grant.raw_token) + assert_match(/\Afizzy_rt_[1-9A-HJ-NP-Za-km-z]{44}\z/, grant.raw_refresh_token) end test "a rotation issues prefixed tokens, even for a grant issued before prefixes" do grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), - token: "legacyAccessToken0123456789", refresh_token: "legacyRefreshToken0123456789") + raw_token: "legacyAccessToken0123456789", raw_refresh_token: "legacyRefreshToken0123456789") assert grant.refresh - assert grant.token.start_with?("fizzy_at_") - assert grant.refresh_token.start_with?("fizzy_rt_") + assert grant.raw_token.start_with?("fizzy_at_") + assert grant.raw_refresh_token.start_with?("fizzy_rt_") end test "expired?" do @@ -46,13 +46,13 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase test "refresh rotates both tokens and extends expiry" do token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) - old_token, old_refresh_token = token.token, token.refresh_token + old_token, old_refresh_token = token.raw_token, token.raw_refresh_token travel Identity::AccessToken::EXPIRES_IN + 1.minute do assert token.refresh - assert_not_equal old_token, token.token - assert_not_equal old_refresh_token, token.refresh_token + assert_not_equal old_token, token.raw_token + assert_not_equal old_refresh_token, token.raw_refresh_token assert_not token.expired? end end @@ -62,10 +62,10 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase stale = Identity::AccessToken.find(token.id) assert token.refresh - winner = token.token + winner = token.token_digest assert_not stale.refresh - assert_equal winner, token.reload.token + assert_equal winner, token.reload.token_digest end test "active scope excludes expired tokens" do @@ -81,9 +81,9 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase test "find_honored rejects expired tokens" do token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), permission: :write) - assert_equal token, Identity::AccessToken.find_honored(token.token) + assert_equal token, Identity::AccessToken.find_honored(token.raw_token) travel Identity::AccessToken::EXPIRES_IN + 1.second do - assert_nil Identity::AccessToken.find_honored(token.token) + assert_nil Identity::AccessToken.find_honored(token.raw_token) end end @@ -111,4 +111,165 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase end end end + + test "a personal access token is stored digested and, for now, in plaintext" do + token = identities(:david).access_tokens.create!(description: "Personal").reload + + assert_equal TokenDigester.digest(token.token), token.token_digest + assert_nil token.refresh_token_digest + end + + test "an OAuth grant stores only digests of its tokens" do + grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + raw_token, raw_refresh_token = grant.raw_token, grant.raw_refresh_token + grant.reload + + assert_nil grant.token + assert_equal TokenDigester.digest(raw_token), grant.token_digest + assert_equal TokenDigester.digest(raw_refresh_token), grant.refresh_token_digest + assert_nil grant.raw_token + end + + test "rotation stores only digests of the new tokens" do + grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + grant.refresh + raw_token, raw_refresh_token = grant.raw_token, grant.raw_refresh_token + grant.reload + + assert_nil grant.token + assert_equal TokenDigester.digest(raw_token), grant.token_digest + assert_equal TokenDigester.digest(raw_refresh_token), grant.refresh_token_digest + end + + test "tokens are found by digest" do + personal = identities(:david).access_tokens.create!(description: "Personal") + grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + + assert_equal personal, Identity::AccessToken.find_by_raw_token(personal.raw_token) + assert_equal grant, Identity::AccessToken.find_by_raw_token(grant.raw_token) + assert_equal grant, Identity::AccessToken.find_by_refresh_token(grant.raw_refresh_token) + + assert_nil Identity::AccessToken.find_by_raw_token(grant.raw_refresh_token) + assert_nil Identity::AccessToken.find_by_raw_token(grant.token_digest) + assert_nil Identity::AccessToken.find_by_raw_token("") + assert_nil Identity::AccessToken.find_by_raw_token(nil) + end + + test "a lookup by digest never reads plaintext" do + personal = identities(:david).access_tokens.create!(description: "Personal") + statements = capture_sql { Identity::AccessToken.find_honored(personal.raw_token) } + + assert statements.any? { |sql| sql.include?("token_digest") }, statements.join("\n") + assert statements.none? { |sql| sql.match?(/\Wtoken\W*=/) }, statements.join("\n") + end + + # Production code that predates digests writes personal access tokens in + # plaintext only. Such a token still authenticates, and gets its digest the + # first time it's used, without moving its updated_at. + test "a personal access token without a digest is found by plaintext and digested" do + token = undigested_personal_token("legacyPersonalToken0123456789") + updated_at = token.updated_at + + travel 1.day do + assert_equal token, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") + end + + token.reload + assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), token.token_digest + assert_equal updated_at, token.updated_at + + statements = capture_sql { Identity::AccessToken.find_honored("legacyPersonalToken0123456789") } + assert statements.none? { |sql| sql.match?(/\Wtoken\W*=/) }, statements.join("\n") + end + + # SaaS reads GET requests from the replica, where writes are refused. + test "a personal access token without a digest is digested when found from the replica" do + token = undigested_personal_token("legacyPersonalToken0123456789") + + found = ApplicationRecord.with_reading_role do + Identity::AccessToken.find_honored("legacyPersonalToken0123456789") + end + + assert_equal token, found + assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), token.reload.token_digest + end + + # Non-primary datacenters read local replicas and can't reach the primary. + test "a personal access token without a digest authenticates outside the primary datacenter, undigested" do + token = undigested_personal_token("legacyPersonalToken0123456789") + Deployment::DatabaseResolver.stubs(:in_primary_datacenter?).returns(false) + + statements = capture_sql { assert_equal token, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") } + + assert statements.none? { |sql| sql.match?(/\AUPDATE/i) }, statements.join("\n") + assert_nil token.reload.token_digest + end + + # Code that stores only plaintext issues prefixed personal access tokens too. + test "a prefixed personal access token without a digest is found by plaintext and digested" do + token = undigested_personal_token("fizzy_pat_#{"x" * 24}") + + assert_equal token, Identity::AccessToken.find_honored("fizzy_pat_#{"x" * 24}") + assert_equal TokenDigester.digest("fizzy_pat_#{"x" * 24}"), token.reload.token_digest + end + + # MySQL's collation matches `token` case-insensitively. + test "a differently cased token neither authenticates nor digests the one it resembles" do + token = undigested_personal_token("legacyPersonalToken0123456789") + + assert_nil Identity::AccessToken.find_honored("LEGACYPERSONALTOKEN0123456789") + assert_nil token.reload.token_digest + + assert_equal token, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") + assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), token.reload.token_digest + end + + test "only personal access tokens fall back to plaintext" do + grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + grant.update_columns token: "legacyAccessToken0123456789", token_digest: nil + + assert_nil Identity::AccessToken.find_by_raw_token("legacyAccessToken0123456789") + end + + test "the fallback stays within the scope it's called on" do + undigested_personal_token("legacyPersonalToken0123456789") + + assert_nil Identity::AccessToken.oauth.find_by_raw_token("legacyPersonalToken0123456789") + assert_nil Identity::AccessToken.where(identity: identities(:kevin)).find_by_raw_token("legacyPersonalToken0123456789") + end + + # Dropped beside `token` only once every process runs code that ignores it. + test "the plaintext refresh token column is ignored" do + assert_not_includes Identity::AccessToken.column_names, "refresh_token" + assert_not Identity::AccessToken.new.respond_to?(:refresh_token) + end + + test "two tokens can't share a digest" do + token = identities(:david).access_tokens.create!(description: "Personal") + + assert_raises ActiveRecord::RecordNotUnique do + identities(:kevin).access_tokens.create!(description: "Copy", raw_token: token.raw_token) + end + + grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + assert_raises ActiveRecord::RecordNotUnique do + identities(:kevin).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), raw_refresh_token: grant.raw_refresh_token) + end + end + + private + def undigested_personal_token(raw) + identities(:david).access_tokens.create!(description: "Before digests").tap do |token| + token.update_columns token: raw, token_digest: nil + end + end + + def capture_sql(&block) + statements = [] + subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") { |*, payload| statements << payload[:sql] } + block.call + statements + ensure + ActiveSupport::Notifications.unsubscribe(subscriber) + end end diff --git a/test/models/oauth/client_test.rb b/test/models/oauth/client_test.rb index 928abf529f..fdbae59c0d 100644 --- a/test/models/oauth/client_test.rb +++ b/test/models/oauth/client_test.rb @@ -246,13 +246,15 @@ class Oauth::ClientTest < ActiveSupport::TestCase token_endpoint_auth_method: "client_secret_post" ) assert client.confidential? - assert_match(/\Afizzy_cs_[1-9A-HJ-NP-Za-km-z]{44}\z/, client.client_secret) + assert_match(/\Afizzy_cs_[1-9A-HJ-NP-Za-km-z]{44}\z/, client.raw_client_secret) + assert_equal TokenDigester.digest(client.raw_client_secret), client.reload.client_secret_digest end test "public clients get no client secret" do client = Oauth::Client.create!(name: "Public", redirect_uris: %w[ http://127.0.0.1:8888/callback ]) assert_not client.confidential? - assert_nil client.client_secret + assert_nil client.raw_client_secret + assert_nil client.client_secret_digest end test "client_secret_basic clients are confidential" do @@ -262,7 +264,7 @@ class Oauth::ClientTest < ActiveSupport::TestCase token_endpoint_auth_method: "client_secret_basic" ) assert client.confidential? - assert_not_nil client.client_secret + assert_not_nil client.raw_client_secret end test "token_endpoint_auth_method must be supported" do diff --git a/test/models/oauth/retired_refresh_token_test.rb b/test/models/oauth/retired_refresh_token_test.rb index 3d1c0118ea..aa253ef2e8 100644 --- a/test/models/oauth/retired_refresh_token_test.rb +++ b/test/models/oauth/retired_refresh_token_test.rb @@ -6,22 +6,22 @@ class Oauth::RetiredRefreshTokenTest < ActiveSupport::TestCase end test "rotation retires the presented refresh token" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token assert @grant.refresh - assert_equal [ presented ], @grant.retired_refresh_tokens.pluck(:refresh_token) + assert_equal [ TokenDigester.digest(presented) ], @grant.retired_refresh_tokens.pluck(:refresh_token_digest) end test "a rotation that loses the race retires nothing and leaves the winner's successor retryable" do - presented = @grant.refresh_token + presented = @grant.raw_refresh_token loser = Identity::AccessToken.find(@grant.id) assert @grant.refresh assert_nil loser.refresh assert_equal 1, Oauth::RetiredRefreshToken.count - assert Oauth::RetiredRefreshToken.find_by(refresh_token: presented).retryable?(@grant.reload) + assert Oauth::RetiredRefreshToken.find_by_raw_token(presented).retryable?(@grant.reload) end test "revoking a grant deletes its retired refresh tokens" do @@ -56,4 +56,59 @@ class Oauth::RetiredRefreshTokenTest < ActiveSupport::TestCase Oauth::RetiredRefreshToken.cleanup end end + + test "a retired token keeps digests, and the successor pair only encrypted" do + presented = @grant.raw_refresh_token + @grant.refresh + retired = @grant.retired_refresh_tokens.sole + + assert_equal TokenDigester.digest(presented), retired.refresh_token_digest + assert_equal TokenDigester.digest(@grant.raw_refresh_token), retired.successor_refresh_token_digest + assert_equal({ access_token: @grant.raw_token, refresh_token: @grant.raw_refresh_token }, retired.successor) + + [ presented, @grant.raw_token, @grant.raw_refresh_token ].each do |raw| + assert_not_includes retired.successor_payload, raw + end + end + + test "the successor pair can't be read once the grace window has passed" do + @grant.refresh + retired = @grant.retired_refresh_tokens.sole + + travel Oauth::RetiredRefreshToken::GRACE - 1.second + assert_not_nil retired.successor + + travel 2.seconds + assert_nil retired.successor + end + + test "forget_successors blanks pairs whose grace window has passed" do + @grant.refresh + retired = @grant.retired_refresh_tokens.sole + + Oauth::RetiredRefreshToken.forget_successors + assert_not_nil retired.reload.successor_payload + + travel Oauth::RetiredRefreshToken::GRACE + 1.second + Oauth::RetiredRefreshToken.forget_successors + assert_nil retired.reload.successor_payload + end + + test "superseded compares the grant's current refresh digest with the successor's" do + @grant.refresh + retired = @grant.retired_refresh_tokens.sole + + assert_not retired.superseded?(Identity::AccessToken.find(@grant.id)) + + @grant.refresh + assert retired.superseded?(Identity::AccessToken.find(@grant.id)) + end + + test "a tampered successor payload reads as none" do + @grant.refresh + retired = @grant.retired_refresh_tokens.sole + retired.successor_payload = retired.successor_payload.reverse + + assert_nil retired.successor + end end diff --git a/test/models/token_digester_test.rb b/test/models/token_digester_test.rb new file mode 100644 index 0000000000..ba612f8862 --- /dev/null +++ b/test/models/token_digester_test.rb @@ -0,0 +1,94 @@ +require "test_helper" + +class TokenDigesterTest < ActiveSupport::TestCase + # HMAC-SHA256 with key "key", the standard published vector. bc3's + # Oauth::TokenDigester is OpenSSL::HMAC.hexdigest("SHA256", pepper, raw), so + # matching it here keeps the two byte-compatible. + test "a digest is lowercase hex HMAC-SHA256 under the pepper, as in bc3" do + with_peppers 1 => "key" do + assert_equal "f7bc83f430538424b13298e6aa6fb143ef4d59a14946175997479dbc2d1a3cd8", + TokenDigester.digest("The quick brown fox jumps over the lazy dog") + end + end + + test "a version 1 digest is bare; later versions carry their number" do + with_peppers 1 => "a" * 32, 2 => "b" * 32 do + assert_match(/\A\h{64}\z/, TokenDigester.digest("raw")) + assert_match(/\A2\$\h{64}\z/, TokenDigester.digest("raw", version: 2)) + assert_equal [ TokenDigester.digest("raw"), TokenDigester.digest("raw", version: 2) ], TokenDigester.digests("raw") + end + end + + test "verify checks a digest under the pepper its version names" do + with_peppers 1 => "a" * 32, 2 => "b" * 32 do + assert TokenDigester.verify("raw", TokenDigester.digest("raw")) + assert TokenDigester.verify("raw", TokenDigester.digest("raw", version: 2)) + + assert_not TokenDigester.verify("other", TokenDigester.digest("raw")) + assert_not TokenDigester.verify("raw", "3$" + TokenDigester.digest("raw")) + assert_not TokenDigester.verify("raw", "x$" + TokenDigester.digest("raw")) + assert_not TokenDigester.verify("", TokenDigester.digest("")) + assert_not TokenDigester.verify(nil, TokenDigester.digest("raw")) + assert_not TokenDigester.verify([ "raw" ], TokenDigester.digest("raw")) + assert_not TokenDigester.verify("raw", nil) + end + end + + test "a configured pepper is used as given" do + assert_equal "p" * 40, TokenDigester.send(:configured_pepper, "p" * 40, required: true) + end + + # The deploy configs and the self-hosting docs name OAUTH_TOKEN_PEPPER; + # reading any other variable would quietly fall back to the derived pepper. + test "the pepper is read from OAUTH_TOKEN_PEPPER" do + with_peppers nil do + with_env "OAUTH_TOKEN_PEPPER" => "e" * 40 do + assert_equal OpenSSL::HMAC.hexdigest("SHA256", "e" * 40, "raw"), TokenDigester.digest("raw") + end + end + end + + test "a deployment that requires a pepper refuses to run without one" do + assert_raises(TokenDigester::MissingPepper) { TokenDigester.send(:configured_pepper, nil, required: true) } + assert_raises(TokenDigester::MissingPepper) { TokenDigester.send(:configured_pepper, "", required: true) } + end + + test "a short pepper is refused" do + assert_raises(TokenDigester::MissingPepper) { TokenDigester.send(:configured_pepper, "short", required: false) } + end + + test "without a pepper, one is derived from secret_key_base" do + assert_equal Rails.application.key_generator.generate_key("token_digest_pepper", 32), + TokenDigester.send(:configured_pepper, nil, required: false) + end + + test "SaaS deployments require a pepper; development, test and self-hosted installs don't" do + Fizzy.stubs(:saas?).returns(true) + assert_not TokenDigester.pepper_required? + + %w[ beta staging production ].each do |env| + Rails.stubs(:env).returns(ActiveSupport::EnvironmentInquirer.new(env)) + assert TokenDigester.pepper_required?, env + end + + Fizzy.stubs(:saas?).returns(false) + assert_not TokenDigester.pepper_required? + end + + private + def with_peppers(peppers) + original = TokenDigester.instance_variable_get(:@peppers) + TokenDigester.instance_variable_set(:@peppers, peppers) + yield + ensure + TokenDigester.instance_variable_set(:@peppers, original) + end + + def with_env(values) + original = values.keys.index_with { |key| ENV[key] } + values.each { |key, value| ENV[key] = value } + yield + ensure + original.each { |key, value| ENV[key] = value } + end +end diff --git a/test/test_helpers/oauth_client_credentials_test_helper.rb b/test/test_helpers/oauth_client_credentials_test_helper.rb index fa89adb51f..67286f745c 100644 --- a/test/test_helpers/oauth_client_credentials_test_helper.rb +++ b/test/test_helpers/oauth_client_credentials_test_helper.rb @@ -14,7 +14,7 @@ def assert_client_authentication_failed(message = nil, basic: true) assert_equal "invalid_client", response.parsed_body["error"], message if basic - assert_equal %(Basic realm="http://www.example.com/"), response.headers["WWW-Authenticate"], message + assert_equal %(Basic realm="http://www.example.com"), response.headers["WWW-Authenticate"], message else assert_nil response.headers["WWW-Authenticate"], message end