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 👍 / 👎.
jokemanfire
left a comment
jokemanfire
left a comment
There was a problem hiding this comment.
Ran the head commit instead of reading the description. Logic looks right; two things to settle before merge.
What I verified (f2df667, base 3e0156c)
cargo test -p rmcp --lib --all-features → 796 passed, 0 failed. Note the default feature set only runs 278 lib tests, so the 792 passed in the description is not reproducible as written — say --all-features (796 ≠ 792 anyway, looks like base drift; not a problem, just stale).
Mutation check, to prove the new tests are not vacuous:
- Replacing
Self::refresh_request_scopes(self.refresh_scopes.as_deref(), &granted_scopes)with the oldgranted_scopes.clone()failsrefresh_token_omits_scope_unless_caller_sets_onewith exactly the pre-fix request body:
grant_type=refresh_token&refresh_token=my-refresh-token&scope=mcp_api+refresh_token&resource=... - Replacing
stored_grantwithgranted_scopesfailsnarrowed_refresh_keeps_original_grant(["read"]vs["read","write"]).
So both behaviours are actually pinned.
Semantics I checked by hand: with scope omitted and a response that omits scope, requested_scopes falls back to the stored grant before resolve_granted_scopes, so current_scopes does not collapse to empty; original_grant/stored_grant keeps a narrowed refresh from shrinking the persisted grant. RFC 6749 §6 filtering (never ask for more than granted) holds.
Heads-up: no CI ran on this PR (the only check run is Assign labels), so the numbers above are local-only evidence.
Asks
-
Default behaviour change for existing callers. Before this PR granted scopes were always forwarded, and the docs explicitly cited Azure AD v2 as a provider that needs them. After this PR those deployments refresh without
scopeunless they callset_refresh_scopes, which is a silent behaviour change in a patch release. Please make that explicit in the doc/release note (theOAUTH_SUPPORT.mdline now says "can opt in", but does not say that callers who previously relied on the old default must act). -
Empty intersection is silent. If the caller sets scopes that are all outside the original grant (e.g. grant
["read","write"],set_refresh_scopes(["admin"])),refresh_request_scopesreturns empty, the parameter is omitted, and the server reissues the full original grant. The caller's narrowing is silently discarded and the token is broader than requested. Either document that case inset_refresh_scopes's doc comment or emit adebug!/warn!whenexplicitis non-empty but the filtered set is empty.
Neither is a correctness bug in the happy path; 1 is a comms/rollout issue, 2 is a sharp edge worth a line of docs.
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