Repository navigation
Conversation
3c26897 to
ee24b6f
Compare
|
@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. |
There was a problem hiding this comment.
🔵 Needs a closer look
The destructive migration requires human confirmation that D2 replaced every older process and the shared-production manual migration gates are satisfied.
No open issues 🎉
What changed in this PR
Completes D3 of digest-only token storage by removing the remaining plaintext personal access token column.
Changes:
- Adds the column-drop migration and updates both database schemas.
- Removes the model’s column-ignore setting and updates tests for the final schema.
- Documents completion of digest-only storage.
[!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/identity/access_token_test.rb | Asserts the plaintext column is absent. |
| test/migrations/backfill_token_digests_test.rb | Removes tests requiring the dropped column. |
| test/migrations/backfill_remaining_token_digests_test.rb | Removes tests requiring the dropped column. |
| docs/oauth/spec_conformance.md | Records completion of D3. |
| docs/docker-deployment.md | Documents digest-only storage. |
| db/schema.rb | Removes the MySQL plaintext column. |
| db/schema_sqlite.rb | Removes the SQLite plaintext column. |
| db/migrate/20261016130000_drop_identity_access_token_plaintext.rb | Drops plaintext storage; rollback restores an empty column. |
| app/models/identity/access_token.rb | Removes the obsolete column-ignore setting. |
Review effort: Balanced
![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: ee24b6f171
ℹ️ 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".
ee24b6f to
f339274
Compare
c8786a9 to
135b057
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". |
135b057 to
b6675c2
Compare
715995f to
a7b7748
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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: 3 resolved (2 fixed, 1 declined with the reasoning in the thread). |
7a68c24 to
22b9b57
Compare
a7b7748 to
64402c6
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
4cd1b5e to
3ac03cc
Compare
10a6f57 to
5c04610
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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 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.
…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.
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.
3ac03cc to
457a45e
Compare
5c04610 to
a420fae
Compare
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Restacked onto #3192 |
|
@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 D2 (#3202) is deployed to production and to beta (every beta destination: they share
fizzy_production), and to staging, and every process that might nametokenon INSERT has been replaced. Production runs withpartial_insertsoff (load_defaults 8.1), so a process running D1 or anything older names every column it knows on every INSERT. Oncetokenis gone, such a process fails to create personal access tokens (Unknown column 'token'), fails OAuth refreshes (D1's rotation writestoken: nil), and answers an unknown Bearer token with a 500 instead of a 401 (D1's plaintext fallback queriestoken). D2 ignores the column, so only D2 or later may be running when this migrates.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 #3202 (train: … → #3169 → #3192 → #3202 → this).
Implements step D3, the last, of hashing tokens at rest (rubric TK-2, bearer artifacts stored only as digests):
tokenidentity_access_tokens.tokenWhat D3 does
DropIdentityAccessTokenPlaintext: on MySQL,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, a table rebuild; elsewhereremove_indexandremove_column. The plaintextrefresh_tokencolumn was kept out of Store tokens and client secrets as digests (D1 of 3: read by digest, plaintext fallback) #3192'sDropPlaintextOauthCredentials, so that no migration dropped a column productionmainhad cached; Store tokens and client secrets as digests (D1 of 3: read by digest, plaintext fallback) #3192 ignores it and empties it, so it goes here. MySQL 8.4 would otherwise drop it INSTANT, from the data dictionary alone, leaving each row's plaintext in the table's pages until the row is next rewritten, which for a personal access token is usually never (updated_atdoesn't move on use). With it go the plaintext personal access tokens D1 and D2 left beside their digests. No token is stored in plaintext any more.downrestores both columns empty: nullablestrings,refresh_tokenwith its unique index, as they were before any token was written. The values are unrecoverable: only digests remain, so code that reads only plaintext (productionmainbefore D1) can't authenticate any token again. D1 and D2 read digests and keep working.Identity::AccessTokenno longer needsignored_columns, so it's removed.db/schema.rbanddb/schema_sqlite.rbdrop both columns and the index, and move to2026_10_17_130000.db:migratebefore this one.Identity::AccessTokenTestasserts both columns are gone.test/migrations/drop_identity_access_token_plaintext_test.rbcaches the table's columns as D2's code does (ignoring both), runs the migration, and creates a token with them.docs/docker-deployment.mdnow says tokens are stored only as digests, not just looked up by them.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 D3-1 so a sibling PR's rows after #3192's 8–10 don't collide:20261017130000_drop_identity_access_token_plaintext.rbALTER TABLE identity_access_tokens DROP INDEX index_identity_access_tokens_on_refresh_token, DROP COLUMN refresh_token, DROP COLUMN token, ALGORITHM=INPLACE, LOCK=NONE.downadds both back, nullable and empty, with the indexidentity_access_tokens: ~3,500 rowsTABLE_IDchanging andTOTAL_ROW_VERSIONSresetting to 0.tokenhas no index of its own;refresh_token's goes in the same statementNotes for the runbook:
identity_access_tokens, which every Bearer request reads, briefly at its start and again at its end. Each time it waits for every open transaction that has touched the table, and while it waits, every new query on the table queues behind it: API traffic stalls. TheMIGRATE=1connection has nomax_execution_time, and onmainnothing setslock_wait_timeout(the integration branch sets 5 s). Checkinformation_schema.innodb_trxjust before, and if the step sits for more than a few seconds, cancel it rather than let the queue build; it's one statement, so it's safe to rerun.bin/kamal app details -d <dest>). A D1 process left running is the failure in the caution above.bin/rails db:migrate:down:primary VERSION=20261017130000. It restores empty columns, not the tokens. Neverdb:rollback(see the integration doc). Undoing D2's20261017120000is a no-op either way.20261015120100) would fail looking for it.mainseparately, so an install that followsmainpasses through D2 first. Stopping the old container before migrating would close the window and cost every upgrade its zero-downtime cutover, so it isn't done.remove_columnrebuilds the table, but the old pages stay in the file as free pages until aVACUUM.Verified
On thelio at the
dcb914cebhead; this PR's diff over #3202 is byte-identical since. 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: continuing #3202's (#3192's schema, a pre-D1 plaintext-only token, a D1 token, an OAuth grant): up through
20261017130000, the column is gone, every index is intact, both personal tokens and a newly created one authenticate, noupdated_atmoved, and the dump matchesdb/schema.rb/db/schema_sqlite.rbexactly. Then down newest first: the column comes back nullable and empty, and the dump differs from #3192's only in that the restored column is appended (and, on SQLite, declared as the originalCreateIdentityAccessTokensdeclared it, without the dump'slimit: 255). Then up again andredoof20261017120000with the column gone (a no-op): checks pass, dump matches.#3191 shipped-client pins need the same two-line
.token→.raw_tokenchange #3202 describes. With it, they passed against this branch before the rebase ontodcb914ceb(OSS 19 tests, SaaS 28 with the auditor pins), and against #3202's current head. This branch's own diff over #3202 is unchanged since. The token string and the create JSON shape are unchanged.Not done here, on purpose