Skip to content

fix(auth): let url origin decide protected resource metadata discovery #1264

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
DaleSeo wants to merge 1 commit into
base: main
Choose a base branch
from
+198 −26
Open

fix(auth): let url origin decide protected resource metadata discovery #1264

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
224 changes: 198 additions & 26 deletions crates/rmcp/src/transport/auth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,11 +689,12 @@ struct ResourceServerMetadata {
/// decides what a document that is not metadata means there.
#[derive(Debug, Clone, Copy)]
enum ResourceMetadataUrlOrigin {
/// The server named this url in the `resource_metadata` parameter of a
/// `WWW-Authenticate` challenge.
/// The resource itself named this url in the `resource_metadata` parameter
/// of its `WWW-Authenticate` challenge.
Advertised,
/// The url was derived from the base url, on the chance that the document is
/// published there.
/// The url was derived from the base url on the chance that the document is
/// published there, or named by a challenge on such a url. Either way the
/// run only guessed its way here.
WellKnownGuess,
}

Expand Down Expand Up @@ -2570,7 +2571,9 @@ impl AuthorizationManager {
}

/// Walk the authorization servers a protected resource metadata document
/// names, keeping the first one that answers with usable metadata.
/// names, keeping the first one that answers with usable metadata. A document
/// that names servers and none of them answers is an error, not a reason to
/// go looking elsewhere; a document that names none returns `Ok(None)`.
///
/// The document arrives here through `read_resource_metadata`, which is where
/// it is decided to be this resource's metadata at all.
Expand Down Expand Up @@ -2609,10 +2612,17 @@ impl AuthorizationManager {
push_candidate(candidate);
}

for candidate in candidates {
let candidate_url = match Url::parse(&candidate) {
if candidates.is_empty() {
debug!(
"protected resource metadata at {resource_metadata_url} names no authorization server"
);
return Ok(None);
}

for candidate in &candidates {
let candidate_url = match Url::parse(candidate) {
Ok(url) => url,
Err(_) => match resource_metadata_url.join(&candidate) {
Err(_) => match resource_metadata_url.join(candidate) {
Ok(url) => url,
Err(e) => {
debug!("Failed to resolve authorization server URL `{candidate}`: {e}");
Expand Down Expand Up @@ -2646,7 +2656,12 @@ impl AuthorizationManager {
}
}

Ok(None)
// 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(", ")
)))
}

fn validate_resource_metadata_resource(
Expand Down Expand Up @@ -2762,23 +2777,25 @@ impl AuthorizationManager {
}
}
StatusCode::UNAUTHORIZED => {
let Some(advertised_url) = self
let Some(pointer_url) = self
.extract_resource_metadata_url_from_www_authenticate(&response)
.await
else {
continue;
};
if !requested.insert(advertised_url.clone()) {
if !requested.insert(pointer_url.clone()) {
continue;
}
// Found on a url this run guessed at, so a wrong answer only rules
// out the pointer.
if let Some(metadata) = self
.fetch_resource_metadata_from_url(
&advertised_url,
ResourceMetadataUrlOrigin::Advertised,
&pointer_url,
ResourceMetadataUrlOrigin::WellKnownGuess,
)
.await?
{
return Ok(Some((advertised_url, metadata)));
return Ok(Some((pointer_url, metadata)));
}
}
status => debug!("resource metadata probe returned unexpected status: {status}"),
Expand Down Expand Up @@ -5496,8 +5513,8 @@ mod tests {
}),
),
];
// the authorization server publishes no metadata, so every form of its
// discovery url is tried before the run settles on the legacy endpoints
// the authorization server publishes no metadata; more 404s than the walk
// can use, so that requesting it twice would show up in the sequence
responses.extend(std::iter::repeat_with(|| empty_response(404)).take(10));
let client = RecordingOAuthHttpClient::with_responses(responses);
let recorder = client.clone();
Expand All @@ -5508,24 +5525,179 @@ mod tests {
.await
.unwrap();

manager.resolve_metadata().await.unwrap_err();

let requests = recorder.requests();
let requested: Vec<_> = requests
.iter()
.map(|request| request.uri.as_str())
.collect();
assert_eq!(
requested,
[
"https://mcp.example.com/",
"https://mcp.example.com/.well-known/oauth-protected-resource",
"https://auth.example.com/.well-known/oauth-authorization-server",
"https://auth.example.com/.well-known/openid-configuration",
]
);
}

#[tokio::test]
async fn resolve_metadata_reports_named_authorization_servers_that_publish_no_metadata() {
let mut responses = vec![
// the MCP endpoint answers GET with a health payload, not metadata
http_response(
200,
serde_json::json!({"status": "healthy", "message": "MCP server is running"}),
),
// the document names two authorization servers
http_response(
200,
serde_json::json!({
"resource": "https://mcp.example.com/",
"authorization_servers": ["https://auth.example.com", "https://sso.example.com"]
}),
),
];
// neither publishes metadata; the surplus 404s would let the run reach the
// resource host's own well-known urls and the legacy endpoints if it kept going
responses.extend(std::iter::repeat_with(|| empty_response(404)).take(10));
let client = RecordingOAuthHttpClient::with_responses(responses);
let recorder = client.clone();
let manager = AuthorizationManager::new_with_oauth_http_client(
"https://mcp.example.com/",
Arc::new(client),
)
.await
.unwrap();

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"
);
let requests = recorder.requests();
let requested: Vec<_> = requests
.iter()
.map(|request| request.uri.as_str())
.collect();
assert_eq!(
requested,
[
"https://mcp.example.com/",
"https://mcp.example.com/.well-known/oauth-protected-resource",
"https://auth.example.com/.well-known/oauth-authorization-server",
"https://auth.example.com/.well-known/openid-configuration",
"https://sso.example.com/.well-known/oauth-authorization-server",
"https://sso.example.com/.well-known/openid-configuration",
]
);
}

#[tokio::test]
async fn resolve_metadata_keeps_falling_back_when_the_document_names_no_authorization_server() {
let mut responses = vec![
// the MCP endpoint answers GET with a health payload, not metadata
http_response(
200,
serde_json::json!({"status": "healthy", "message": "MCP server is running"}),
),
// the document says nothing about where to authorize
http_response(
200,
serde_json::json!({"resource": "https://mcp.example.com/"}),
),
];
responses.extend(std::iter::repeat_with(|| empty_response(404)).take(10));
let client = RecordingOAuthHttpClient::with_responses(responses);
let manager = AuthorizationManager::new_with_oauth_http_client(
"https://mcp.example.com/",
Arc::new(client),
)
.await
.unwrap();

let resolution = manager.resolve_metadata().await.unwrap();

assert_eq!(
resolution.source,
AuthorizationMetadataSource::LegacyEndpointFallback
);
let discovery_requests = recorder
.requests()
}

#[tokio::test]
async fn resolve_metadata_tries_the_next_candidate_past_a_pointer_found_while_guessing() {
let challenge = oauth2::http::Response::builder()
.status(401)
.header(
"www-authenticate",
r#"Bearer resource_metadata="https://mcp.example.com/prm""#,
)
.body(Vec::new())
.unwrap();
let client = RecordingOAuthHttpClient::with_responses(vec![
// the MCP endpoint answers GET with a health payload, not metadata
http_response(
200,
serde_json::json!({"status": "healthy", "message": "MCP server is running"}),
),
// auth sits in front of the first candidate and its challenge names a url
challenge,
// which serves something that is not the document
http_response(200, serde_json::json!({})),
// the second candidate carries the real document
http_response(
200,
serde_json::json!({
"resource": "https://mcp.example.com/mcp",
"authorization_servers": ["https://auth.example.com"]
}),
),
http_response(
200,
serde_json::json!({
"issuer": "https://auth.example.com",
"authorization_endpoint": "https://auth.example.com/authorize",
"token_endpoint": "https://auth.example.com/token"
}),
),
]);
let recorder = client.clone();
let manager = AuthorizationManager::new_with_oauth_http_client(
"https://mcp.example.com/mcp",
Arc::new(client),
)
.await
.unwrap();

let resolution = manager.resolve_metadata().await.unwrap();

assert_eq!(
(
resolution.source,
resolution.metadata.token_endpoint.as_str(),
),
(
AuthorizationMetadataSource::ProtectedResourceMetadata,
"https://auth.example.com/token",
)
);
let requests = recorder.requests();
let requested: Vec<_> = requests
.iter()
.filter(|request| {
request.uri == "https://auth.example.com/.well-known/oauth-authorization-server"
})
.count();
.map(|request| request.uri.as_str())
.collect();
assert_eq!(
discovery_requests,
1,
"the authorization server was walked once per field naming it: {:?}",
recorder.requests()
requested,
[
"https://mcp.example.com/mcp",
"https://mcp.example.com/.well-known/oauth-protected-resource/mcp",
"https://mcp.example.com/prm",
"https://mcp.example.com/mcp/.well-known/oauth-protected-resource",
"https://auth.example.com/.well-known/oauth-authorization-server",
]
);
}

Expand Down
Loading