Repository navigation
Conversation
github-actions
Bot
added
T-core
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. |
|
@jamadeo is on triage rotation this week and is the reviewer for this one. Labels come from the PR labeler. Please review the change. Automated triage. Labels are a first pass and the assigned maintainer has the final say. |
![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: e982f62091
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let granted_scopes = stored_credentials.granted_scopes.clone(); | ||
| let scopes_to_send = | ||
| Self::refresh_request_scopes(self.refresh_scopes.as_deref(), &granted_scopes); |
There was a problem hiding this comment.
When the original grant is read write, one configured refresh can narrow it to read, and refresh_token then saves that latest access-token scope as stored_credentials.granted_scopes. If the caller later sets refresh scopes to write, these lines pass only read to refresh_request_scopes, which filters out the valid write; the empty result makes the next request omit scope, so a compliant server restores the full read write grant instead of issuing the requested write-only token. Preserve the refresh token's original grant separately (or reject rather than turning an empty filtered set into omission).
Useful? React with 👍 / 👎.
| /// RFC 6749 §6 makes `scope` optional. Refresh omits it unless the caller | ||
| /// sets one here. A scope that was not part of the original grant is not |
There was a problem hiding this comment.
When using a provider that requires scope on refresh, the new default now omits it, but docs/OAUTH_SUPPORT.md:455 still promises that previously granted scopes are forwarded automatically and names Azure AD v2 as requiring this. The recommended OAuthState walkthrough never calls this setter, so users following the guide can start failing refreshes after upgrading; update the guide/example to describe the opt-in.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2df667513
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Self::dedup_scopes( | ||
| explicit | ||
| .iter() | ||
| .filter(|scope| granted_set.contains(scope.as_str())) | ||
| .cloned() |
There was a problem hiding this comment.
When the original grant is read write and the caller sets set_refresh_scopes(["admin"]) (or simply has a typo), this filter returns an empty vector. The downstream empty-vector path omits scope, which this implementation treats as a request for the full original read write grant, so the invalid narrowing silently yields a more privileged token instead of failing. Distinguish a nonempty explicit request that filtered to empty from an intentionally empty setting; the new stored_grant branch only fixes the earlier valid read-to-write sequence and does not distinguish this all-ungranted case.
Useful? React with 👍 / 👎.
| let stored_grant = if original_grant.is_empty() || self.refresh_scopes.is_none() { | ||
| granted_scopes | ||
| } else { | ||
| original_grant |
There was a problem hiding this comment.
When an unscoped refresh for an original read write grant returns an explicit narrower scope: read and no new refresh token, the same refresh token still carries the original grant, but this self.refresh_scopes.is_none() arm replaces the stored grant with read. A later set_refresh_scopes(["write"]) is then filtered against the wrong set and cannot send the still-valid scope (and a provider that requires scope will fail the request). The new Some arm handles caller-requested narrowing, but the None arm still needs the refresh-token grant stored separately from the current access-token scopes.
Useful? React with 👍 / 👎.
fixes #1408
refresh was always sending
scopefrom the stored grant, which some servers (salesforce) reject. rfc 6749 §6 says scope is optional on refresh and leaving it out keeps the original grant.now refresh omits
scopeunless the caller sets it viaAuthorizationManager::set_refresh_scopes(). when set, only scopes from the original grant get sent (keeps the #1331 behaviour). if the token response has noscope, granted scopes stay as the fallback.added
refresh_token_omits_scope_unless_caller_sets_one. existing refresh tests now set scopes explicitly. fmt + clippy clean, rmcp lib tests 792 passed.I'm @no-hup. I spotted this from the issue. Claude Opus wrote the fix with Grok's help, I reviewed the diff and ran the tests.
🤖 Generated with Claude Code