Repository navigation
Revoke the grant when a rotated refresh token is replayed, with a 60s retry grace - #3160
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ 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: D1 (refresh replay detection). A rotated refresh token presented again revokes the grant's whole token family. The exception is a 60s grace window: inside it, a retry of the rotation that just happened gets the same successor pair back. That is bc3's |
![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: 51a35cf0bb
ℹ️ 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
|
@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". |
51fe3ff to
677752a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 677752a782
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e783576920
ℹ️ 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. 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 review |
|
Codex Review: Didn't find any major issues. Bravo. 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 (D1) Head Fixed and resolved: check/read atomicity, grant deleted mid-replay, clock-order supersession (now causal, via Declined and resolved: keep retired tokens beyond 90 days. This matches bc3's refresh TTL plus cleanup, a token that old is dead either way, and #3161 bounds grant idleness at the same limit. Not re-reviewed: Codex last reviewed |
Review ResultNo additional issues found at head The convergence summary’s “not re-reviewed” note is now stale: the supplied PR comments show subsequent Codex reviews of The final implementation addresses the previously raised concerns:
No code changes were necessary, so I did not create a commit or a new pull request. 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.
69b77e7 to
d071ad7
Compare
49ba964 to
0267b88
Compare
0267b88 to
b361a77
Compare
d071ad7 to
8947b42
Compare
* commit '0956b57aafc7f44fda25ee9b556fb2c3f9162ede': Refuse a superseded retry as superseded before weighing its scope Refuse a grace retry that asks for a different scope than its rotation produced Weigh the scope of a grace retry as the live refresh it repeats Lock a grant before deleting its retired tokens Judge supersession by the successor a rotation produced, not by timestamps Read a grace retry's successor before checking that it's still current Revoke the grant when a rotated refresh token is replayed, with a retry grace
* 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 ...
…topping partway * origin/oauth-bearer-challenge: Make the OAuth migrations safe to run again after stopping partway
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ec68226bc
ℹ️ 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".
* origin/oauth-bearer-challenge: Answer 404 from a dark endpoint however many times it's asked
* tp/oauth-bearer-challenge: (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-bearer-challenge: Bring main's tests up to this branch: Ruby 4 and the access token finder
* tp/oauth-bearer-challenge: 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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5cf7ecbdd
ℹ️ 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-bearer-challenge: 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
A retired refresh token's row holds the token and the pair that succeeded it, so its statements are dropped from logs like the other credential tables'.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 8fb89c98c2
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
* tp/oauth-bearer-challenge: 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: 4e397adb3a
ℹ️ 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".
The request may already have looked the grant up, and a cached read could answer a retry with the pair of a grant revoked since.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! 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. |
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 #3082 (train: #2296 → #3080 → #3081 → #3082 → this → #3161 → #3159 → #3163). Implements D1: refresh replay detection of the Fizzy OAuth conformance decisions. It is a new PR, not a commit on #3081, because #3081 is converged and adding to it would mean rebasing #3082, #3158 and #3159. The "Deliberately not done: refresh-token reuse detection" line in #3081's body is superseded by this PR.
Problem
Since #3081, each refresh rotates the grant's refresh token in place, and the old token is forgotten. A rotated token that came back failed as unknown with
invalid_grant. Nothing else happened. If an attacker stole a refresh token and refreshed first, the attacker owned the grant. The legitimate client's next refresh failed quietly, and nothing revoked the thief's copy. OAuth 2.1 §4.3.1 and RFC 9700 §4.14.2 call for rotation with replay detection: a rotated token presented again means two parties hold the grant, so the server revokes it.Change (1b0c206, then 677752a and e783576; 49ba964 locks the grant before its retired tokens are deleted, so destroy and rotation take locks in the same order). Each retired row records the successor its rotation issued. A retry is answerable only while that successor is still the grant's current refresh token, judged against a single read of the grant, so the check depends neither on clock order nor on a second query.
Each rotation retires the presented token into
oauth_retired_refresh_tokens. A retired token presented by the grant's own client is answered as follows:rotate!grace branch,within_grace_window?invalid_grant"Refresh token superseded", no revocation. The client demonstrably holds the live descendant.successor_for_retry!invalid_grantrevoke_family!(reason: "refresh_reuse")client_idinvalid_grantas before, no revocation. The destructive branch is reachable only by the grant's own client.find_presented!checks the client before rotationrefresh_replay_gracedefaults to 60 (README). Fizzy has no per-client TTL columns, so it uses that default. The boundary matches bc3's strictrotated_at > grace.ago(L38): 59s retries, and exactly 60s revokes.invalid_scopeand skip the revocation./oauth/revocationrevokes its grant, as bc3's revocation of any family member does. The ownership check from Support confidential clients with client_secret_post #3082 still applies.Oauth::RetiredRefreshToken.cleanupinconfig/recurring.yml). That matches bc3's 90-dayrefresh_token_ttl, after which a rotated bc3 token has expired too. Revoking a grant deletes its retired tokens.Accepted trade-off (same as bc3)
For 60s after a rotation, anyone holding the just-rotated token who can name the client gets the successor pair back. For a public client, naming the client takes only its
client_id. This is the cost of the grace window D1 asks for: without it, a client that lost a response would be revoked as a thief. Once the window closes, the same presentation revokes the grant.Tests (written first; 6 of 10 integration tests failed before the change, and the other 4 guard against over-revoking)
test/integration/oauth_refresh_replay_test.rb: same successor at 59s, revoke at exactly 60s, revoke after a day, an older ancestor revokes, superseded refused without revoking, another client's replay doesn't revoke, a lost race serves the winner, an out-of-grant scope still revokes, and revoking through a rotated token works but only for its client.invalid_grant, not a 500.test/models/oauth/retired_refresh_token_test.rb: rotation retires the token, the race loser retires nothing and the winner's successor stays retryable, revocation deletes retired tokens, and cleanup respects retention.>=at the boundary fails the 60s test, a window 2s shorter fails the 59s test, dropping!superseded?fails the superseded test, and removing the scope guard fails the scope test.oauth_flow_test"refresh grant invalidates the previous refresh token" now replays after the grace window.Locally:
bin/rails test test/integration test/models test/controllerspasses on SQLite (1531 tests). The MySQLschema.rbentry was written by hand to match the dumper's sorted format, and CI's MySQL jobs load it.