Repository navigation
Conversation
|
@codex review |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8786a9dcc
ℹ️ 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".
There was a problem hiding this comment.
🔵 Needs a closer look
A human must confirm D1 is deployed everywhere and the production backfill has completed before digest-only authentication serves traffic.
No open issues 🎉
What changed in this PR
Implements D2 of the token-digest rollout, moving personal access token reads and writes to digests while preparing for D3’s plaintext-column removal.
Changes:
- Adds a rerunnable backfill for remaining plaintext-only tokens.
- Removes plaintext authentication fallback and writes; ignores the retained column.
- Updates fixtures, regression tests, and conformance documentation.
[!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/test_helpers/access_token_test_helper.rb | Supplies raw fixture tokens in memory. |
| test/test_helper.rb | Includes the token fixture helper. |
| test/models/identity/access_token_test.rb | Tests digest-only storage and authentication. |
| test/migrations/backfill_token_digests_test.rb | Accesses plaintext through the migration model. |
| test/migrations/backfill_remaining_token_digests_test.rb | Tests backfill safety and rerunnability. |
| test/integration/oauth_revocation_test.rb | Creates legacy tokens through raw_token. |
| test/integration/oauth_grant_reach_test.rb | Uses raw fixture credentials. |
| test/integration/oauth_flow_test.rb | Preserves legacy-token authentication coverage. |
| test/integration/oauth_availability_test.rb | Uses raw credentials with OAuth disabled. |
| test/integration/active_storage_authorization_test.rb | Updates bearer credentials for download tests. |
| test/fixtures/identity/access_tokens.yml | Stores only token digests. |
| test/controllers/users_controller_test.rb | Updates bearer authentication tests. |
| test/controllers/search_rate_limiting_test.rb | Updates bearer credential helper. |
| test/controllers/my/access_tokens_controller_test.rb | Updates token-management authentication tests. |
| test/controllers/api_test.rb | Uses raw fixture bearer tokens. |
| test/controllers/active_storage/direct_uploads_controller_test.rb | Updates upload authentication tests. |
| test/controllers/accounts/exports_controller_test.rb | Updates export authentication test. |
| docs/oauth/spec_conformance.md | Documents D2’s digest-only behavior. |
| db/schema.rb | Advances MySQL schema version. |
| db/schema_sqlite.rb | Advances SQLite schema version. |
| db/migrate/20261016120000_backfill_remaining_token_digests.rb | Backfills remaining digests without changing timestamps. |
| app/models/identity/access_token.rb | Stops plaintext reads and writes; ignores the column. |
Review effort: Balanced
5cf35e0 to
1ef16f3
Compare
c8786a9 to
135b057
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
1ef16f3 to
2bf7e87
Compare
135b057 to
b6675c2
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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". |
|
Review threads: 1 resolved (0 fixed, 1 declined with the reasoning in the thread). |
2bf7e87 to
dcb914c
Compare
7a68c24 to
22b9b57
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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". |
73f186d to
1b3ab1f
Compare
407d452 to
5c3c9cd
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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". |
1b3ab1f to
4cfbbd2
Compare
5c3c9cd to
4cd1b5e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
Step D2 of hashing tokens at rest. Personal access tokens are no longer written in plaintext, and a token without a digest no longer falls back to a plaintext lookup. Identity::AccessToken ignores the token column, so every process running this code leaves it out of its INSERTs and SELECTs, ready for the column to be dropped. BackfillRemainingTokenDigests reruns the backfill first: production code that stores only plaintext kept writing personal access tokens until step D1 reached it, and one never used since has no digest. Fixture tokens now carry their raw value in memory, as raw_token, the way an issued token does.
… renumber it A personal access token may be issued by the personal access client rather than carrying no client, so the rerun keeps the plaintext of either, as BackfillTokenDigests does once that client exists. Renumbered to 20261017120000: 20261016120000 is taken by CreatePersonalAccessClient on a sibling branch.
4cd1b5e to
3ac03cc
Compare
|
@codex review |
|
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". |
Caution
Explicitly gated. Don't merge this until production is running D1 (#3192), on every production container, and the backfill rerun in this PR (
20261017120000) has run againstfizzy_production, digesting every personal access token productionmainwrote in plaintext meanwhile. This code stops falling back to plaintext: a token productionmainwrote without a digest, and never used since, stops authenticating the moment this code serves it, unless the rerun has digested it first.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.Stacked on #3192 (train: #2296 → #3080 → #3081 → #3082 → #3160 → #3161 → #3159 → #3163 → #3169 → #3192 → this → #3203).
Implements step D2 of hashing tokens at rest (rubric TK-2, bearer artifacts stored only as digests):
maintokenidentity_access_tokens.tokenWhat D2 does
BackfillRemainingTokenDigests). The same digesting as D1'sBackfillTokenDigests, for the one column left: every row with atokenand notoken_digestgets its digest, and an OAuth row's plaintext is cleared (none should exist). It's self-contained, with its own model, rather than requiring D1's migration file, so loading both in onedb:migratedoesn't redefine a class.update_all, oneUPDATE … WHERE id = ? AND token_digest IS NULLper row, soupdated_at, the grant idle clock, doesn't move.disable_ddl_transaction!.downis a no-op, sobin/rails db:migrate:redo:primary VERSION=20261017120000runs it again.client_idfizzy-personal-access), not only of one with no client. This rerun does the same: it looks the client up by thatclient_idand treats its tokens as personal, so whichever order the two land in, no personal token loses its plaintext before D3. Before Issue personal access tokens from a first-party client, and backfill existing ones onto it #3205 the lookup finds nothing and only client-less tokens count as personal, as in D1. A test stands a fixture client in for it. Versions don't collide either: Issue personal access tokens from a first-party client, and backfill existing ones onto it #3205 takes20261016120000and20261016120100, so this is20261017120000(and D3's20261017130000). In the model, Issue personal access tokens from a first-party client, and backfill existing ones onto it #3205 rewritesset_tokento branch onpersonal?; this PR removes the lines it rewrites, so whichever rebases second keeps Issue personal access tokens from a first-party client, and backfill existing ones onto it #3205'spersonal?branch and dropstoken.presenceand the plaintext write. A trial merge of the two (local, not pushed) conflicts only there, in both schema version lines and in the conformance doc; with that resolved, this PR's tests pass, and Issue personal access tokens from a first-party client, and backfill existing ones onto it #3205's tests that pin D1's plaintext write and fallback (inpersonal_access_token_forms_test.rbandpersonal_access_client_test.rb) fail as they should once D2 is below them, and need the same.token→.raw_token/ digest-only rewrite this PR gives D1's tests.set_tokenwrites onlytoken_digest. A rotation no longer namestokeneither.find_by_raw_tokenisfind_by(token_digest: TokenDigester.digests(raw))and nothing else;find_and_digest_undigestedis gone, and with it Store tokens and client secrets as digests (D1 of 3: read by digest, plaintext fallback) #3192's rule that a datacenter outside the primary authenticates an undigested token without healing it. Those tokens are left to D2-1 as well.find_honored, which bearer authentication uses, goes through it, so it finds by digest alone.self.ignored_columns += %w[ token ]onIdentity::AccessToken. Withpartial_insertsoff (load_defaults 8.1), a model names every column it knows on INSERT; ignoringtokentakes it out of every INSERT and, since Rails enumerates columns when any are ignored, every SELECT. That is what lets D3 drop it under running D2 processes.Unchanged: the token format (
fizzy_pat_+ base58 body), the create JSON fizzy-cli reads (id,token,description,permission,created_at), the revocation endpoint (it already goes throughfind_by_raw_token), and every OAuth path, which D1 already made digest-only.Tests
ApiTest: the two scheme pins (bearermatched case-insensitively,Basic Bearer …not bearer) present the fixture'sraw_token. (Store tokens and client secrets as digests (D1 of 3: read by digest, plaintext fallback) #3192 now sends grants' raw tokens in its three challenge pins itself.)AccessTokenTestHelper::RAW_FIXTURE_TOKENS, andidentity_access_tokens(...)hands back records carryingraw_token, as an issued token does. Call sites moved from.tokento.raw_token.Identity::AccessTokenTest: personal tokens store only their digest (the column is NULL), a token stored only in plaintext doesn't authenticate and issues no plaintext query, and the model ignores the column. D1's fallback tests are gone with the fallback.BackfillRemainingTokenDigestsTest: digests the leftovers, keeps personal plaintext and clears a grant's, leavesupdated_atalone, is idempotent, the result authenticates, refuses duplicates (including one already digested) before any write without naming the token, keeps the plaintext of the personal access client's tokens, and is a no-op once the column is gone. D1'sBackfillTokenDigestsTestnow writes and reads plaintext through the migration's own model.Migration inventory rows for the integration branch
For
docs/oauth/integration-migrations.mdonoauth-integration-2026-10-15. This PR doesn't change that branch. Labeled D2-1 rather than numbered, so a sibling PR claiming the rows after #3192's 8–10 doesn't collide:20261017120000_backfill_remaining_token_digests.rbtokens first. Then oneUPDATE … WHERE id = ? AND token_digest IS NULLper row with atokenand no digest:token_digest, andtokenNULLed for OAuth rows (a personal access client's rows, once #3205 adds it, keep it).updated_atdoesn't move.disable_ddl_transaction!, batches of 500.downis a no-opidentity_access_tokens: ~3,500 rowstokenhas no index, so the duplicate check and the batch selection each scan ~3,500 rows, milliseconds. Expect a handful of updates: only tokens productionmainwrote before D1 reached it and nobody has used sinceNotes for the runbook:
mainprocess older than D1 left running. (2) Run D2-1 on beta1. (3) Only then let D2 code serve, on beta or anywhere: beta's web role doesn't migrate on boot (see the integration doc), so either migrate with the new image first (its option B) or accept that until D2-1 runs, beta refuses the leftover plaintext-only tokens (production, still on D1, keeps accepting and healing them).bin/rails db:migrate:redo:primary VERSION=20261017120000. A quick check before landing:SELECT COUNT(*) FROM identity_access_tokens WHERE token_digest IS NULLshould be 0.bin/rails db:migrate:down:primary VERSION=20261017120000is a no-op (the digests stay, and D1 reads them). Undoing the code is a plain revert: D1 still reads digests first and falls back to the plaintext that's still there. Neverdb:rollback(see the integration doc).db:prepare), so one that upgrades straight past D1 runs D1's backfill and D2-1 before this code answers a request. With Kamal, the old container keeps serving until the new one passes its health check, so a personal access token created on a pre-D1 container in those seconds, after D2-1 has scanned, has no digest and never authenticates. Its holder sees a 401 on first use and creates another. Closing that would mean keeping the plaintext fallback D2 exists to remove, so it isn't done; D1 and D2 reachmainseparately, so an install that followsmainpasses through D1 first.Verified
On thelio at the
dcb914cebhead, whose diff differs from this head's only in threeapi_test.rblines that #3192 now carries itself, gated on exit status (CI's commands:bin/rails db:setupthenbin/rails test, against a dedicatedmysql:8.4container):DATABASE_ADAPTER=mysql)Migration round trip, SQLite and a dedicated MySQL 8.4 container: loaded #3192's schema and seeded a personal access token written by pre-D1 code (plaintext only, never used), one written by D1 (plaintext and digest) and an OAuth grant (digest only), all with
updated_atfive days back. Then up to20261017120000: the pre-D1 token got its digest, everything kept its digest, personal plaintext stayed, noupdated_atmoved, and the dump matches this branch'sdb/schema.rb/db/schema_sqlite.rbexactly.redoof the rerun: same. (D3 continues the round trip.)#3191 shipped-client pins, at 8340a8d (this head before a rebase that touched only files this PR doesn't) merged with
test/pin-shipped-client-authat 98572aa, locally, not pushed. Unchanged,ShippedClientAuthenticationTesterrors (OSS: 8 tests, 6 errors), because itsbearerhelper reads the fixture's plaintext as.token, which no longer exists, and so does one SaaS audits test. With those two lines reading.raw_token, they pass: OSS (SQLite) 8 tests, SaaS (MySQL 8.4) 17 with the auditor pins, 0 failures. Whichever of #3191 and this lands second carries that two-line change. The token string and the create JSON shape are unchanged.Not done here, on purpose
oauth_*: D1 already made OAuth credentials digest-only.