feat(mcp): send RFC 8707 resource indicator in OAuth flows

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,36 +103,57 @@ 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");
-    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<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()?;
-
-    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(&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_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)
+        );
+    }
 }