diff --git a/crates/rmcp/src/error.rs b/crates/rmcp/src/error.rs index 0940f29c2..ec8840c99 100644 --- a/crates/rmcp/src/error.rs +++ b/crates/rmcp/src/error.rs @@ -13,20 +13,6 @@ impl Display for ErrorData { impl std::error::Error for ErrorData {} -#[cfg(all(feature = "auth", any(feature = "client", feature = "server")))] -pub(crate) struct ErrorChain<'a>(pub(crate) &'a (dyn std::error::Error + 'static)); - -#[cfg(all(feature = "auth", any(feature = "client", feature = "server")))] -impl Display for ErrorChain<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.0)?; - for source in std::iter::successors(self.0.source(), |source| source.source()) { - write!(f, "\n Caused by: {source}")?; - } - Ok(()) - } -} - /// This is an unified error type for the errors could be returned by the service. #[derive(Debug, thiserror::Error)] #[allow(clippy::large_enum_variant)] diff --git a/crates/rmcp/src/transport/auth.rs b/crates/rmcp/src/transport/auth.rs index 0727368cf..18d681afe 100644 --- a/crates/rmcp/src/transport/auth.rs +++ b/crates/rmcp/src/transport/auth.rs @@ -539,6 +539,31 @@ impl AuthClient { } } +/// Why a protected resource metadata document cannot be used for this resource. +#[derive(Debug, Clone, PartialEq, Eq, Error)] +#[non_exhaustive] +pub enum ProtectedResourceMetadataError { + /// The required RFC 9728 `resource` field is absent. + #[error("missing required resource field")] + MissingResource, + + /// The `resource` field is not an absolute URL. + #[error("resource field is not a valid URL: {resource}")] + ResourceNotAUrl { resource: String }, + + /// The resource identifier contains a fragment, which RFC 8707 forbids. + #[error("resource does not permit fragment in URL as specified by RFC 8707: {resource}")] + ResourceHasFragment { resource: Url }, + + /// The advertised resource identifier does not identify the requested resource. + #[error("resource mismatch: reference '{expected}', permitted '{actual}'")] + ResourceMismatch { expected: Url, actual: Url }, + + /// The advertised document has none of the fields that identify resource metadata. + #[error("document carries neither `resource` nor an authorization server reference")] + NotAMetadataDocument, +} + /// Auth error #[derive(Debug, Error)] #[non_exhaustive] @@ -574,6 +599,31 @@ pub enum AuthError { #[error("Metadata error: {0}")] MetadataError(String), + /// A metadata discovery HTTP request could not be completed. + #[error("OAuth metadata discovery failed for {url}")] + DiscoveryRequestFailed { + url: Url, + #[source] + source: OAuthHttpClientError, + }, + + /// A protected resource metadata document is not valid for this resource. + #[error("protected resource metadata at {url} is unusable: {reason}")] + ProtectedResourceMetadataInvalid { + url: Url, + reason: Box, + }, + + /// None of the authorization servers named by resource metadata published usable metadata. + #[error( + "protected resource metadata at {resource_metadata_url} names authorization servers {}, but none published usable metadata", + authorization_servers.join(", ") + )] + AuthorizationServersUnavailable { + resource_metadata_url: Url, + authorization_servers: Vec, + }, + #[error("Authorization server does not support the required PKCE code challenge method (S256)")] PkceUnsupported, @@ -2658,39 +2708,44 @@ impl AuthorizationManager { // Falling back to the base url would send the user somewhere the resource // never named. - Err(AuthError::MetadataError(format!( - "protected resource metadata at {resource_metadata_url} names authorization servers {}, but none published usable metadata", - candidates.join(", ") - ))) + Err(AuthError::AuthorizationServersUnavailable { + resource_metadata_url: resource_metadata_url.clone(), + authorization_servers: candidates, + }) } fn validate_resource_metadata_resource( &self, + resource_metadata_url: &Url, metadata: &ResourceServerMetadata, ) -> Result<(), AuthError> { + let invalid = |reason| AuthError::ProtectedResourceMetadataInvalid { + url: resource_metadata_url.clone(), + reason: Box::new(reason), + }; let Some(resource) = metadata.resource.as_deref() else { - return Err(AuthError::MetadataError( - "Protected resource metadata missing required resource field".to_string(), - )); + return Err(invalid(ProtectedResourceMetadataError::MissingResource)); }; let Ok(resource_url) = Url::parse(resource) else { - return Err(AuthError::MetadataError( - "Protected resource metadata resource field is not a valid URL".to_string(), - )); + return Err(invalid(ProtectedResourceMetadataError::ResourceNotAUrl { + resource: resource.to_string(), + })); }; if resource_url.fragment().is_some() { - return Err(AuthError::MetadataError( - "Protected resource metadata resource does not permit fragment in URL as specified by RFC 8707".to_string() + return Err(invalid( + ProtectedResourceMetadataError::ResourceHasFragment { + resource: resource_url, + }, )); } if !Self::is_resource_identifier_valid(&self.base_url, &resource_url) { - return Err(AuthError::MetadataError(format!( - "Protected resource metadata resource mismatch: reference '{}', permitted '{}'", - self.base_url, resource - ))); + return Err(invalid(ProtectedResourceMetadataError::ResourceMismatch { + expected: self.base_url.clone(), + actual: resource_url, + })); } Ok(()) @@ -2908,9 +2963,12 @@ impl AuthorizationManager { // The server named this url, so there is nothing better to move on // to: the alternatives all drop the resource binding the document // was supposed to carry. Report it instead. - ResourceMetadataUrlOrigin::Advertised => Err(AuthError::MetadataError(format!( - "the server advertised {resource_metadata_url} as protected resource metadata, but the document carries neither `resource` nor an authorization server reference" - ))), + ResourceMetadataUrlOrigin::Advertised => { + Err(AuthError::ProtectedResourceMetadataInvalid { + url: resource_metadata_url.clone(), + reason: Box::new(ProtectedResourceMetadataError::NotAMetadataDocument), + }) + } // Nothing advertised this url, so its answer only rules out this // candidate. ResourceMetadataUrlOrigin::WellKnownGuess => { @@ -2925,7 +2983,9 @@ impl AuthorizationManager { // Carrying those fields only makes the body a metadata document; validation // is what makes it this resource's. Both answers rule out the url the same // way, so both are read the same way. - if let Err(error) = self.validate_resource_metadata_resource(&metadata) { + if let Err(error) = + self.validate_resource_metadata_resource(resource_metadata_url, &metadata) + { return match origin { ResourceMetadataUrlOrigin::Advertised => Err(error), ResourceMetadataUrlOrigin::WellKnownGuess => { @@ -2941,10 +3001,10 @@ impl AuthorizationManager { } fn discovery_failed(url: &Url, error: OAuthHttpClientError) -> AuthError { - AuthError::MetadataError(format!( - "OAuth metadata discovery failed for {url}\n Caused by: {}", - crate::error::ErrorChain(error.as_ref()) - )) + AuthError::DiscoveryRequestFailed { + url: url.clone(), + source: error, + } } async fn discovery_request( @@ -4151,8 +4211,9 @@ mod tests { AuthorizationMetadataSource, AuthorizationRequest, AuthorizationSession, CredentialRefreshGuard, CredentialStore, InMemoryCredentialStore, InMemoryStateStore, OAuthClientConfig, OAuthHttpClient, OAuthHttpClientError, OAuthHttpClientFuture, - OAuthHttpRedirectPolicy, OAuthHttpRequest, ScopeUpgradeConfig, StateStore, - StoredAuthorizationState, is_https_url, + OAuthHttpRedirectPolicy, OAuthHttpRequest, ProtectedResourceMetadataError, + ResourceServerMetadata, ScopeUpgradeConfig, StateStore, StoredAuthorizationState, + is_https_url, }; use crate::transport::auth::VendorExtraTokenFields; @@ -4232,9 +4293,25 @@ mod tests { let url = Url::parse("https://mcp.example.com/mcp").unwrap(); let error = AuthorizationManager::discovery_failed(&url, error); + assert!( + matches!( + &error, + AuthError::DiscoveryRequestFailed { + url: failed_url, + source, + } if failed_url == &url && source.downcast_ref::().is_some() + ), + "unexpected discovery error: {error:?}" + ); + let source = std::error::Error::source(&error).unwrap(); + assert_eq!(source.to_string(), "request failed"); + assert_eq!( + source.source().unwrap().to_string(), + "certificate signed by unknown authority" + ); assert_eq!( error.to_string(), - "Metadata error: OAuth metadata discovery failed for https://mcp.example.com/mcp\n Caused by: request failed\n Caused by: certificate signed by unknown authority" + "OAuth metadata discovery failed for https://mcp.example.com/mcp" ); } @@ -4299,17 +4376,24 @@ mod tests { let manager = AuthorizationManager::new(&url).await.unwrap(); let error = manager.resolve_metadata().await.unwrap_err(); - assert!( - matches!( - error, - AuthError::MetadataError(ref reason) - if reason.contains(&url) - && reason.contains("\n Caused by: error sending request for url") - && reason.matches("error sending request for url").count() == 1 - && reason.to_ascii_lowercase().contains("connection refused") - ), - "unexpected discovery error: {error}" - ); + let AuthError::DiscoveryRequestFailed { + url: failed_url, + source, + } = error + else { + panic!("unexpected discovery error: {error}"); + }; + assert_eq!(failed_url.as_str(), url); + let mut reasons = vec![source.to_string()]; + let mut next = source.source(); + while let Some(error) = next { + reasons.push(error.to_string()); + next = error.source(); + } + let reason = reasons.join("\n"); + assert!(reason.contains("error sending request for url")); + assert_eq!(reason.matches("error sending request for url").count(), 1); + assert!(reason.to_ascii_lowercase().contains("connection refused")); } #[tokio::test] @@ -4331,9 +4415,12 @@ mod tests { assert!( matches!( error, - AuthError::MetadataError(ref reason) - if reason.contains("https://auth.example.com/.well-known/oauth-authorization-server") - && reason.contains("missing fake response") + AuthError::DiscoveryRequestFailed { + ref url, + ref source, + } if url.as_str() + == "https://auth.example.com/.well-known/oauth-authorization-server" + && source.to_string().contains("missing fake response") ), "unexpected discovery error: {error}" ); @@ -4356,8 +4443,11 @@ mod tests { assert!( matches!( error, - AuthError::MetadataError(ref reason) - if reason.contains("https://mcp.example.com/mcp") && reason.contains("503") + AuthError::DiscoveryRequestFailed { + ref url, + ref source, + } if url.as_str() == "https://mcp.example.com/mcp" + && source.to_string().contains("503") ), "unexpected discovery error: {error}" ); @@ -4423,8 +4513,11 @@ mod tests { assert!( matches!( error, - AuthError::MetadataError(ref reason) - if reason.contains(expected_url) && reason.contains(status.as_str()) + AuthError::DiscoveryRequestFailed { + ref url, + ref source, + } if url.as_str() == expected_url + && source.to_string().contains(status.as_str()) ), "unexpected discovery error for {status}: {error}" ); @@ -5311,9 +5404,20 @@ mod tests { .await .unwrap_err(); - assert_eq!( - error.to_string(), - "Metadata error: the server advertised https://mcp.example.com/.well-known/oauth-protected-resource as protected resource metadata, but the document carries neither `resource` nor an authorization server reference" + assert!( + matches!( + error, + AuthError::ProtectedResourceMetadataInvalid { + ref url, + ref reason, + } if url.as_str() + == "https://mcp.example.com/.well-known/oauth-protected-resource" + && matches!( + reason.as_ref(), + ProtectedResourceMetadataError::NotAMetadataDocument + ) + ), + "unexpected protected resource metadata error: {error:?}" ); } @@ -5574,9 +5678,18 @@ mod tests { let error = manager.resolve_metadata().await.unwrap_err(); - assert_eq!( - error.to_string(), - "Metadata error: protected resource metadata at https://mcp.example.com/.well-known/oauth-protected-resource names authorization servers https://auth.example.com, https://sso.example.com, but none published usable metadata" + assert!( + matches!( + error, + AuthError::AuthorizationServersUnavailable { + ref resource_metadata_url, + ref authorization_servers, + } if resource_metadata_url.as_str() + == "https://mcp.example.com/.well-known/oauth-protected-resource" + && authorization_servers + == &["https://auth.example.com", "https://sso.example.com"] + ), + "unexpected authorization servers error: {error:?}" ); let requests = recorder.requests(); let requested: Vec<_> = requests @@ -6394,7 +6507,22 @@ mod tests { let error = manager.resolve_metadata().await.unwrap_err(); assert!( - matches!(error, AuthError::MetadataError(ref message) if message.contains("resource mismatch")), + matches!( + error, + AuthError::ProtectedResourceMetadataInvalid { + ref url, + ref reason, + } if url.as_str() + == "https://mcp.example.com/.well-known/oauth-protected-resource" + && matches!( + reason.as_ref(), + ProtectedResourceMetadataError::ResourceMismatch { + expected, + actual, + } if expected.as_str() == "https://mcp.example.com/mcp" + && actual.as_str() == "https://real.example.com/mcp" + ) + ), "expected resource mismatch metadata error, got: {error:?}" ); assert_eq!(client.requests().len(), 2); @@ -6429,12 +6557,94 @@ mod tests { let error = manager.resolve_metadata().await.unwrap_err(); assert!( - matches!(error, AuthError::MetadataError(ref message) if message.contains("missing required resource")), + matches!( + error, + AuthError::ProtectedResourceMetadataInvalid { + ref url, + ref reason, + } if url.as_str() + == "https://mcp.example.com/.well-known/oauth-protected-resource" + && matches!( + reason.as_ref(), + ProtectedResourceMetadataError::MissingResource + ) + ), "expected missing resource metadata error, got: {error:?}" ); assert_eq!(client.requests().len(), 2); } + #[tokio::test] + async fn protected_resource_metadata_reports_invalid_resource_url() { + let manager = AuthorizationManager::new("https://mcp.example.com/mcp") + .await + .unwrap(); + let metadata_url = + Url::parse("https://mcp.example.com/.well-known/oauth-protected-resource").unwrap(); + let metadata = ResourceServerMetadata { + resource: Some("not a URL".to_string()), + authorization_server: Some("https://auth.example.com".to_string()), + authorization_servers: None, + scopes_supported: None, + }; + + let error = manager + .validate_resource_metadata_resource(&metadata_url, &metadata) + .unwrap_err(); + + assert!( + matches!( + &error, + AuthError::ProtectedResourceMetadataInvalid { + url, + reason, + } if url == &metadata_url + && matches!( + reason.as_ref(), + ProtectedResourceMetadataError::ResourceNotAUrl { resource } + if resource == "not a URL" + ) + ), + "expected invalid resource URL error, got: {error:?}" + ); + } + + #[tokio::test] + async fn protected_resource_metadata_reports_resource_fragment() { + let manager = AuthorizationManager::new("https://mcp.example.com/mcp") + .await + .unwrap(); + let metadata_url = + Url::parse("https://mcp.example.com/.well-known/oauth-protected-resource").unwrap(); + let metadata = ResourceServerMetadata { + resource: Some("https://mcp.example.com/mcp#fragment".to_string()), + authorization_server: Some("https://auth.example.com".to_string()), + authorization_servers: None, + scopes_supported: None, + }; + + let error = manager + .validate_resource_metadata_resource(&metadata_url, &metadata) + .unwrap_err(); + + assert!( + matches!( + &error, + AuthError::ProtectedResourceMetadataInvalid { + url, + reason, + } if url == &metadata_url + && matches!( + reason.as_ref(), + ProtectedResourceMetadataError::ResourceHasFragment { resource } + if resource.as_str() + == "https://mcp.example.com/mcp#fragment" + ) + ), + "expected resource fragment error, got: {error:?}" + ); + } + #[test] fn resource_identifier_matching_allows_matching_host_or_parent_path() { assert!(AuthorizationManager::is_resource_identifier_valid(