diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 62210c8f3a..c45b708d1a 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -29,9 +29,26 @@ 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. 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 before_create :set_expiry_and_refresh_token, if: :oauth_client_id? # Issuing or destroying a grant restarts its client's retention period @@ -50,6 +67,10 @@ def find_honored(token) end end + def generate_token(kind) + PREFIXES.fetch(kind) + SecureRandom.alphanumeric(BODY_LENGTH, chars: BASE58) + 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 +127,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 +149,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/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 fd89a1ad9c..0e747491e6 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 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) | Requirement | Status | diff --git a/test/integration/oauth_flow_test.rb b/test/integration/oauth_flow_test.rb index 3530307d3d..4af1c0a05d 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..29c1f2ad8f 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]{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 + 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)) 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