From 0e61f98a3d34f808e28492c5fcf9946add4a2dee Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 03:20:36 -0700 Subject: [PATCH 1/3] Prefix newly issued access tokens, refresh tokens and personal access tokens New tokens start with a prefix that names their kind: - fizzy_pat_ for personal access tokens - fizzy_at_ for OAuth access tokens - fizzy_rt_ for OAuth refresh tokens A prefix lets a person, a log scrubber or a secret scanner recognize a Fizzy credential on sight. The random part is unchanged: 24 base58 characters from generate_unique_secure_token. Tokens issued before this have no prefix and still work, since every lookup matches the whole string. A grant issued before this gets prefixed tokens at its next rotation. Personal access tokens keep the token they were issued with. This follows Basecamp, which prefixes the tokens it issues (bc_at_, bc_rt_). --- app/models/identity/access_token.rb | 21 ++++++++++++++---- docs/oauth/spec_conformance.md | 6 +++++ test/integration/oauth_flow_test.rb | 27 +++++++++++++++++++++-- test/integration/oauth_revocation_test.rb | 12 ++++++++++ test/models/identity/access_token_test.rb | 19 ++++++++++++++++ 5 files changed, 79 insertions(+), 6 deletions(-) diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 62210c8f3a..1c260a8474 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -29,9 +29,14 @@ class Identity::AccessToken < ApplicationRecord scope :unlapsed, -> { where(updated_at: REFRESH_IDLE_LIMIT.ago..) } scope :lapsed, -> { oauth.where(updated_at: ...REFRESH_IDLE_LIMIT.ago) } - has_secure_token + # A prefix makes a token recognizable on sight, in logs, pastes and + # repositories, and names its kind. Tokens issued before prefixes carry none + # and keep working: every lookup matches the whole string. + PREFIXES = { personal: "fizzy_pat_", access: "fizzy_at_", refresh: "fizzy_rt_" } + enum :permission, %w[ read write ].index_by(&:itself), default: :read + before_create :set_token before_create :set_expiry_and_refresh_token, if: :oauth_client_id? # Issuing or destroying a grant restarts its client's retention period @@ -50,6 +55,10 @@ def find_honored(token) end end + def generate_token(kind) + PREFIXES.fetch(kind) + generate_unique_secure_token + end + # Each grant is rechecked under its lock, so one renewed after it was # selected (by code from before idle expiry, mid-deploy, whose rotation # doesn't check for lapse) is spared. @@ -106,8 +115,8 @@ def lapsed? # token is retired, not forgotten, so presenting it again is recognized as # a retry or a replay (see Oauth::RetiredRefreshToken). def refresh(permission: self.permission) - rotated = { token: self.class.generate_unique_secure_token, - refresh_token: self.class.generate_unique_secure_token, + rotated = { token: self.class.generate_token(:access), + refresh_token: self.class.generate_token(:refresh), expires_at: EXPIRES_IN.from_now, permission: permission, updated_at: Time.current } transaction do @@ -128,8 +137,12 @@ def lock_grant self.class.lock.where(id: id).pluck(:id) end + def set_token + self.token ||= self.class.generate_token(oauth_client_id? ? :access : :personal) + end + def set_expiry_and_refresh_token self.expires_at ||= EXPIRES_IN.from_now - self.refresh_token ||= self.class.generate_unique_secure_token + self.refresh_token ||= self.class.generate_token(:refresh) end end diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index fd89a1ad9c..cc9af3a2a1 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -84,6 +84,12 @@ Status key: | Secrets stored in plaintext, as access tokens are | **Deliberate** (#3082 body) | | 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 | +|---|---| +| 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 are not prefixed yet; bc3 uses `bc_cs_` for the ones its rake task issues. 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) | Requirement | Status | diff --git a/test/integration/oauth_flow_test.rb b/test/integration/oauth_flow_test.rb index 6e1aadde6d..13fc9df5be 100644 --- a/test/integration/oauth_flow_test.rb +++ b/test/integration/oauth_flow_test.rb @@ -524,8 +524,8 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_response :success body = response.parsed_body - assert_not_nil body["access_token"] - assert_not_nil body["refresh_token"] + assert body["access_token"].start_with?("fizzy_at_") + assert body["refresh_token"].start_with?("fizzy_rt_") assert_operator body["expires_in"], :>, 0 assert_equal "Bearer", body["token_type"] assert_equal "read", body["scope"] @@ -1166,6 +1166,29 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_not_equal old_refresh_token, body["refresh_token"] end + 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") + + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{grant.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 + end + + assert_response :success + assert response.parsed_body["access_token"].start_with?("fizzy_at_") + assert response.parsed_body["refresh_token"].start_with?("fizzy_rt_") + end + + test "a personal access token issued before prefixes still authenticates" do + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" }, as: :json + + assert_response :success + end + test "refresh grant for a token whose client is gone fails as invalid_grant" do client = oauth_clients(:mcp_client) token = identities(:david).access_tokens.create!(oauth_client: client) diff --git a/test/integration/oauth_revocation_test.rb b/test/integration/oauth_revocation_test.rb index 34af3825a0..ad7ef680dc 100644 --- a/test/integration/oauth_revocation_test.rb +++ b/test/integration/oauth_revocation_test.rb @@ -128,6 +128,18 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest assert Identity::AccessToken.exists?(token.id) end + 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") + + revoke personal.token + revoke grant.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) + end + 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 diff --git a/test/models/identity/access_token_test.rb b/test/models/identity/access_token_test.rb index a0f1a83654..2eda8c0594 100644 --- a/test/models/identity/access_token_test.rb +++ b/test/models/identity/access_token_test.rb @@ -16,6 +16,25 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase assert_not token.expired? end + test "new tokens carry a prefix naming their kind" do + 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]{24}\z/, personal.token) + assert_match(/\Afizzy_at_[1-9A-HJ-NP-Za-km-z]{24}\z/, grant.token) + assert_match(/\Afizzy_rt_[1-9A-HJ-NP-Za-km-z]{24}\z/, grant.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") + + assert grant.refresh + + assert grant.token.start_with?("fizzy_at_") + assert grant.refresh_token.start_with?("fizzy_rt_") + end + test "expired?" do token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) From 3c8f6664dedfb5554ac9a387aa7cd84cf64899f1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 04:37:05 -0700 Subject: [PATCH 2/3] Draw token bodies through Ruby's SecureRandom, not ActiveSupport's base58 generate_unique_secure_token calls SecureRandom.base58, an ActiveSupport extension that only has_secure_token requires. In a lazily loaded process (the dev server, a console) where this was the first secure-token model touched, minting or rotating a token could raise NoMethodError. Ruby's own SecureRandom.alphanumeric with the same base58 alphabet keeps the format and drops the dependency. --- app/models/identity/access_token.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 1c260a8474..672d0a3e19 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -34,6 +34,11 @@ class Identity::AccessToken < ApplicationRecord # and keep working: every lookup matches the whole string. PREFIXES = { personal: "fizzy_pat_", access: "fizzy_at_", refresh: "fizzy_rt_" } + # 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 + # extension, which only has_secure_token loads. + BASE58 = [ *"1".."9", *"A".."H", *"J".."N", *"P".."Z", *"a".."k", *"m".."z" ] + enum :permission, %w[ read write ].index_by(&:itself), default: :read before_create :set_token @@ -56,7 +61,7 @@ def find_honored(token) end def generate_token(kind) - PREFIXES.fetch(kind) + generate_unique_secure_token + PREFIXES.fetch(kind) + SecureRandom.alphanumeric(24, chars: BASE58) end # Each grant is rechecked under its lock, so one renewed after it was From 918f2566ddca90348a5eced35e8bb2da7155febf Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 00:11:58 -0700 Subject: [PATCH 3/3] Prefix client secrets, and draw every new credential from 256 bits Client secrets now carry fizzy_cs_, as bc3's carry bc_cs_, so secret scanning can recognize them too. Every newly issued token and secret has 44 base58 characters after its prefix, 257 bits, where 24 gave about 140: at least bc3's 32 bytes. Earlier credentials keep working, since every lookup matches the whole string. --- app/models/identity/access_token.rb | 13 ++++++++++--- app/models/oauth/client.rb | 2 +- docs/oauth/spec_conformance.md | 2 +- test/models/identity/access_token_test.rb | 6 +++--- test/models/oauth/client_test.rb | 2 +- 5 files changed, 16 insertions(+), 9 deletions(-) diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 672d0a3e19..c45b708d1a 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -31,14 +31,21 @@ class Identity::AccessToken < ApplicationRecord # A prefix makes a token recognizable on sight, in logs, pastes and # repositories, and names its kind. Tokens issued before prefixes carry none - # and keep working: every lookup matches the whole string. - PREFIXES = { personal: "fizzy_pat_", access: "fizzy_at_", refresh: "fizzy_rt_" } + # and keep working: every lookup matches the whole string. OAuth client + # 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_" } # 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 # extension, which only has_secure_token loads. BASE58 = [ *"1".."9", *"A".."H", *"J".."N", *"P".."Z", *"a".."k", *"m".."z" ] + # 44 base58 characters carry 257 bits, at least the 256 bits of randomness + # bc3's tokens have (32 bytes). Tokens issued earlier are shorter and keep + # working. + BODY_LENGTH = 44 + enum :permission, %w[ read write ].index_by(&:itself), default: :read before_create :set_token @@ -61,7 +68,7 @@ def find_honored(token) end def generate_token(kind) - PREFIXES.fetch(kind) + SecureRandom.alphanumeric(24, chars: BASE58) + PREFIXES.fetch(kind) + SecureRandom.alphanumeric(BODY_LENGTH, chars: BASE58) end # Each grant is rechecked under its lock, so one renewed after it was diff --git a/app/models/oauth/client.rb b/app/models/oauth/client.rb index 77852bc059..25a2d0cc8b 100644 --- a/app/models/oauth/client.rb +++ b/app/models/oauth/client.rb @@ -103,7 +103,7 @@ def lock_client end def generate_client_secret - self.client_secret ||= self.class.generate_unique_secure_token + self.client_secret ||= Identity::AccessToken.generate_token(:client_secret) end def redirect_uris_are_valid diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index cc9af3a2a1..0e747491e6 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -88,7 +88,7 @@ Status key: | Requirement | Status | |---|---| -| 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 are not prefixed yet; bc3 uses `bc_cs_` for the ones its rake task issues. Nobody has registered with GitHub's secret-scanning partner program, either here or 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) diff --git a/test/models/identity/access_token_test.rb b/test/models/identity/access_token_test.rb index 2eda8c0594..29c1f2ad8f 100644 --- a/test/models/identity/access_token_test.rb +++ b/test/models/identity/access_token_test.rb @@ -20,9 +20,9 @@ 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]{24}\z/, personal.token) - assert_match(/\Afizzy_at_[1-9A-HJ-NP-Za-km-z]{24}\z/, grant.token) - assert_match(/\Afizzy_rt_[1-9A-HJ-NP-Za-km-z]{24}\z/, grant.refresh_token) + 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) end test "a rotation issues prefixed tokens, even for a grant issued before prefixes" do diff --git a/test/models/oauth/client_test.rb b/test/models/oauth/client_test.rb index 5354b8cad6..928abf529f 100644 --- a/test/models/oauth/client_test.rb +++ b/test/models/oauth/client_test.rb @@ -246,7 +246,7 @@ class Oauth::ClientTest < ActiveSupport::TestCase token_endpoint_auth_method: "client_secret_post" ) assert client.confidential? - assert_not_nil client.client_secret + assert_match(/\Afizzy_cs_[1-9A-HJ-NP-Za-km-z]{44}\z/, client.client_secret) end test "public clients get no client secret" do