Repository navigation
Sweep self-registered OAuth clients left without a grant for 30 days - #3159
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. |
![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: d26a9ac74d
ℹ️ 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".
|
Added |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21c86ba45b
ℹ️ 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".
69b77e7 to
d071ad7
Compare
21c86ba to
c6401f3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6401f341e
ℹ️ 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".
c6401f3 to
afc9599
Compare
9d25c0a to
5e40b3d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afc9599f95
ℹ️ 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".
afc9599 to
fc3f2da
Compare
5e40b3d to
cf6a208
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc3f2dac92
ℹ️ 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".
fc3f2da to
98e7112
Compare
cf6a208 to
332c2da
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98e711283a
ℹ️ 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".
98e7112 to
9ec1432
Compare
332c2da to
fde6755
Compare
#3159's lock_client no longer reloads the row, so the guard reads the client_id in the database rather than one changed in memory but not saved.
#3159's lock_client no longer reloads the row, so the guard reads the client_id in the database rather than one changed in memory but not saved.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
#3159's lock_client no longer reloads the row, so the guard reads the client_id in the database rather than one changed in memory but not saved.
#3159's lock_client no longer reloads the row, so the guard reads the client_id in the database rather than one changed in memory but not saved.
* commit 'eac532ca82a4a6b77555099bb32f255c5e6fd126': (28 commits) Run the dirty-client destroy pin: it sat below private, so Minitest skipped it Lock a client for destruction without reloading, so unsaved changes don't stop it Record D6 and refresh narrowing as deliberate deviations in the ledger Say a concurrent rotation's loser gets the winner's pair, as #3160 made it Record optional state, uncached OAuth responses and log filtering in the ledger Record the invalid_client status policy in the conformance record Repoint the conformance record, and add email changes to the grant reach rule Record the revocation throttle and the OAuth grant reach rule Repoint the conformance record after the latest restack Reload a client's grants when locking it for destroy Record the PKCE verifier format and the PAT-management refusal Repoint the conformance record after the latest restack Repoint the conformance record after the latest restack Repoint the conformance record after the latest restack Lock a client before its grants cascade on destroy Repoint the conformance record after the latest restack Carry the sweep past a client deleted after it was picked Repoint the conformance record after the latest restack Issue no code when consent's touch finds the client already swept Repoint the conformance record after the latest restack ...
#3159's lock_client no longer reloads the row, so the guard reads the client_id in the database rather than one changed in memory but not saved.
#3159's lock_client no longer reloads the row, so the guard reads the client_id in the database rather than one changed in memory but not saved.
…stopping partway * origin/oauth-grant-idle-expiry: Make the OAuth migrations safe to run again after stopping partway
* origin/oauth-grant-idle-expiry: Answer 404 from a dark endpoint however many times it's asked
* tp/oauth-grant-idle-expiry: (60 commits) Name v0.2.0 in the pin's comment Pin fizzy-mcp-server v0.2.0 Refuse a join code that was reset while it was being redeemed Filter presigned storage signatures from logs Keep the reviewer's memberships in reach, and print the member audit escaped Import an export that lacks a column this database has only just gained List the reviewer accounts' members when signing the reviewer out Bump thruster from 0.1.26 to 0.1.27 (#3199) Give the asset build a placeholder direct-upload credential Anchor the stubbed storage host pattern Sign direct uploads with a write-only storage credential Lock the reviewer's email address, so a code holder can't move its membership out of revocation's reach Use Ruby's GC defaults and log job memory (#3238) Pin the fizzy_mcp accessory to fizzy-mcp-server v0.1.0 by digest Scope webhook dispatch by account as well as board Test the reviewer audit against the structured log line production writes Revoke on the email alone, and record each use in the request's structured log Remove the reviewer's access tokens and passkeys on sign-out, so a code holder can't keep a way back in Wire the reviewer sign-in into production from Deploy/Fizzy Close the reviewer's live sockets on sign-out, and keep the identity id out of Sentry ...
* tp/oauth-grant-idle-expiry: Bring main's tests up to this branch: Ruby 4 and the access token finder
* tp/oauth-grant-idle-expiry: Pin the load balancers to kamal-proxy v0.10.2, and say why configure-lb-mcp.sh still checks Give the load balancer the real client IP
* tp/oauth-grant-idle-expiry: Bring main's SQL redaction tests up to bearer lookup by find_honored Redact OAuth client statements from logged SQL, now that a client has a secret
* tp/oauth-grant-idle-expiry: Redact retired refresh tokens from logged SQL
* tp/oauth-grant-idle-expiry: Filter the code parameter by its whole name, so code_challenge stays readable
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c2d31eecb
ℹ️ 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".
* tp/oauth-grant-idle-expiry: Read a replayed refresh token's grant past the query cache
Stacked on #3161 (
oauth-grant-idle-expiry): #2296 → #3080 → #3081 → #3082 → #3160 → #3161 → #3159 → #3163. It moved there from #3082 on 2026-10-06 so that its overlap with #3160 and #3161 (the tokens controller,Identity::AccessTokenassociations and grant destruction) is settled in this PR instead of at merge time. See the restack comment for details.Client registration is open and unauthenticated. Its only brake is 10 registrations per minute per IP. Nothing ever deletes an
oauth_clientsrow, so abandoned registrations only accumulate, and MCP clients often re-register on every install or reconnect (RFC 7591 §5 asks the server to mitigate this). bc3'sOauth::CleanupJob#cleanup_unused_dcr_clientssweeps never-authorized DCR clients after 30 days. This PR adds the same sweep to Fizzy.What changes
config/recurring.ymlrunsOauth::Client.cleanupevery day at 04:12. It destroys dynamically registered clients that hold no access token and have had no grant activity forUNUSED_RETENTION(30 days). Operator-provisioned clients (dynamically_registered: false) are never swept.after_commit, notbelongs_to touch: true, so no transaction holds a grant while waiting on its client, which used to deadlock against a replayed code exchange). Consent touches the client too, so a client isn't swept while its 60-second code is live. Fizzy has no record of whether a client was ever authorized. Without the touch, an app registered 60 days ago that the user disconnected yesterday in Connected Apps would be swept tonight. On reconnect it would then hit "Unknown client" at authorize. Refresh rotation usesupdate_alland doesn't touch, so steady use adds no writes.destroy_if_still_unusedlocks the client row and reruns the full stale test (tokens and timestamp) before destroying it, like bc3'sobliterate_if_still_unauthorized. The code exchange now issues throughOauth::Client#redeem, which takes the same row lock. There's no foreign key to stop it, so before this a token could commit between the sweep's recheck and its delete. Now the grant either lands before the recheck, which then spares the client, or finds the client already gone and getsinvalid_grant"Unknown client". Codex caught this in review, and it was fixed in fa904e5.has_many :access_tokens, dependent: :destroy: a manual clientdestroycan't strand tokens, and each grant's own cascade runs, so retired refresh tokens from Revoke the grant when a rotated refresh token is replayed, with a 60s retry grace #3160 go with it.delete) withinvalid_grantinstead ofNoMethodError(a 500). The code grant already answered a missing client withinvalid_grant.Tests
touch: truemakes this test fail.I wrote the tests first and watched each one fail before its change. I mutation-checked three mechanisms, and each mutation fails its test:
touch: trueThe race test deletes the client between the exchange reading it and issuing the token.
The full OSS SQLite suite passes locally: 1802 tests, 0 failures, 1 skip. Rubocop is clean.
Fixed underneath, in #2296 ("Point a client's access_tokens at oauth_client_id, and drop the stray foreign key", now 65e4d3e)
Oauth::Client#access_tokenswas broken: it inferredclient_idas the foreign key, a columnidentity_access_tokensdoesn't have. Nothing used the association until this sweep.db/schema.rb: it declaredadd_foreign_key "identity_access_tokens", "oauth_clients", but the AddOauth migration saysforeign_key: falseanddb/schema_sqlite.rbhas no such key. So databases built by running migrations had no foreign key, while MySQL databases loaded from the schema (CI and fresh installs) had one.Not done, on purpose
Behaviour to know
invalid_client status policy: the conformance record now reflects #3082
b6b83296a. A failed Basic attempt gets 401 + Basic; any otherinvalid_clientgets 400. The shipped-client matrix is in #3082: no first-party Fizzy client calls/oauth/*.