Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adds OAuth 2.1 access-token expiry, refresh-token rotation, expired-token rejection, and refresh-token revocation while preserving personal-token behavior.
Changes:
- Adds expiring OAuth tokens and rotating refresh tokens.
- Supports refresh grants and revocation by either token.
- Advertises and tests the expanded OAuth lifecycle.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
app/models/identity/access_token.rb |
Implements expiry and token rotation. |
app/models/identity.rb |
Rejects expired bearer tokens. |
app/controllers/oauth/tokens_controller.rb |
Handles refresh grants and token responses. |
app/controllers/oauth/revocations_controller.rb |
Supports refresh-token revocation. |
app/controllers/oauth/metadata_controller.rb |
Advertises refresh grants. |
app/controllers/oauth/clients_controller.rb |
Adds refresh grants to DCR responses. |
db/migrate/20260827100000_add_oauth_token_expiry.rb |
Adds lifecycle columns and index. |
db/schema.rb |
Updates the MySQL schema. |
db/schema_sqlite.rb |
Updates the SQLite schema. |
test/models/identity/access_token_test.rb |
Tests expiry and rotation behavior. |
test/integration/oauth_flow_test.rb |
Tests refresh and revocation flows. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
7041285 to
8af37c3
Compare
![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: 7041285c7b
ℹ️ 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".
4d696f6 to
530a7bb
Compare
530a7bb to
598de77
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 598de77115
ℹ️ 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".
598de77 to
0cd462c
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cd462c536
ℹ️ 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".
8f5ef69 to
5737957
Compare
|
Rebased onto #2296's token-endpoint conformance fix (comment). New commit 5737957a9:
Refresh rotates the row in place, so it keeps the code's jti. Replaying the code after a refresh still revokes the rotated tokens, and a test now covers that. Behavior change: the existing "blank refresh token" test now expects |
5737957 to
52e2e56
Compare
52e2e56 to
37b5604
Compare
37b5604 to
a72890b
Compare
a72890b to
d05407c
Compare
|
Rebased onto #2296's availability switch (2b6ea64). Resolved a conflict in Identity.find_by_permissable_access_token, which keeps both .active and honored?. New commit d05407c pins refresh behind the switch (404 dark, 503 paused) and updates the docs: a pause lets issued tokens expire within the hour. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d05407c47c
ℹ️ 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".
d41a590 to
243fd48
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Access-token scope narrowing incorrectly reduces the replacement refresh token’s authorized scope.
2 open issues
4 resolved since last review
grant_types, leaving this newly advertised…grant_types_supported, so this newly…STYLE.md:99-103reserves bang names for methods that have a corresponding non-bang variant, but…
Review effort: Balanced
OAuth-issued access tokens now expire an hour after issuance and carry a refresh token, generated at creation. Personal access tokens are untouched: no expiry, no refresh token. The token endpoint gains the refresh_token grant with OAuth 2.1 rotation semantics: each refresh rotates both the access and refresh token on the grant record, so the previous refresh token dies with the rotation and the Connected Apps list still shows one grant per client. Refresh requests must present the client_id the grant was issued to. Bearer authentication ignores expired tokens, and revocation accepts either token, killing the whole grant. Discovery and DCR responses advertise the refresh_token grant.
Refresh-token consumption raced: two concurrent refreshes could both load the grant and both answer 200, one with already-dead credentials. Rotation now updates guarded on the presented refresh token, so the loser matches no row and gets invalid_grant. The rename to refresh follows STYLE.md's bang rule — no non-bang counterpart, no bang. Also assert grant_types in the discovery and DCR responses so the advertised refresh grant can't silently regress.
A refresh request that carried a scope was silently ignored: the response omitted the scope field while the rotated token kept the grant's original permission, so a client narrowing a write token to read believed it held a read-only token but received a write-capable one, and a broadening request was accepted rather than rejected. Validate the requested scope against the grant (RFC 6749 §6): reject anything beyond it with invalid_scope, narrow the rotated token to a requested subset, and always echo the effective scope.
A refresh request missing refresh_token or client_id answered invalid_grant, which tells a client its grant is dead, so a client-side serialization bug ends with the client discarding a valid refresh token. Missing parameters are now invalid_request (RFC 6749 §5.2), checked before any lookup, the same as on the code grant. The two "issued to this client" checks become one, run after the grant is loaded for either grant type, so client_id is required and compared the same way on both (RFC 6749 §4.1.3, §6). Refresh rotates the grant row in place, so it keeps the authorization code's jti: replaying the code after a refresh still revokes the rotated tokens. A test now covers that.
Both grants now describe a write grant as "read write" from one helper, Oauth.canonical_scope, so the code exchange and the refreshes after it can't drift apart.
… expiring tokens The refresh grant shares the token endpoint, so it already inherits the gate: 404 while dark, 503 while issuance is paused. Pin that, and the pilot exemption for refresh, with tests. Now that OAuth access tokens expire, a pause lets issued tokens run out within the hour; the deployment doc and Oauth::Availability say so.
…rant Only an absent scope keeps the grant. A JSON null used to read as absent, so a refresh asking for no scope at all silently came back with full write.
An array refresh_token reached the lookup as an IN list and refreshed; an object raised and answered 500. The string check inherited from the code grant now answers both with invalid_request.
… uses #2296 moved the bearer lookup from Identity.find_by_permissable_access_token to Identity::AccessToken.find_permissable, so that requests can tell which token authenticated them. Expired tokens are still refused.
…eloading reload replaced token on both sides of the assertion, so it held even if the losing refresh had overwritten the winner.
243fd48 to
017291f
Compare
|
Review threads: 1 resolved (1 fixed). |
* commit '017291f81210a3ef6fafa2f5a8545859a15abf4b': Compare a lost rotation's grant with the winner's token read before reloading Pin token expiry on find_permissable, which bearer authentication now uses Pin that refresh rejects a non-string refresh_token or client_id Refuse an explicit null refresh scope instead of restoring the full grant Keep refresh behind the issuance switch, and say what pausing does to expiring tokens Report refresh scopes through the same canonical scope as the code grant Require refresh_token and client_id on refresh as request parameters Reject a blank scope on refresh instead of restoring the full grant Validate and echo scope on refresh Rotate refresh tokens atomically and drop the bang Expire OAuth access tokens and rotate refresh tokens
… partway * origin/oauth-dcr-https: Make the OAuth migrations safe to run again after stopping partway
* origin/oauth-dcr-https: Answer 404 from a dark endpoint however many times it's asked
* tp/oauth-dcr-https: (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-dcr-https: Bring main's tests up to this branch: Ruby 4 and the access token finder
* tp/oauth-dcr-https: 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-dcr-https: Filter the code parameter by its whole name, so code_challenge stays readable
Stacked on #3080. Second of three gap-closers for #2296.
The minimal stack issued non-expiring access tokens with no refresh path. 7041285 adds OAuth 2.1 token lifecycle:
refresh_tokengrant with rotation semantics: each refresh rotates both the access token and the refresh token on the grant record, so the previous refresh token dies with the rotation and Connected Apps still shows one grant per client. Refresh requests must present theclient_idthe grant was issued to.Identity.find_by_permissable_access_tokengoes through anactivescope).refresh_tokengrant.Token responses now include
expires_inandrefresh_token; the refresh response omitsscope(unchanged from the original grant, per RFC 6749 §6).Deliberately not done: refresh-token reuse detection beyond rotation (a reused rotated token simply fails as unknown), and refresh-token expiry — grants live until revoked from Connected Apps, matching the existing revocation model.
Update: both are now done, stacked above #3082. #3160 adds replay detection: a rotated token presented again revokes the grant, except for a 60s retry grace. #3161 adds idle expiry: a grant lapses after 90 days without a refresh.