From 36fd94ecc3695a7d7851de43c4afbc4e64c0c0a2 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 00:01:25 -0700 Subject: [PATCH 1/6] Drop the plaintext personal access token column Step D3 of hashing tokens at rest. Tokens have been found only by digest, and the column ignored, since step D2, so this drops identity_access_tokens.token and the plaintext it still held for tokens issued before digests. Undoing it restores an empty, nullable column: the tokens can't be recovered from their digests. --- app/models/identity/access_token.rb | 9 +- ...00_drop_identity_access_token_plaintext.rb | 13 +++ db/schema.rb | 3 +- db/schema_sqlite.rb | 3 +- .../backfill_remaining_token_digests_test.rb | 103 ------------------ .../migrations/backfill_token_digests_test.rb | 85 --------------- test/models/identity/access_token_test.rb | 23 +--- 7 files changed, 20 insertions(+), 219 deletions(-) create mode 100644 db/migrate/20261017130000_drop_identity_access_token_plaintext.rb delete mode 100644 test/migrations/backfill_remaining_token_digests_test.rb delete mode 100644 test/migrations/backfill_token_digests_test.rb diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index a02f2fcea..4a246ff07 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -52,11 +52,10 @@ class Identity::AccessToken < ApplicationRecord # only in memory, on the record that issued or rotated them, to be handed to # their holder once. # - # The plaintext `token` and `refresh_token` columns are ignored until they're - # 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 refresh_token ] + # The plaintext `refresh_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[ refresh_token ] attribute :raw_token, :string attribute :raw_refresh_token, :string diff --git a/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb b/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb new file mode 100644 index 000000000..c42eca347 --- /dev/null +++ b/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb @@ -0,0 +1,13 @@ +# Drops the plaintext personal access tokens that BackfillTokenDigests and +# BackfillRemainingTokenDigests left beside their digests. Tokens are found +# only by digest, and Identity::AccessToken has ignored this column since +# those backfills, so no process running that code names it. +# +# Undoing this restores the column empty and nullable, as it was before any +# token was written. The tokens it held can't be recovered from their +# digests, so code that reads only plaintext can't authenticate them again. +class DropIdentityAccessTokenPlaintext < ActiveRecord::Migration[8.2] + def change + remove_column :identity_access_tokens, :token, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 670c8e164..4a3b1c49c 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_17_120000) do +ActiveRecord::Schema[8.2].define(version: 2026_10_17_130000) 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 @@ -361,7 +361,6 @@ t.string "permission" t.string "refresh_token" t.string "refresh_token_digest", limit: 80 - t.string "token" t.string "token_digest", limit: 80 t.datetime "updated_at", null: false t.index ["authorization_code_jti"], name: "index_identity_access_tokens_on_authorization_code_jti", unique: true diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb index 14117a38f..a138a9a97 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_17_120000) do +ActiveRecord::Schema[8.2].define(version: 2026_10_17_130000) do create_table "accesses", id: :uuid, force: :cascade do |t| t.datetime "accessed_at" t.uuid "account_id", null: false @@ -361,7 +361,6 @@ t.string "permission", limit: 255 t.string "refresh_token", limit: 255 t.string "refresh_token_digest", limit: 80 - t.string "token", limit: 255 t.string "token_digest", limit: 80 t.datetime "updated_at", null: false t.index ["authorization_code_jti"], name: "index_identity_access_tokens_on_authorization_code_jti", unique: true diff --git a/test/migrations/backfill_remaining_token_digests_test.rb b/test/migrations/backfill_remaining_token_digests_test.rb deleted file mode 100644 index 31d516ed2..000000000 --- a/test/migrations/backfill_remaining_token_digests_test.rb +++ /dev/null @@ -1,103 +0,0 @@ -require "test_helper" -require Rails.root.join("db/migrate/20261017120000_backfill_remaining_token_digests") - -class BackfillRemainingTokenDigestsTest < ActiveSupport::TestCase - # Production code that stores only plaintext kept writing personal access - # tokens after the first backfill, and one not used since has no digest. - setup do - @personal = identities(:david).access_tokens.create!(description: "Before digests") - store_plaintext_only @personal, "legacyPersonalToken0123456789", updated_at: 3.days.ago.round - - @grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) - store_plaintext_only @grant, "legacyAccessToken0123456789", updated_at: 80.days.ago.round - end - - test "digests every undigested token, keeping a personal token's plaintext and clearing a grant's" do - backfill - - @personal.reload - assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), @personal.token_digest - assert_equal "legacyPersonalToken0123456789", stored_plaintext(@personal) - - @grant.reload - assert_equal TokenDigester.digest("legacyAccessToken0123456789"), @grant.token_digest - assert_nil stored_plaintext(@grant) - end - - test "keeps the plaintext of a token issued by the personal access client" do - store_plaintext_only @grant, "legacyAccessToken0123456789", oauth_client_id: personal_access_client.id - - backfill - - assert_equal TokenDigester.digest("legacyAccessToken0123456789"), @grant.reload.token_digest - assert_equal "legacyAccessToken0123456789", stored_plaintext(@grant) - end - - # A grant's updated_at is its idle-lapse clock. - test "leaves updated_at alone" do - assert_no_changes -> { [ @personal.reload.updated_at, @grant.reload.updated_at ] } do - backfill - end - end - - test "leaves digested tokens alone, so it can run again" do - digested = identity_access_tokens(:davids_api_token) - - assert_no_changes -> { digested.reload.attributes } do - backfill - backfill - end - end - - test "the backfilled token authenticates by digest" do - backfill - - assert_equal @personal, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") - assert_equal @grant, Identity::AccessToken.find_by_raw_token("legacyAccessToken0123456789") - end - - # The first backfill kept personal access tokens' plaintext beside their - # digests, so a later token can duplicate one already digested. - test "refuses duplicate tokens before digesting anything, without naming them" do - copy = identities(:kevin).access_tokens.create!(description: "Digested", raw_token: "legacyPersonalToken0123456789") - BackfillRemainingTokenDigests::AccessToken.where(id: copy.id).update_all token: "legacyPersonalToken0123456789" - - error = assert_raises(RuntimeError) { backfill } - - assert_match @personal.id, error.message - assert_match copy.id, error.message - assert_no_match(/legacyPersonalToken/, error.message) - assert_nil @personal.reload.token_digest - assert_nil @grant.reload.token_digest - end - - test "does nothing once the plaintext column is gone" do - migration = BackfillRemainingTokenDigests.new.tap { |migration| migration.verbose = false } - migration.stubs(:column_exists?).with(:identity_access_tokens, :token).returns(false) - - migration.up - - assert_nil @personal.reload.token_digest - end - - private - # Identity::AccessToken ignores the plaintext column, so it's written and - # read through the migration's own model. - def store_plaintext_only(access_token, raw, **attributes) - BackfillRemainingTokenDigests::AccessToken.where(id: access_token.id).update_all token: raw, token_digest: nil, **attributes - end - - # The personal access client may not exist yet, so the grant's client stands in for it. - def personal_access_client - Oauth::Client.find_by(client_id: BackfillRemainingTokenDigests::PERSONAL_ACCESS_CLIENT_ID) || - oauth_clients(:mcp_client).tap { |client| client.update_columns client_id: BackfillRemainingTokenDigests::PERSONAL_ACCESS_CLIENT_ID } - end - - def stored_plaintext(access_token) - BackfillRemainingTokenDigests::AccessToken.find(access_token.id).token - end - - def backfill - BackfillRemainingTokenDigests.new.tap { |migration| migration.verbose = false }.up - end -end diff --git a/test/migrations/backfill_token_digests_test.rb b/test/migrations/backfill_token_digests_test.rb deleted file mode 100644 index 41f3b2504..000000000 --- a/test/migrations/backfill_token_digests_test.rb +++ /dev/null @@ -1,85 +0,0 @@ -require "test_helper" -require Rails.root.join("db/migrate/20261015120100_backfill_token_digests") - -class BackfillTokenDigestsTest < ActiveSupport::TestCase - setup do - @personal = identities(:david).access_tokens.create!(description: "Before digests") - store_plaintext_only @personal, "legacyPersonalToken0123456789", updated_at: 3.days.ago.round - - @grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) - store_plaintext_only @grant, "legacyAccessToken0123456789", updated_at: 80.days.ago.round - end - - test "digests every undigested token, keeping a personal token's plaintext and clearing a grant's" do - backfill - - @personal.reload - assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), @personal.token_digest - assert_equal "legacyPersonalToken0123456789", stored_plaintext(@personal) - - @grant.reload - assert_equal TokenDigester.digest("legacyAccessToken0123456789"), @grant.token_digest - assert_nil stored_plaintext(@grant) - end - - # A grant's updated_at is its idle-lapse clock. - test "leaves updated_at alone" do - assert_no_changes -> { [ @personal.reload.updated_at, @grant.reload.updated_at ] } do - backfill - end - end - - test "leaves digested tokens alone, so it can run again" do - digested = identity_access_tokens(:davids_api_token) - - assert_no_changes -> { digested.reload.attributes } do - backfill - backfill - end - end - - test "the backfilled token authenticates by digest" do - backfill - - assert_equal @personal, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") - assert_equal @grant, Identity::AccessToken.find_by_raw_token("legacyAccessToken0123456789") - end - - # The plaintext refresh token column outlives this stack, so it's emptied. - test "digests a grant's refresh token and clears its plaintext" do - BackfillTokenDigests::AccessToken.where(id: @grant.id).update_all refresh_token: "legacyRefreshToken0123456789", refresh_token_digest: nil - - backfill - - assert_equal @grant, Identity::AccessToken.find_by_refresh_token("legacyRefreshToken0123456789") - assert_nil BackfillTokenDigests::AccessToken.find(@grant.id).refresh_token - end - - test "refuses duplicate tokens before digesting anything, without naming them" do - copy = identities(:kevin).access_tokens.create!(description: "Copy") - store_plaintext_only copy, "legacyPersonalToken0123456789" - - error = assert_raises(RuntimeError) { backfill } - - assert_match @personal.id, error.message - assert_match copy.id, error.message - assert_no_match(/legacyPersonalToken/, error.message) - assert_nil @personal.reload.token_digest - assert_nil @grant.reload.token_digest - end - - private - # Identity::AccessToken ignores the plaintext column, so it's written and - # read through the migration's own model. - def store_plaintext_only(access_token, raw, **attributes) - BackfillTokenDigests::AccessToken.where(id: access_token.id).update_all token: raw, token_digest: nil, **attributes - end - - def stored_plaintext(access_token) - BackfillTokenDigests::AccessToken.find(access_token.id).token - end - - def backfill - BackfillTokenDigests.new.tap { |migration| migration.verbose = false }.up - end -end diff --git a/test/models/identity/access_token_test.rb b/test/models/identity/access_token_test.rb index 9582012a8..28e209acd 100644 --- a/test/models/identity/access_token_test.rb +++ b/test/models/identity/access_token_test.rb @@ -119,7 +119,6 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase assert_equal TokenDigester.digest(raw_token), token.token_digest assert_nil token.refresh_token_digest - assert_nil stored_plaintext(token) end test "an OAuth grant stores only digests of its tokens" do @@ -127,7 +126,6 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase raw_token, raw_refresh_token = grant.raw_token, grant.raw_refresh_token grant.reload - assert_nil stored_plaintext(grant) assert_equal TokenDigester.digest(raw_token), grant.token_digest assert_equal TokenDigester.digest(raw_refresh_token), grant.refresh_token_digest assert_nil grant.raw_token @@ -139,7 +137,6 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase raw_token, raw_refresh_token = grant.raw_token, grant.raw_refresh_token grant.reload - assert_nil stored_plaintext(grant) assert_equal TokenDigester.digest(raw_token), grant.token_digest assert_equal TokenDigester.digest(raw_refresh_token), grant.refresh_token_digest end @@ -166,22 +163,8 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase assert statements.none? { |sql| sql.match?(/\Wtoken\W*=/) }, statements.join("\n") end - # BackfillRemainingTokenDigests digests any token stored only in plaintext - # before this code reads by digest alone. - test "a token stored only in plaintext doesn't authenticate" do - token = identities(:david).access_tokens.create!(description: "Before digests") - Identity::AccessToken.where(id: token.id).update_all token: "legacyPersonalToken0123456789", token_digest: nil - - statements = capture_sql do - assert_nil Identity::AccessToken.find_honored("legacyPersonalToken0123456789") - end - assert statements.none? { |sql| sql.match?(/\Wtoken\W*=/) }, statements.join("\n") - assert_nil token.reload.token_digest - end - - test "the plaintext column is ignored" do + test "no token is stored in plaintext" do assert_not_includes Identity::AccessToken.column_names, "token" - assert_not Identity::AccessToken.new.respond_to?(:token) end # Dropped beside `token` only once every process runs code that ignores it. @@ -204,10 +187,6 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase end private - def stored_plaintext(access_token) - Identity::AccessToken.where(id: access_token.id).pick(Arel.sql("token")) - end - def capture_sql(&block) statements = [] subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") { |*, payload| statements << payload[:sql] } From 99ba98c8be81b4ea6cef2e27e43ecef5185af46a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 00:02:35 -0700 Subject: [PATCH 2/6] Record step 3 of token digests in the conformance record and deployment docs --- docs/docker-deployment.md | 2 +- docs/oauth/spec_conformance.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/docker-deployment.md b/docs/docker-deployment.md index b02508402..2db1bdc4e 100644 --- a/docs/docker-deployment.md +++ b/docs/docker-deployment.md @@ -48,7 +48,7 @@ Once you have one, set it in the `SECRET_KEY_BASE` environment variable: docker run --env SECRET_KEY_BASE=abcdefabcdef ... ``` -Personal access tokens and OAuth credentials are looked up by a keyed digest (HMAC-SHA256) rather than by the token itself. +Personal access tokens and OAuth credentials are stored only as a keyed digest (HMAC-SHA256), and looked up by it: the tokens themselves aren't kept. By default the key is derived from `SECRET_KEY_BASE`, so **changing `SECRET_KEY_BASE` invalidates every personal access token and every connected OAuth app**: people have to generate new tokens and reconnect their apps. To keep tokens working across a change of `SECRET_KEY_BASE`, set the key separately in `OAUTH_TOKEN_PEPPER` (at least 32 characters, for example from `bin/rails secret`). Set it before upgrading to a version that digests tokens, or before anyone creates one, and then keep it: changing, adding or removing it later invalidates tokens in the same way. diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index 2bd1bfbb3..0491b33fb 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -19,6 +19,7 @@ Snapshot: 2026-10-06, at the heads of the OAuth train, rebased onto main: | [#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) | +| [#3203](https://github.com/basecamp/fizzy/pull/3203) | `oauth-token-digests-d3` | Plaintext personal access token column dropped, step 3 of 3 (on #3202) | 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. @@ -90,7 +91,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. 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. | +| 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. Step 3 (#3203 `oauth-token-digests-d3`) drops the plaintext column, and with it the plaintext of tokens issued before digests: no token is stored in plaintext. | | Newly issued tokens carry a distinctive prefix naming their kind: `fizzy_pat_` (personal access token), `fizzy_at_` (OAuth access token), `fizzy_rt_` (OAuth refresh token). Tokens issued earlier have no prefix and still work | **Fixed** #3163. bc3 prefixes similarly (`bc_at_`, `bc_rt_`, and PATs as `bc_at_`). Client secrets carry `fizzy_cs_`, as bc3's carry `bc_cs_`. New tokens and secrets carry 44 base58 characters after the prefix, 257 bits, at least bc3's 32 bytes (rubric TK-1); earlier ones are 24 characters and keep working. Nobody has registered with GitHub's secret-scanning partner program, either here or in bc3. | ## Refresh tokens (RFC 6749 §6, OAuth 2.1 §4.3, RFC 9700 §4.14) From fecaa4a85a12ec53f869a7725ccb964a7c46a948 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 01:13:09 -0700 Subject: [PATCH 3/6] Rebuild the table when dropping the plaintext on MySQL, and name the refresh retry's encrypted pair An INSTANT drop only removes the column from the data dictionary, so a token's plaintext would stay in its row until the row is rewritten. The deployment docs now name the one credential kept other than as a digest: a refresh's successor pair, encrypted for the 60-second retry window. --- ...30000_drop_identity_access_token_plaintext.rb | 16 ++++++++++++++-- docs/docker-deployment.md | 1 + 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb b/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb index c42eca347..2566265d6 100644 --- a/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb +++ b/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb @@ -7,7 +7,19 @@ # token was written. The tokens it held can't be recovered from their # digests, so code that reads only plaintext can't authenticate them again. class DropIdentityAccessTokenPlaintext < ActiveRecord::Migration[8.2] - def change - remove_column :identity_access_tokens, :token, :string + def up + if connection.adapter_name.in?(%w[ Mysql2 Trilogy ]) + # MySQL would drop the column INSTANT, from the data dictionary alone, + # leaving each row's plaintext in the table's pages until that row is + # next rewritten, which for most tokens is never. Rebuilding the table + # writes every row without it, still without blocking reads or writes. + execute "ALTER TABLE identity_access_tokens DROP COLUMN token, ALGORITHM=INPLACE, LOCK=NONE" + else + remove_column :identity_access_tokens, :token + end + end + + def down + add_column :identity_access_tokens, :token, :string end end diff --git a/docs/docker-deployment.md b/docs/docker-deployment.md index 2db1bdc4e..3fbf7b5e7 100644 --- a/docs/docker-deployment.md +++ b/docs/docker-deployment.md @@ -49,6 +49,7 @@ docker run --env SECRET_KEY_BASE=abcdefabcdef ... ``` Personal access tokens and OAuth credentials are stored only as a keyed digest (HMAC-SHA256), and looked up by it: the tokens themselves aren't kept. +The one exception is the token pair an OAuth refresh issues, which is kept encrypted, readable only for the 60 seconds in which a client may retry that refresh, and cleared within minutes after. By default the key is derived from `SECRET_KEY_BASE`, so **changing `SECRET_KEY_BASE` invalidates every personal access token and every connected OAuth app**: people have to generate new tokens and reconnect their apps. To keep tokens working across a change of `SECRET_KEY_BASE`, set the key separately in `OAUTH_TOKEN_PEPPER` (at least 32 characters, for example from `bin/rails secret`). Set it before upgrading to a version that digests tokens, or before anyone creates one, and then keep it: changing, adding or removing it later invalidates tokens in the same way. From 6d1e545acf1a805b72becc61b78c382b19106d17 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 02:40:50 -0700 Subject: [PATCH 4/6] Say where plaintext tokens from before the upgrade may linger --- docs/docker-deployment.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/docker-deployment.md b/docs/docker-deployment.md index 3fbf7b5e7..66f283256 100644 --- a/docs/docker-deployment.md +++ b/docs/docker-deployment.md @@ -50,6 +50,7 @@ docker run --env SECRET_KEY_BASE=abcdefabcdef ... Personal access tokens and OAuth credentials are stored only as a keyed digest (HMAC-SHA256), and looked up by it: the tokens themselves aren't kept. The one exception is the token pair an OAuth refresh issues, which is kept encrypted, readable only for the 60 seconds in which a client may retry that refresh, and cleared within minutes after. +Tokens issued by versions that stored them in plaintext may still be readable in backups taken before the upgrade, and, on SQLite, in the database file's free pages until you run `VACUUM` (with the app stopped, and free disk space the size of the database). By default the key is derived from `SECRET_KEY_BASE`, so **changing `SECRET_KEY_BASE` invalidates every personal access token and every connected OAuth app**: people have to generate new tokens and reconnect their apps. To keep tokens working across a change of `SECRET_KEY_BASE`, set the key separately in `OAUTH_TOKEN_PEPPER` (at least 32 characters, for example from `bin/rails secret`). Set it before upgrading to a version that digests tokens, or before anyone creates one, and then keep it: changing, adding or removing it later invalidates tokens in the same way. From 9b670d8dd54bb258e4665b6faca9c445a8d8ad18 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 03:46:16 -0700 Subject: [PATCH 5/6] Name the code that must be running before the plaintext is dropped --- ...61017130000_drop_identity_access_token_plaintext.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb b/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb index 2566265d6..595187e06 100644 --- a/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb +++ b/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb @@ -1,7 +1,11 @@ # Drops the plaintext personal access tokens that BackfillTokenDigests and -# BackfillRemainingTokenDigests left beside their digests. Tokens are found -# only by digest, and Identity::AccessToken has ignored this column since -# those backfills, so no process running that code names it. +# BackfillRemainingTokenDigests left beside their digests. +# +# Run it only once every process runs the code that shipped with +# BackfillRemainingTokenDigests, or later. That code finds tokens only by +# digest and ignores this column, so it never names it. Code from before it +# still writes personal access tokens' plaintext, and falls back to reading +# it, so it fails once the column is gone. # # Undoing this restores the column empty and nullable, as it was before any # token was written. The tokens it held can't be recovered from their From a420fae0448b2e7bb04d7a2e5635fab4bfc49dd6 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 17 Oct 2026 22:42:43 -0700 Subject: [PATCH 6/6] Drop the plaintext refresh token column beside token identity_access_tokens.refresh_token stayed past DropPlaintextOauthCredentials, so that production code from before the OAuth stack never had a cached column dropped under it. Every process that runs this migration's prerequisite code ignores it, and BackfillTokenDigests emptied it, so it goes in the same table rebuild as token, with its unique index. A migration test caches the table's columns as the code before this migration does, runs the migration, and creates a token with them. --- app/models/identity/access_token.rb | 6 --- ...00_drop_identity_access_token_plaintext.rb | 24 ++++++--- db/schema.rb | 2 - db/schema_sqlite.rb | 2 - docs/oauth/spec_conformance.md | 4 +- ...op_identity_access_token_plaintext_test.rb | 50 +++++++++++++++++++ test/models/identity/access_token_test.rb | 9 +--- 7 files changed, 70 insertions(+), 27 deletions(-) create mode 100644 test/migrations/drop_identity_access_token_plaintext_test.rb diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 4a246ff07..d9ab0db63 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -51,12 +51,6 @@ class Identity::AccessToken < ApplicationRecord # 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 `refresh_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[ refresh_token ] - attribute :raw_token, :string attribute :raw_refresh_token, :string diff --git a/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb b/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb index 595187e06..0dbee629e 100644 --- a/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb +++ b/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb @@ -1,29 +1,37 @@ # Drops the plaintext personal access tokens that BackfillTokenDigests and -# BackfillRemainingTokenDigests left beside their digests. +# BackfillRemainingTokenDigests left beside their digests, and the plaintext +# refresh token column, empty since BackfillTokenDigests. # # Run it only once every process runs the code that shipped with # BackfillRemainingTokenDigests, or later. That code finds tokens only by -# digest and ignores this column, so it never names it. Code from before it +# digest and ignores both columns, so it never names them. Code from before it # still writes personal access tokens' plaintext, and falls back to reading -# it, so it fails once the column is gone. +# it, so it fails once the column is gone. Code from before +# DropPlaintextOauthCredentials, which doesn't ignore refresh_token, names it +# on every INSERT. # -# Undoing this restores the column empty and nullable, as it was before any -# token was written. The tokens it held can't be recovered from their +# Undoing this restores both columns empty and nullable, as they were before +# any token was written. The tokens they held can't be recovered from their # digests, so code that reads only plaintext can't authenticate them again. class DropIdentityAccessTokenPlaintext < ActiveRecord::Migration[8.2] def up if connection.adapter_name.in?(%w[ Mysql2 Trilogy ]) - # MySQL would drop the column INSTANT, from the data dictionary alone, + # MySQL would drop the columns INSTANT, from the data dictionary alone, # leaving each row's plaintext in the table's pages until that row is # next rewritten, which for most tokens is never. Rebuilding the table - # writes every row without it, still without blocking reads or writes. - execute "ALTER TABLE identity_access_tokens DROP COLUMN token, ALGORITHM=INPLACE, LOCK=NONE" + # writes every row without them, still without blocking reads or writes. + execute "ALTER TABLE identity_access_tokens DROP INDEX index_identity_access_tokens_on_refresh_token, " \ + "DROP COLUMN refresh_token, DROP COLUMN token, ALGORITHM=INPLACE, LOCK=NONE" else + remove_index :identity_access_tokens, :refresh_token, unique: true + remove_column :identity_access_tokens, :refresh_token remove_column :identity_access_tokens, :token end end def down add_column :identity_access_tokens, :token, :string + add_column :identity_access_tokens, :refresh_token, :string + add_index :identity_access_tokens, :refresh_token, unique: true end end diff --git a/db/schema.rb b/db/schema.rb index 4a3b1c49c..7e5ffc34f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -359,14 +359,12 @@ t.uuid "identity_id", null: false t.uuid "oauth_client_id" t.string "permission" - t.string "refresh_token" t.string "refresh_token_digest", limit: 80 t.string "token_digest", limit: 80 t.datetime "updated_at", null: false t.index ["authorization_code_jti"], name: "index_identity_access_tokens_on_authorization_code_jti", unique: true t.index ["identity_id"], name: "index_access_token_on_identity_id" t.index ["oauth_client_id"], name: "index_identity_access_tokens_on_oauth_client_id" - t.index ["refresh_token"], name: "index_identity_access_tokens_on_refresh_token", unique: true t.index ["refresh_token_digest"], name: "index_identity_access_tokens_on_refresh_token_digest", unique: true t.index ["token_digest"], name: "index_identity_access_tokens_on_token_digest", unique: true end diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb index a138a9a97..a0145cf3f 100644 --- a/db/schema_sqlite.rb +++ b/db/schema_sqlite.rb @@ -359,14 +359,12 @@ t.uuid "identity_id", null: false t.uuid "oauth_client_id" t.string "permission", limit: 255 - t.string "refresh_token", limit: 255 t.string "refresh_token_digest", limit: 80 t.string "token_digest", limit: 80 t.datetime "updated_at", null: false t.index ["authorization_code_jti"], name: "index_identity_access_tokens_on_authorization_code_jti", unique: true t.index ["identity_id"], name: "index_access_token_on_identity_id" t.index ["oauth_client_id"], name: "index_identity_access_tokens_on_oauth_client_id" - t.index ["refresh_token"], name: "index_identity_access_tokens_on_refresh_token", unique: true t.index ["refresh_token_digest"], name: "index_identity_access_tokens_on_refresh_token_digest", unique: true t.index ["token_digest"], name: "index_identity_access_tokens_on_token_digest", unique: true end diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index 0491b33fb..c8a13d709 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -19,7 +19,7 @@ Snapshot: 2026-10-06, at the heads of the OAuth train, rebased onto main: | [#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) | -| [#3203](https://github.com/basecamp/fizzy/pull/3203) | `oauth-token-digests-d3` | Plaintext personal access token column dropped, step 3 of 3 (on #3202) | +| [#3203](https://github.com/basecamp/fizzy/pull/3203) | `oauth-token-digests-d3` | Plaintext token columns dropped, step 3 of 3 (on #3202) | 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. @@ -91,7 +91,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. 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. Step 3 (#3203 `oauth-token-digests-d3`) drops the plaintext column, and with it the plaintext of tokens issued before digests: no token is stored in plaintext. | +| 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. Step 3 (#3203 `oauth-token-digests-d3`) drops the plaintext columns (personal access tokens', and refresh tokens', empty since step 1), and with them the plaintext of tokens issued before digests: no token is stored in plaintext. | | 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/migrations/drop_identity_access_token_plaintext_test.rb b/test/migrations/drop_identity_access_token_plaintext_test.rb new file mode 100644 index 000000000..845a62e16 --- /dev/null +++ b/test/migrations/drop_identity_access_token_plaintext_test.rb @@ -0,0 +1,50 @@ +require "test_helper" +require Rails.root.join("db/migrate/20261017130000_drop_identity_access_token_plaintext") + +# DropIdentityAccessTokenPlaintext runs while processes still serve the code +# that shipped with BackfillRemainingTokenDigests, which ignores both plaintext +# columns. With partial_inserts off, a process names every column it has cached +# on each INSERT, so one that loaded identity_access_tokens' columns before the +# migration must still create tokens after it. +class DropIdentityAccessTokenPlaintextTest < ActiveSupport::TestCase + # Each step is DDL, which MySQL commits on its own. + self.use_transactional_tests = false + + # identity_access_tokens as the code before this migration models it. + class PreviousAccessToken < ApplicationRecord + self.table_name = "identity_access_tokens" + self.ignored_columns = %w[ token refresh_token ] + end + + setup do + migrate :down + PreviousAccessToken.reset_column_information.then { PreviousAccessToken.columns } + end + + teardown do + migrate :up if ActiveRecord::Base.connection.column_exists?(:identity_access_tokens, :token) + [ PreviousAccessToken, Identity::AccessToken ].each(&:reset_column_information) + PreviousAccessToken.where(description: "Cached before the migration").delete_all + end + + test "drops both plaintext columns" do + migrate :up + + assert_not ActiveRecord::Base.connection.column_exists?(:identity_access_tokens, :token) + assert_not ActiveRecord::Base.connection.column_exists?(:identity_access_tokens, :refresh_token) + end + + test "a process that cached identity_access_tokens' columns before it creates tokens after it" do + migrate :up + + assert_difference -> { PreviousAccessToken.count } do + PreviousAccessToken.create! identity_id: identities(:david).id, permission: "write", description: "Cached before the migration", + token_digest: TokenDigester.digest(SecureRandom.base58(24)) + end + end + + private + def migrate(direction) + DropIdentityAccessTokenPlaintext.new.tap { |migration| migration.verbose = false }.migrate(direction) + end +end diff --git a/test/models/identity/access_token_test.rb b/test/models/identity/access_token_test.rb index 28e209acd..3302ae64a 100644 --- a/test/models/identity/access_token_test.rb +++ b/test/models/identity/access_token_test.rb @@ -164,13 +164,8 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase end test "no token is stored in plaintext" do - assert_not_includes Identity::AccessToken.column_names, "token" - end - - # Dropped beside `token` only once every process runs code that ignores it. - test "the plaintext refresh token column is ignored" do - assert_not_includes Identity::AccessToken.column_names, "refresh_token" - assert_not Identity::AccessToken.new.respond_to?(:refresh_token) + assert_not Identity::AccessToken.connection.column_exists?(:identity_access_tokens, :token) + assert_not Identity::AccessToken.connection.column_exists?(:identity_access_tokens, :refresh_token) end test "two tokens can't share a digest" do