From a81e72782b1c7e68cbb5b0eb36ae8df89596bd6e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 15 Oct 2026 12:30:54 -0700 Subject: [PATCH 01/20] Store tokens and client secrets as digests, reading by digest first Step 1 of 3 toward hashing credentials at rest. Access tokens, refresh tokens and client secrets are stored as HMAC-SHA256 digests (TokenDigester, byte-compatible with bc3's Oauth::TokenDigester) and looked up through a unique index on the digest. Personal access tokens are still written in plaintext too, because production code that reads only plaintext shares the database while beta runs this. A personal access token without a digest is found by plaintext once and given its digest; prefixed tokens never fall back. OAuth access tokens, refresh tokens and client secrets are digest-only, and their plaintext columns go. A refresh retry within the grace window is answered from an encrypted copy of the successor pair that expires with the window and is swept soon after. A new personal access token is shown only in the response that creates it, which isn't cached, rather than on a page that can be loaded again. SaaS deployments read the pepper from OAUTH_TOKEN_PEPPER, mirroring bc3's credentials.oauth.token_pepper, and refuse to boot without it. Beta reads production's, since it writes production's database. Elsewhere it falls back to a key derived from secret_key_base. --- .../my/access_tokens_controller.rb | 25 +-- app/controllers/oauth/clients_controller.rb | 4 +- .../oauth/revocations_controller.rb | 6 +- app/controllers/oauth/tokens_controller.rb | 24 ++- app/models/identity/access_token.rb | 71 ++++++-- app/models/oauth/client.rb | 11 +- app/models/oauth/retired_refresh_token.rb | 39 ++++- app/models/token_digester.rb | 94 ++++++++++ app/views/my/access_tokens/_reveal.html.erb | 16 ++ app/views/my/access_tokens/create.html.erb | 11 ++ .../my/access_tokens/create.turbo_stream.erb | 1 + app/views/my/access_tokens/new.html.erb | 2 +- app/views/my/access_tokens/show.html.erb | 26 --- config/initializers/token_digester.rb | 6 + config/recurring.yml | 3 + config/routes.rb | 2 +- .../20261015120000_add_token_digests.rb | 17 ++ .../20261015120100_backfill_token_digests.rb | 76 +++++++++ ...120200_drop_plaintext_oauth_credentials.rb | 38 +++++ db/schema.rb | 17 +- db/schema_sqlite.rb | 17 +- docs/docker-deployment.md | 5 + docs/kamal-deployment.md | 2 +- docs/oauth/spec_conformance.md | 4 +- saas/.kamal/secrets.beta | 3 +- saas/.kamal/secrets.production | 3 +- saas/.kamal/secrets.staging | 3 +- saas/config/deploy.beta.yml | 1 + saas/config/deploy.production.yml | 1 + saas/config/deploy.staging.yml | 1 + .../my/access_tokens_controller_test.rb | 51 ++++-- test/fixtures/identity/access_tokens.yml | 2 + test/fixtures/oauth/clients.yml | 2 +- test/integration/oauth_availability_test.rb | 23 +-- .../oauth_client_secret_basic_test.rb | 20 +-- .../oauth_delegate_resources_test.rb | 8 +- test/integration/oauth_flow_test.rb | 102 +++++++---- .../oauth_grant_idle_expiry_test.rb | 12 +- test/integration/oauth_grant_reach_test.rb | 6 +- test/integration/oauth_refresh_replay_test.rb | 51 ++++-- test/integration/oauth_revocation_test.rb | 38 ++--- .../migrations/backfill_token_digests_test.rb | 65 +++++++ test/models/identity/access_token_test.rb | 161 ++++++++++++++++-- test/models/oauth/client_test.rb | 8 +- .../oauth/retired_refresh_token_test.rb | 63 ++++++- test/models/token_digester_test.rb | 94 ++++++++++ 46 files changed, 998 insertions(+), 237 deletions(-) create mode 100644 app/models/token_digester.rb create mode 100644 app/views/my/access_tokens/_reveal.html.erb create mode 100644 app/views/my/access_tokens/create.html.erb create mode 100644 app/views/my/access_tokens/create.turbo_stream.erb delete mode 100644 app/views/my/access_tokens/show.html.erb create mode 100644 config/initializers/token_digester.rb create mode 100644 db/migrate/20261015120000_add_token_digests.rb create mode 100644 db/migrate/20261015120100_backfill_token_digests.rb create mode 100644 db/migrate/20261015120200_drop_plaintext_oauth_credentials.rb create mode 100644 test/migrations/backfill_token_digests_test.rb create mode 100644 test/models/token_digester_test.rb 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/clients_controller.rb b/app/controllers/oauth/clients_controller.rb index 1d5b1c1b01..a736d6aa92 100644 --- a/app/controllers/oauth/clients_controller.rb +++ b/app/controllers/oauth/clients_controller.rb @@ -99,8 +99,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..64eb4fe647 100644 --- a/app/controllers/oauth/revocations_controller.rb +++ b/app/controllers/oauth/revocations_controller.rb @@ -42,7 +42,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 +51,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 1b575e73af..ba5ede54cf 100644 --- a/app/controllers/oauth/tokens_controller.rb +++ b/app/controllers/oauth/tokens_controller.rb @@ -53,7 +53,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 @@ -131,7 +131,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" @@ -211,7 +211,7 @@ def authenticate_client def answer_refresh_replay(retired_refresh_token) if grant = 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 @@ -225,12 +225,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 @@ -240,10 +246,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..44a228d188 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -35,6 +35,7 @@ class Identity::AccessToken < ApplicationRecord # secrets are drawn here too (Oauth::Client), so every credential Fizzy # issues has one shape. PREFIXES = { personal: "fizzy_pat_", access: "fizzy_at_", refresh: "fizzy_rt_", client_secret: "fizzy_cs_" } + PREFIX = "fizzy_" # The base58 alphabet has_secure_token uses: no 0, O, I or l. Drawn through # Ruby's own SecureRandom, so it doesn't depend on ActiveSupport's base58 @@ -48,6 +49,14 @@ 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. + 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 +83,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. A prefixed token was issued by code that + # digests, so it never falls back. + 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 +120,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 +129,17 @@ def redeem(authorization_code, **attributes) where(authorization_code_jti: authorization_code.jti).destroy_all nil end + + private + # Writes the digest with update_columns, so updated_at, a grant's idle + # clock, stays put. + def find_and_digest_undigested(raw) + unless raw.start_with?(PREFIX) + personal.where(token_digest: nil).find_by(token: raw)&.tap do |access_token| + 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 +170,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 +206,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..9eb3e9d035 --- /dev/null +++ b/app/views/my/access_tokens/_reveal.html.erb @@ -0,0 +1,16 @@ +
+
+ +

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..de0b1867e6 100644 --- a/app/views/my/access_tokens/new.html.erb +++ b/app/views/my/access_tokens/new.html.erb @@ -8,7 +8,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 14c93a9320..a84bd46df8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -190,7 +190,7 @@ namespace :my do resource :passkey_challenge, only: :create resource :identity, only: :show - resources :access_tokens + resources :access_tokens, except: %i[ show edit update ] resources :connected_apps, only: %i[ index destroy ] resources :passkeys, except: %i[ show new ] resources :pins diff --git a/db/migrate/20261015120000_add_token_digests.rb b/db/migrate/20261015120000_add_token_digests.rb new file mode 100644 index 0000000000..6d71517f8e --- /dev/null +++ b/db/migrate/20261015120000_add_token_digests.rb @@ -0,0 +1,17 @@ +# 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. +class AddTokenDigests < ActiveRecord::Migration[8.2] + def change + change_table :identity_access_tokens, bulk: true do |t| + t.string :token_digest, limit: 80 + t.string :refresh_token_digest, limit: 80 + + t.index :token_digest, unique: true + t.index :refresh_token_digest, unique: true + end + + add_column :oauth_clients, :client_secret_digest, :string, limit: 80 + 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..cd76e3ecb4 --- /dev/null +++ b/db/migrate/20261015120100_backfill_token_digests.rb @@ -0,0 +1,76 @@ +# 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; their plaintext columns are dropped by the next migration. +# +# 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 + + # Gone once DropPlaintextOauthCredentials has run, so a rerun skips them. + 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 + 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..2ce2517eef --- /dev/null +++ b/db/migrate/20261015120200_drop_plaintext_oauth_credentials.rb @@ -0,0 +1,38 @@ +# Refresh tokens and client secrets have been digested (BackfillTokenDigests) +# and are never read in plaintext again. Nothing in production holds them: +# the OAuth tables and columns 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. +class DropPlaintextOauthCredentials < ActiveRecord::Migration[8.2] + def change + remove_index :identity_access_tokens, :refresh_token, unique: true + remove_column :identity_access_tokens, :refresh_token, :string + + remove_column :oauth_clients, :client_secret, :string + + drop_table :oauth_retired_refresh_tokens, id: :uuid 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 + + create_table :oauth_retired_refresh_tokens, id: :uuid 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 +end diff --git a/db/schema.rb b/db/schema.rb index 2870409449..08dd5fb600 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 @@ -359,13 +359,15 @@ t.uuid "identity_id", null: false 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 +440,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 +455,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..d5eb08cdb1 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 @@ -359,13 +359,15 @@ t.uuid "identity_id", null: false 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 +440,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 +454,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..dd0fa5f285 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)). - `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..a5236cf38a 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) | +| [#PRNUM](https://github.com/basecamp/fizzy/pull/PRNUM) | `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** #PRNUM `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, about 140 bits of randomness, 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** #PRNUM `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) diff --git a/saas/.kamal/secrets.beta b/saas/.kamal/secrets.beta index 4810ef7869..17dea2e6b2 100644 --- a/saas/.kamal/secrets.beta +++ b/saas/.kamal/secrets.beta @@ -1,4 +1,4 @@ -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/QUEENBEE_API_TOKEN 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/QUEENBEE_API_TOKEN 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) @@ -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.production b/saas/.kamal/secrets.production index 626b784f79..8fdab2f37d 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/QUEENBEE_API_TOKEN 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) +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/QUEENBEE_API_TOKEN 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/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 ed8a869956..7579bd1c65 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/QUEENBEE_API_TOKEN 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/QUEENBEE_API_TOKEN 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 7e60f41a40..9528d96061 100644 --- a/saas/config/deploy.beta.yml +++ b/saas/config/deploy.beta.yml @@ -62,6 +62,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 d1dd705b0a..8968a1876e 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 b45fabcdd1..b4e6dc276c 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/my/access_tokens_controller_test.rb b/test/controllers/my/access_tokens_controller_test.rb index 9bec86fad0..f65f7482c6 100644 --- a/test/controllers/my/access_tokens_controller_test.rb +++ b/test/controllers/my/access_tokens_controller_test.rb @@ -27,9 +27,37 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest 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 +65,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 +94,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 +150,12 @@ 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 + + def assert_revealed(access_token) + assert_equal access_token.token_digest, TokenDigester.digest(token_in_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 6a2be65dae..c343094e2f 100644 --- a/test/integration/oauth_availability_test.rb +++ b/test/integration/oauth_availability_test.rb @@ -173,18 +173,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 @@ -198,7 +199,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 @@ -206,11 +207,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 @@ -222,7 +223,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 @@ -231,7 +232,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 @@ -249,7 +250,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 @@ -262,7 +263,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 @@ -315,7 +316,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..da89972f6c 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,7 +95,7 @@ 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 @@ -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 13fc9df5be..89a8676de3 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 @@ -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 @@ -1823,6 +1823,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..0ed07a14ce 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,7 +91,7 @@ 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 @@ -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/migrations/backfill_token_digests_test.rb b/test/migrations/backfill_token_digests_test.rb new file mode 100644 index 0000000000..fcbf150005 --- /dev/null +++ b/test/migrations/backfill_token_digests_test.rb @@ -0,0 +1,65 @@ +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 + + 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/models/identity/access_token_test.rb b/test/models/identity/access_token_test.rb index 29c1f2ad8f..5a1f928d9a 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,135 @@ 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 + + test "a prefixed token is never looked up by plaintext" do + undigested_personal_token("fizzy_pat_#{"x" * 24}") + + assert_nil Identity::AccessToken.find_honored("fizzy_pat_#{"x" * 24}") + 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 + + 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 From c5f94f0c0a5769c22170f165d4849339a88fc966 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 15 Oct 2026 12:31:47 -0700 Subject: [PATCH 02/20] Write a healed token's digest to the primary, even when it was found on the replica --- app/models/identity/access_token.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 44a228d188..962ad724b7 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -131,12 +131,12 @@ def redeem(authorization_code, **attributes) end private - # Writes the digest with update_columns, so updated_at, a grant's idle - # clock, stays put. + # 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. def find_and_digest_undigested(raw) unless raw.start_with?(PREFIX) personal.where(token_digest: nil).find_by(token: raw)&.tap do |access_token| - access_token.update_columns token_digest: TokenDigester.digest(raw) + ApplicationRecord.with_writing_role { access_token.update_columns token_digest: TokenDigester.digest(raw) } end end end From 7babdf8e4f964653bb87f1e0520f47f8b575beaf Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 15 Oct 2026 12:42:34 -0700 Subject: [PATCH 03/20] Point the conformance record at #3192 --- docs/oauth/spec_conformance.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index a5236cf38a..72ce08ef8d 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -17,7 +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) | -| [#PRNUM](https://github.com/basecamp/fizzy/pull/PRNUM) | `oauth-token-digests` | Tokens and client secrets stored as digests, step 1 of 3 (on #3169) | +| [#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. @@ -82,14 +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. | -| Client secrets are stored as digests, never in plaintext, and shown once, in the registration response | **Fixed** #PRNUM `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, about 140 bits of randomness, 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. | +| 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, about 140 bits of randomness, 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** #PRNUM `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. | +| 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) From 265889cf077fda3b6cabcc98a6606bf0cfe199b1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 15 Oct 2026 14:56:06 -0700 Subject: [PATCH 04/20] Keep a revealed token out of Turbo's snapshot cache --- app/views/my/access_tokens/_reveal.html.erb | 3 ++- test/controllers/my/access_tokens_controller_test.rb | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/views/my/access_tokens/_reveal.html.erb b/app/views/my/access_tokens/_reveal.html.erb index 9eb3e9d035..7900852076 100644 --- a/app/views/my/access_tokens/_reveal.html.erb +++ b/app/views/my/access_tokens/_reveal.html.erb @@ -1,4 +1,5 @@ -
+<%# Temporary, so Turbo's snapshot cache doesn't keep the token for Back to restore. %> +