From 8c7281de1217a4cfcd3a35275dbde876a77d8b74 Mon Sep 17 00:00:00 2001 From: Michael Neale Date: Tue, 9 Jun 2026 11:57:30 +1000 Subject: [PATCH 1/2] feat: validate OAuth authorization response issuer --- crates/rmcp/src/transport/auth.rs | 348 +++++++++++++++++++++- examples/clients/src/auth/oauth_client.rs | 4 +- 2 files changed, 344 insertions(+), 8 deletions(-) diff --git a/crates/rmcp/src/transport/auth.rs b/crates/rmcp/src/transport/auth.rs index 3c0b5583..cd6e504a 100644 --- a/crates/rmcp/src/transport/auth.rs +++ b/crates/rmcp/src/transport/auth.rs @@ -159,6 +159,10 @@ impl CredentialStore for InMemoryCredentialStore { pub struct StoredAuthorizationState { pub pkce_verifier: String, pub csrf_token: String, + #[serde(default)] + pub expected_issuer: Option, + #[serde(default)] + pub require_issuer: bool, pub created_at: u64, } @@ -167,6 +171,8 @@ impl std::fmt::Debug for StoredAuthorizationState { f.debug_struct("StoredAuthorizationState") .field("pkce_verifier", &"[REDACTED]") .field("csrf_token", &"[REDACTED]") + .field("expected_issuer", &self.expected_issuer) + .field("require_issuer", &self.require_issuer) .field("created_at", &self.created_at) .finish() } @@ -201,9 +207,20 @@ impl ExtraTokenFields for VendorExtraTokenFields {} impl StoredAuthorizationState { pub fn new(pkce_verifier: &PkceCodeVerifier, csrf_token: &CsrfToken) -> Self { + Self::new_with_expected_issuer(pkce_verifier, csrf_token, None, false) + } + + pub fn new_with_expected_issuer( + pkce_verifier: &PkceCodeVerifier, + csrf_token: &CsrfToken, + expected_issuer: Option, + require_issuer: bool, + ) -> Self { Self { pkce_verifier: pkce_verifier.secret().to_string(), csrf_token: csrf_token.secret().to_string(), + expected_issuer, + require_issuer, created_at: std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_secs()) @@ -364,6 +381,17 @@ pub enum AuthError { upgrade_url: Option, }, + #[error( + "Authorization server issuer mismatch: expected {expected_issuer}, received {received_issuer}" + )] + AuthorizationServerMismatch { + expected_issuer: String, + received_issuer: String, + }, + + #[error("Authorization server response missing required issuer: expected {expected_issuer}")] + AuthorizationServerMissingIssuer { expected_issuer: String }, + #[error("Client credentials error: {0}")] ClientCredentialsError(String), @@ -1026,8 +1054,27 @@ impl AuthorizationManager { let (auth_url, csrf_token) = auth_request.url(); - // store pkce verifier for later use via state store - let stored_state = StoredAuthorizationState::new(&pkce_verifier, &csrf_token); + // store pkce verifier and expected issuer for later use via state store + let expected_issuer = self + .metadata + .as_ref() + .and_then(|metadata| metadata.issuer.clone()); + let require_issuer = self + .metadata + .as_ref() + .and_then(|metadata| { + metadata + .additional_fields + .get("authorization_response_iss_parameter_supported") + .and_then(|value| value.as_bool()) + }) + .unwrap_or(false); + let stored_state = StoredAuthorizationState::new_with_expected_issuer( + &pkce_verifier, + &csrf_token, + expected_issuer, + require_issuer, + ); self.state_store .save(csrf_token.secret(), stored_state) .await?; @@ -1166,11 +1213,50 @@ impl AuthorizationManager { *self.scope_upgrade_attempts.read().await } + fn validate_authorization_response_issuer( + stored_state: &StoredAuthorizationState, + received_issuer: Option<&str>, + ) -> Result<(), AuthError> { + let Some(expected_issuer) = stored_state.expected_issuer.as_deref() else { + // Without issuer metadata from discovery, there is no stable value to bind to. + return Ok(()); + }; + let Some(received_issuer) = received_issuer else { + if stored_state.require_issuer { + return Err(AuthError::AuthorizationServerMissingIssuer { + expected_issuer: expected_issuer.to_string(), + }); + } + // SEP-2468 recommends RFC 9207 `iss`, but tolerate older authorization + // servers that do not advertise support for backwards compatibility. + return Ok(()); + }; + if received_issuer != expected_issuer { + return Err(AuthError::AuthorizationServerMismatch { + expected_issuer: expected_issuer.to_string(), + received_issuer: received_issuer.to_string(), + }); + } + Ok(()) + } + /// exchange authorization code for access token pub async fn exchange_code_for_token( &self, code: &str, csrf_token: &str, + ) -> Result { + self.exchange_code_for_token_with_issuer(code, csrf_token, None) + .await + } + + /// exchange authorization code for access token, validating the optional + /// RFC 9207 authorization response issuer (`iss`) when present. + pub async fn exchange_code_for_token_with_issuer( + &self, + code: &str, + csrf_token: &str, + received_issuer: Option<&str>, ) -> Result { debug!("start exchange code for token: {:?}", code); let oauth_client = self @@ -1187,6 +1273,8 @@ impl AuthorizationManager { // Delete state after retrieval (one-time use) self.state_store.delete(csrf_token).await?; + Self::validate_authorization_response_issuer(&stored_state, received_issuer)?; + // Reconstruct the PKCE verifier let pkce_verifier = stored_state.into_pkce_verifier(); @@ -2132,6 +2220,47 @@ impl AuthorizationManager { } } +/// Parameters returned by an OAuth authorization redirect. +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub struct AuthorizationCallback { + pub code: String, + pub csrf_token: String, + pub issuer: Option, +} + +impl AuthorizationCallback { + /// Parse an OAuth redirect URL and extract `code`, `state`, and optional RFC 9207 `iss`. + pub fn from_redirect_url(url: &str) -> Result { + let url = Url::parse(url)?; + let mut code = None; + let mut csrf_token = None; + let mut issuer = None; + + for (key, value) in url.query_pairs() { + match key.as_ref() { + "code" => code = Some(value.into_owned()), + "state" => csrf_token = Some(value.into_owned()), + "iss" => issuer = Some(value.into_owned()), + _ => {} + } + } + + let code = code.ok_or_else(|| { + AuthError::AuthorizationFailed("Authorization callback missing code".to_string()) + })?; + let csrf_token = csrf_token.ok_or_else(|| { + AuthError::AuthorizationFailed("Authorization callback missing state".to_string()) + })?; + + Ok(Self { + code, + csrf_token, + issuer, + }) + } +} + /// oauth2 authorization session, for guiding user to complete the authorization process #[non_exhaustive] pub struct AuthorizationSession { @@ -2239,11 +2368,37 @@ impl AuthorizationSession { &self, code: &str, csrf_token: &str, + ) -> Result { + self.handle_callback_with_issuer(code, csrf_token, None) + .await + } + + /// handle authorization code callback, validating the optional RFC 9207 + /// authorization response issuer (`iss`) when present. + pub async fn handle_callback_with_issuer( + &self, + code: &str, + csrf_token: &str, + issuer: Option<&str>, ) -> Result { self.auth_manager - .exchange_code_for_token(code, csrf_token) + .exchange_code_for_token_with_issuer(code, csrf_token, issuer) .await } + + /// handle an OAuth redirect URL, including optional RFC 9207 `iss` validation. + pub async fn handle_callback_url( + &self, + callback_url: &str, + ) -> Result { + let callback = AuthorizationCallback::from_redirect_url(callback_url)?; + self.handle_callback_with_issuer( + &callback.code, + &callback.csrf_token, + callback.issuer.as_deref(), + ) + .await + } } /// http client extension, automatically add authorization header @@ -2496,9 +2651,23 @@ impl OAuthState { /// handle authorization callback pub async fn handle_callback(&mut self, code: &str, csrf_token: &str) -> Result<(), AuthError> { + self.handle_callback_with_issuer(code, csrf_token, None) + .await + } + + /// handle authorization callback, validating the optional RFC 9207 + /// authorization response issuer (`iss`) when present. + pub async fn handle_callback_with_issuer( + &mut self, + code: &str, + csrf_token: &str, + issuer: Option<&str>, + ) -> Result<(), AuthError> { match self { OAuthState::Session(session) => { - session.handle_callback(code, csrf_token).await?; + session + .handle_callback_with_issuer(code, csrf_token, issuer) + .await?; self.complete_authorization().await } OAuthState::Unauthorized(_) => { @@ -2513,6 +2682,17 @@ impl OAuthState { } } + /// handle an OAuth redirect URL, including optional RFC 9207 `iss` validation. + pub async fn handle_callback_url(&mut self, callback_url: &str) -> Result<(), AuthError> { + let callback = AuthorizationCallback::from_redirect_url(callback_url)?; + self.handle_callback_with_issuer( + &callback.code, + &callback.csrf_token, + callback.issuer.as_deref(), + ) + .await + } + /// get access token pub async fn get_access_token(&self) -> Result { match self { @@ -2599,8 +2779,9 @@ mod tests { use url::Url; use super::{ - AuthError, AuthorizationManager, AuthorizationMetadata, InMemoryStateStore, - OAuthClientConfig, ScopeUpgradeConfig, StateStore, StoredAuthorizationState, is_https_url, + AuthError, AuthorizationCallback, AuthorizationManager, AuthorizationMetadata, + InMemoryStateStore, OAuthClientConfig, ScopeUpgradeConfig, StateStore, + StoredAuthorizationState, is_https_url, }; use crate::transport::auth::VendorExtraTokenFields; @@ -2922,6 +3103,29 @@ mod tests { assert_eq!(deserialized.pkce_verifier, "my-verifier"); assert_eq!(deserialized.csrf_token, "my-csrf"); + assert_eq!(deserialized.expected_issuer, None); + assert!(!deserialized.require_issuer); + } + + #[test] + fn test_stored_authorization_state_records_expected_issuer() { + let pkce = PkceCodeVerifier::new("my-verifier".to_string()); + let csrf = CsrfToken::new("my-csrf".to_string()); + let state = StoredAuthorizationState::new_with_expected_issuer( + &pkce, + &csrf, + Some("https://auth.example.com".to_string()), + false, + ); + + let json = serde_json::to_string(&state).unwrap(); + let deserialized: StoredAuthorizationState = serde_json::from_str(&json).unwrap(); + + assert_eq!( + deserialized.expected_issuer.as_deref(), + Some("https://auth.example.com") + ); + assert!(!deserialized.require_issuer); } #[test] @@ -2934,7 +3138,7 @@ mod tests { assert!(!debug_output.contains("super-secret-verifier")); assert!(!debug_output.contains("super-secret-csrf")); assert!(debug_output.contains("[REDACTED]")); - assert!(debug_output.contains("created_at")); + assert!(debug_output.contains("expected_issuer")); assert!(debug_output.contains("created_at")); } @@ -3369,6 +3573,136 @@ mod tests { assert!(scope.contains("write")); } + #[test] + fn authorization_callback_parses_optional_issuer() { + let callback = AuthorizationCallback::from_redirect_url( + "http://localhost/callback?code=abc&state=csrf&iss=https%3A%2F%2Fauth.example.com", + ) + .unwrap(); + + assert_eq!(callback.code, "abc"); + assert_eq!(callback.csrf_token, "csrf"); + assert_eq!(callback.issuer.as_deref(), Some("https://auth.example.com")); + } + + #[test] + fn authorization_callback_requires_code_and_state() { + let missing_code = AuthorizationCallback::from_redirect_url( + "http://localhost/callback?state=csrf&iss=https%3A%2F%2Fauth.example.com", + ); + assert!(matches!( + missing_code, + Err(AuthError::AuthorizationFailed(message)) if message.contains("missing code") + )); + + let missing_state = AuthorizationCallback::from_redirect_url( + "http://localhost/callback?code=abc&iss=https%3A%2F%2Fauth.example.com", + ); + assert!(matches!( + missing_state, + Err(AuthError::AuthorizationFailed(message)) if message.contains("missing state") + )); + } + + #[test] + fn validate_authorization_response_issuer_accepts_match_and_missing_issuer() { + let pkce = PkceCodeVerifier::new("verifier".to_string()); + let csrf = CsrfToken::new("csrf".to_string()); + let state = StoredAuthorizationState::new_with_expected_issuer( + &pkce, + &csrf, + Some("https://auth.example.com".to_string()), + false, + ); + + assert!( + AuthorizationManager::validate_authorization_response_issuer( + &state, + Some("https://auth.example.com") + ) + .is_ok() + ); + assert!(AuthorizationManager::validate_authorization_response_issuer(&state, None).is_ok()); + } + + #[test] + fn validate_authorization_response_issuer_requires_issuer_when_advertised() { + let pkce = PkceCodeVerifier::new("verifier".to_string()); + let csrf = CsrfToken::new("csrf".to_string()); + let state = StoredAuthorizationState::new_with_expected_issuer( + &pkce, + &csrf, + Some("https://auth.example.com".to_string()), + true, + ); + + let error = + AuthorizationManager::validate_authorization_response_issuer(&state, None).unwrap_err(); + + assert!(matches!( + error, + AuthError::AuthorizationServerMissingIssuer { expected_issuer } + if expected_issuer == "https://auth.example.com" + )); + } + + #[test] + fn validate_authorization_response_issuer_rejects_mismatch() { + let pkce = PkceCodeVerifier::new("verifier".to_string()); + let csrf = CsrfToken::new("csrf".to_string()); + let state = StoredAuthorizationState::new_with_expected_issuer( + &pkce, + &csrf, + Some("https://auth.example.com".to_string()), + false, + ); + + let error = AuthorizationManager::validate_authorization_response_issuer( + &state, + Some("https://evil.example.com"), + ) + .unwrap_err(); + + assert!(matches!( + error, + AuthError::AuthorizationServerMismatch { + expected_issuer, + received_issuer + } if expected_issuer == "https://auth.example.com" + && received_issuer == "https://evil.example.com" + )); + } + + #[tokio::test] + async fn authorization_url_stores_expected_issuer_for_callback_validation() { + let mut manager = manager_with_metadata(Some(AuthorizationMetadata { + authorization_endpoint: "https://auth.example.com/authorize".to_string(), + token_endpoint: "https://auth.example.com/token".to_string(), + issuer: Some("https://auth.example.com".to_string()), + ..Default::default() + })) + .await; + manager.configure_client_id("test-client-id").unwrap(); + + let auth_url = manager.get_authorization_url(&[]).await.unwrap(); + let parsed = Url::parse(&auth_url).unwrap(); + let state = parsed + .query_pairs() + .find_map(|(key, value)| (key == "state").then(|| value.into_owned())) + .expect("authorization URL should contain state"); + + let stored_state = manager + .state_store + .load(&state) + .await + .unwrap() + .expect("authorization state should be stored"); + assert_eq!( + stored_state.expected_issuer.as_deref(), + Some("https://auth.example.com") + ); + } + // -- scope management -- #[test] diff --git a/examples/clients/src/auth/oauth_client.rs b/examples/clients/src/auth/oauth_client.rs index 456f3269..7a2bfab0 100644 --- a/examples/clients/src/auth/oauth_client.rs +++ b/examples/clients/src/auth/oauth_client.rs @@ -38,6 +38,7 @@ struct AppState { struct CallbackParams { code: String, state: String, + iss: Option, } async fn callback_handler( @@ -150,6 +151,7 @@ async fn main() -> Result<()> { let CallbackParams { code: auth_code, state: csrf_token, + iss, } = code_receiver .await .context("Failed to get authorization code")?; @@ -157,7 +159,7 @@ async fn main() -> Result<()> { // Exchange code for access token tracing::info!("Exchanging authorization code for access token..."); oauth_state - .handle_callback(&auth_code, &csrf_token) + .handle_callback_with_issuer(&auth_code, &csrf_token, iss.as_deref()) .await .context("Failed to handle callback")?; tracing::info!("Successfully obtained access token"); From b00d3ceb25e1c60ebba15fdcc8a2253b6bd608d9 Mon Sep 17 00:00:00 2001 From: Michael Neale Date: Tue, 9 Jun 2026 12:07:39 +1000 Subject: [PATCH 2/2] fix: tighten issuer validation callbacks --- conformance/src/bin/client.rs | 70 ++++++++++++++++--------------- crates/rmcp/src/transport/auth.rs | 41 +++++++++++++++++- 2 files changed, 77 insertions(+), 34 deletions(-) diff --git a/conformance/src/bin/client.rs b/conformance/src/bin/client.rs index 49afb75f..41a94f70 100644 --- a/conformance/src/bin/client.rs +++ b/conformance/src/bin/client.rs @@ -3,7 +3,8 @@ use rmcp::{ model::*, service::RequestContext, transport::{ - AuthClient, AuthorizationManager, StreamableHttpClientTransport, auth::OAuthState, + AuthClient, AuthorizationManager, StreamableHttpClientTransport, + auth::{AuthorizationCallback, OAuthState}, streamable_http_client::StreamableHttpClientTransportConfig, }, }; @@ -221,20 +222,16 @@ async fn perform_oauth_flow( .and_then(|v| v.to_str().ok()) .ok_or_else(|| anyhow::anyhow!("No Location header in auth redirect"))?; - let redirect_url = url::Url::parse(location)?; - let code = redirect_url - .query_pairs() - .find(|(k, _)| k == "code") - .map(|(_, v)| v.to_string()) - .ok_or_else(|| anyhow::anyhow!("No code in redirect URL"))?; - let state = redirect_url - .query_pairs() - .find(|(k, _)| k == "state") - .map(|(_, v)| v.to_string()) - .ok_or_else(|| anyhow::anyhow!("No state in redirect URL"))?; + let callback = AuthorizationCallback::from_redirect_url(location)?; tracing::debug!("Got auth code, exchanging for token..."); - oauth.handle_callback(&code, &state).await?; + oauth + .handle_callback_with_issuer( + &callback.code, + &callback.csrf_token, + callback.issuer.as_deref(), + ) + .await?; let am = oauth .into_authorization_manager() @@ -334,8 +331,14 @@ async fn run_auth_scope_step_up_client( .await?; let auth_url = oauth.get_authorization_url().await?; - let (code, state) = headless_authorize(&auth_url).await?; - oauth.handle_callback(&code, &state).await?; + let callback = headless_authorize(&auth_url).await?; + oauth + .handle_callback_with_issuer( + &callback.code, + &callback.csrf_token, + callback.issuer.as_deref(), + ) + .await?; let am = oauth .into_authorization_manager() @@ -380,8 +383,14 @@ async fn run_auth_scope_step_up_client( ) .await?; let auth_url2 = oauth2.get_authorization_url().await?; - let (code2, state2) = headless_authorize(&auth_url2).await?; - oauth2.handle_callback(&code2, &state2).await?; + let callback2 = headless_authorize(&auth_url2).await?; + oauth2 + .handle_callback_with_issuer( + &callback2.code, + &callback2.csrf_token, + callback2.issuer.as_deref(), + ) + .await?; let am2 = oauth2.into_authorization_manager().unwrap(); let auth_client2 = AuthClient::new(reqwest::Client::default(), am2); @@ -422,8 +431,14 @@ async fn run_auth_scope_retry_limit_client( ) .await?; let auth_url = oauth.get_authorization_url().await?; - let (code, state) = headless_authorize(&auth_url).await?; - oauth.handle_callback(&code, &state).await?; + let callback = headless_authorize(&auth_url).await?; + oauth + .handle_callback_with_issuer( + &callback.code, + &callback.csrf_token, + callback.issuer.as_deref(), + ) + .await?; let am = oauth.into_authorization_manager().unwrap(); let auth_client = AuthClient::new(reqwest::Client::default(), am); @@ -696,8 +711,8 @@ async fn run_cross_app_access_client( // ─── Helpers ──────────────────────────────────────────────────────────────── -/// Fetch an authorization URL headlessly, returning (code, state). -async fn headless_authorize(auth_url: &str) -> anyhow::Result<(String, String)> { +/// Fetch an authorization URL headlessly, returning the callback parameters. +async fn headless_authorize(auth_url: &str) -> anyhow::Result { let http = reqwest::Client::builder() .redirect(reqwest::redirect::Policy::none()) .build()?; @@ -707,18 +722,7 @@ async fn headless_authorize(auth_url: &str) -> anyhow::Result<(String, String)> .get("location") .and_then(|v| v.to_str().ok()) .ok_or_else(|| anyhow::anyhow!("No Location header in auth redirect"))?; - let redirect_url = url::Url::parse(location)?; - let code = redirect_url - .query_pairs() - .find(|(k, _)| k == "code") - .map(|(_, v)| v.to_string()) - .ok_or_else(|| anyhow::anyhow!("No code in redirect URL"))?; - let state = redirect_url - .query_pairs() - .find(|(k, _)| k == "state") - .map(|(_, v)| v.to_string()) - .ok_or_else(|| anyhow::anyhow!("No state in redirect URL"))?; - Ok((code, state)) + AuthorizationCallback::from_redirect_url(location).map_err(Into::into) } /// Build a `CallToolRequestParams` for a tool, optionally with arguments. diff --git a/crates/rmcp/src/transport/auth.rs b/crates/rmcp/src/transport/auth.rs index cd6e504a..be9abf14 100644 --- a/crates/rmcp/src/transport/auth.rs +++ b/crates/rmcp/src/transport/auth.rs @@ -1218,7 +1218,15 @@ impl AuthorizationManager { received_issuer: Option<&str>, ) -> Result<(), AuthError> { let Some(expected_issuer) = stored_state.expected_issuer.as_deref() else { - // Without issuer metadata from discovery, there is no stable value to bind to. + if received_issuer.is_some() || stored_state.require_issuer { + return Err(AuthError::AuthorizationFailed( + "Authorization callback issuer cannot be validated because expected issuer was not recorded" + .to_string(), + )); + } + // Without issuer metadata from discovery and without an issuer-bearing + // callback, there is no stable value to bind to. This preserves + // compatibility with older authorization servers. return Ok(()); }; let Some(received_issuer) = received_issuer else { @@ -3646,6 +3654,37 @@ mod tests { )); } + #[test] + fn validate_authorization_response_issuer_rejects_present_issuer_without_expected_issuer() { + let pkce = PkceCodeVerifier::new("verifier".to_string()); + let csrf = CsrfToken::new("csrf".to_string()); + let state = StoredAuthorizationState::new_with_expected_issuer(&pkce, &csrf, None, false); + + let error = AuthorizationManager::validate_authorization_response_issuer( + &state, + Some("https://auth.example.com"), + ) + .unwrap_err(); + + assert!( + matches!(error, AuthError::AuthorizationFailed(message) if message.contains("expected issuer was not recorded")) + ); + } + + #[test] + fn validate_authorization_response_issuer_rejects_required_issuer_without_expected_issuer() { + let pkce = PkceCodeVerifier::new("verifier".to_string()); + let csrf = CsrfToken::new("csrf".to_string()); + let state = StoredAuthorizationState::new_with_expected_issuer(&pkce, &csrf, None, true); + + let error = + AuthorizationManager::validate_authorization_response_issuer(&state, None).unwrap_err(); + + assert!( + matches!(error, AuthError::AuthorizationFailed(message) if message.contains("expected issuer was not recorded")) + ); + } + #[test] fn validate_authorization_response_issuer_rejects_mismatch() { let pkce = PkceCodeVerifier::new("verifier".to_string());