Repository navigation
fix: reject discovery for legacy-only servers #1269
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -61,6 +61,16 @@ impl<H: ServerHandler> Service<RoleServer> for H { | |||||||||||||||||||||||||||||||||||||||||||||||
| .is_some_and(|v| v.as_str() >= ProtocolVersion::V_2026_07_28.as_str()); | ||||||||||||||||||||||||||||||||||||||||||||||||
| let requested_version = context.meta.protocol_version(); | ||||||||||||||||||||||||||||||||||||||||||||||||
| let uses_inline_negotiation = !matches!(&request, ClientRequest::InitializeRequest(_)); | ||||||||||||||||||||||||||||||||||||||||||||||||
| // Legacy-only servers do not implement discovery. MethodNotFound tells | ||||||||||||||||||||||||||||||||||||||||||||||||
| // dual-lifecycle clients to fall back to initialize. | ||||||||||||||||||||||||||||||||||||||||||||||||
| if matches!(&request, ClientRequest::DiscoverRequest(_)) | ||||||||||||||||||||||||||||||||||||||||||||||||
| && self | ||||||||||||||||||||||||||||||||||||||||||||||||
| .supported_protocol_versions() | ||||||||||||||||||||||||||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||||||||||||||||||||||||||
| .all(ProtocolVersion::has_initialize) | ||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||
| return Err(McpError::method_not_found::<DiscoverRequestMethod>()); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| if uses_inline_negotiation && let Some(requested_version) = requested_version.as_ref() { | ||||||||||||||||||||||||||||||||||||||||||||||||
| let supported_versions = self.supported_protocol_versions(); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+64
to
75
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The guard and the version check below both call
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
| if !supported_versions.contains(requested_version) { | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ impl ServerHandler for DiscoveryServer { | |
| } | ||
|
|
||
| fn supported_protocol_versions(&self) -> Cow<'static, [ProtocolVersion]> { | ||
| Cow::Borrowed(&[ProtocolVersion::V_2025_11_25]) | ||
| Cow::Borrowed(&[ProtocolVersion::V_2025_11_25, ProtocolVersion::V_2026_07_28]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With streamable HTTP, |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -120,7 +120,7 @@ async fn discover_returns_server_metadata_without_session() { | |
| body["result"], | ||
| json!({ | ||
| "resultType": "complete", | ||
| "supportedVersions": ["2025-11-25"], | ||
| "supportedVersions": ["2025-11-25", "2026-07-28"], | ||
| "capabilities": { "tools": {} }, | ||
| "_meta": { | ||
| "io.modelcontextprotocol/serverInfo": { | ||
|
|
@@ -153,16 +153,16 @@ async fn discover_does_not_require_initialization_in_legacy_session_mode() { | |
| async fn discover_rejects_unsupported_version_with_http_400() { | ||
| let (client, url, cancellation_token) = spawn_server(true).await; | ||
|
|
||
| let response = post_discover(&client, &url, "2026-07-28", Some("2026-07-28")).await; | ||
| let response = post_discover(&client, &url, "2099-01-01", Some("2099-01-01")).await; | ||
|
|
||
| assert_eq!(response.status(), 400); | ||
| let body: serde_json::Value = response.json().await.expect("response should be JSON"); | ||
| assert_eq!(body["error"]["code"], -32022); | ||
| assert_eq!( | ||
| body["error"]["data"], | ||
| json!({ | ||
| "requested": "2026-07-28", | ||
| "supported": ["2025-11-25"] | ||
| "requested": "2099-01-01", | ||
| "supported": ["2025-11-25", "2026-07-28"] | ||
| }) | ||
| ); | ||
|
|
||
|
|
@@ -191,7 +191,7 @@ async fn regular_request_rejects_server_unsupported_meta_version() { | |
| "method": "tools/list", | ||
| "params": { | ||
| "_meta": { | ||
| "io.modelcontextprotocol/protocolVersion": "2026-07-28" | ||
| "io.modelcontextprotocol/protocolVersion": "2099-01-01" | ||
| } | ||
| } | ||
| }); | ||
|
|
@@ -200,7 +200,7 @@ async fn regular_request_rejects_server_unsupported_meta_version() { | |
| .post(&url) | ||
| .header("Content-Type", "application/json") | ||
| .header("Accept", "application/json, text/event-stream") | ||
| .header("MCP-Protocol-Version", "2026-07-28") | ||
| .header("MCP-Protocol-Version", "2099-01-01") | ||
| .header("Mcp-Method", "tools/list") | ||
| .json(&body) | ||
| .send() | ||
|
|
@@ -365,7 +365,7 @@ async fn discover_accepts_missing_optional_client_info() { | |
| async fn discover_error_uses_http_400_when_sse_is_configured() { | ||
| let (client, url, cancellation_token) = spawn_server(false).await; | ||
|
|
||
| let response = post_discover(&client, &url, "2026-07-28", Some("2026-07-28")).await; | ||
| let response = post_discover(&client, &url, "2099-01-01", Some("2099-01-01")).await; | ||
|
|
||
| assert_eq!(response.status(), 400); | ||
| assert_eq!( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,18 +1,22 @@ | ||||||||||||||
| #![cfg(all(feature = "server", not(feature = "local")))] | ||||||||||||||
|
|
||||||||||||||
| use std::{ | ||||||||||||||
| sync::{Arc, Mutex}, | ||||||||||||||
| borrow::Cow, | ||||||||||||||
| sync::{ | ||||||||||||||
| Arc, Mutex, | ||||||||||||||
| atomic::{AtomicBool, Ordering}, | ||||||||||||||
| }, | ||||||||||||||
| time::Duration, | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| use rmcp::{ | ||||||||||||||
| ServerHandler, ServiceExt, | ||||||||||||||
| ClientLifecycleMode, ClientServiceExt, ServerHandler, ServiceExt, | ||||||||||||||
| model::{ | ||||||||||||||
| ClientCapabilities, ClientJsonRpcMessage, ClientRequest, DiscoverRequest, | ||||||||||||||
| DiscoverRequestParams, ErrorCode, ErrorData, Implementation, ListToolsRequest, | ||||||||||||||
| ListToolsResult, NumberOrString, PaginatedRequestParams, ProgressNotificationParam, | ||||||||||||||
| ProgressToken, ProtocolVersion, RequestId, RequestMetaObject, ServerJsonRpcMessage, | ||||||||||||||
| ServerNotification, | ||||||||||||||
| DiscoverRequestMethod, DiscoverRequestParams, DiscoverResult, ErrorCode, ErrorData, | ||||||||||||||
| Implementation, ListToolsRequest, ListToolsResult, NumberOrString, PaginatedRequestParams, | ||||||||||||||
| ProgressNotificationParam, ProgressToken, ProtocolVersion, RequestId, RequestMetaObject, | ||||||||||||||
| ServerJsonRpcMessage, ServerNotification, | ||||||||||||||
| }, | ||||||||||||||
| service::{MaybeSendFuture, RequestContext, RoleServer}, | ||||||||||||||
| transport::{IntoTransport, Transport}, | ||||||||||||||
|
|
@@ -23,6 +27,25 @@ struct StatelessServer; | |||||||||||||
|
|
||||||||||||||
| impl ServerHandler for StatelessServer {} | ||||||||||||||
|
|
||||||||||||||
| #[derive(Clone)] | ||||||||||||||
| struct LegacyServer { | ||||||||||||||
| discover_called: Arc<AtomicBool>, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| impl ServerHandler for LegacyServer { | ||||||||||||||
| fn supported_protocol_versions(&self) -> Cow<'static, [ProtocolVersion]> { | ||||||||||||||
| Cow::Borrowed(ProtocolVersion::known_up_to(&ProtocolVersion::V_2025_11_25)) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| async fn discover( | ||||||||||||||
| &self, | ||||||||||||||
| _context: RequestContext<RoleServer>, | ||||||||||||||
| ) -> Result<DiscoverResult, ErrorData> { | ||||||||||||||
| self.discover_called.store(true, Ordering::Relaxed); | ||||||||||||||
| Err(ErrorData::method_not_found::<DiscoverRequestMethod>()) | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this stub also returns
Suggested change
|
||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| fn complete_meta() -> RequestMetaObject { | ||||||||||||||
| complete_meta_for("stateless-client") | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -123,6 +146,42 @@ async fn stateless_server_rejects_missing_metadata_on_every_request() { | |||||||||||||
| running.cancel().await.expect("cancel server"); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| #[tokio::test] | ||||||||||||||
| async fn legacy_only_server_rejects_discovery_before_dispatch() { | ||||||||||||||
| let discover_called = Arc::new(AtomicBool::new(false)); | ||||||||||||||
| let server = LegacyServer { | ||||||||||||||
| discover_called: Arc::clone(&discover_called), | ||||||||||||||
| }; | ||||||||||||||
| let (server_transport, client_transport) = tokio::io::duplex(4096); | ||||||||||||||
| let server_task = tokio::spawn(async move { | ||||||||||||||
| server | ||||||||||||||
| .serve(server_transport) | ||||||||||||||
| .await | ||||||||||||||
| .expect("server should start") | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| let client = () | ||||||||||||||
| .serve_with_lifecycle( | ||||||||||||||
| client_transport, | ||||||||||||||
| ClientLifecycleMode::Auto { | ||||||||||||||
| preferred_versions: vec![ProtocolVersion::V_2026_07_28], | ||||||||||||||
| legacy_version: Some(ProtocolVersion::V_2025_11_25), | ||||||||||||||
| }, | ||||||||||||||
| ) | ||||||||||||||
| .await | ||||||||||||||
| .expect("client should fall back to initialize"); | ||||||||||||||
|
|
||||||||||||||
| assert!(!discover_called.load(Ordering::Relaxed)); | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This confirms the client started, but not that it actually used the
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| client.cancel().await.expect("cancel client"); | ||||||||||||||
| server_task | ||||||||||||||
| .await | ||||||||||||||
| .expect("server task") | ||||||||||||||
| .cancel() | ||||||||||||||
| .await | ||||||||||||||
| .expect("cancel server"); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| #[derive(Clone)] | ||||||||||||||
| struct ContextServer { | ||||||||||||||
| seen_clients: Arc<Mutex<Vec<String>>>, | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A custom
ServerHandler::discoveroverride is no longer called whensupported_protocol_versionsreturns only versions from theinitializeera. Implementers can't tell that from the trait, so could we add a short note to thediscoverdoc comment inserver_handler_methods!?