Merge branch 'main'

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<String, Value> = params
+            let body: serde_json::Map<String, Value> = 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<String, String> = params
+            let mut form: HashMap<String, String> = 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}"
+        );
+    }
 }

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");

src/mcp/oauth.rs

@@ -12,6 +12,8 @@ use url::Url;
 
 #[derive(Debug, Deserialize)]
 struct ProtectedResourceMetadata {
+    #[serde(default)]
+    resource: Option<String>,
     #[serde(default)]
     authorization_servers: Vec<String>,
     #[serde(default)]
@@ -30,6 +32,13 @@ struct OAuthServerMetadata {
 #[derive(Serialize, Deserialize)]
 struct McpRegistration {
     client_id: String,
+    #[serde(default)]
+    redirect_uri: Option<String>,
+}
+
+struct DiscoveredOAuth {
+    metadata: OAuthServerMetadata,
+    resource: Option<String>,
 }
 
 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<String> {
         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,9 +103,30 @@ pub async fn run_mcp_oauth_flow(
     callback_port: Option<u16>,
     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");
+
+    // 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 {
+        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();
@@ -96,12 +135,10 @@ pub async fn run_mcp_oauth_flow(
 
         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);
+                    let _ = save_registration(server_name, &id, &redirect_uri);
                     id
                 }
                 Err(e) => {
@@ -116,6 +153,8 @@ pub async fn run_mcp_oauth_flow(
                 .prompt()
                 .context("Failed to read client ID")?
         };
+        (client_id, redirect_uri)
+    };
 
     let provider = McpOAuthProvider {
         client_id,
@@ -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<String> {
+fn load_registration(server_name: &str) -> Option<McpRegistration> {
     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()?;
+    serde_json::from_str(&content).ok()
-
-    Some(reg.client_id)
 }
 
-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(&reg)?)?;
@@ -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<u16>,
+) -> Option<u16> {
+    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<String> {
     let body = serde_json::json!({
         "client_name": "Coyote",
@@ -188,7 +248,44 @@ async fn register_client(endpoint: &str, redirect_uri: &str) -> Result<String> {
         .map(|s| s.to_string())
 }
 
-async fn discover_oauth_metadata(server_url: &str) -> Result<OAuthServerMetadata> {
+/// 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<String> {
+    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<String>, server_url: &str) -> Result<String> {
+    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<DiscoveredOAuth> {
     let client = Client::new();
     let mut tried: Vec<String> = Vec::new();
 
@@ -231,7 +328,10 @@ async fn discover_oauth_metadata(server_url: &str) -> Result<OAuthServerMetadata
                 if meta.scopes_supported.is_empty() {
                     meta.scopes_supported = pr.scopes_supported.clone();
                 }
-                return Ok(meta);
+                return Ok(DiscoveredOAuth {
+                    metadata: meta,
+                    resource: pr.resource.clone(),
+                });
             }
         }
     }
@@ -245,7 +345,11 @@ async fn discover_oauth_metadata(server_url: &str) -> Result<OAuthServerMetadata
             return resp
                 .json::<OAuthServerMetadata>()
                 .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_registration("github", "first-id", "http://127.0.0.1:49152/callback").unwrap();
-            save_registered_client_id("github", "second-id").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)
+        );
+    }
 }