Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 14 additions & 35 deletions app/models/identity/access_token.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,15 @@ class Identity::AccessToken < ApplicationRecord

enum :permission, %w[ read write ].index_by(&:itself), default: :read

# Tokens are stored as digests (TokenDigester). The raw values exist only in
# memory, on the record that issued or rotated them, to be handed to their
# holder once. Personal access tokens also keep their plaintext in `token`
# for now, because production code that reads only `token` shares this
# database until this code is deployed there. OAuth access tokens never do.
# Tokens are stored only as digests (TokenDigester). The raw values exist
# only in memory, on the record that issued or rotated them, to be handed to
# their holder once.
#
# The plaintext `token` column is ignored until it's dropped. Without
# partial_inserts, a model that knows a column names it on every INSERT, so
# only code that ignores it can keep creating tokens once it's gone.
self.ignored_columns += %w[ token ]

attribute :raw_token, :string
attribute :raw_refresh_token, :string

Expand Down Expand Up @@ -87,14 +91,10 @@ def find_honored(token)
end
end

# Finds the token within the current scope by its digest. A personal
# access token written by code that stores only plaintext has no digest
# yet, so it's found by plaintext and given one: no lookup but the first
# ever matches on plaintext. Code that stores only plaintext issues
# prefixed tokens too, so a prefix doesn't exempt a token from this.
# Finds the token within the current scope by its digest.
def find_by_raw_token(raw)
if raw.is_a?(String) && raw.present?
find_by(token_digest: TokenDigester.digests(raw)) || find_and_digest_undigested(raw)
find_by(token_digest: TokenDigester.digests(raw))
Comment thread
jeremy marked this conversation as resolved.
end
end

Expand Down Expand Up @@ -128,24 +128,6 @@ def redeem(authorization_code, **attributes)
where(authorization_code_jti: authorization_code.jti).destroy_all
nil
end

private
# MySQL compares `token` case-insensitively, so a match is confirmed
# byte for byte: a differently cased token neither authenticates nor
# installs a digest that the real token would then never match.
#
# The digest goes to the primary even when the token was found on the
# replica, as GET requests read. update_columns leaves updated_at alone.
# A datacenter that can't reach the primary (Deployment::DatabaseResolver)
# still authenticates the token and leaves the digest to one that can,
# or to the backfill rerun.
def find_and_digest_undigested(raw)
personal.where(token_digest: nil, token: raw).detect { |candidate| candidate.token == raw }&.tap do |access_token|
if Deployment::DatabaseResolver.in_primary_datacenter?
ApplicationRecord.with_writing_role { access_token.update_columns token_digest: TokenDigester.digest(raw) }
end
end
end
end

# OAuth tokens are honored only while OAuth acceptance is on (see
Expand Down Expand Up @@ -181,7 +163,7 @@ def lapsed?
# answered with it.
def refresh(permission: self.permission)
raw_token, raw_refresh_token = self.class.generate_token(:access), self.class.generate_token(:refresh)
rotated = { token: nil, token_digest: TokenDigester.digest(raw_token),
rotated = { token_digest: TokenDigester.digest(raw_token),
refresh_token_digest: TokenDigester.digest(raw_refresh_token),
expires_at: EXPIRES_IN.from_now, permission: permission, updated_at: Time.current }

Expand Down Expand Up @@ -212,13 +194,10 @@ def lock_grant
self.class.lock.where(id: id).pluck(:id)
end

# Personal access tokens are written in plaintext as well as digested,
# for production code that reads only plaintext. An explicitly given
# token is digested like a generated one.
# An explicitly given raw 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 ||= self.class.generate_token(oauth_client_id? ? :access : :personal)
self.token_digest = TokenDigester.digest(raw_token)
self.token = oauth_client_id? ? nil : raw_token
end

def set_expiry_and_refresh_token
Expand Down
73 changes: 73 additions & 0 deletions db/migrate/20261017120000_backfill_remaining_token_digests.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# Digests every access token still without one, as BackfillTokenDigests did,
# before code that finds tokens only by digest is deployed. Until then,
# production code that stores only plaintext kept writing personal access
# tokens, and each was digested only when it was first used. One never used
# since is digested here, or it would stop authenticating.
#
# MySQL and SQLite have no HMAC, so this runs in Ruby, a batch at a time,
# outside one long transaction. update_all writes only the digest: a grant's
# updated_at is its idle-lapse clock (Identity::AccessToken::REFRESH_IDLE_LIMIT),
# and backfilling must not restart it.
#
# Personal access tokens keep their plaintext, as BackfillTokenDigests left
# it, until the column is dropped, so rolling back to code that reads only
# plaintext still finds them. A personal access token is one without a
# client, or one issued by the personal access client where that client
# exists. OAuth access tokens are digest-only, so any plaintext one is
# cleared once digested.
#
# Rerunnable: it only touches rows without a digest, and does nothing once the
# plaintext column is gone. Its down is a no-op, so
# `db:migrate:redo:primary VERSION=20261017120000` runs it again.
class BackfillRemainingTokenDigests < ActiveRecord::Migration[8.2]
disable_ddl_transaction!

BATCH_SIZE = 500

# Identity::AccessToken ignores the plaintext column, so this reads it
# through a model of its own.
class AccessToken < ActiveRecord::Base
self.table_name = "identity_access_tokens"
end

class Client < ActiveRecord::Base
self.table_name = "oauth_clients"
end

PERSONAL_ACCESS_CLIENT_ID = "fizzy-personal-access"

def up
if column_exists?(:identity_access_tokens, :token)
refuse_duplicate_tokens
personal_client_ids = [ nil, *Client.where(client_id: PERSONAL_ACCESS_CLIENT_ID).pluck(:id) ]

undigested.in_batches(of: BATCH_SIZE) do |batch|
batch.select(:id, :token, :oauth_client_id).each do |row|
AccessToken.where(id: row.id, token_digest: nil).update_all \
token_digest: TokenDigester.digest(row.token), token: (row.oauth_client_id.in?(personal_client_ids) ? row.token : nil)
end
end
end
end

def down
# Digests are derived from what's still stored: nothing to undo.
end

private
def undigested
AccessToken.where(token_digest: nil).where.not(token: nil)
end

# Two rows sharing a token would share a digest, which the unique index
# refuses partway through. Stop before writing anything instead, naming
# the rows but never the tokens.
def refuse_duplicate_tokens
duplicated = AccessToken.where.not(token: nil).group(:token).having("COUNT(*) > 1").pluck(:token)

if duplicated.any?
ids = AccessToken.where(token: duplicated).pluck(:id)
raise "identity_access_tokens.token has #{duplicated.size} duplicated value(s), in rows #{ids.join(", ")}. Resolve them before digesting."
end
end
end
2 changes: 1 addition & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion db/schema_sqlite.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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_17_120000) do
create_table "accesses", id: :uuid, force: :cascade do |t|
t.datetime "accessed_at"
t.uuid "account_id", null: false
Expand Down
3 changes: 2 additions & 1 deletion docs/oauth/spec_conformance.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ Snapshot: 2026-10-06, at the heads of the OAuth train, rebased onto main:
| [#3161](https://github.com/basecamp/fizzy/pull/3161) | `oauth-grant-idle-expiry` | 90-day idle expiry of grants (on #3160) |
| [#3159](https://github.com/basecamp/fizzy/pull/3159) | `oauth-client-sweep` | Sweep of abandoned DCR clients (on #3161) |
| [#3192](https://github.com/basecamp/fizzy/pull/3192) | `oauth-token-digests` | Tokens and client secrets stored as digests, step 1 of 3 (on #3169) |
| [#3202](https://github.com/basecamp/fizzy/pull/3202) | `oauth-token-digests-d2` | Personal access tokens stored and found only by digest, step 2 of 3 (on #3192) |

Commits are cited by subject, with the short SHA as of the snapshot. Rebases change
the SHAs of #3080 and above; the subjects stay the same.
Expand Down Expand Up @@ -89,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. |
| 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. Step 2 (#3202 `oauth-token-digests-d2`) makes personal access tokens digest-only too: they're written and found only by digest, after a rerun of the backfill digests any that production wrote in plaintext before step 1 reached it. The plaintext they held before then stays in a column the code ignores, until step 3 drops it. |
| 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)
Expand Down
2 changes: 1 addition & 1 deletion test/controllers/accounts/exports_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class Account::ExportsControllerTest < ActionDispatch::IntegrationTest
export = Account::Export.create!(account: Current.account, user: users(:jason))
export.build
sign_out
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).token}" }
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).raw_token}" }

get account_export_path(export), as: :json, env: bearer_token
assert_response :success
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class ActiveStorage::DirectUploadsControllerTest < ActionDispatch::IntegrationTe

post rails_direct_uploads_path,
params: @blob_params,
headers: bearer_token_header(identity_access_tokens(:davids_api_token).token),
headers: bearer_token_header(identity_access_tokens(:davids_api_token).raw_token),
as: :json

assert_response :success
Expand All @@ -27,7 +27,7 @@ class ActiveStorage::DirectUploadsControllerTest < ActionDispatch::IntegrationTe
test "create with valid access token" do
post rails_direct_uploads_path,
params: @blob_params,
headers: bearer_token_header(identity_access_tokens(:davids_api_token).token),
headers: bearer_token_header(identity_access_tokens(:davids_api_token).raw_token),
as: :json

assert_response :success
Expand Down Expand Up @@ -74,7 +74,7 @@ class ActiveStorage::DirectUploadsControllerTest < ActionDispatch::IntegrationTe
test "create with read-only access token" do
post rails_direct_uploads_path,
params: @blob_params,
headers: bearer_token_header(identity_access_tokens(:jasons_api_token).token),
headers: bearer_token_header(identity_access_tokens(:jasons_api_token).raw_token),
as: :json

assert_response :unauthorized
Expand Down Expand Up @@ -121,7 +121,7 @@ class ActiveStorage::DirectUploadsControllerTest < ActionDispatch::IntegrationTe
test "create with valid access token in another account is forbidden" do
post rails_direct_uploads_path(script_name: "/#{ActiveRecord::FixtureSet.identify("initech")}"),
params: @blob_params,
headers: bearer_token_header(identity_access_tokens(:davids_api_token).token),
headers: bearer_token_header(identity_access_tokens(:davids_api_token).raw_token),
as: :json

assert_response :forbidden
Expand Down
8 changes: 4 additions & 4 deletions test/controllers/api_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ class ApiTest < ActionDispatch::IntegrationTest
include OauthAvailabilityTestHelper

setup do
@davids_bearer_token = bearer_token_env(identity_access_tokens(:davids_api_token).token)
@jasons_bearer_token = bearer_token_env(identity_access_tokens(:jasons_api_token).token)
@davids_bearer_token = bearer_token_env(identity_access_tokens(:davids_api_token).raw_token)
@jasons_bearer_token = bearer_token_env(identity_access_tokens(:jasons_api_token).raw_token)
end

test "authenticate with user credentials" do
Expand Down Expand Up @@ -155,7 +155,7 @@ class ApiTest < ActionDispatch::IntegrationTest
end

test "the bearer scheme is matched case-insensitively" do
get boards_path(format: :json), env: { "HTTP_AUTHORIZATION" => "bearer #{identity_access_tokens(:davids_api_token).token}" }
get boards_path(format: :json), env: { "HTTP_AUTHORIZATION" => "bearer #{identity_access_tokens(:davids_api_token).raw_token}" }
assert_response :success

get boards_path(format: :json), env: { "HTTP_AUTHORIZATION" => "BEARER nonsense" }
Expand All @@ -164,7 +164,7 @@ class ApiTest < ActionDispatch::IntegrationTest
end

test "Bearer elsewhere in another scheme's credentials is not bearer authentication" do
get boards_path(format: :json), env: { "HTTP_AUTHORIZATION" => "Basic Bearer #{identity_access_tokens(:davids_api_token).token}" }
get boards_path(format: :json), env: { "HTTP_AUTHORIZATION" => "Basic Bearer #{identity_access_tokens(:davids_api_token).raw_token}" }

assert_response :redirect
end
Expand Down
6 changes: 3 additions & 3 deletions test/controllers/my/access_tokens_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest

test "create new token via JSON with bearer token" do
sign_out
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" }
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" }

assert_difference -> { identities(:david).access_tokens.count }, +1 do
post my_access_tokens_path, params: { access_token: { description: "Fizzy CLI", permission: "read" } }, env: bearer_token, as: :json
Expand Down Expand Up @@ -113,7 +113,7 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest

test "cannot create new token via JSON with read-only bearer token" do
sign_out
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).token}" }
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).raw_token}" }

assert_no_difference -> { identities(:jason).access_tokens.count } do
post my_access_tokens_path, params: { access_token: { description: "Fizzy CLI", permission: "read" } }, env: bearer_token, as: :json
Expand All @@ -131,7 +131,7 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest

test "index as JSON with bearer token and no account scope" do
sign_out
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" }
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" }

untenanted do
get my_access_tokens_path, as: :json, env: bearer_token
Expand Down
2 changes: 1 addition & 1 deletion test/controllers/search_rate_limiting_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,6 @@ class SearchRateLimitingTest < ActionDispatch::IntegrationTest

private
def bearer_token_header(fixture)
{ "Authorization" => "Bearer #{identity_access_tokens(fixture).token}" }
{ "Authorization" => "Bearer #{identity_access_tokens(fixture).raw_token}" }
end
end
4 changes: 2 additions & 2 deletions test/controllers/users_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
sign_in_as :jason
sign_out

bearer = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).token}" }
bearer = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).raw_token}" }
get user_path(users(:jason)), env: bearer

assert_response :unauthorized
Expand All @@ -222,7 +222,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
sign_in_as :jason
sign_out

bearer = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).token}" }
bearer = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).raw_token}" }
get user_path(users(:jason)), env: bearer, as: :json

assert_response :success
Expand Down
6 changes: 2 additions & 4 deletions test/fixtures/identity/access_tokens.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
jasons_api_token:
identity: jason
token: 018cf1425682700098f24f0799e3fe20
token_digest: <%= TokenDigester.digest("018cf1425682700098f24f0799e3fe20") %>
token_digest: <%= TokenDigester.digest(AccessTokenTestHelper::RAW_FIXTURE_TOKENS.fetch(:jasons_api_token)) %>
description: My Superscript
permission: read

davids_api_token:
identity: david
token: x18cf1425682700098f24f0799e3fe20
token_digest: <%= TokenDigester.digest("x18cf1425682700098f24f0799e3fe20") %>
token_digest: <%= TokenDigester.digest(AccessTokenTestHelper::RAW_FIXTURE_TOKENS.fetch(:davids_api_token)) %>
description: My Superscript
permission: write
6 changes: 3 additions & 3 deletions test/integration/active_storage_authorization_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class ActiveStorageAuthorizationTest < ActionDispatch::IntegrationTest
end

test "bearer token with board access can view blob" do
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" }
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" }

get rails_blob_path(@blob, disposition: :inline), env: bearer_token

Expand Down Expand Up @@ -48,7 +48,7 @@ class ActiveStorageAuthorizationTest < ActionDispatch::IntegrationTest
end

test "bearer token with board access can view representation" do
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" }
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" }

get rails_representation_path(@blob.representation(resize_to_limit: [ 100, 100 ])), env: bearer_token

Expand Down Expand Up @@ -247,7 +247,7 @@ class ActiveStorageAuthorizationTest < ActionDispatch::IntegrationTest

test "export owner can download their export with bearer token" do
blob = create_export_blob_for(users(:david))
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" }
bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" }

get rails_blob_path(blob, disposition: :attachment), env: bearer_token

Expand Down
Loading
Loading