Repository navigation
Sweep dynamic registrations nobody authorized after a day - #3255
Conversation
Registration is unauthenticated and limited only per address, so its rows were bounded by ten a minute per address over thirty days. A registration whose row nothing has touched since it was written, no consent approved and no grant issued or ended, now lapses after 24 hours, and the sweep runs hourly so that holds. Anything that was ever authorized, metadata document clients and operator-provisioned clients keep the rules they had.
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.
🟢 Approved
No active review issues block the change to shorten retention for unused OAuth registrations.
No open issues 🎉
What changed in this PR
Reduces storage growth from unused OAuth registrations by shortening retention for never-authorized dynamic clients from 30 days to 24 hours, while preserving existing retention and exemptions for other clients.
Changes:
- Adds a shorter retention rule for untouched dynamic registrations with no access tokens.
- Runs OAuth client cleanup hourly instead of daily.
- Adds model and integration coverage for expiry, consent, grants, and exempt clients.
[!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/oauth/client_test.rb |
Tests one-day expiry and preservation of authorized and exempt clients. |
test/integration/oauth_flow_test.rb |
Verifies consent removes a registration from short-retention cleanup eligibility. |
config/recurring.yml |
Schedules OAuth client cleanup hourly at minute 14. |
app/models/oauth/client.rb |
Adds the 24-hour retention rule and incorporates it into stale-client selection. |
Review effort: Balanced
|
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Answers the dynamic-registration volume finding on #3247 (thread).
POST /oauth/clientsis unauthenticated and limited to 10 a minute per address. Each registration can hold ten 2,000-character redirect URIs, and a registration nobody authorized was kept forUNUSED_RETENTION, 30 days. This shortens that one case to 24 hours, so the rows such registrations can pile up are bounded by the registration rate over a day rather than a month.What counts as never authorized
Oauth::Client.never_authorized: a dynamically registered client whoseupdated_atstill equals itscreated_at, and which holds no access token (stalekeeps itswhere.missing(:access_tokens)). Nothing has touched the row since registration wrote it. The code already touches a client:Oauth::AuthorizationsController#create);Identity::AccessToken#touch_oauth_client).So a client that ever got a code or a grant, including one whose grant has since been revoked or lapsed, has
updated_at > created_atand keeps the 30-day rule counted from its last grant activity, as before. Denying consent touches nothing, so a denied registration lapses after a day too.If some future write updates a dynamically registered client, it moves
updated_at, so the client falls back to 30 days. That errs toward keeping rows, never toward deleting one.What's untouched
POST /oauth/clients, so they keep 30 days.dynamically_registered: false) were never swept and still aren't. That includes the personal access issuer.destroy_if_still_unusedre-runs the wholestaletest, so a consent or grant that lands between picking a client and destroying it still spares it.Schedule
cleanup_oauth_clientsmoves from daily (04:12) to hourly (minute 14), so a never-authorized registration lives at most about 25 hours. The sweep scansoauth_clients, which this change keeps small.Accepted loss
If someone registers and is still on the consent screen when the registration turns 24 hours old, their approval gets "Unknown client". MCP clients register again on reconnect, so the cost is one retry.
Not doing: an account-wide or global registration budget
A global cap swaps a storage problem for an availability one. Someone registering from many addresses could use it up, and then real Claude or Cursor users couldn't register at all, which is worse than some extra rows. With the per-address limit and a one-day life, the row count is already bounded by the request rate, and the rows are small and grant nothing without a user's consent.
Tests
stale.Each new test was checked by breaking what it covers: dropping the
updated_at = created_attest, dropping thedynamically_registeredrestriction, setting the retention to 30 days, setting it to 22 hours, and droppingwhere.missing(:access_tokens). Each break made at least one test fail. The OAuth model and integration tests pass on SQLite and MySQL 8.4. Nothing tests the hourly schedule.