From efa570267da5621fe2e76611170441bd797413a6 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Thu, 6 Aug 2026 16:13:06 -0600 Subject: [PATCH] feat(mcp): send RFC 8707 resource indicator in OAuth flows --- src/client/oauth.rs | 91 ++++++++- src/config/install_remote.rs | 7 + src/mcp/oauth.rs | 366 ++++++++++++++++++++++++++++++----- 3 files changed, 418 insertions(+), 46 deletions(-) diff --git a/src/client/oauth.rs b/src/client/oauth.rs index ae4cc44..d45fdd5 100644 --- a/src/client/oauth.rs +++ b/src/client/oauth.rs @@ -131,6 +131,17 @@ pub trait OAuthProvider: Send + Sync { vec![] } + /// Extra form/body parameters appended to every token request routed + /// through `build_token_request` (authorization-code exchange, refresh, + /// client_credentials, and device-code polling). Used e.g. for the + /// RFC 8707 `resource` indicator required by the MCP spec. + /// NOTE: these are merged AFTER the caller's params and will overwrite + /// a colliding key; do not return protocol parameter names + /// (grant_type, client_id, code, refresh_token, ...). + fn extra_token_params(&self) -> Vec<(&str, &str)> { + vec![] + } + fn token_request_format(&self) -> TokenRequestFormat { TokenRequestFormat::Json } @@ -642,9 +653,14 @@ fn build_token_request( provider: &(impl OAuthProvider + ?Sized), params: &[(&str, &str)], ) -> RequestBuilder { + let all_params: Vec<(&str, &str)> = params + .iter() + .copied() + .chain(provider.extra_token_params()) + .collect(); let mut request = match provider.token_request_format() { TokenRequestFormat::Json => { - let body: serde_json::Map = params + let body: serde_json::Map = all_params .iter() .map(|(k, v)| (k.to_string(), Value::String(v.to_string()))) .collect(); @@ -660,7 +676,7 @@ fn build_token_request( } } TokenRequestFormat::FormUrlEncoded => { - let mut form: HashMap = params + let mut form: HashMap = all_params .iter() .map(|(k, v)| (k.to_string(), v.to_string())) .collect(); @@ -870,6 +886,8 @@ pub(crate) fn client_config_info( #[cfg(test)] mod tests { + use std::str; + use super::*; use crate::client::openai_compatible::OpenAICompatibleConfig; use crate::client::{ModelData, ProviderModels}; @@ -1161,6 +1179,16 @@ echo_pkce_in_token_exchange: true assert!(provider.fixed_redirect_uri().is_none()); } + #[test] + fn default_extra_token_params_is_empty() { + let provider = OpenAICompatibleOAuthProvider { + config: base_config(), + client_name: "test".into(), + }; + + assert!(provider.extra_token_params().is_empty()); + } + #[test] fn oauth_flow_device_code_parses() { let yaml = "client_id: x\ntoken_url: y\nflow: device_code"; @@ -1381,4 +1409,63 @@ scopes: assert!(cfg.use_pkce_in_device_flow); assert_eq!(cfg.scopes, vec!["read", "write"]); } + + struct ResourceStubProvider; + + impl OAuthProvider for ResourceStubProvider { + fn provider_name(&self) -> &str { + "stub" + } + + fn client_id(&self) -> &str { + "stub-client" + } + + fn authorize_url(&self) -> &str { + "https://as.example/authorize" + } + + fn token_url(&self) -> &str { + "https://as.example/token" + } + + fn redirect_uri(&self) -> &str { + "" + } + + fn scopes(&self) -> String { + String::new() + } + + fn token_request_format(&self) -> TokenRequestFormat { + TokenRequestFormat::FormUrlEncoded + } + + fn extra_token_params(&self) -> Vec<(&str, &str)> { + vec![("resource", "https://rs.example/mcp")] + } + } + + #[test] + fn build_token_request_appends_extra_token_params_to_form_body() { + let provider = ResourceStubProvider; + + let request = build_token_request( + &ReqwestClient::new(), + &provider, + &[("grant_type", "authorization_code")], + ) + .build() + .unwrap(); + + let body = str::from_utf8(request.body().unwrap().as_bytes().unwrap()).unwrap(); + assert!( + body.contains("resource=https%3A%2F%2Frs.example%2Fmcp"), + "body missing resource param: {body}" + ); + assert!( + body.contains("grant_type=authorization_code"), + "body missing grant_type param: {body}" + ); + } } diff --git a/src/config/install_remote.rs b/src/config/install_remote.rs index 0606f50..f5a8eaf 100644 --- a/src/config/install_remote.rs +++ b/src/config/install_remote.rs @@ -947,6 +947,7 @@ fn print_secret_summary(added: &[String], deferred: &[String]) { #[cfg(test)] mod tests { use super::*; + use crate::sandbox::SANDBOX_ENV_FLAG; use crate::utils::get_env_name; use serial_test::serial; use std::env; @@ -1431,6 +1432,12 @@ mod tests { #[tokio::test(flavor = "multi_thread", worker_threads = 1)] #[serial] async fn merge_detects_missing_secrets_in_output() { + if env::var_os(SANDBOX_ENV_FLAG).is_some() { + eprintln!( + "Skipping merge_detects_missing_secrets_in_output: secret interpolation is disabled inside a sandbox" + ); + return; + } let _guard = TestVaultConfigGuard::new("merge-secret"); let dir = fresh_temp_dir("merge-secret-"); let remote = dir.join("remote.json"); diff --git a/src/mcp/oauth.rs b/src/mcp/oauth.rs index 2c2cbc2..3455251 100644 --- a/src/mcp/oauth.rs +++ b/src/mcp/oauth.rs @@ -12,6 +12,8 @@ use url::Url; #[derive(Debug, Deserialize)] struct ProtectedResourceMetadata { + #[serde(default)] + resource: Option, #[serde(default)] authorization_servers: Vec, #[serde(default)] @@ -30,6 +32,13 @@ struct OAuthServerMetadata { #[derive(Serialize, Deserialize)] struct McpRegistration { client_id: String, + #[serde(default)] + redirect_uri: Option, +} + +struct DiscoveredOAuth { + metadata: OAuthServerMetadata, + resource: Option, } struct McpOAuthProvider { @@ -38,6 +47,7 @@ struct McpOAuthProvider { token_url: String, scopes: String, fixed_redirect: String, + resource: String, } impl OAuthProvider for McpOAuthProvider { @@ -76,6 +86,14 @@ impl OAuthProvider for McpOAuthProvider { fn fixed_redirect_uri(&self) -> Option { Some(self.fixed_redirect.clone()) } + + fn extra_authorize_params(&self) -> Vec<(&str, &str)> { + vec![("resource", self.resource.as_str())] + } + + fn extra_token_params(&self) -> Vec<(&str, &str)> { + vec![("resource", self.resource.as_str())] + } } pub async fn run_mcp_oauth_flow( @@ -85,36 +103,57 @@ pub async fn run_mcp_oauth_flow( callback_port: Option, redirect_host: Option<&str>, ) -> Result<()> { - let metadata = discover_oauth_metadata(server_url).await?; + let discovered = discover_oauth_metadata(server_url).await?; + let metadata = discovered.metadata; + let resource = resolve_resource(discovered.resource, server_url)?; let host = redirect_host.unwrap_or("127.0.0.1"); - let bind_addr = format!("127.0.0.1:{}", callback_port.unwrap_or(0)); - let listener = TcpListener::bind(&bind_addr)?; - let port = listener.local_addr()?.port(); - drop(listener); - let redirect_uri = format!("http://{host}:{port}/callback"); - let client_id = if let Some(id) = configured_client_id { - id.to_string() - } else if let Some(cached) = load_registered_client_id(server_name) { - cached - } else if let Some(reg_endpoint) = &metadata.registration_endpoint { - match register_client(reg_endpoint, &redirect_uri).await { - Ok(id) => { - let _ = save_registered_client_id(server_name, &id); - id - } - Err(e) => { - warn!("Dynamic client registration failed: {e}. Falling back to manual entry."); - Text::new("Enter the OAuth client ID for this MCP server:") - .prompt() - .context("Failed to read client ID")? - } - } + // Reuse a cached dynamic registration together with the exact redirect + // URI it was registered with (AWS et al. match redirect URIs exactly). + // Only when no client_id is configured explicitly. + let cached_reuse: Option<(String, String)> = if configured_client_id.is_none() { + load_registration(server_name).and_then(|reg| { + let redirect = reg.redirect_uri?; + let port = cached_redirect_port(&redirect, host, callback_port)?; + // The registered port must still be free for our callback listener. + TcpListener::bind(format!("127.0.0.1:{port}")).ok()?; + Some((reg.client_id, redirect)) + }) } else { - Text::new("Enter the OAuth client ID for this MCP server:") - .prompt() - .context("Failed to read client ID")? + None + }; + + let (client_id, redirect_uri) = if let Some(reused) = cached_reuse { + reused + } else { + let bind_addr = format!("127.0.0.1:{}", callback_port.unwrap_or(0)); + let listener = TcpListener::bind(&bind_addr)?; + let port = listener.local_addr()?.port(); + drop(listener); + let redirect_uri = format!("http://{host}:{port}/callback"); + + let client_id = if let Some(id) = configured_client_id { + id.to_string() + } else if let Some(reg_endpoint) = &metadata.registration_endpoint { + match register_client(reg_endpoint, &redirect_uri).await { + Ok(id) => { + let _ = save_registration(server_name, &id, &redirect_uri); + id + } + Err(e) => { + warn!("Dynamic client registration failed: {e}. Falling back to manual entry."); + Text::new("Enter the OAuth client ID for this MCP server:") + .prompt() + .context("Failed to read client ID")? + } + } + } else { + Text::new("Enter the OAuth client ID for this MCP server:") + .prompt() + .context("Failed to read client ID")? + }; + (client_id, redirect_uri) }; let provider = McpOAuthProvider { @@ -123,6 +162,7 @@ pub async fn run_mcp_oauth_flow( token_url: metadata.token_endpoint, scopes: metadata.scopes_supported.join(" "), fixed_redirect: redirect_uri, + resource, }; run_oauth_flow(&provider, &mcp_token_key(server_name)).await @@ -141,21 +181,20 @@ fn mcp_token_key(server_name: &str) -> String { format!("mcp_{server_name}") } -fn load_registered_client_id(server_name: &str) -> Option { +fn load_registration(server_name: &str) -> Option { let path = paths::oauth_tokens_dir().join(format!("mcp_{server_name}_registration.json")); let content = fs::read_to_string(path).ok()?; - let reg: McpRegistration = serde_json::from_str(&content).ok()?; - - Some(reg.client_id) + serde_json::from_str(&content).ok() } -fn save_registered_client_id(server_name: &str, client_id: &str) -> Result<()> { +fn save_registration(server_name: &str, client_id: &str, redirect_uri: &str) -> Result<()> { let dir = paths::oauth_tokens_dir(); fs::create_dir_all(&dir)?; let path = dir.join(format!("mcp_{server_name}_registration.json")); let reg = McpRegistration { client_id: client_id.to_string(), + redirect_uri: Some(redirect_uri.to_string()), }; fs::write(path, serde_json::to_string_pretty(®)?)?; @@ -163,6 +202,27 @@ fn save_registered_client_id(server_name: &str, client_id: &str) -> Result<()> { Ok(()) } +/// Returns the port of a cached registered redirect URI if it is still +/// compatible with the current configuration: same redirect host, and, when +/// a callback port is pinned in config, the same port. Servers like AWS +/// match redirect URIs exactly, so a cached registration is only reusable +/// with the identical redirect URI it was registered with. +fn cached_redirect_port( + cached_redirect: &str, + host: &str, + pinned_port: Option, +) -> Option { + let url = Url::parse(cached_redirect).ok()?; + if url.host_str() != Some(host) { + return None; + } + let port = url.port()?; + if pinned_port.is_some_and(|p| p != port) { + return None; + } + Some(port) +} + async fn register_client(endpoint: &str, redirect_uri: &str) -> Result { let body = serde_json::json!({ "client_name": "Coyote", @@ -188,7 +248,44 @@ async fn register_client(endpoint: &str, redirect_uri: &str) -> Result { .map(|s| s.to_string()) } -async fn discover_oauth_metadata(server_url: &str) -> Result { +/// Derives the canonical resource URI for an MCP server per RFC 8707 @ 2 and +/// the MCP spec: the configured server URL with query and fragment stripped. +fn canonical_resource(server_url: &str) -> Result { + let mut url = + Url::parse(server_url).with_context(|| format!("Invalid MCP server URL: {server_url}"))?; + url.set_query(None); + url.set_fragment(None); + + let s = url.to_string(); + Ok(match url.path() { + "/" => s.trim_end_matches('/').to_string(), + _ => s, + }) +} + +/// Resolves the RFC 8707 resource indicator: prefers the value advertised in +/// the protected resource metadata, but only after validating it identifies +/// the server we are connecting to (RFC 9728 @ 3.3); same scheme/host/port +/// as the configured server URL. Falls back to the canonical server URL on +/// mismatch, empty value, or absence. +fn resolve_resource(advertised: Option, server_url: &str) -> Result { + let canonical = canonical_resource(server_url)?; + let Some(advertised) = advertised.filter(|r| !r.is_empty()) else { + return Ok(canonical); + }; + match (Url::parse(&advertised), Url::parse(server_url)) { + (Ok(a), Ok(s)) if a.origin() == s.origin() => Ok(advertised), + _ => { + warn!( + "Ignoring protected resource metadata resource '{advertised}': \ + it does not match the MCP server origin. Using '{canonical}' instead." + ); + Ok(canonical) + } + } +} + +async fn discover_oauth_metadata(server_url: &str) -> Result { let client = Client::new(); let mut tried: Vec = Vec::new(); @@ -231,7 +328,10 @@ async fn discover_oauth_metadata(server_url: &str) -> Result Result() .await - .with_context(|| format!("Failed to parse OAuth metadata from {as_url}")); + .with_context(|| format!("Failed to parse OAuth metadata from {as_url}")) + .map(|metadata| DiscoveredOAuth { + metadata, + resource: None, + }); } } @@ -469,23 +573,132 @@ mod tests { ); } + #[test] + fn canonical_resource_strips_query() { + let result = canonical_resource("https://aws-mcp.us-east-1.api.aws/mcp?oauth=initialize"); + + assert_eq!(result.unwrap(), "https://aws-mcp.us-east-1.api.aws/mcp"); + } + + #[test] + fn canonical_resource_strips_fragment() { + let result = canonical_resource("https://example.com/mcp#section"); + + assert_eq!(result.unwrap(), "https://example.com/mcp"); + } + + #[test] + fn canonical_resource_preserves_path_and_port() { + let result = canonical_resource("http://localhost:8080/mcp/v1?x=1"); + + assert_eq!(result.unwrap(), "http://localhost:8080/mcp/v1"); + } + + #[test] + fn canonical_resource_rejects_invalid_url() { + assert!(canonical_resource("not-a-url").is_err()); + } + + #[test] + fn canonical_resource_bare_host_has_no_trailing_slash() { + let result = canonical_resource("https://mcp.example.com"); + + assert_eq!(result.unwrap(), "https://mcp.example.com"); + } + + #[test] + fn resolve_resource_prefers_matching_advertised() { + let result = resolve_resource( + Some("https://aws-mcp.us-east-1.api.aws/mcp".into()), + "https://aws-mcp.us-east-1.api.aws/mcp?oauth=initialize", + ); + + assert_eq!(result.unwrap(), "https://aws-mcp.us-east-1.api.aws/mcp"); + } + + #[test] + fn resolve_resource_rejects_cross_origin_advertised() { + let result = resolve_resource( + Some("https://evil.example.com/mcp".into()), + "https://aws-mcp.us-east-1.api.aws/mcp", + ); + + assert_eq!(result.unwrap(), "https://aws-mcp.us-east-1.api.aws/mcp"); + } + + #[test] + fn resolve_resource_empty_falls_back_to_canonical() { + let result = resolve_resource(Some(String::new()), "https://example.com/mcp"); + + assert_eq!(result.unwrap(), "https://example.com/mcp"); + } + + #[test] + fn resolve_resource_none_falls_back_to_canonical() { + let result = resolve_resource(None, "https://example.com/mcp"); + + assert_eq!(result.unwrap(), "https://example.com/mcp"); + } + + #[test] + fn protected_resource_metadata_deserializes_resource_field() { + let json = r#"{"resource":"https://aws-mcp.us-east-1.api.aws/mcp","authorization_servers":["https://us-east-1.oauth.signin.aws/"]}"#; + + let pr: ProtectedResourceMetadata = serde_json::from_str(json).unwrap(); + + assert_eq!( + pr.resource.as_deref(), + Some("https://aws-mcp.us-east-1.api.aws/mcp") + ); + assert_eq!( + pr.authorization_servers, + vec!["https://us-east-1.oauth.signin.aws/"] + ); + } + + #[test] + fn mcp_provider_sends_resource_in_authorize_and_token_params() { + let provider = McpOAuthProvider { + client_id: "client-123".into(), + authorize_url: "https://as.example/authorize".into(), + token_url: "https://as.example/token".into(), + scopes: String::new(), + fixed_redirect: "http://127.0.0.1:9000/callback".into(), + resource: "https://aws-mcp.us-east-1.api.aws/mcp".into(), + }; + + assert_eq!( + provider.extra_authorize_params(), + vec![("resource", "https://aws-mcp.us-east-1.api.aws/mcp")] + ); + assert_eq!( + provider.extra_token_params(), + vec![("resource", "https://aws-mcp.us-east-1.api.aws/mcp")] + ); + } + #[test] #[serial] fn registered_client_id_roundtrip() { with_temp_cache(|| { - save_registered_client_id("notion", "client-xyz-123").unwrap(); + save_registration( + "notion", + "client-xyz-123", + "http://127.0.0.1:49152/callback", + ) + .unwrap(); - let loaded = load_registered_client_id("notion"); + let loaded = load_registration("notion"); - assert_eq!(loaded, Some("client-xyz-123".to_string())); + assert_eq!(loaded.unwrap().client_id, "client-xyz-123"); }); } #[test] #[serial] - fn load_registered_client_id_returns_none_for_missing() { + fn load_registration_returns_none_for_missing() { with_temp_cache(|| { - let loaded = load_registered_client_id("no-such-server"); + let loaded = load_registration("no-such-server"); assert!(loaded.is_none()); }); @@ -493,14 +706,79 @@ mod tests { #[test] #[serial] - fn registered_client_id_second_save_overwrites_first() { + fn registration_second_save_overwrites_first() { with_temp_cache(|| { - save_registered_client_id("github", "first-id").unwrap(); - save_registered_client_id("github", "second-id").unwrap(); + save_registration("github", "first-id", "http://127.0.0.1:49152/callback").unwrap(); + save_registration("github", "second-id", "http://127.0.0.1:49153/callback").unwrap(); - let loaded = load_registered_client_id("github"); + let loaded = load_registration("github").unwrap(); - assert_eq!(loaded, Some("second-id".to_string())); + assert_eq!(loaded.client_id, "second-id"); + assert_eq!( + loaded.redirect_uri.as_deref(), + Some("http://127.0.0.1:49153/callback") + ); }); } + + #[test] + #[serial] + fn old_format_registration_still_loads() { + with_temp_cache(|| { + let dir = paths::oauth_tokens_dir(); + fs::create_dir_all(&dir).unwrap(); + fs::write( + dir.join("mcp_legacy_registration.json"), + r#"{"client_id":"legacy-id"}"#, + ) + .unwrap(); + + let loaded = load_registration("legacy").unwrap(); + + assert_eq!(loaded.client_id, "legacy-id"); + assert_eq!(loaded.redirect_uri, None); + }); + } + + #[test] + #[serial] + fn save_registration_persists_redirect_uri() { + with_temp_cache(|| { + save_registration("aws", "client-abc", "http://127.0.0.1:49152/callback").unwrap(); + + let loaded = load_registration("aws").unwrap(); + + assert_eq!(loaded.client_id, "client-abc"); + assert_eq!( + loaded.redirect_uri.as_deref(), + Some("http://127.0.0.1:49152/callback") + ); + }); + } + + #[test] + fn cached_redirect_port_matches() { + let port = cached_redirect_port("http://127.0.0.1:49152/callback", "127.0.0.1", None); + + assert_eq!(port, Some(49152)); + } + + #[test] + fn cached_redirect_port_rejects_host_mismatch() { + let port = cached_redirect_port("http://127.0.0.1:49152/callback", "localhost", None); + + assert_eq!(port, None); + } + + #[test] + fn cached_redirect_port_respects_pinned_port() { + assert_eq!( + cached_redirect_port("http://127.0.0.1:49152/callback", "127.0.0.1", Some(50000)), + None + ); + assert_eq!( + cached_redirect_port("http://127.0.0.1:49152/callback", "127.0.0.1", Some(49152)), + Some(49152) + ); + } }