Repository navigation
Lapse OAuth grants left idle for 90 days - #3161
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. |
|
Decision implemented here: D2 (idle expiry). A grant that goes 90 days without a refresh lapses, and the client has to send the user back through consent. As in bc3, idle means time since the last refresh: each rotation restarts the 90-day |
![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: 842cdcd17c
ℹ️ 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".
51a35cf to
51fe3ff
Compare
842cdcd to
a620cfa
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a620cfa8c0
ℹ️ 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".
51fe3ff to
677752a
Compare
1177afb to
1938e05
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
1938e05 to
8535922
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". |
8535922 to
8aafeae
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8aafeae3eb
ℹ️ 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 |
|
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". |
|
Convergence summary (D2) Head Design change in review: the idle clock moved from a stored Fixed and resolved: atomic lapse at rotation, NULL backfill during rollout, stale deadline after an old-code rotation, profile entry point, sweep recheck under lock. Declined and resolved: use the database clock in the rotation predicate (the skew is bounded by one request's latency at the deadline, and the worst case is the re-consent that lapsing means anyway) and index Not re-reviewed: Codex last reviewed |
Review ResultNo additional issues found at The final implementation consistently:
The worktree remains clean, so there were no changes to commit and no follow-up pull request was created. Testing
|
D1 through D4 are decided and built: replay detection (#3160), idle expiry (#3161), client_secret_basic and client authentication at revocation (#3082). The record now cites those PRs, the narrower Basic challenge, the stricter refusal of a client secret that authenticates no client, and the restored revocation of personal access tokens. Every SHA now points at the train as rebased onto main.
cf33269 to
9d25c0a
Compare
49ba964 to
0267b88
Compare
|
Review threads: 2 resolved (1 fixed, 1 declined with the reasoning in the thread). |
Before this, a grant lived until it was revoked: a refresh token left on a laptop that was never opened again stayed good indefinitely. bc3 handles this through its refresh tokens. Each rotation mints a token that is good for refresh_token_ttl (90 days), so a grant lapses once it has gone 90 days without a refresh. This applies the same rule to Fizzy grants. - Each grant carries refresh_token_expires_at. It is set 90 days out when the grant is issued and again on every rotation. As in bc3, idle means no refresh: using the access token doesn't extend the grant, because that token expires an hour after the last refresh anyway. - Refreshing a lapsed grant answers invalid_grant. The client has to send the user through authorization again, and the consent screen always shows. - Lapsed grants drop out of Connected Apps, and a daily sweep deletes them. - The boundary matches bc3's Oauth::RefreshToken#expired? (expires_at < now). A refresh at exactly 90 days works. One second later, it doesn't. - Retired refresh tokens are kept for the same window, since a lapsed grant leaves no replay to catch. Existing OAuth grants are backfilled to lapse 90 days after their last rotation.
…en without an expiry Rotation now matches only unlapsed grants. A refresh that passed the lapse check just before the deadline can no longer renew the grant after it, and the daily sweep can no longer delete a grant that was renewed between selecting it and destroying it. During a rolling deploy, old code can write grants without refresh_token_expires_at. Such a grant is now idle from its last write (updated_at), both at refresh and in the sweep. Connected Apps keeps listing it, so it can't become a grant that is live but hidden.
The refresh_token_expires_at column existed only to record a deadline that updated_at already implies. Only issuance and rotation write a grant, and every rotation sets updated_at, including rotations by code that predates idle expiry. The column, on the other hand, could fall out of step during a rolling deploy. Old code could write a grant without it, or rotate a grant and leave a backfilled deadline in place, which would lapse the fresh token early. Removing the column removes the backfill, the NULL fallback and the stale-deadline case together. The profile page's "apps you've authorized" link now hides once every grant has lapsed, matching Connected Apps.
During a rolling deploy, code from before idle expiry can rotate a grant without checking for lapse. A grant the sweep selected could therefore be renewed before the sweep reached it. The sweep now locks each grant, rechecks it, and spares one that is no longer lapsed.
Requests run in the browser's time zone, where 90.days is calendar arithmetic. Across a daylight-saving change the token endpoint's cutoff moved by an hour, and it then disagreed with the UTC sweep. The limit is now 90 days of seconds everywhere.
The stubbed selection was a lazy relation, read after the renewal, so the pin held without with_lock's reload. Now the sweep gets the pre-renewal record, and the test fails without the lock.
2cf65f6 to
0956b57
Compare
5d8dbba to
9bf0974
Compare
* commit '9bf097414cdb6fe87e5026778de922ac02783633': Hand the sweep the grant as it was read, so the race pin needs the lock Measure the idle limit in fixed seconds Recheck each lapsed grant under its lock before the sweep deletes it Measure idleness from the grant's last rotation, not a stored deadline Enforce the idle limit in the rotation itself, and lapse grants written without an expiry Lapse OAuth grants left idle for 90 days
…pping partway * origin/oauth-refresh-replay: Make the OAuth migrations safe to run again after stopping partway
* origin/oauth-refresh-replay: Answer 404 from a dark endpoint however many times it's asked
* tp/oauth-refresh-replay: (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-refresh-replay: Bring main's tests up to this branch: Ruby 4 and the access token finder
* tp/oauth-refresh-replay: 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-refresh-replay: 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-refresh-replay: Redact retired refresh tokens from logged SQL
* tp/oauth-refresh-replay: Filter the code parameter by its whole name, so code_challenge stays readable
* tp/oauth-refresh-replay: Read a replayed refresh token's grant past the query cache
D1 through D4 are decided and built: replay detection (basecamp#3160), idle expiry (basecamp#3161), client_secret_basic and client authentication at revocation (basecamp#3082). The record now cites those PRs, the narrower Basic challenge, the stricter refusal of a client secret that authenticates no client, and the restored revocation of personal access tokens. Every SHA now points at the train as rebased onto main.
Stacked on #3160 (train: #2296 → #3080 → #3081 → #3082 → #3160 → this → #3159 → #3163). Implements D2: idle expiry of the Fizzy OAuth conformance decisions. It is a new PR for the same reason as #3160: #3081 owns expiry but is converged, and reopening it would rebase the train. The "Deliberately not done: refresh-token expiry" line in #3081's body is superseded here.
Problem
A Fizzy OAuth grant lived until someone revoked it. A refresh token left in an abandoned client stayed good forever.
What bc3 does
bc3 has no separate idle timer. Every rotation mints a refresh token with
expires_at: client.refresh_token_ttl.seconds.from_now(refresh_token.rb L80, L113), and the TTL is 90 days. So "idle" means no refresh for 90 days, and every refresh restarts the clock. Using an access token doesn't extend anything. A token is expired onceexpires_at < Time.current, and the token endpoint then refuses it withinvalid_grant(L148).Change (3c78975, 21f8ada, 8aafeae, cf33269)
updated_at. Only issuance and rotation write a grant, and every rotation setsupdated_at, including rotations by code from before this PR. A grant lapses onceupdated_at + 90 days(Identity::AccessToken::REFRESH_IDLE_LIMIT) has passed. The first version stored arefresh_token_expires_atcolumn. Codex found two ways it could fall out of step during a rolling deploy: a NULL written by old code, and a stale backfilled deadline left behind when old code rotated. 8aafeae dropped the column, so there is no migration. The trade-off: any future code that writes to a grant row also restarts its idle clock, and the constant's comment says so.invalid_grant"Refresh token expired". The client has to restart authorization, and the user re-consents, since Fizzy's consent screen always shows. This check runs after the client check and does not apply to rotated tokens. Those still go through Revoke the grant when a rotated refresh token is replayed, with a 60s retry grace #3160's replay handling, so a replay of a lapsed grant's old token revokes it, as bc3's rotated-and-expired branch does.Identity::AccessToken.cleanup,config/recurring.yml) deletes them along with their retired tokens. bc3 keeps expired refresh rows 7 more days for debugging (cleanup_job.rb). Fizzy has no audit consumer for them, so it deletes them at once.Oauth::RetiredRefreshToken::RETENTIONnow references the same limit.Tests (written first; each failed before the change it pins)
test/integration/oauth_grant_idle_expiry_test.rb:invalid_grantupdated_at) restarts the clockMutation check: removing the
unlapsedfilter from the profile page fails its test.The tests move the clock with
travel_to Time.current + d, nottravel d.traveladds calendar days in the machine's local zone, so across a DST change it lands an hour off the model's UTC arithmetic. That flipped the exact-boundary test locally.Locally:
bin/rails test test/integration test/models test/controllerspasses on SQLite (1540 tests). This PR makes no schema change.