Skip to content

fix(auth): omit scope on refresh unless caller sets one #1433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
no-hup wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(auth): omit scope on refresh unless caller sets one #1433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e982f62
fix(auth): omit scope on refresh unless caller sets one
no-hup Nov 5, 2026
f2df667
fix(auth): keep original grant on narrowed refresh, update refresh docs
no-hup Nov 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 148 additions & 3 deletions crates/rmcp/src/transport/auth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,6 +1266,8 @@ pub struct AuthorizationManager {
application_type: Option<String>,
allow_missing_issuer: bool,
allow_private_metadata_addresses: bool,
/// Scopes the caller asked to send on refresh. `None` omits the parameter.
refresh_scopes: Option<Vec<String>>,
}

#[derive(Debug, Clone, Serialize, Deserialize)]
Expand Down Expand Up @@ -1578,6 +1580,7 @@ impl AuthorizationManager {
application_type: Some(DEFAULT_APPLICATION_TYPE.to_string()),
allow_missing_issuer: false,
allow_private_metadata_addresses: false,
refresh_scopes: None,
};

Ok(manager)
Expand All @@ -1588,6 +1591,19 @@ impl AuthorizationManager {
self.scope_upgrade_config = config;
}

/// Set the scopes to include on a refresh-token request.
///
/// 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
Comment on lines +1596 to +1597

@chatgpt-codex-connector chatgpt-codex-connector Bot Nov 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Document the refresh-scope opt-in in the OAuth guide

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 👍 / 👎.

/// sent. An empty list omits the parameter again.
pub fn set_refresh_scopes<I, S>(&mut self, scopes: I)
where
I: IntoIterator<Item = S>,
S: Into<String>,
{
self.refresh_scopes = Some(scopes.into_iter().map(Into::into).collect());
}

/// Configure whether authorization server metadata discovery tolerates a
/// missing `issuer` field.
///
Expand Down Expand Up @@ -2123,6 +2139,28 @@ impl AuthorizationManager {
Self::dedup_scopes(scopes)
}

/// Scopes to put on a refresh request.
///
/// `None` or an empty list omits the parameter. Otherwise only scopes from
/// the original grant are kept. When that grant was not recorded, the
/// caller-provided scopes are used as-is.
fn refresh_request_scopes(explicit: Option<&[String]>, granted: &[String]) -> Vec<String> {
let Some(explicit) = explicit.filter(|scopes| !scopes.is_empty()) else {
return Vec::new();
};
if granted.is_empty() {
return Self::dedup_scopes(explicit.to_vec());
}
let granted_set: HashSet<&str> = granted.iter().map(String::as_str).collect();
Self::dedup_scopes(
explicit
.iter()
.filter(|scope| granted_set.contains(scope.as_str()))
.cloned()
Comment on lines +2155 to +2159

@chatgpt-codex-connector chatgpt-codex-connector Bot Nov 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject refresh scope sets that filter to empty

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 👍 / 👎.

.collect(),
)
}

/// deduplicate scopes preserving first-seen order (SEP-2350: stable for testability)
fn dedup_scopes(scopes: Vec<String>) -> Vec<String> {
let mut seen = std::collections::HashSet::new();
Expand Down Expand Up @@ -2561,10 +2599,22 @@ impl AuthorizationManager {
.exchange_refresh_token(&refresh_token_value)
// RFC 8707: the resource indicator is required on token requests, including refreshes
.add_extra_param("resource", self.oauth_resource().await);
let requested_scopes = stored_credentials.granted_scopes.clone();
for scope in requested_scopes.iter().cloned() {
// RFC 6749 §6: `scope` is optional on refresh and must not exceed the
// original grant. Omit it unless the caller set scopes explicitly.
let granted_scopes = stored_credentials.granted_scopes.clone();
let original_grant = granted_scopes.clone();
let scopes_to_send =
Self::refresh_request_scopes(self.refresh_scopes.as_deref(), &granted_scopes);
Comment on lines +2604 to +2607

@chatgpt-codex-connector chatgpt-codex-connector Bot Nov 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the original grant when filtering refresh scopes

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 👍 / 👎.

for scope in scopes_to_send.iter().cloned() {
refresh_request = refresh_request.add_scope(Scope::new(scope));
}
// Omitting `scope` asks for the original grant. Keep that as the
// fallback when the token response also omits `scope` (RFC 6749 §5.1).
let requested_scopes = if scopes_to_send.is_empty() {
granted_scopes
} else {
scopes_to_send
};
let mut token_result = match refresh_request
.request_async(&OAuth2HttpClient {
client: self.http_client.as_ref(),
Expand Down Expand Up @@ -2610,11 +2660,19 @@ impl AuthorizationManager {

*self.current_scopes.write().await = granted_scopes.clone();

// A caller-narrowed refresh must not shrink the stored grant, or later
// refreshes could never ask for the dropped scopes again.
let stored_grant = if original_grant.is_empty() || self.refresh_scopes.is_none() {
granted_scopes
} else {
original_grant
Comment on lines +2665 to +2668

@chatgpt-codex-connector chatgpt-codex-connector Bot Nov 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the grant across unscoped narrowed responses

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 👍 / 👎.

};

let client_id = oauth_client.client_id().to_string();
let stored = StoredCredentials {
client_id,
token_response: Some(token_result.clone()),
granted_scopes,
granted_scopes: stored_grant,
token_received_at: Some(Self::now_epoch_secs()),
issuer: self.metadata_issuer(),
};
Expand Down Expand Up @@ -10015,6 +10073,7 @@ mod tests {
}))
.await;
manager.configure_client(test_client_config()).unwrap();
manager.set_refresh_scopes(["read", "write"]);

let stored = StoredCredentials {
client_id: "my-client".to_string(),
Expand Down Expand Up @@ -10091,6 +10150,7 @@ mod tests {
}))
.await;
manager.configure_client(test_client_config()).unwrap();
manager.set_refresh_scopes(["read"]);

let stored = StoredCredentials {
client_id: "my-client".to_string(),
Expand Down Expand Up @@ -10130,6 +10190,7 @@ mod tests {
}))
.await;
manager.configure_client(test_client_config()).unwrap();
manager.set_refresh_scopes(["read", "offline_access"]);

let stored = StoredCredentials {
client_id: "my-client".to_string(),
Expand Down Expand Up @@ -10193,6 +10254,90 @@ mod tests {
);
}

#[tokio::test]
async fn refresh_token_omits_scope_unless_caller_sets_one() {
let (base_url, captured) = start_token_server().await;

let mut manager = manager_with_metadata(Some(AuthorizationMetadata {
authorization_endpoint: format!("{}/authorize", base_url),
token_endpoint: format!("{}/token", base_url),
..Default::default()
}))
.await;
manager.configure_client(test_client_config()).unwrap();

// The token response recorded these scopes. That must not put `scope`
// on the refresh by itself: Salesforce rejects the parameter, and
// RFC 6749 §6 says omitting it keeps the original grant.
let stored = StoredCredentials {
client_id: "my-client".to_string(),
token_response: Some(make_token_response_with_refresh(
"old-token",
"my-refresh-token",
)),
granted_scopes: vec!["mcp_api".to_string(), "refresh_token".to_string()],
token_received_at: Some(AuthorizationManager::now_epoch_secs()),
issuer: None,
};
manager.credential_store.save(stored).await.unwrap();

manager.refresh_token().await.unwrap();

let body = captured.lock().unwrap().take().unwrap();
let params: std::collections::HashMap<_, _> = url::form_urlencoded::parse(body.as_bytes())
.into_owned()
.collect();
assert!(
!params.contains_key("scope"),
"scope should be absent when the caller did not set one, body: {body}"
);
}

#[tokio::test]
async fn narrowed_refresh_keeps_original_grant() {
let (base_url, captured) = start_token_server().await;

let mut manager = manager_with_metadata(Some(AuthorizationMetadata {
authorization_endpoint: format!("{}/authorize", base_url),
token_endpoint: format!("{}/token", base_url),
..Default::default()
}))
.await;
manager.configure_client(test_client_config()).unwrap();
manager.set_refresh_scopes(["read"]);

let stored = StoredCredentials {
client_id: "my-client".to_string(),
token_response: Some(make_token_response_with_refresh(
"old-token",
"my-refresh-token",
)),
granted_scopes: vec!["read".to_string(), "write".to_string()],
token_received_at: Some(AuthorizationManager::now_epoch_secs()),
issuer: None,
};
manager.credential_store.save(stored).await.unwrap();

manager.refresh_token().await.unwrap();
captured.lock().unwrap().take();

let saved = manager.credential_store.load().await.unwrap().unwrap();
assert_eq!(
saved.granted_scopes,
vec!["read".to_string(), "write".to_string()],
"a narrowed refresh must not shrink the stored grant"
);

// A later refresh can still ask for the full grant.
manager.set_refresh_scopes(["read", "write"]);
manager.refresh_token().await.unwrap();
let body = captured.lock().unwrap().take().unwrap();
let params: std::collections::HashMap<_, _> = url::form_urlencoded::parse(body.as_bytes())
.into_owned()
.collect();
assert_eq!(params.get("scope").map(String::as_str), Some("read write"));
}

#[tokio::test]
async fn refresh_token_preserves_existing_refresh_token_when_response_omits_it() {
use oauth2::TokenResponse;
Expand Down
2 changes: 1 addition & 1 deletion docs/OAUTH_SUPPORT.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ cargo run -p mcp-client-examples --example clients_client_credentials -- \
6. **Authorization Request**: Build authorization URL with PKCE (S256) and RFC 8707 resource parameter
7. **Authorization Code Exchange**: After user authorization, exchange code for access token (with resource parameter)
8. **Token Usage**: Use access token for API calls via `AuthClient` or `AuthorizedHttpClient`
9. **Token Refresh**: Automatically use refresh token to get new access token when current one expires; previously granted scopes are forwarded in the refresh request so providers that require them (e.g. Azure AD v2) work correctly
9. **Token Refresh**: Automatically use refresh token to get new access token when current one expires; `scope` is omitted by default, which keeps the original grant (RFC 6749 §6). Providers that require it (e.g. Azure AD v2) can opt in with `AuthorizationManager::set_refresh_scopes`; only scopes from the original grant are sent, and a narrowed refresh does not shrink the stored grant
10. **Scope Upgrade**: On 403 insufficient_scope, compute scope union and re-authorize with upgraded scopes

## Security Considerations
Expand Down
Loading