From a16f4ee00f3a4789246f5128e4f2455424e0c9ac Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 00:02:13 -0700 Subject: [PATCH 01/12] Issue personal access tokens from a first-party client, and read both forms as personal A personal access token becomes a one-off access token of the fizzy-personal-access client, as bc3's are of bc-personal-access. The client is never self-registered, so the sweep passes it by, and it is listed for no grant type, so the authorization and token endpoints refuse it. Every rule that read "no client" as "personal" now reads either form: the personal and oauth scopes, honoring while OAuth is dark, idle lapse, expiry and refresh tokens, the token prefix and plaintext, revocation by possession, Connected Apps, the OAuth grant fence, and webhook and push attribution. Client-less tokens stay personal for production code that still writes them. --- app/controllers/concerns/authentication.rb | 2 +- .../my/access_tokens_controller.rb | 2 +- .../oauth/authorizations_controller.rb | 9 +- .../oauth/revocations_controller.rb | 8 +- app/controllers/oauth/tokens_controller.rb | 10 + .../users/push_subscriptions_controller.rb | 4 +- app/models/identity/access_token.rb | 44 +++- app/models/oauth/client.rb | 55 ++++- app/models/webhook.rb | 2 +- test/fixtures/oauth/clients.yml | 10 + .../personal_access_token_forms_test.rb | 222 ++++++++++++++++++ test/models/oauth/client_test.rb | 54 +++++ 12 files changed, 399 insertions(+), 23 deletions(-) create mode 100644 test/integration/personal_access_token_forms_test.rb diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index a60773c598..caae9440da 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -49,7 +49,7 @@ def authenticated? end def oauth_grant? - Current.access_token&.oauth_client_id? + Current.access_token&.oauth? || false end def forbid_oauth_grant diff --git a/app/controllers/my/access_tokens_controller.rb b/app/controllers/my/access_tokens_controller.rb index b32d11846a..2e06388e01 100644 --- a/app/controllers/my/access_tokens_controller.rb +++ b/app/controllers/my/access_tokens_controller.rb @@ -15,7 +15,7 @@ def new # 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.merge(oauth_client: Oauth::Client.personal_access_issuer)) no_store respond_to do |format| diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 7aec59607b..c7833c3d47 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -70,9 +70,16 @@ def save_oauth_return_url end end + # A client not registered for the code grant, like the personal access + # token issuer, has nothing to authorize here. def set_client @client = Oauth::Client.find_by(client_id: params[:client_id]) - oauth_error("invalid_request", "Unknown client") unless @client + + if @client.nil? + oauth_error "invalid_request", "Unknown client" + elsif !@client.allows_grant_type?("authorization_code") + oauth_error "unauthorized_client", "Client may not use the authorization code grant" + end end def validate_redirect_uri diff --git a/app/controllers/oauth/revocations_controller.rb b/app/controllers/oauth/revocations_controller.rb index 64eb4fe647..827be26413 100644 --- a/app/controllers/oauth/revocations_controller.rb +++ b/app/controllers/oauth/revocations_controller.rb @@ -8,9 +8,11 @@ # with a missing or wrong secret, a Basic header that doesn't authenticate, # or a client secret that authenticates no client. # -# A personal access token was issued to no client, so possession of it is the -# credential: whoever presents one revokes it, client or not. Client -# credentials presented beside it must still authenticate, by the same rules. +# A personal access token is issued to no client that can authenticate here +# (none at all, or the personal access token issuer, which has no secret and no +# grant to revoke), so possession of it is the credential: whoever presents one +# revokes it, client or not. Client credentials presented beside it must still +# authenticate, by the same rules. class Oauth::RevocationsController < Oauth::BaseController include Oauth::ClientAuthentication diff --git a/app/controllers/oauth/tokens_controller.rb b/app/controllers/oauth/tokens_controller.rb index ba5ede54cf..7e50116831 100644 --- a/app/controllers/oauth/tokens_controller.rb +++ b/app/controllers/oauth/tokens_controller.rb @@ -15,6 +15,7 @@ class Oauth::TokensController < Oauth::BaseController # the answer must not depend on whether the code or refresh token is live, # or it tells whoever holds one without the secret. before_action :authenticate_client + before_action :validate_client_grant_type before_action :require_params with_options if: :authorization_code_grant? do @@ -76,6 +77,15 @@ def validate_grant_type end end + # A client is refused a grant type it isn't registered for (RFC 6749 §5.2), + # so the personal access token issuer, registered for none, can never + # mint, refresh or exchange a token here. + def validate_client_grant_type + if requesting_client && !requesting_client.allows_grant_type?(params[:grant_type]) + oauth_error "unauthorized_client", "Client may not use the #{params[:grant_type]} grant" + end + end + # A missing parameter is a malformed request (invalid_request), not a dead # grant (invalid_grant), which a client would act on by discarding it. # client_id is required of every client, in the body unless a Basic diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb index 7521300c76..2498100a0a 100644 --- a/app/controllers/users/push_subscriptions_controller.rb +++ b/app/controllers/users/push_subscriptions_controller.rb @@ -9,8 +9,8 @@ def index def create # A new subscription records its app as it's inserted, so no crash between # two writes can leave an app's endpoint looking like a browser's. - subscription = @push_subscriptions.create_with(user_agent: request.user_agent, oauth_client: Current.access_token&.oauth_client).create_or_find_by!(push_subscription_params) - subscription.claim_for Current.access_token&.oauth_client + subscription = @push_subscriptions.create_with(user_agent: request.user_agent, oauth_client: Current.access_token&.connected_app).create_or_find_by!(push_subscription_params) + subscription.claim_for Current.access_token&.connected_app respond_to do |format| format.html { head :no_content } diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 8d4199f213..26831bc41d 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -23,8 +23,15 @@ class Identity::AccessToken < ApplicationRecord # revocation waits rather than deadlocking against the retired-token delete. before_destroy :lock_grant, prepend: true - scope :personal, -> { where oauth_client_id: nil } - scope :oauth, -> { where.not oauth_client_id: nil } + # A personal access token is issued by the personal access client + # (Oauth::Client::PERSONAL_ACCESS_CLIENT_ID). Ones made before that client + # existed, and ones production code that predates it still makes, have no + # client, and are just as personal. Every rule that sets personal tokens + # apart (honoring while OAuth is dark, never lapsing, no expiry or refresh + # token, revocation by possession, Connected Apps) reads one of these, or + # #personal?, so it holds for both forms alike. + scope :personal, -> { where(oauth_client_id: nil).or(where(oauth_client_id: Oauth::Client.personal_access.select(:id))) } + scope :oauth, -> { where.not(oauth_client_id: nil).where.not(oauth_client_id: Oauth::Client.personal_access.select(:id)) } scope :active, -> { where(expires_at: nil).or(where(expires_at: Time.current..)) } scope :unlapsed, -> { where(updated_at: REFRESH_IDLE_LIMIT.ago..) } scope :lapsed, -> { oauth.where(updated_at: ...REFRESH_IDLE_LIMIT.ago) } @@ -64,14 +71,14 @@ class Identity::AccessToken < ApplicationRecord attribute :raw_refresh_token, :string before_create :set_token - before_create :set_expiry_and_refresh_token, if: :oauth_client_id? + before_create :set_expiry_and_refresh_token, if: :oauth? # Issuing or destroying a grant restarts its client's retention period # (Oauth::Client.cleanup). The touch waits until the grant's own transaction # commits. A revocation locks the grant, and touching the client in the same # transaction would then wait on the client's lock. A replayed code exchange # holds that lock while it waits on the grant, so the two would deadlock. - after_commit :touch_oauth_client, on: %i[ create destroy ] + after_commit :touch_oauth_client, on: %i[ create destroy ], if: :oauth? # Every way a grant ends destroys it: disconnecting the app, revocation, the # idle-lapse sweep, a replayed refresh token or authorization code, and @@ -83,7 +90,7 @@ class Identity::AccessToken < ApplicationRecord # # Checked after commit, so when two grants to one client end at once, # whichever commits second finds none left. - after_destroy_commit :end_app_plumbing, if: :oauth_client_id? + after_destroy_commit :end_app_plumbing, if: :oauth? class << self # The token a bearer request may authenticate with: active and honored. @@ -157,9 +164,24 @@ def find_and_digest_undigested(raw) # OAuth tokens are honored only while OAuth acceptance is on (see # Oauth::Availability); personal tokens always are. The client is loaded only - # when the server is dark and a pilot exemption might apply. + # when the server is dark, to tell a personal token from a grant and to see + # whether a pilot exemption applies. def honored? - oauth_client_id.nil? || Oauth::Availability.acceptance_enabled? || Oauth::Availability.acceptance_enabled?(oauth_client&.client_id) + oauth_client_id.nil? || Oauth::Availability.acceptance_enabled? || personal? || Oauth::Availability.acceptance_enabled?(oauth_client&.client_id) + end + + def personal? + oauth_client_id.nil? || oauth_client&.personal_access? || false + end + + def oauth? + !personal? + end + + # The app this token acts through, if it is a grant. A personal access token + # acts for its holder alone, so what it sets up is attributed to no app. + def connected_app + oauth_client if oauth? end def allows?(method) @@ -175,7 +197,7 @@ def expires_in end def lapsed? - oauth_client_id? && (updated_at + REFRESH_IDLE_LIMIT).past? + oauth? && (updated_at + REFRESH_IDLE_LIMIT).past? end # Rotates atomically on the presented refresh token, so a concurrent @@ -212,7 +234,7 @@ def end_app_plumbing end def touch_oauth_client - Oauth::Client.where(id: oauth_client_id).touch_all if oauth_client_id? + Oauth::Client.where(id: oauth_client_id).touch_all end def lock_grant @@ -223,9 +245,9 @@ def lock_grant # for production code that reads only plaintext. An explicitly given # token is digested like a generated one. def set_token - self.raw_token ||= token.presence || self.class.generate_token(oauth_client_id? ? :access : :personal) + self.raw_token ||= token.presence || self.class.generate_token(personal? ? :personal : :access) self.token_digest = TokenDigester.digest(raw_token) - self.token = oauth_client_id? ? nil : raw_token + self.token = personal? ? raw_token : nil end def set_expiry_and_refresh_token diff --git a/app/models/oauth/client.rb b/app/models/oauth/client.rb index 28d103eb52..8eb72e3b7a 100644 --- a/app/models/oauth/client.rb +++ b/app/models/oauth/client.rb @@ -9,6 +9,22 @@ class Oauth::Client < ApplicationRecord # this long is abandoned (RFC 7591 §5). UNUSED_RETENTION = 30.days + # The grants a client may obtain at the authorization and token endpoints. + # An allowlist, so a client is refused any grant type it doesn't list. + GRANT_TYPES = %w[ authorization_code refresh_token ] + + # Personal access tokens are issued by this first-party client, as bc3's are + # by bc-personal-access, so they're one-off access tokens of the same kind + # as every other. It is provisioned by a migration and on first use, never + # registered: the sweep, which takes only self-registered clients, leaves it + # alone. It is listed for no grant type, so the authorization and token + # endpoints never mint, refresh or exchange its tokens: only + # My::AccessTokensController issues them. Its client_id has a hyphen, which + # no generated client_id (base58) can, so no other client can ever claim it. + PERSONAL_ACCESS_CLIENT_ID = "fizzy-personal-access" + PERSONAL_ACCESS_ATTRIBUTES = { client_id: PERSONAL_ACCESS_CLIENT_ID, name: "Personal Access Tokens", + trusted: true, dynamically_registered: false, redirect_uris: [], scopes: %w[ read write ], token_endpoint_auth_method: "none" } + has_many :access_tokens, class_name: "Identity::AccessToken", foreign_key: :oauth_client_id, inverse_of: :oauth_client, dependent: :destroy has_secure_token :client_id, length: 32 @@ -18,6 +34,10 @@ class Oauth::Client < ApplicationRecord # same order, so no grant can land after the cascade and be stranded. before_destroy :lock_client, prepend: true + # Destroying the issuer would cascade to every client-backed personal access + # token on the install. + before_destroy :refuse_to_destroy_personal_access, prepend: true + validates :name, presence: true, length: { maximum: 255 } validates :client_id, uniqueness: true, allow_nil: true # Registration is open and unauthenticated, so what it stores is bounded: @@ -25,7 +45,7 @@ class Oauth::Client < ApplicationRecord MAXIMUM_REDIRECT_URIS = 10 MAXIMUM_REDIRECT_URI_LENGTH = 2000 - validates :redirect_uris, presence: true, length: { maximum: MAXIMUM_REDIRECT_URIS } + validates :redirect_uris, presence: { unless: :personal_access? }, length: { maximum: MAXIMUM_REDIRECT_URIS } validates :token_endpoint_auth_method, inclusion: { in: AUTH_METHODS } validate :redirect_uris_are_valid @@ -47,8 +67,18 @@ class Oauth::Client < ApplicationRecord # swept for having registered long ago. scope :stale, -> { dynamically_registered.where.missing(:access_tokens).where(updated_at: ...UNUSED_RETENTION.ago) } - def self.cleanup - stale.find_each(&:destroy_if_still_unused) + scope :personal_access, -> { where client_id: PERSONAL_ACCESS_CLIENT_ID } + + class << self + def cleanup + stale.find_each(&:destroy_if_still_unused) + end + + # A fresh install loads the schema rather than running migrations, so the + # issuer may not exist yet when the first personal access token is made. + def personal_access_issuer + personal_access.take || create_or_find_by!(PERSONAL_ACCESS_ATTRIBUTES) + end end # The sweep and #redeem take the same row lock, so a grant either lands @@ -73,6 +103,18 @@ def redeem(authorization_code, identity:, permission:) end end + def personal_access? + client_id == PERSONAL_ACCESS_CLIENT_ID + end + + def grant_types + personal_access? ? [] : GRANT_TYPES + end + + def allows_grant_type?(grant_type) + grant_type.in?(grant_types) + end + def confidential? token_endpoint_auth_method.in?(SECRET_AUTH_METHODS) end @@ -106,6 +148,13 @@ def lock_client access_tokens.reset end + def refuse_to_destroy_personal_access + if personal_access? + errors.add :base, "The personal access token issuer can't be deleted" + throw :abort + end + end + def generate_client_secret self.raw_client_secret ||= Identity::AccessToken.generate_token(:client_secret) self.client_secret_digest = TokenDigester.digest(raw_client_secret) diff --git a/app/models/webhook.rb b/app/models/webhook.rb index 59c3efc85c..cdb0a86509 100644 --- a/app/models/webhook.rb +++ b/app/models/webhook.rb @@ -87,7 +87,7 @@ def for_slack? private def attribute_creation self.creator ||= Current.user - self.created_via ||= Current.access_token&.oauth_client + self.created_via ||= Current.access_token&.connected_app end def validate_url diff --git a/test/fixtures/oauth/clients.yml b/test/fixtures/oauth/clients.yml index f2aaf446d7..841b858da8 100644 --- a/test/fixtures/oauth/clients.yml +++ b/test/fixtures/oauth/clients.yml @@ -32,3 +32,13 @@ confidential_client: token_endpoint_auth_method: client_secret_post dynamically_registered: true trusted: false + +personal_access: + client_id: fizzy-personal-access + name: Personal Access Tokens + redirect_uris: [] + scopes: + - read + - write + dynamically_registered: false + trusted: true diff --git a/test/integration/personal_access_token_forms_test.rb b/test/integration/personal_access_token_forms_test.rb new file mode 100644 index 0000000000..4153b4a39a --- /dev/null +++ b/test/integration/personal_access_token_forms_test.rb @@ -0,0 +1,222 @@ +require "test_helper" + +# A personal access token takes one of two forms: issued by the personal access +# client, as every token made from now on is, or with no client at all, as every +# token made before that client existed is, and as production code that predates +# it still makes them. Each rule that sets personal tokens apart from OAuth +# grants must hold for both alike, so every test here runs once per form. +class PersonalAccessTokenFormsTest < ActionDispatch::IntegrationTest + include OauthAvailabilityTestHelper + + ENDPOINT = "https://fcm.googleapis.com/fcm/send/cli-device" + + { "client-less" => nil, "client-backed" => :personal_access }.each do |form, issuer| + test "a #{form} personal access token is personal, and no app's" do + token = personal_token(issuer) + + assert token.personal? + assert_not token.oauth? + assert_nil token.connected_app + assert_includes Identity::AccessToken.personal, token + assert_not_includes Identity::AccessToken.oauth, token + end + + test "a #{form} personal access token has the personal prefix, its plaintext, no expiry and no refresh token" do + token = personal_token(issuer) + + assert token.raw_token.start_with?("fizzy_pat_") + assert_equal token.raw_token, token.reload.token + assert_nil token.expires_at + assert_nil token.refresh_token_digest + assert_nil token.raw_refresh_token + end + + test "a #{form} personal access token is honored while OAuth is dark" do + token = personal_token(issuer) + + with_oauth_availability acceptance: false do + get user_path(users(:david)), env: bearer(token), as: :json + end + + assert_response :success + end + + test "a #{form} personal access token never lapses, however long it sits idle" do + token = personal_token(issuer) + + travel Identity::AccessToken::REFRESH_IDLE_LIMIT + 1.day + assert_not token.lapsed? + assert_not_includes Identity::AccessToken.lapsed, token + + Identity::AccessToken.cleanup + assert Identity::AccessToken.exists?(token.id) + + get user_path(users(:david)), env: bearer(token), as: :json + assert_response :success + end + + test "a #{form} personal access token is revoked by whoever presents it" do + [ {}, { client_id: oauth_clients(:mcp_client).client_id }, { client_id: Oauth::Client::PERSONAL_ACCESS_CLIENT_ID } ].each do |params| + token = personal_token(issuer) + + untenanted { post oauth_revocation_path, params: { token: token.raw_token, **params } } + + assert_response :success + assert_not Identity::AccessToken.exists?(token.id), "expected revocation with #{params}" + end + end + + test "a #{form} personal access token gets no refresh at the token endpoint" do + token = personal_token(issuer) + + untenanted do + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: token.raw_token, client_id: Oauth::Client::PERSONAL_ACCESS_CLIENT_ID } + end + + assert_response :bad_request + assert_equal "unauthorized_client", response.parsed_body["error"] + + untenanted do + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: token.raw_token, client_id: oauth_clients(:mcp_client).client_id } + end + + assert_response :bad_request + assert_equal "invalid_grant", response.parsed_body["error"] + assert_equal token.token_digest, token.reload.token_digest + end + + test "a #{form} personal access token manages personal access tokens" do + token = personal_token(issuer, permission: :write) + + untenanted do + post my_access_tokens_path, params: { access_token: { description: "Fizzy CLI", permission: "read" } }, env: bearer(token), as: :json + assert_response :created + + get my_access_tokens_path, env: bearer(token), as: :json + assert_response :success + assert_includes response.parsed_body.pluck("id"), token.id + end + end + + test "a #{form} personal access token is listed with personal access tokens, and not under Connected Apps" do + token = personal_token(issuer, description: "Shell script") + sign_in_as :david + + untenanted { get my_access_tokens_path } + assert_in_body "Shell script" + + get my_connected_apps_path + assert_response :success + assert_not_in_body "Shell script" + assert_not_in_body "Personal Access Tokens" + + delete my_connected_app_path(oauth_clients(:personal_access)) + assert_response :not_found + assert Identity::AccessToken.exists?(token.id) + end + + test "a #{form} personal access token is revoked from the personal access tokens page" do + token = personal_token(issuer) + sign_in_as :david + + untenanted { delete my_access_token_path(token), as: :json } + + assert_response :no_content + assert_not Identity::AccessToken.exists?(token.id) + end + + test "a webhook a #{form} personal access token sets up is attributed to the person alone" do + users(:kevin).update!(role: :admin) + token = personal_token(issuer, identity: :kevin, permission: :write) + + post board_webhooks_path(boards(:writebook)), params: { webhook: { name: "Feed", url: "https://app.example.com/hooks" } }, env: bearer(token), as: :json + + assert_response :created + webhook = Webhook.find(response.parsed_body["id"]) + assert_equal users(:kevin), webhook.creator + assert_nil webhook.created_via + end + + test "a push subscription a #{form} personal access token registers is no app's, and survives the token" do + stub_web_push_dns_resolution + token = personal_token(issuer, identity: :kevin, permission: :write) + + post user_push_subscriptions_path(users(:kevin)), params: { push_subscription: { endpoint: ENDPOINT, p256dh_key: "p256dh", auth_key: "auth" } }, env: bearer(token), as: :json + + assert_response :created + subscription = Push::Subscription.find_by!(endpoint: ENDPOINT) + assert_nil subscription.oauth_client + assert_includes Push::Subscription.deliverable_to(users(:kevin)), subscription + + token.destroy + assert Push::Subscription.exists?(subscription.id) + end + + test "a #{form} personal access token reaches an account only while its user is active there" do + token = personal_token(issuer) + + get boards_path, env: bearer(token), as: :json + assert_response :success + + users(:david).deactivate + + get boards_path, env: bearer(token), as: :json + assert_response :forbidden + end + + test "a #{form} personal access token without a digest is found by its plaintext and digested" do + token = personal_token(issuer) + token.update_columns token_digest: nil + + assert_equal token, Identity::AccessToken.find_honored(token.raw_token) + assert_equal TokenDigester.digest(token.raw_token), token.reload.token_digest + end + + test "issuing or revoking a #{form} personal access token leaves the issuer's retention clock alone" do + assert_no_changes -> { oauth_clients(:personal_access).reload.updated_at } do + personal_token(issuer).destroy + end + end + end + + test "a personal access token made from the page is issued by the personal access client" do + sign_in_as :david + + untenanted { post my_access_tokens_path, params: { access_token: { description: "Fizzy CLI", permission: "write" } }, as: :json } + + assert_response :created + assert_equal %w[ created_at description id permission token ], response.parsed_body.keys.sort + assert_equal oauth_clients(:personal_access), Identity::AccessToken.find(response.parsed_body["id"]).oauth_client + end + + test "the authorization endpoint refuses the personal access client" do + sign_in_as :david + + untenanted do + get new_oauth_authorization_path, params: { client_id: Oauth::Client::PERSONAL_ACCESS_CLIENT_ID, response_type: "code", + redirect_uri: "http://127.0.0.1:8888/callback", code_challenge: "challenge", code_challenge_method: "S256", state: "state" } + end + + assert_response :bad_request + assert_equal "unauthorized_client", response.parsed_body["error"] + end + + test "the token endpoint refuses the personal access client the code grant" do + untenanted do + post oauth_token_path, params: { grant_type: "authorization_code", code: "code", code_verifier: "verifier", + redirect_uri: "http://127.0.0.1:8888/callback", client_id: Oauth::Client::PERSONAL_ACCESS_CLIENT_ID } + end + + assert_response :bad_request + assert_equal "unauthorized_client", response.parsed_body["error"] + end + + private + def personal_token(issuer, identity: :david, **attributes) + identities(identity).access_tokens.create!(oauth_client: issuer && oauth_clients(issuer), permission: :read, **attributes) + end + + def bearer(token) + { "HTTP_AUTHORIZATION" => "Bearer #{token.raw_token}" } + end +end diff --git a/test/models/oauth/client_test.rb b/test/models/oauth/client_test.rb index fdbae59c0d..95856a1de4 100644 --- a/test/models/oauth/client_test.rb +++ b/test/models/oauth/client_test.rb @@ -445,6 +445,60 @@ class Oauth::ClientTest < ActiveSupport::TestCase assert_not Identity::AccessToken.exists?(grant.id) end + test "the personal access token issuer is the provisioned client" do + assert_equal oauth_clients(:personal_access), Oauth::Client.personal_access_issuer + end + + # A fresh install loads the schema instead of migrating. + test "the personal access token issuer is provisioned on first use where it's missing" do + Oauth::Client.personal_access.delete_all + + issuer = assert_difference("Oauth::Client.count", +1) { Oauth::Client.personal_access_issuer } + + assert issuer.personal_access? + assert issuer.trusted? + assert_not issuer.dynamically_registered? + assert_not issuer.confidential? + assert_empty issuer.redirect_uris + assert_equal issuer, Oauth::Client.personal_access_issuer + end + + test "the personal access token issuer may obtain no grant, and every other client the code and refresh grants" do + assert_empty oauth_clients(:personal_access).grant_types + assert_not oauth_clients(:personal_access).allows_grant_type?("authorization_code") + assert_not oauth_clients(:personal_access).allows_grant_type?("refresh_token") + + assert oauth_clients(:mcp_client).allows_grant_type?("authorization_code") + assert oauth_clients(:mcp_client).allows_grant_type?("refresh_token") + assert_not oauth_clients(:mcp_client).allows_grant_type?("client_credentials") + end + + test "the personal access token issuer has no redirect to send a code to" do + assert_not oauth_clients(:personal_access).allows_redirect?("http://127.0.0.1:8888/callback") + end + + test "only the personal access token issuer goes without redirect URIs" do + client = Oauth::Client.new(name: "Unregistered", client_id: "fizzy-personal-access-impostor", redirect_uris: []) + + assert_not client.valid? + assert client.errors[:redirect_uris].any? + end + + test "the personal access token issuer can't be destroyed, so its tokens can't cascade away with it" do + token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:personal_access)) + + assert_not oauth_clients(:personal_access).destroy + assert Oauth::Client.exists?(oauth_clients(:personal_access).id) + assert Identity::AccessToken.exists?(token.id) + end + + test "cleanup never sweeps the personal access token issuer, however long it goes unused" do + oauth_clients(:personal_access).update_columns updated_at: 1.year.ago + + Oauth::Client.cleanup + + assert Oauth::Client.exists?(oauth_clients(:personal_access).id) + end test "destroying a client with unsaved changes still destroys it and its grants" do client = Oauth::Client.create!(name: "Dirty", redirect_uris: %w[ http://127.0.0.1:8888/callback ]) From 0733a2dfd67a6d9e866b38976760a4a08da91a29 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 00:02:22 -0700 Subject: [PATCH 02/12] Provision the personal access client and backfill existing tokens onto it The client is created by migration, so self-hosted installs that migrate get it too, and on first use where the schema was loaded instead. The backfill only sets oauth_client_id, in batches with update_all: token strings, digests, reach and non-expiry stay, updated_at doesn't move, and nothing is re-issued. It reruns over client-less tokens written since. Down returns the tokens to the client-less form before removing the client. The digest backfill, which is rerun for tokens production writes in plaintext only, now keeps the plaintext of client-backed personal tokens as well. --- .../20261015120100_backfill_token_digests.rb | 9 +- ...016120000_create_personal_access_client.rb | 40 ++++++++ ...6120100_backfill_personal_access_client.rb | 39 ++++++++ db/schema.rb | 2 +- db/schema_sqlite.rb | 2 +- .../migrations/personal_access_client_test.rb | 97 +++++++++++++++++++ 6 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 db/migrate/20261016120000_create_personal_access_client.rb create mode 100644 db/migrate/20261016120100_backfill_personal_access_client.rb create mode 100644 test/migrations/personal_access_client_test.rb diff --git a/db/migrate/20261015120100_backfill_token_digests.rb b/db/migrate/20261015120100_backfill_token_digests.rb index 483514ddf8..b66da22e47 100644 --- a/db/migrate/20261015120100_backfill_token_digests.rb +++ b/db/migrate/20261015120100_backfill_token_digests.rb @@ -6,8 +6,10 @@ # 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 +# this code reaches it. That's any token without a client, or with the personal +# access client (CreatePersonalAccessClient), since a rerun may come after +# that client exists. OAuth access tokens are digest-only, so any plaintext one +# is cleared once digested. Refresh tokens and client secrets are moved # wholesale: a refresh token's plaintext is cleared once digested, and the # client secret column is dropped by the next migration. The refresh token # column stays, empty, until it's dropped beside `token`. @@ -31,10 +33,11 @@ class Client < ActiveRecord::Base def up refuse_duplicates AccessToken, :token + personal_client_ids = [ nil, *Client.where(client_id: "fizzy-personal-access").pluck(:id) ] 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) + token_digest: digest, token: (row.oauth_client_id.in?(personal_client_ids) ? row.token : nil) end # Each is gone once dropped, so a rerun skips it. diff --git a/db/migrate/20261016120000_create_personal_access_client.rb b/db/migrate/20261016120000_create_personal_access_client.rb new file mode 100644 index 0000000000..dbee65f11f --- /dev/null +++ b/db/migrate/20261016120000_create_personal_access_client.rb @@ -0,0 +1,40 @@ +# Provisions the first-party client that issues personal access tokens +# (Oauth::Client::PERSONAL_ACCESS_CLIENT_ID), on every install that migrates, +# SaaS or self-hosted. Oauth::Client.personal_access_issuer makes it on first +# use where the schema was loaded instead. It's never registered, so the sweep +# of abandoned registrations passes it by. +# +# Down hands its tokens back to the client-less form, which every version of +# the code reads as personal, before removing it. Neither direction moves +# updated_at. +class CreatePersonalAccessClient < ActiveRecord::Migration[8.2] + disable_ddl_transaction! + + CLIENT_ID = "fizzy-personal-access" + BATCH_SIZE = 500 + + class Client < ActiveRecord::Base + self.table_name = "oauth_clients" + end + + class AccessToken < ActiveRecord::Base + self.table_name = "identity_access_tokens" + end + + def up + unless Client.exists?(client_id: CLIENT_ID) + Client.create! client_id: CLIENT_ID, name: "Personal Access Tokens", trusted: true, dynamically_registered: false, + redirect_uris: [], scopes: %w[ read write ], token_endpoint_auth_method: "none" + end + end + + def down + if client = Client.find_by(client_id: CLIENT_ID) + AccessToken.where(oauth_client_id: client.id).in_batches(of: BATCH_SIZE) do |batch| + batch.update_all oauth_client_id: nil + end + + client.delete + end + end +end diff --git a/db/migrate/20261016120100_backfill_personal_access_client.rb b/db/migrate/20261016120100_backfill_personal_access_client.rb new file mode 100644 index 0000000000..fd1152cc05 --- /dev/null +++ b/db/migrate/20261016120100_backfill_personal_access_client.rb @@ -0,0 +1,39 @@ +# Gives every client-less personal access token the personal access client, +# so each is a one-off access token of that first-party client, as in bc3. +# Only oauth_client_id changes: the token string, its digest, its permission +# and its lack of expiry all stay, and nothing is re-issued. +# +# update_all leaves updated_at alone. It's no lapse clock for a personal token, +# but it's what people see as the token's history, and the backfill isn't use. +# +# Rerunnable: it touches only client-less rows, and its down is a no-op (the +# client's own migration, CreatePersonalAccessClient, undoes the assignment). +# Production code that predates the client keeps writing client-less tokens, +# which every version reads as personal, so +# `db:migrate:redo:primary VERSION=20261016120100` picks them up later. +class BackfillPersonalAccessClient < ActiveRecord::Migration[8.2] + disable_ddl_transaction! + + CLIENT_ID = "fizzy-personal-access" + BATCH_SIZE = 500 + + class Client < ActiveRecord::Base + self.table_name = "oauth_clients" + end + + class AccessToken < ActiveRecord::Base + self.table_name = "identity_access_tokens" + end + + def up + client_id = Client.where(client_id: CLIENT_ID).pick(:id) or raise "#{CLIENT_ID} isn't provisioned: run CreatePersonalAccessClient first" + + AccessToken.where(oauth_client_id: nil).in_batches(of: BATCH_SIZE) do |batch| + batch.update_all oauth_client_id: client_id + end + end + + def down + # CreatePersonalAccessClient's down returns the tokens to the client-less form. + end +end diff --git a/db/schema.rb b/db/schema.rb index 84cc4c91b7..58232ccd8e 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_15_120200) do +ActiveRecord::Schema[8.2].define(version: 2026_10_16_120100) 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 diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb index 1033aa3dc9..90797cce38 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_15_120200) do +ActiveRecord::Schema[8.2].define(version: 2026_10_16_120100) do create_table "accesses", id: :uuid, force: :cascade do |t| t.datetime "accessed_at" t.uuid "account_id", null: false diff --git a/test/migrations/personal_access_client_test.rb b/test/migrations/personal_access_client_test.rb new file mode 100644 index 0000000000..ed34974682 --- /dev/null +++ b/test/migrations/personal_access_client_test.rb @@ -0,0 +1,97 @@ +require "test_helper" +require Rails.root.join("db/migrate/20261015120100_backfill_token_digests") +require Rails.root.join("db/migrate/20261016120000_create_personal_access_client") +require Rails.root.join("db/migrate/20261016120100_backfill_personal_access_client") + +class PersonalAccessClientMigrationTest < ActiveSupport::TestCase + setup do + Oauth::Client.personal_access.delete_all + + @personal = identities(:david).access_tokens.create!(description: "Before the client", permission: :write) + @personal.update_columns updated_at: 200.days.ago.round + @grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + end + + test "provisions the issuer once" do + assert_difference("Oauth::Client.count", +1) { migrate CreatePersonalAccessClient, :up } + assert_no_difference("Oauth::Client.count") { migrate CreatePersonalAccessClient, :up } + + issuer = Oauth::Client.personal_access.sole + assert_equal "Personal Access Tokens", issuer.name + assert issuer.trusted? + assert_not issuer.dynamically_registered? + assert_equal "none", issuer.token_endpoint_auth_method + assert_empty issuer.grant_types + assert issuer.valid? + end + + test "the backfill gives client-less tokens the issuer, keeping their string, digest, reach, non-expiry and updated_at" do + kept = %w[ token token_digest refresh_token_digest permission expires_at description updated_at ] + before = @personal.reload.attributes.slice(*kept) + migrate CreatePersonalAccessClient, :up + + assert_no_changes -> { @grant.reload.attributes } do + migrate BackfillPersonalAccessClient, :up + end + + @personal.reload + assert_equal Oauth::Client.personal_access.sole, @personal.oauth_client + assert_equal before, @personal.attributes.slice(*kept) + assert_nil @personal.expires_at + assert @personal.personal? + + assert_equal @personal, Identity::AccessToken.find_honored(before["token"]) + assert @personal.allows?("POST") + end + + test "the backfill reruns over tokens written client-less since" do + migrate CreatePersonalAccessClient, :up + migrate BackfillPersonalAccessClient, :up + + later = identities(:kevin).access_tokens.create!(description: "From older code") + assert_nil later.oauth_client_id + + migrate BackfillPersonalAccessClient, :up + assert_equal Oauth::Client.personal_access.sole, later.reload.oauth_client + end + + test "the backfill refuses to run before the issuer exists" do + assert_raises(RuntimeError) { migrate BackfillPersonalAccessClient, :up } + assert_nil @personal.reload.oauth_client_id + end + + test "down hands the tokens back to the client-less form and removes the issuer" do + migrate CreatePersonalAccessClient, :up + migrate BackfillPersonalAccessClient, :up + + assert_no_changes -> { @personal.reload.updated_at } do + migrate BackfillPersonalAccessClient, :down + migrate CreatePersonalAccessClient, :down + end + + assert_nil @personal.reload.oauth_client_id + assert @personal.personal? + assert_equal oauth_clients(:mcp_client), @grant.reload.oauth_client + assert_not Oauth::Client.personal_access.exists? + end + + # BackfillTokenDigests is rerun to digest tokens production writes in + # plaintext only. Once those have the issuer, the rerun must still treat them + # as personal and keep the plaintext production reads. + test "rerunning the digest backfill keeps a client-backed personal token's plaintext" do + migrate CreatePersonalAccessClient, :up + migrate BackfillPersonalAccessClient, :up + @personal.update_columns token: "legacyPersonalToken0123456789", token_digest: nil + + migrate BackfillTokenDigests, :up + + @personal.reload + assert_equal "legacyPersonalToken0123456789", @personal.token + assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), @personal.token_digest + end + + private + def migrate(migration, direction) + ActiveRecord::Migration.suppress_messages { migration.new.migrate(direction) } + end +end From 51d155b0a8a68969d66183c5772f1939b7a176ce Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 00:02:25 -0700 Subject: [PATCH 03/12] Record personal access tokens as tokens of a first-party client --- docs/oauth/spec_conformance.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index 856dcffc5d..0595af530e 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -90,6 +90,7 @@ Status key: | Requirement | Status | |---|---| | Tokens are stored as HMAC-SHA256 digests under a server pepper, byte-compatible with bc3's `Oauth::TokenDigester`, and looked up by a unique index on the digest. Raw values are shown once: in the token response, the registration response, or the response that creates a personal access token | **Fixed** #3192 `oauth-token-digests`, step 1 of 3. OAuth access and refresh tokens and client secrets are digest-only, and a refresh retry within the grace window is answered from an encrypted copy of the successor pair that expires with the window. Personal access tokens are digested and, until step 3, still also kept in plaintext, because production code that reads only plaintext shares the database while beta runs this. One written without a digest is found by plaintext once and digested. Step 2 stops reading and writing plaintext; step 3 drops the column. | +| A personal access token is a one-off access token of a first-party client, `fizzy-personal-access` ("Personal Access Tokens"), in the same digest-stored table as every grant, as bc3's are of `bc-personal-access`. That client is provisioned by migration and on first use, is never self-registered (so the registration sweep skips it), and is listed for no grant type, so the authorization and token endpoints refuse it with `unauthorized_client`. A personal token is honored while OAuth is dark, never lapses, gets no expiry or refresh token, keeps the `fizzy_pat_` prefix and is revoked by possession, whichever form it takes. Existing tokens are backfilled in place: same string, reach and non-expiry, nothing re-issued | **Fixed** `oauth-pats-as-grants` (#3164 steps 1 to 3). Tokens with no client still read as personal, because production code that predates the client keeps writing them until it's deployed. Dropping that branch and making `oauth_client_id` NOT NULL wait until the backfill has run in production. Personal access tokens are still listed under personal access tokens, not Connected Apps, as in bc3 | | 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) @@ -111,7 +112,7 @@ Status key: | Missing token answers `invalid_request` JSON (§2.2.1) | **Fixed** #2296 `51155c5e3` | | Confidential clients authenticate, and the token must have been issued to the requesting client (§2.1) | **Fixed** #3082 `e80b5b615` "Authenticate clients at revocation and revoke only their own tokens". A public client names its `client_id`. Unknown tokens, and tokens belonging to another client, get 200 and nothing is revoked. | | A presented client secret that authenticates no client is refused with 400 `invalid_client` (401 + Basic if it came in a Basic header), whatever the token | **Deliberate**, and stricter than bc3: #3082 `c9375f186`, `29a5d118c`. A 200 would tell a client with a mistyped `client_id` that a live grant was dead. bc3 is being aligned to this. | -| A personal access token can be revoked here by whoever holds it. It has no client, so holding it is the credential; client credentials sent alongside must still authenticate | **Fixed** #3082 `98619ab8f` "Revoke personal access tokens at the revocation endpoint again". Revocation works the same for every kind of token, which helps incident response such as revoking a leaked token. It also keeps the endpoint ready if personal access tokens become OAuth tokens, as in bc3. | +| A personal access token can be revoked here by whoever holds it. It's issued by no client that can authenticate (none, or the first-party `fizzy-personal-access` client, which has no secret), so holding it is the credential; client credentials sent alongside must still authenticate | **Fixed** #3082 `98619ab8f` "Revoke personal access tokens at the revocation endpoint again". Revocation works the same for every kind of token, which helps incident response such as revoking a leaked token. It held unchanged when personal access tokens became tokens of the `fizzy-personal-access` client (`oauth-pats-as-grants`). | | Revocation is throttled like the token endpoint: 20 per minute per IP, then 429 `slow_down` (RFC 6749 §2.3.1, RFC 7009 §5) | **Fixed** #3082 `11e8e1ad8` "Throttle revocation like the token endpoint". Once revocation authenticated confidential clients, it became a place to guess secrets | ## RFC 8414 / RFC 9728: metadata From cc390c285f52d9fb644fa20f81d890fde611d8e0 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 00:46:56 -0700 Subject: [PATCH 04/12] Point the conformance record at #3205 --- docs/oauth/spec_conformance.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index 0595af530e..f7a31a89a5 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -90,7 +90,7 @@ Status key: | Requirement | Status | |---|---| | Tokens are stored as HMAC-SHA256 digests under a server pepper, byte-compatible with bc3's `Oauth::TokenDigester`, and looked up by a unique index on the digest. Raw values are shown once: in the token response, the registration response, or the response that creates a personal access token | **Fixed** #3192 `oauth-token-digests`, step 1 of 3. OAuth access and refresh tokens and client secrets are digest-only, and a refresh retry within the grace window is answered from an encrypted copy of the successor pair that expires with the window. Personal access tokens are digested and, until step 3, still also kept in plaintext, because production code that reads only plaintext shares the database while beta runs this. One written without a digest is found by plaintext once and digested. Step 2 stops reading and writing plaintext; step 3 drops the column. | -| A personal access token is a one-off access token of a first-party client, `fizzy-personal-access` ("Personal Access Tokens"), in the same digest-stored table as every grant, as bc3's are of `bc-personal-access`. That client is provisioned by migration and on first use, is never self-registered (so the registration sweep skips it), and is listed for no grant type, so the authorization and token endpoints refuse it with `unauthorized_client`. A personal token is honored while OAuth is dark, never lapses, gets no expiry or refresh token, keeps the `fizzy_pat_` prefix and is revoked by possession, whichever form it takes. Existing tokens are backfilled in place: same string, reach and non-expiry, nothing re-issued | **Fixed** `oauth-pats-as-grants` (#3164 steps 1 to 3). Tokens with no client still read as personal, because production code that predates the client keeps writing them until it's deployed. Dropping that branch and making `oauth_client_id` NOT NULL wait until the backfill has run in production. Personal access tokens are still listed under personal access tokens, not Connected Apps, as in bc3 | +| A personal access token is a one-off access token of a first-party client, `fizzy-personal-access` ("Personal Access Tokens"), in the same digest-stored table as every grant, as bc3's are of `bc-personal-access`. That client is provisioned by migration and on first use, is never self-registered (so the registration sweep skips it), and is listed for no grant type, so the authorization and token endpoints refuse it with `unauthorized_client`. A personal token is honored while OAuth is dark, never lapses, gets no expiry or refresh token, keeps the `fizzy_pat_` prefix and is revoked by possession, whichever form it takes. Existing tokens are backfilled in place: same string, reach and non-expiry, nothing re-issued | **Fixed** #3205 `oauth-pats-as-grants` (#3164 steps 1 to 3). Tokens with no client still read as personal, because production code that predates the client keeps writing them until it's deployed. Dropping that branch and making `oauth_client_id` NOT NULL wait until the backfill has run in production. Personal access tokens are still listed under personal access tokens, not Connected Apps, as in bc3 | | 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) @@ -112,7 +112,7 @@ Status key: | Missing token answers `invalid_request` JSON (§2.2.1) | **Fixed** #2296 `51155c5e3` | | Confidential clients authenticate, and the token must have been issued to the requesting client (§2.1) | **Fixed** #3082 `e80b5b615` "Authenticate clients at revocation and revoke only their own tokens". A public client names its `client_id`. Unknown tokens, and tokens belonging to another client, get 200 and nothing is revoked. | | A presented client secret that authenticates no client is refused with 400 `invalid_client` (401 + Basic if it came in a Basic header), whatever the token | **Deliberate**, and stricter than bc3: #3082 `c9375f186`, `29a5d118c`. A 200 would tell a client with a mistyped `client_id` that a live grant was dead. bc3 is being aligned to this. | -| A personal access token can be revoked here by whoever holds it. It's issued by no client that can authenticate (none, or the first-party `fizzy-personal-access` client, which has no secret), so holding it is the credential; client credentials sent alongside must still authenticate | **Fixed** #3082 `98619ab8f` "Revoke personal access tokens at the revocation endpoint again". Revocation works the same for every kind of token, which helps incident response such as revoking a leaked token. It held unchanged when personal access tokens became tokens of the `fizzy-personal-access` client (`oauth-pats-as-grants`). | +| A personal access token can be revoked here by whoever holds it. It's issued by no client that can authenticate (none, or the first-party `fizzy-personal-access` client, which has no secret), so holding it is the credential; client credentials sent alongside must still authenticate | **Fixed** #3082 `98619ab8f` "Revoke personal access tokens at the revocation endpoint again". Revocation works the same for every kind of token, which helps incident response such as revoking a leaked token. It held unchanged when personal access tokens became tokens of the `fizzy-personal-access` client (#3205). | | Revocation is throttled like the token endpoint: 20 per minute per IP, then 429 `slow_down` (RFC 6749 §2.3.1, RFC 7009 §5) | **Fixed** #3082 `11e8e1ad8` "Throttle revocation like the token endpoint". Once revocation authenticated confidential clients, it became a place to guess secrets | ## RFC 8414 / RFC 9728: metadata From 5fb52f716330bbc44f0ced92fb0e1943abf24859 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 00:59:56 -0700 Subject: [PATCH 05/12] Find the personal access issuer a concurrent first use provisioned Losing the race at the uniqueness validation raised RecordInvalid, which create_or_find_by! doesn't recover, and losing it at the unique index retried a find on every attribute, JSON columns included. Both now find the issuer by client_id. --- app/models/oauth/client.rb | 8 +++++++- test/models/oauth/client_test.rb | 21 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/app/models/oauth/client.rb b/app/models/oauth/client.rb index 8eb72e3b7a..6845cb10a2 100644 --- a/app/models/oauth/client.rb +++ b/app/models/oauth/client.rb @@ -76,8 +76,14 @@ def cleanup # A fresh install loads the schema rather than running migrations, so the # issuer may not exist yet when the first personal access token is made. + # Two first uses can race: the loser either hits the unique index, which + # create_or_find_by! answers by finding on client_id alone, or, when the + # winner committed before its uniqueness validation ran, fails that + # validation, and then finds the winner's row the same way. def personal_access_issuer - personal_access.take || create_or_find_by!(PERSONAL_ACCESS_ATTRIBUTES) + personal_access.take || create_or_find_by!(client_id: PERSONAL_ACCESS_CLIENT_ID) { it.assign_attributes PERSONAL_ACCESS_ATTRIBUTES } + rescue ActiveRecord::RecordInvalid + personal_access.take or raise end end diff --git a/test/models/oauth/client_test.rb b/test/models/oauth/client_test.rb index 95856a1de4..65af851fa4 100644 --- a/test/models/oauth/client_test.rb +++ b/test/models/oauth/client_test.rb @@ -463,6 +463,27 @@ class Oauth::ClientTest < ActiveSupport::TestCase assert_equal issuer, Oauth::Client.personal_access_issuer end + # Another first use committed the issuer after this one looked and before its + # uniqueness validation ran. + test "the personal access token issuer is found when a concurrent first use provisioned it" do + issuer = oauth_clients(:personal_access) + Oauth::Client.stubs(:personal_access).returns(Oauth::Client.none).then.returns(Oauth::Client.where(id: issuer.id)) + + assert_no_difference("Oauth::Client.count") do + assert_equal issuer, Oauth::Client.personal_access_issuer + end + end + + test "the personal access token issuer is found when a concurrent first use won the unique index" do + issuer = oauth_clients(:personal_access) + Oauth::Client.stubs(:personal_access).returns(Oauth::Client.none) + Oauth::Client.any_instance.stubs(:valid?).returns(true) + + assert_no_difference("Oauth::Client.count") do + assert_equal issuer, Oauth::Client.personal_access_issuer + end + end + test "the personal access token issuer may obtain no grant, and every other client the code and refresh grants" do assert_empty oauth_clients(:personal_access).grant_types assert_not oauth_clients(:personal_access).allows_grant_type?("authorization_code") From d4c8746988f22d5faece92696aebbdf510d4b25a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 02:26:17 -0700 Subject: [PATCH 06/12] Keep a read-only personal access token's write a 401, whichever form it takes #3158 answers a read-only grant's write with 403 and keeps 401 for personal tokens, telling them apart by whether they have a client. A token of the personal access client has one, so it reads the token's connected app instead. --- app/controllers/concerns/authentication.rb | 2 +- test/integration/personal_access_token_forms_test.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index caae9440da..861978829e 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -105,7 +105,7 @@ def authenticate_with_bearer_token Current.access_token = access_token Current.identity = access_token.identity elsif access_token - request_bearer_authentication status: access_token.oauth_client_id? ? :forbidden : :unauthorized, + request_bearer_authentication status: access_token.connected_app ? :forbidden : :unauthorized, error: "insufficient_scope", error_description: "The access token is read-only", scope: "write" else request_bearer_authentication error: "invalid_token", error_description: "The access token is expired, revoked, or invalid" diff --git a/test/integration/personal_access_token_forms_test.rb b/test/integration/personal_access_token_forms_test.rb index 4153b4a39a..fbe911fdfd 100644 --- a/test/integration/personal_access_token_forms_test.rb +++ b/test/integration/personal_access_token_forms_test.rb @@ -41,6 +41,18 @@ class PersonalAccessTokenFormsTest < ActionDispatch::IntegrationTest assert_response :success end + # Shipped clients (fizzy-cli, the native apps) have always had a 401 for a + # read-only personal token's write. Only an app's grant gets the 403. + test "a #{form} read-only personal access token's write is a 401, as shipped clients expect" do + token = personal_token(issuer) + + post boards_path(format: :json), params: { board: { name: "Not made" } }, env: bearer(token) + + assert_response :unauthorized + assert_match 'error="insufficient_scope"', response.headers["WWW-Authenticate"] + assert_match 'scope="write"', response.headers["WWW-Authenticate"] + end + test "a #{form} personal access token never lapses, however long it sits idle" do token = personal_token(issuer) From b090b1c60b82307d688a387585ebc861f03204c1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 03:00:18 -0700 Subject: [PATCH 07/12] Refuse to destroy the personal access issuer by its stored client_id #3159's lock_client no longer reloads the row, so the guard reads the client_id in the database rather than one changed in memory but not saved. --- app/models/oauth/client.rb | 4 +++- test/models/oauth/client_test.rb | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/models/oauth/client.rb b/app/models/oauth/client.rb index 6845cb10a2..1d36a5bff3 100644 --- a/app/models/oauth/client.rb +++ b/app/models/oauth/client.rb @@ -154,8 +154,10 @@ def lock_client access_tokens.reset end + # Reads the stored client_id: lock_client no longer reloads the row, so an + # unsaved change to the issuer's client_id mustn't let it cascade. def refuse_to_destroy_personal_access - if personal_access? + if client_id_in_database == PERSONAL_ACCESS_CLIENT_ID errors.add :base, "The personal access token issuer can't be deleted" throw :abort end diff --git a/test/models/oauth/client_test.rb b/test/models/oauth/client_test.rb index 65af851fa4..6c177e537c 100644 --- a/test/models/oauth/client_test.rb +++ b/test/models/oauth/client_test.rb @@ -513,6 +513,16 @@ class Oauth::ClientTest < ActiveSupport::TestCase assert Identity::AccessToken.exists?(token.id) end + test "the personal access token issuer can't be destroyed with unsaved changes either" do + issuer = oauth_clients(:personal_access) + token = identities(:david).access_tokens.create!(oauth_client: issuer) + issuer.assign_attributes client_id: "renamed", name: "Renamed" + + assert_not issuer.destroy + assert Oauth::Client.exists?(issuer.id) + assert Identity::AccessToken.exists?(token.id) + end + test "cleanup never sweeps the personal access token issuer, however long it goes unused" do oauth_clients(:personal_access).update_columns updated_at: 1.year.ago From 77276b042d7ee39ef657f15987d914c3ee180f32 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 04:14:46 -0700 Subject: [PATCH 08/12] Pin both personal token forms to the train's dark delivery and replica heal A personal token's push subscription has no app, so it delivers while OAuth is dark, and outside the primary datacenter an undigested personal token of either form authenticates without being healed. --- test/integration/personal_access_token_forms_test.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/integration/personal_access_token_forms_test.rb b/test/integration/personal_access_token_forms_test.rb index fbe911fdfd..a32b540b6d 100644 --- a/test/integration/personal_access_token_forms_test.rb +++ b/test/integration/personal_access_token_forms_test.rb @@ -159,6 +159,9 @@ class PersonalAccessTokenFormsTest < ActionDispatch::IntegrationTest subscription = Push::Subscription.find_by!(endpoint: ENDPOINT) assert_nil subscription.oauth_client assert_includes Push::Subscription.deliverable_to(users(:kevin)), subscription + with_oauth_availability acceptance: false do + assert_includes Push::Subscription.deliverable_to(users(:kevin)), subscription + end token.destroy assert Push::Subscription.exists?(subscription.id) @@ -184,6 +187,15 @@ class PersonalAccessTokenFormsTest < ActionDispatch::IntegrationTest assert_equal TokenDigester.digest(token.raw_token), token.reload.token_digest end + test "a #{form} personal access token without a digest authenticates outside the primary datacenter, undigested" do + token = personal_token(issuer) + token.update_columns token_digest: nil + Deployment::DatabaseResolver.stubs(:in_primary_datacenter?).returns(false) + + assert_equal token, Identity::AccessToken.find_honored(token.raw_token) + assert_nil token.reload.token_digest + end + test "issuing or revoking a #{form} personal access token leaves the issuer's retention clock alone" do assert_no_changes -> { oauth_clients(:personal_access).reload.updated_at } do personal_token(issuer).destroy From 570272808b86c5330755265c0e6304c59196177a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 04:21:24 -0700 Subject: [PATCH 09/12] Remove the personal access issuer before handing its tokens back A request that looked the issuer up before down removed it could issue a token after its batch was cleared, leaving it naming a client that's gone, which every version reads as an OAuth grant. The issuer now goes first and the batches repeat until no token names it. Down is for after code that doesn't know the issuer is deployed. --- ...016120000_create_personal_access_client.rb | 20 +++++++++++------ .../migrations/personal_access_client_test.rb | 22 +++++++++++++++++++ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/db/migrate/20261016120000_create_personal_access_client.rb b/db/migrate/20261016120000_create_personal_access_client.rb index dbee65f11f..54e847c6cb 100644 --- a/db/migrate/20261016120000_create_personal_access_client.rb +++ b/db/migrate/20261016120000_create_personal_access_client.rb @@ -4,9 +4,13 @@ # use where the schema was loaded instead. It's never registered, so the sweep # of abandoned registrations passes it by. # -# Down hands its tokens back to the client-less form, which every version of -# the code reads as personal, before removing it. Neither direction moves -# updated_at. +# Down removes it and hands its tokens back to the client-less form, which +# every version of the code reads as personal. Neither direction moves +# updated_at. Run it once code that doesn't know the client is deployed: +# code that does keeps issuing the client's tokens, and makes the client +# again on first use. The client goes first, so only a request that looked +# it up before then can still issue one of its tokens, and the batches repeat +# until none is left naming a client that's gone. class CreatePersonalAccessClient < ActiveRecord::Migration[8.2] disable_ddl_transaction! @@ -30,11 +34,13 @@ def up def down if client = Client.find_by(client_id: CLIENT_ID) - AccessToken.where(oauth_client_id: client.id).in_batches(of: BATCH_SIZE) do |batch| - batch.update_all oauth_client_id: nil - end - client.delete + + tokens = AccessToken.where(oauth_client_id: client.id) + + while tokens.exists? + tokens.in_batches(of: BATCH_SIZE) { |batch| batch.update_all oauth_client_id: nil } + end end end end diff --git a/test/migrations/personal_access_client_test.rb b/test/migrations/personal_access_client_test.rb index ed34974682..624427d916 100644 --- a/test/migrations/personal_access_client_test.rb +++ b/test/migrations/personal_access_client_test.rb @@ -78,6 +78,28 @@ class PersonalAccessClientMigrationTest < ActiveSupport::TestCase # BackfillTokenDigests is rerun to digest tokens production writes in # plaintext only. Once those have the issuer, the rerun must still treat them # as personal and keep the plaintext production reads. + # Code that knows the issuer may still be serving a request that looked it + # up just before down removed it. + test "down leaves no token naming the removed issuer, even one issued as it went" do + migrate CreatePersonalAccessClient, :up + migrate BackfillPersonalAccessClient, :up + in_flight = nil + # Mocha stubs take no implementation, so the matcher both deletes the + # issuer and issues the in-flight token, then matches. + CreatePersonalAccessClient::Client.any_instance.stubs(:delete).with do + client = CreatePersonalAccessClient::Client.find_by!(client_id: Oauth::Client::PERSONAL_ACCESS_CLIENT_ID) + CreatePersonalAccessClient::Client.where(id: client.id).delete_all + in_flight = identities(:david).access_tokens.create!(oauth_client_id: client.id) + end + + migrate CreatePersonalAccessClient, :down + + assert_not Oauth::Client.personal_access.exists? + assert_nil @personal.reload.oauth_client_id + assert_nil in_flight.reload.oauth_client_id + assert in_flight.personal? + end + test "rerunning the digest backfill keeps a client-backed personal token's plaintext" do migrate CreatePersonalAccessClient, :up migrate BackfillPersonalAccessClient, :up From d9c68dc3e7c6e05c8650886199c17cc70c2fc582 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 04:29:32 -0700 Subject: [PATCH 10/12] Hand the issuer's tokens back before removing it, and run down only once no code knows it Deleting the issuer first and repeating the batches only narrowed the window for a token issued alongside down: issuance takes no lock, and code that knows the issuer makes it again on first use. Down's precondition is what closes it. With that settled, clearing the tokens before removing the issuer makes an interrupted down safe to retry, since the issuer is still there to find. --- ...016120000_create_personal_access_client.rb | 24 +++++++++---------- .../migrations/personal_access_client_test.rb | 21 +++++++--------- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/db/migrate/20261016120000_create_personal_access_client.rb b/db/migrate/20261016120000_create_personal_access_client.rb index 54e847c6cb..d3f7b61470 100644 --- a/db/migrate/20261016120000_create_personal_access_client.rb +++ b/db/migrate/20261016120000_create_personal_access_client.rb @@ -4,13 +4,13 @@ # use where the schema was loaded instead. It's never registered, so the sweep # of abandoned registrations passes it by. # -# Down removes it and hands its tokens back to the client-less form, which -# every version of the code reads as personal. Neither direction moves -# updated_at. Run it once code that doesn't know the client is deployed: -# code that does keeps issuing the client's tokens, and makes the client -# again on first use. The client goes first, so only a request that looked -# it up before then can still issue one of its tokens, and the batches repeat -# until none is left naming a client that's gone. +# Down hands its tokens back to the client-less form, which every version of +# the code reads as personal, and then removes it. Neither direction moves +# updated_at. Run it only once no code with the client is serving: that code +# issues the client's tokens, takes no lock doing so, and makes the client +# again on first use, so nothing down could do would keep up with it. With +# that settled, the order makes down safe to retry: the client stays until +# every token is handed back, so a down that stops partway finds it again. class CreatePersonalAccessClient < ActiveRecord::Migration[8.2] disable_ddl_transaction! @@ -34,13 +34,11 @@ def up def down if client = Client.find_by(client_id: CLIENT_ID) - client.delete - - tokens = AccessToken.where(oauth_client_id: client.id) - - while tokens.exists? - tokens.in_batches(of: BATCH_SIZE) { |batch| batch.update_all oauth_client_id: nil } + AccessToken.where(oauth_client_id: client.id).in_batches(of: BATCH_SIZE) do |batch| + batch.update_all oauth_client_id: nil end + + client.delete end end end diff --git a/test/migrations/personal_access_client_test.rb b/test/migrations/personal_access_client_test.rb index 624427d916..1ed0765344 100644 --- a/test/migrations/personal_access_client_test.rb +++ b/test/migrations/personal_access_client_test.rb @@ -78,26 +78,21 @@ class PersonalAccessClientMigrationTest < ActiveSupport::TestCase # BackfillTokenDigests is rerun to digest tokens production writes in # plaintext only. Once those have the issuer, the rerun must still treat them # as personal and keep the plaintext production reads. - # Code that knows the issuer may still be serving a request that looked it - # up just before down removed it. - test "down leaves no token naming the removed issuer, even one issued as it went" do + test "a down that stops partway finds the issuer again and finishes" do migrate CreatePersonalAccessClient, :up migrate BackfillPersonalAccessClient, :up - in_flight = nil - # Mocha stubs take no implementation, so the matcher both deletes the - # issuer and issues the in-flight token, then matches. - CreatePersonalAccessClient::Client.any_instance.stubs(:delete).with do - client = CreatePersonalAccessClient::Client.find_by!(client_id: Oauth::Client::PERSONAL_ACCESS_CLIENT_ID) - CreatePersonalAccessClient::Client.where(id: client.id).delete_all - in_flight = identities(:david).access_tokens.create!(oauth_client_id: client.id) - end + second = identities(:kevin).access_tokens.create!(oauth_client: Oauth::Client.personal_access.sole) + ActiveRecord::Relation.any_instance.stubs(:update_all).raises(ActiveRecord::StatementInvalid, "interrupted") + assert_raises(ActiveRecord::StatementInvalid) { migrate CreatePersonalAccessClient, :down } + ActiveRecord::Relation.any_instance.unstub(:update_all) + + assert Oauth::Client.personal_access.exists? migrate CreatePersonalAccessClient, :down assert_not Oauth::Client.personal_access.exists? assert_nil @personal.reload.oauth_client_id - assert_nil in_flight.reload.oauth_client_id - assert in_flight.personal? + assert_nil second.reload.oauth_client_id end test "rerunning the digest backfill keeps a client-backed personal token's plaintext" do From e3d6d2afb29c6271dd52cbf91e36e7d16fa8d62f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 24 Oct 2026 14:21:57 -0700 Subject: [PATCH 11/12] Report a personal access token as pat whether or not it has the personal access client A token issued by the personal access client has an oauth_client_id, so the audit's "no client means pat" rule labelled new tokens by their client. Label by personal? and personal_access? instead. --- app/models/oauth/audit.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/models/oauth/audit.rb b/app/models/oauth/audit.rb index bc21f28122..ebe0e1cf8c 100644 --- a/app/models/oauth/audit.rb +++ b/app/models/oauth/audit.rb @@ -48,8 +48,12 @@ def record(event, grant: nil, client: grant&.oauth_client, identity_id: grant&.i end private + # A personal access token is a "pat" whether it predates the personal + # access client (no client) or was issued by it. def client_class(grant:, client:) - if client + if grant&.personal? || client&.personal_access? + "pat" + elsif client if client.dynamically_registered? "dcr" elsif client.trusted? @@ -57,8 +61,6 @@ def client_class(grant:, client:) else "partner" end - elsif grant && !grant.oauth_client_id? - "pat" end end end From 1c583ab343ae6329aa7dcb780c3e1fe3a2c80a79 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 25 Oct 2026 21:36:05 -0500 Subject: [PATCH 12/12] Audit a personal access token from the personal access client as a pat Its creation and revocation were told apart from a grant's by oauth_client_id?, which a token the personal access client issued has. Ask oauth? instead, as the rest of the model does. --- app/models/identity/access_token.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 2200cdd273..73aaca4ad6 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -272,7 +272,7 @@ def refresh(permission: self.permission) private def record_issuance - if oauth_client_id? + if oauth? Oauth::Audit.record "code.redeemed", grant: self, actor: "client" if authorization_code_jti? Oauth::Audit.record "grant.created", grant: self, actor: "client", permission: permission Oauth::Audit.record "token.issued", grant: self, actor: "client", grant_type: "authorization_code", permission: permission @@ -283,7 +283,7 @@ def record_issuance def record_revocation ending = revocation || { reason: cascade_reason, actor: "system" } - Oauth::Audit.record oauth_client_id? ? "grant.revoked" : "pat.revoked", grant: self, **ending + Oauth::Audit.record oauth? ? "grant.revoked" : "pat.revoked", grant: self, **ending end # A grant destroyed along with its identity or its client, which only the