Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Token reveal caching and legacy-token digest healing need correction before approval.
2 open findings
What changed in this PR
Implements the first stage of digest-based credential storage in Fizzy’s OAuth stack while preserving compatibility with existing personal access tokens.
Changes:
- Adds keyed digests, indexed lookups, legacy-token healing, and credential migrations.
- Stores refresh-retry credentials encrypted and reveals new personal tokens only in creation responses.
- Updates deployment secrets, documentation, and regression tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| test/models/token_digester_test.rb | Tests hashing, versions, and pepper requirements. |
| test/models/oauth/retired_refresh_token_test.rb | Tests encrypted successors and expiry. |
| test/models/oauth/client_test.rb | Verifies client-secret digests. |
| test/models/identity/access_token_test.rb | Tests digest storage, lookup, and healing. |
| test/migrations/backfill_token_digests_test.rb | Tests backfill safety and repeatability. |
| test/integration/oauth_revocation_test.rb | Updates revocation credential handling. |
| test/integration/oauth_refresh_replay_test.rb | Tests digest-based replay handling. |
| test/integration/oauth_grant_reach_test.rb | Uses in-memory bearer credentials. |
| test/integration/oauth_grant_idle_expiry_test.rb | Updates idle-expiry assertions for digests. |
| test/integration/oauth_flow_test.rb | Updates flows and tests secret authentication. |
| test/integration/oauth_delegate_resources_test.rb | Updates delegate-resource credentials. |
| test/integration/oauth_client_secret_basic_test.rb | Updates Basic authentication tests. |
| test/integration/oauth_availability_test.rb | Updates availability tests for digests. |
| test/fixtures/oauth/clients.yml | Provides a client-secret digest. |
| test/fixtures/identity/access_tokens.yml | Adds personal-token digests. |
| test/controllers/my/access_tokens_controller_test.rb | Tests reveal responses and cache headers. |
| saas/config/deploy.staging.yml | Exposes the staging pepper. |
| saas/config/deploy.production.yml | Exposes the production pepper. |
| saas/config/deploy.beta.yml | Exposes the beta pepper. |
| saas/.kamal/secrets.staging | Fetches the staging pepper. |
| saas/.kamal/secrets.production | Fetches the production pepper. |
| saas/.kamal/secrets.beta | Shares production’s pepper with beta. |
| docs/oauth/spec_conformance.md | Records digest-storage conformance decisions. |
| docs/kamal-deployment.md | Documents key-rotation consequences. |
| docs/docker-deployment.md | Explains independent pepper configuration. |
| db/schema.rb | Updates MySQL credential schema. |
| db/schema_sqlite.rb | Updates SQLite credential schema. |
| db/migrate/20261015120200_drop_plaintext_oauth_credentials.rb | Replaces plaintext OAuth storage. |
| db/migrate/20261015120100_backfill_token_digests.rb | Backfills existing credential digests. |
| db/migrate/20261015120000_add_token_digests.rb | Adds digest columns and indexes. |
| config/routes.rb | Removes token-reveal GET routing. |
| config/recurring.yml | Schedules successor-payload cleanup. |
| config/initializers/token_digester.rb | Validates the pepper at boot. |
| app/views/my/access_tokens/show.html.erb | Removes the previous reveal page. |
| app/views/my/access_tokens/new.html.erb | Adds the stream replacement target. |
| app/views/my/access_tokens/create.turbo_stream.erb | Renders the token through a stream. |
| app/views/my/access_tokens/create.html.erb | Adds the HTML creation response. |
| app/views/my/access_tokens/_reveal.html.erb | Shares token-reveal markup. |
| app/models/token_digester.rb | Implements versioned keyed digests. |
| app/models/oauth/retired_refresh_token.rb | Encrypts and expires retry credentials. |
| app/models/oauth/client.rb | Generates and verifies secret digests. |
| app/models/identity/access_token.rb | Implements digest lookup, healing, and rotation. |
| app/controllers/oauth/tokens_controller.rb | Uses digests and encrypted retry successors. |
| app/controllers/oauth/revocations_controller.rb | Finds revocable credentials by digest. |
| app/controllers/oauth/clients_controller.rb | Returns the in-memory registration secret. |
| app/controllers/my/access_tokens_controller.rb | Returns uncached, one-time reveal responses. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 124ce8da02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Review threads: 5 resolved (3 fixed, 2 declined with the reasoning in each thread).
Head 86e7fd7: CI green. OSS SQLite, OSS MySQL and SaaS MySQL suites also pass locally. The two new fallback tests failed on MySQL against the previous model. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
86e7fd7 to
5cf35e0
Compare
ec809fe to
2fd0056
Compare
|
Restacked onto #3169's 2fd0056 (the train is rebased on main). Conflicts with #3163's new |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cf35e0557
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2fd0056 to
aef9fb5
Compare
5cf35e0 to
1ef16f3
Compare
aef9fb5 to
898baf5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcb914cebf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7bf26ec to
ec7441a
Compare
dcb914c to
3a2bb79
Compare
Step 1 of 3 toward hashing credentials at rest. Access tokens, refresh tokens and client secrets are stored as HMAC-SHA256 digests (TokenDigester, byte-compatible with bc3's Oauth::TokenDigester) and looked up through a unique index on the digest. Personal access tokens are still written in plaintext too, because production code that reads only plaintext shares the database while beta runs this. A personal access token without a digest is found by plaintext once and given its digest; prefixed tokens never fall back. OAuth access tokens, refresh tokens and client secrets are digest-only, and their plaintext columns go. A refresh retry within the grace window is answered from an encrypted copy of the successor pair that expires with the window and is swept soon after. A new personal access token is shown only in the response that creates it, which isn't cached, rather than on a page that can be loaded again. SaaS deployments read the pepper from TOKEN_DIGEST_PEPPER and refuse to boot without it. Beta reads production's, since it writes production's database. Elsewhere it falls back to a key derived from secret_key_base.
Kamal hands a container only the secrets config/deploy.yml names, so a pepper added to .kamal/secrets alone never reached the app. The sample config doesn't list it, since a deploy whose secrets lack a listed one fails, and most self-hosted installs keep the derived pepper.
…h form The revealed token is temporary, so the snapshot taken on leaving held neither it nor the form it replaced, and Back restored an empty page.
…digests A grant keeps no plaintext, so three pins sent an empty Bearer token and passed as invalid_token without reaching the expiry, dark-server and scope checks they're named for.
82cdd19 to
73f186d
Compare
ec7441a to
5d41998
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive credential changes and shared-production migrations require human verification of database routing, migration sequencing, and pepper provisioning.
1 open issue
1 resolved since last review
Review effort: Balanced
| # replica, as GET requests read. update_columns leaves updated_at alone. | ||
| def find_and_digest_undigested(raw) | ||
| personal.where(token_digest: nil, token: raw).detect { |candidate| candidate.token == raw }&.tap do |access_token| | ||
| ApplicationRecord.with_writing_role { access_token.update_columns token_digest: TokenDigester.digest(raw) } |


Warning
This PR contains migrations. Don't merge it directly. It has to go through the beta integration branch (
oauth-integration-2026-10-15) and be migrated by hand on beta before it lands. beta1 runs againstfizzy_production(saas/config/database.ymlendsbeta: *production), so migrating on beta1 is migrating production. The rows to add to that branch's migration inventory are below; this PR doesn't push to the integration branch.Before beta boots this code,
TOKEN_DIGEST_PEPPERmust exist in 1Password, Deploy/Fizzy vault, on the Production item (beta's secrets read it from there too) and on the Staging item. A SaaS deploy without it failskamal secrets fetch, and the app refuses to boot. See Deploy prerequisites.Stacked on #3169 (train: #2296 → #3080 → #3081 → #3082 → #3160 → #3161 → #3159 → #3163 → #3169 → this).
Implements step D1 of hashing tokens at rest. The plan has three deploys, so no token stops working at any point:
main. OAuth credentials are digest-only.tokentoignored_columns.identity_access_tokens.token.What D1 does
TokenDigester(app/models/token_digester.rb)OpenSSL::HMAC.hexdigest("SHA256", pepper, raw): lowercase hex, byte-for-byte what bc3'sOauth::TokenDigester#digestcomputes. A test pins it to the published HMAC-SHA256 vector."<n>$<hex>", and a bare digest is version 1. Lookups go throughTokenDigester.digests(raw), one digest per pepper version in service, so a future v2 can be added without a flag day. Only v1 exists today.TOKEN_DIGEST_PEPPER. Outside development and test it's required, and must be at least 32 characters. The initializer checks at boot, so a deploy missing it fails before it serves anything. There's no fallback to a derived key, which would digest with a different key than every stored token, as bc3'sverify_pepper!also refuses to. Asset precompilation (SECRET_KEY_BASE_DUMMY) skips the check.TOKEN_DIGEST_PEPPERwhen it's set. Otherwise it usesRails.application.key_generator.generate_key("token_digest_pepper", 32), a key derived fromSECRET_KEY_BASE.docs/docker-deployment.mdnow says so: on a self-hosted install, rotatingSECRET_KEY_BASEinvalidates every personal access token and connected app unlessTOKEN_DIGEST_PEPPERis set separately.identity_access_tokenstoken_digestandrefresh_token_digest(string, limit 80), each with a unique index. The indexes are created in the schema migration, before any digest is written, so a duplicate fails loudly and no lookup ever scans the table. Before this,tokenhad no index, and every Bearer request scanned all ~3,500 rows.BackfillTokenDigests).disable_ddl_transaction!.update_all, soupdated_at, the grant idle clock from Lapse OAuth grants left idle for 90 days #3161, doesn't move.downdoes nothing.db:migrate:redo:primary VERSION=20261015120100runs it again.tokenandtoken_digest, because productionmainstill readstokenwhile beta runs this.tokenstays NULL.raw_token,raw_refresh_token), on the record that issued or rotated them.find_by_raw_tokenlooks up the digest first. If that misses, it falls back topersonal.where(token_digest: nil).find_by(token: raw)and heals the row it finds: the digest goes to the primary, even when the lookup ran on the replica as GET requests do, viaupdate_columns, soupdated_atdoesn't move.with_reading_rolefailed on MySQL withActiveRecord::ReadOnlyErrorbefore the fix.fizzy_pat_tokens without digests on beta today. A plaintext match is confirmed byte for byte, since MySQL comparestokencase-insensitively, so a differently cased token neither authenticates nor installs a wrong digest.oauth.find_by_raw_tokennever matches a personal token.OAuth-only plaintext columns: replaced now
These have never held production data, since the OAuth tables and columns have never served production traffic, and OAuth has never shipped on
main, so no self-hosted install has them either. So they're replaced in this step rather than kept for two more deploys. That holds only while this PR ships in the same release as the rest of the OAuth stack (#2296 onward).identity_access_tokens.refresh_token→refresh_token_digest.oauth_retired_refresh_tokensis rebuilt keyed by digest:refresh_token_digest,successor_refresh_token_digest, and an encryptedsuccessor_payload. Retired tokens exist only to recognize a replay, so they're dropped rather than converted. A replay of a token rotated before the migration is answered as an unknown token.oauth_clients.client_secret→client_secret_digest.Refresh replay grace with digests (
Oauth::RetiredRefreshToken). A retry within 60s used to answer from the grant's plaintext tokens. Now each rotation stores its successor pair in the retired row, the way bc3'srotate_to!storessuccessor_raw_token:MessageEncryptorkey derived fromsecret_key_base(saltoauth_refresh_token_successor, as in bc3).expires_in: GRACE, so it can't be decrypted after the window even if nothing blanks it.Oauth::RetiredRefreshToken.forget_successorsevery 5 minutes (config/recurring.yml). The sweep looks back one day, so it stays an index range over recent rows rather than a scan of 90 days of retired tokens.superseded?comparesrefresh_token_digestwithsuccessor_refresh_token_digest.invalid_grant"Refresh token no longer retryable", without revoking. That matches bc3's "Grace window token not available".Client secrets.
raw_client_secret).TokenDigester.verify, a constant-time compare of HMACs.has_secure_password :secret). Fizzy only issues generated secrets, 257 bits of randomness since Prefix newly issued access tokens, refresh tokens and personal access tokens #3163 (about 140 before it), so a deliberately slow hash adds no guessing resistance that the entropy doesn't already give, and one keyed digest covers every credential. The deviation is recorded indocs/oauth/spec_conformance.md. That doc's "Secrets stored in plaintext … Deliberate" line is now Fixed, and a Token format row records the digests.Personal access token: shown once
Creating a token used to redirect to
/my/access_tokens/<signed id>, which showed the raw token to anyone who loaded it within 10 seconds. Now:turbo_streamthat replaces the form, or an HTML 201 without Turbo.Cache-Control: no-store.showroute and action are gone.id,token,description,permission,created_at).Deploy prerequisites
TOKEN_DIGEST_PEPPERbefore anything running this code boots on beta, staging or production. Use at least 32 characters, e.g.bin/rails secret.saas/.kamal/secrets.beta, whichsecrets.beta1–4link to, fetchesProduction/TOKEN_DIGEST_PEPPER). Beta and production must share it, because beta writes digests into production's database. Reading the one field from both, rather than copying it into the Beta item, makes sharing structural. Beta deployers already read this vault.TokenDigester), not editing this one.kamal secrets fetchfails for that destination, and a container started without it refuses to boot (TokenDigester::MissingPepper).SECRET_KEY_BASEdiffers between beta and production (the Beta and Production items). It only keys the 60-second successor payload, so a retry answered by the other deployment would get "no longer retryable" rather than revocation. They're different hosts, so in practice this never happens.Migration inventory rows for the integration branch
For
docs/oauth/integration-migrations.mdonoauth-integration-2026-10-15. This PR doesn't change that branch. These rows extend its table, and itsmigrate:up/downlists, after #7:20261015120000_add_token_digests.rbchange_table :identity_access_tokens, bulk: true:token_digest,refresh_token_digest(string 80), a unique index on each.add_column :oauth_clients, :client_secret_digest(string 80)identity_access_tokens: ~3,500 rows20261015120100_backfill_token_digests.rbtokens first. Then oneUPDATE … WHERE id = ?per row:token_digestfor every token (OAuth rows also gettokenNULLed),refresh_token_digestandclient_secret_digestwhere those columns still exist.updated_atdoesn't move.disable_ddl_transaction!, batches of 500.downis a no-opidentity_access_tokens20261015120200_drop_plaintext_oauth_credentials.rbidentity_access_tokens.refresh_tokenand its unique index. Dropoauth_clients.client_secret. Drop and recreateoauth_retired_refresh_tokenskeyed by digest, withsuccessor_payloadtextidentity_access_tokens, plus the emptyoauth_*tablesoauth_*tables are empty in productionNotes for the runbook:
20261015120200,20261015120100(no-op),20261015120000. Undoing Bump rails from16d8b82to16f1222#10 restores empty plaintext columns and an empty retired-token table, not the old values. Undoing Bump rails from16d8b82to4fa5681#8 drops the digests, and the personal access tokens still hold their plaintext.16d8b82to4fa5681#8 to Bump rails from16d8b82to16f1222#10 in the same session as Bump rails from16d8b82tode4d874#1 to Spike running PostgreSQL in app container #7. Productionmainruns withpartial_insertsoff (load_defaults 8.1), so a process names every column it knows on INSERT. A production process that boots after Bump rails from16d8b82to6045f25#3 has addedrefresh_token, and is still running when Bump rails from16d8b82to16f1222#10 drops it, would fail to create personal access tokens. If production deployed or restarted in between, restart it after Bump rails from16d8b82to16f1222#10.fizzy_pat_token written without a digest is never found. Only pre-D1 stack code writes those: the integration branch without this PR, deployed to beta. So merge this PR into the integration branch before its migrations run on beta1. If anything else wrote such tokens after the backfill, rerun it withbin/rails db:migrate:redo:primary VERSION=20261015120100.mainkeeps writing personal access tokens without digests until D1 reaches production. Each is digested the first time it's used, from beta or, after D1 lands, from production. D2 starts by rerunning the backfill for any never used.recurring.ymlruns only Solid Queue maintenance, soforget_successorsdoesn't run from beta. OAuth is dark there, and the payload expires cryptographically anyway.Verified on thelio against a dedicated MySQL 8.4 container (SaaS):
updated_ats were unchanged.redoof the backfill. Each dump differs from the committeddb/schema.rb(or, after down, from End an app's own plumbing with its grant; keep what it set up for the account #3169's) in column order only, physical rather than sorted, the same as the integration doc found for Bump rails from16d8b82tode4d874#1–Spike running PostgreSQL in app container #7.Tests
New:
test/models/token_digester_test.rb:verifyrefuses an unknown version, a malformed prefix, blanks, nil and non-strings.test/models/identity/access_token_test.rb:updated_at, and the next lookup is digest-only.token_digestandrefresh_token_digest.test/migrations/backfill_token_digests_test.rb:updated_atis untouched.test/models/oauth/retired_refresh_token_test.rb:forget_successorsblanks only past the window.superseded?compares by digest.OauthRefreshReplayTest: a retry inside the window whose pair is gone is refused without revoking. The existing Revoke the grant when a rotated refresh token is replayed, with a 60s retry grace #3160 tests (same pair at 59s, revoke at exactly 60s, superseded, lost race, and so on) pass unchanged in substance.OauthFlowTest: a DCR-registered secret is stored only as its digest and authenticates with Basic.Oauth::ClientTestchecks the digest on create and none for public clients.My::AccessTokensControllerTest:no-store.no-store, and its token matches the stored digest.TokenDigester.digest(...)).Existing tests now use
raw_tokenandraw_refresh_tokenfor values held in memory, and compare digests where they used to reread plaintext. Anassert_not exists?(token: …)would now pass vacuously, so each becameexists?(token_digest: TokenDigester.digest(…)).#3191 pins. Merged
test/pin-shipped-client-authinto this branch locally (not pushed) and ranShippedClientAuthenticationTestand the SaaSAdmin::AuditsControllerTest: all pass, in OSS (8 tests) and SaaS (17 tests, auditor pins included). (The two pins the integration doc lists as failing are against #3158, which isn't below this PR.)Suites (thelio, gated on exit status):
DATABASE_ADAPTER=mysql)These ran at
239292edd. The head adds only the PR number to the conformance doc. System tests weren't run locally; CI runs them.RuboCop, Brakeman and gitleaks are clean.
Not done here, on purpose
fizzy_cs_prefix yet. That's out of scope and still the follow-up Prefix newly issued access tokens, refresh tokens and personal access tokens #3163 names.mainreads onlytokenuntil D1 is deployed there.