From 757e5b30c76a4ae0f281ec91503d9a4acd3ef872 Mon Sep 17 00:00:00 2001 From: thomas <291887306+tsarlandie-oai@users.noreply.github.com> Date: Fri, 30 Oct 2026 00:23:48 +0000 Subject: [PATCH 1/3] fix(skills): validate manifest containment and capability negotiation --- crates/rmcp/src/model/skill.rs | 132 ++++++++++++++++++ crates/rmcp/src/service/client.rs | 38 +++-- crates/rmcp/tests/test_skills.rs | 41 +++++- crates/rmcp/tests/test_skills_capabilities.rs | 79 +++++++++++ 4 files changed, 280 insertions(+), 10 deletions(-) create mode 100644 crates/rmcp/tests/test_skills_capabilities.rs diff --git a/crates/rmcp/src/model/skill.rs b/crates/rmcp/src/model/skill.rs index 5216bb054..bae3be358 100644 --- a/crates/rmcp/src/model/skill.rs +++ b/crates/rmcp/src/model/skill.rs @@ -285,6 +285,12 @@ impl Skill { } let mut seen = std::collections::HashSet::with_capacity(files.len()); for file in files { + if !resource_is_within_skill(&self.uri, &file.uri) { + return Err(SkillValidationError::ResourceOutsideSkill { + uri: file.uri.clone(), + skill_uri: self.uri.clone(), + }); + } if !seen.insert(file.uri.as_str()) { return Err(SkillValidationError::DuplicateResource(file.uri.clone())); } @@ -304,11 +310,46 @@ pub const SKILL_MD: &str = "SKILL.md"; /// The skill name encoded in a `SKILL.md` URI: the path segment before `SKILL.md`. pub fn skill_name_from_uri(uri: &str) -> Option<&str> { + if uri.contains(['?', '#']) { + return None; + } let path = uri.strip_suffix(SKILL_MD)?.strip_suffix('/')?; let name = path.rsplit('/').next()?; (!name.is_empty()).then_some(name) } +fn resource_is_within_skill(skill_uri: &str, resource_uri: &str) -> bool { + // Keep the trailing slash to distinguish a child from a sibling whose + // directory name merely starts with the skill's name, under any scheme. + let Some(relative) = skill_uri + .strip_suffix(SKILL_MD) + .and_then(|root| resource_uri.strip_prefix(root)) + else { + return false; + }; + let path = relative.split(['?', '#']).next().unwrap_or_default(); + if path.is_empty() || path.ends_with('/') { + return false; + } + let mut depth = 0usize; + for segment in path.split('/') { + // Dots are unreserved URI characters. Recognize their escaped form, + // but leave reserved delimiters such as %2F and query data untouched. + let segment = segment.replace("%2e", ".").replace("%2E", "."); + match segment.as_str() { + "." => {} + ".." => { + let Some(parent) = depth.checked_sub(1) else { + return false; + }; + depth = parent; + } + _ => depth += 1, + } + } + depth > 0 +} + /// Whether `digest` is `sha256:` followed by 64 lowercase hex characters. pub fn is_sha256_digest(digest: &str) -> bool { digest.strip_prefix("sha256:").is_some_and(|hex| { @@ -340,6 +381,8 @@ pub enum SkillValidationError { MissingSkillMd(String), #[error("skill manifest lists {0} twice")] DuplicateResource(String), + #[error("skill file {uri} is not within the directory of {skill_uri}")] + ResourceOutsideSkill { uri: String, skill_uri: String }, #[error("skill file {uri} has a malformed digest {digest}")] MalformedDigest { uri: String, digest: String }, #[error("skill {0} is listed twice")] @@ -604,6 +647,95 @@ mod tests { assert_eq!(billing_skill().validate(), Ok(())); } + fn with_supporting_file(skill_uri: &str, resource_uri: &str) -> Skill { + Skill::new( + skill_uri, + SkillFrontmatter::new("billing", "Issue invoices"), + SkillResources::Files(vec![ + SkillResource::from_bytes(skill_uri, b"skill"), + SkillResource::from_bytes(resource_uri, b"supporting file"), + ]), + ) + } + + #[test] + fn accepts_supporting_files_within_the_skill_directory() { + for uri in [ + "skill://acme/billing/invoice.md", + "skill://acme/billing/refunds/SKILL.md", + "skill://acme/billing/references/../invoice.md", + "skill://acme/billing/references/%2e%2E/invoice.md", + "skill://acme/billing/templates/.../invoice.md", + "skill://acme/billing/references%2Fguide.md", + "skill://acme/billing/invoice.md?reference=/../../other", + "skill://acme/billing/invoice.md#reference/../../other", + ] { + let skill = with_supporting_file("skill://acme/billing/SKILL.md", uri); + assert_eq!(skill.validate(), Ok(()), "{uri}"); + } + } + + #[test] + fn accepts_supporting_files_under_other_uri_schemes() { + for (skill_uri, resource_uri) in [ + ( + "github://owner/repo/skills/billing/SKILL.md", + "github://owner/repo/skills/billing/references/guide.md", + ), + ( + "file:///skills/billing/SKILL.md", + "file:///skills/billing/templates/invoice.md", + ), + ( + "custom:skills/billing/SKILL.md", + "custom:skills/billing/invoice.md", + ), + ] { + assert_eq!( + with_supporting_file(skill_uri, resource_uri).validate(), + Ok(()), + "{resource_uri}" + ); + } + } + + #[test] + fn rejects_supporting_files_outside_the_skill_directory() { + for uri in [ + "skill://other/billing/invoice.md", + "github://acme/billing/invoice.md", + "skill://acme/billing-other/invoice.md", + "skill://acme/invoice.md", + "skill://acme/billing/../invoice.md", + "skill://acme/billing/references/../../invoice.md", + "skill://acme/billing/%2e%2e/invoice.md", + "skill://acme/billing/.%2E/invoice.md", + "skill://acme/billing/%2E./invoice.md", + "skill://acme/billing/references/%2E%2e/%2e%2E/invoice.md", + "skill://acme/billing", + "skill://acme/billing/", + "skill://acme/billing/references/..", + "skill://acme/billing/references/../", + "skill://acme/billing/references/../?reference=invoice.md", + ] { + let skill = with_supporting_file("skill://acme/billing/SKILL.md", uri); + assert!(skill.validate().is_err(), "accepted {uri}"); + } + } + + #[test] + fn rejects_a_skill_md_suffix_in_a_query_or_fragment() { + for uri in [ + "skill://acme/billing?reference=/billing/SKILL.md", + "skill://acme/billing#reference/billing/SKILL.md", + ] { + assert!( + with_supporting_file(uri, uri).validate().is_err(), + "accepted {uri}" + ); + } + } + #[test] fn accepts_a_dynamic_entry_without_files() { let skill = Skill::new( diff --git a/crates/rmcp/src/service/client.rs b/crates/rmcp/src/service/client.rs index d094ac91b..21d68be89 100644 --- a/crates/rmcp/src/service/client.rs +++ b/crates/rmcp/src/service/client.rs @@ -18,14 +18,14 @@ use crate::{ CompletionContext, CompletionInfo, DEFAULT_MRTR_MAX_ROUNDS, DiscoverRequest, DiscoverRequestParams, DiscoverResult, ErrorData, GetExtensions, GetMeta, GetPromptRequest, GetPromptRequestParams, GetPromptResponse, GetPromptResult, GetSkillRequest, - GetSkillRequestParams, GetSkillResult, GetTaskParams, GetTaskRequest, GetTaskResult, - InitializeRequest, InitializeRequestParams, InitializedNotification, InputRequest, - InputRequiredResult, InputResponses, JsonRpcResponse, ListPromptsRequest, - ListPromptsResult, ListResourceTemplatesRequest, ListResourceTemplatesResult, - ListResourcesRequest, ListResourcesResult, ListSkillsRequest, ListSkillsResult, - ListToolsRequest, ListToolsResult, NumberOrString, PaginatedRequestParams, - ProgressNotification, ProgressNotificationParam, ProtocolVersion, - ReadResourceDirectoryRequest, ReadResourceDirectoryRequestMethod, + GetSkillRequestMethod, GetSkillRequestParams, GetSkillResult, GetTaskParams, + GetTaskRequest, GetTaskResult, InitializeRequest, InitializeRequestParams, + InitializedNotification, InputRequest, InputRequiredResult, InputResponses, + JsonRpcResponse, ListPromptsRequest, ListPromptsResult, ListResourceTemplatesRequest, + ListResourceTemplatesResult, ListResourcesRequest, ListResourcesResult, ListSkillsRequest, + ListSkillsRequestMethod, ListSkillsResult, ListToolsRequest, ListToolsResult, + NumberOrString, PaginatedRequestParams, ProgressNotification, ProgressNotificationParam, + ProtocolVersion, ReadResourceDirectoryRequest, ReadResourceDirectoryRequestMethod, ReadResourceDirectoryRequestParams, ReadResourceDirectoryResult, ReadResourceRequest, ReadResourceRequestParams, ReadResourceResponse, ReadResourceResult, Reference, RequestId, RequestMetaObject, RootsListChangedNotification, ServerJsonRpcMessage, ServerNotification, @@ -1538,10 +1538,21 @@ impl Peer { /// The result carries the server's cache hints but is not stored in the /// client response cache. A 2026-07-28 server that omits the hints is /// accepted, as for `list_tools`. + /// + /// Nothing is sent when the server has not declared the skills extension; + /// the call fails with `-32601` instead. pub async fn list_skills( &self, params: Option, ) -> Result { + if !self + .peer_info() + .is_some_and(|info| info.capabilities.supports_skills()) + { + return Err(ServiceError::McpError(McpError::method_not_found::< + ListSkillsRequestMethod, + >())); + } let result = self .send_request_cancel_on_drop(ClientRequest::ListSkillsRequest(ListSkillsRequest { method: Default::default(), @@ -1557,10 +1568,21 @@ impl Peer { /// SEP-2640 `skills/get`: the entry for one skill. A server that does /// not serve the URI answers with `-32602`, passed through unchanged. + /// + /// Nothing is sent when the server has not declared the skills extension; + /// the call fails with `-32601` instead. pub async fn get_skill( &self, params: GetSkillRequestParams, ) -> Result { + if !self + .peer_info() + .is_some_and(|info| info.capabilities.supports_skills()) + { + return Err(ServiceError::McpError(McpError::method_not_found::< + GetSkillRequestMethod, + >())); + } let result = self .send_request_cancel_on_drop(ClientRequest::GetSkillRequest(GetSkillRequest::new( params, diff --git a/crates/rmcp/tests/test_skills.rs b/crates/rmcp/tests/test_skills.rs index 4fc60c1c6..9378c8b68 100644 --- a/crates/rmcp/tests/test_skills.rs +++ b/crates/rmcp/tests/test_skills.rs @@ -57,6 +57,7 @@ struct SkillServer { declare_skills: bool, declare_directory_read: bool, omit_resources: bool, + include_outside_resource: bool, answer_get_with_other_uri: bool, directory_calls: Arc, } @@ -76,6 +77,20 @@ impl SkillServer { ..Default::default() } } + + fn pdf_skill(&self) -> Skill { + let mut skill = pdf_skill(); + if self.include_outside_resource { + let SkillResources::Files(files) = &mut skill.resources else { + unreachable!() + }; + files.push(SkillResource::from_bytes( + "skill://other-skill/private.md", + b"outside the skill directory", + )); + } + skill + } } impl ServerHandler for SkillServer { @@ -99,7 +114,7 @@ impl ServerHandler for SkillServer { ) -> Result { let cursor = request.and_then(|params| params.cursor); let mut result = match cursor.as_deref() { - None => ListSkillsResult::with_all_items(vec![pdf_skill()]), + None => ListSkillsResult::with_all_items(vec![self.pdf_skill()]), Some("page-2") => ListSkillsResult::with_all_items(vec![dynamic_skill()]), Some(other) => { return Err(ErrorData::invalid_params( @@ -123,7 +138,7 @@ impl ServerHandler for SkillServer { return Ok(GetSkillResult::new(dynamic_skill())); } match request.uri.as_str() { - PDF_SKILL_URI => Ok(GetSkillResult::new(pdf_skill())), + PDF_SKILL_URI => Ok(GetSkillResult::new(self.pdf_skill())), DYNAMIC_SKILL_URI => Ok(GetSkillResult::new(dynamic_skill())), uri => Err(ErrorData::invalid_params( format!("no skill at {uri}"), @@ -345,6 +360,28 @@ async fn skills_methods_should_be_method_not_found_when_the_extension_is_undecla assert_eq!(error_code(error), ErrorCode::METHOD_NOT_FOUND); } +#[tokio::test] +async fn skills_methods_should_reject_handler_manifests_with_outside_resources() { + let server = SkillServer { + include_outside_resource: true, + ..SkillServer::with_directory_read() + }; + let (client, server_task) = connect(server, ProtocolVersion::V_2026_07_28); + + let list_error = client + .list_skills(None) + .await + .expect_err("invalid manifest"); + let get_error = client + .get_skill(GetSkillRequestParams::new(PDF_SKILL_URI)) + .await + .expect_err("invalid manifest"); + disconnect(client, server_task).await; + + assert_eq!(error_code(list_error), ErrorCode::INTERNAL_ERROR); + assert_eq!(error_code(get_error), ErrorCode::INTERNAL_ERROR); +} + #[tokio::test] async fn skills_methods_should_fail_with_internal_error_without_the_resources_capability() { let server = SkillServer { diff --git a/crates/rmcp/tests/test_skills_capabilities.rs b/crates/rmcp/tests/test_skills_capabilities.rs new file mode 100644 index 000000000..d2e3a14ad --- /dev/null +++ b/crates/rmcp/tests/test_skills_capabilities.rs @@ -0,0 +1,79 @@ +//! Skills methods must stay local until the server declares the extension. +#![cfg(not(feature = "local"))] +#![cfg(all(feature = "client", feature = "server"))] + +use rmcp::{ + RoleClient, + model::{ + ErrorCode, GetSkillRequestParams, InitializeResult, ServerCapabilities, ServerPeerInfo, + }, + service::{ServiceError, serve_directly}, +}; +use serde_json::{Value, json}; +use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; + +async fn assert_no_skills_requests(peer_info: Option) { + let (client_transport, raw_transport) = tokio::io::duplex(4096); + // Use a raw peer instead of a ServerHandler: the SDK's server-side + // capability check would otherwise hide requests sent by the client. + let raw_peer = tokio::spawn(async move { + let (read, mut write) = tokio::io::split(raw_transport); + let mut lines = BufReader::new(read).lines(); + let mut requests = Vec::new(); + while let Some(line) = lines.next_line().await.expect("read request") { + let request: Value = serde_json::from_str(&line).expect("JSON request"); + requests.push( + request["method"] + .as_str() + .expect("request method") + .to_owned(), + ); + let response = json!({ + "jsonrpc": "2.0", + "id": request["id"], + "error": { "code": -32601, "message": "Method not found" }, + }); + let mut bytes = serde_json::to_vec(&response).expect("JSON response"); + bytes.push(b'\n'); + write.write_all(&bytes).await.expect("write response"); + } + requests + }); + let client = serve_directly::((), client_transport, peer_info); + + let errors = [ + client + .list_skills(None) + .await + .expect_err("skills undeclared"), + client + .get_skill(GetSkillRequestParams::new("skill://example/SKILL.md")) + .await + .expect_err("skills undeclared"), + client + .list_all_skills() + .await + .expect_err("skills undeclared"), + ]; + client.cancel().await.expect("cancel client"); + let requests = raw_peer.await.expect("raw peer task"); + + for error in errors { + assert!(matches!( + error, + ServiceError::McpError(error) if error.code == ErrorCode::METHOD_NOT_FOUND + )); + } + assert!(requests.is_empty(), "unexpected requests: {requests:?}"); +} + +#[tokio::test] +async fn skills_methods_do_not_send_without_peer_info() { + assert_no_skills_requests(None).await; +} + +#[tokio::test] +async fn skills_methods_do_not_send_without_extension_declaration() { + let capabilities = ServerCapabilities::builder().enable_resources().build(); + assert_no_skills_requests(Some(InitializeResult::new(capabilities).into())).await; +} From ee4a600404bcbbbdd9eeef5849fdb7f6b0a63569 Mon Sep 17 00:00:00 2001 From: thomas <291887306+tsarlandie-oai@users.noreply.github.com> Date: Fri, 30 Oct 2026 00:42:42 +0000 Subject: [PATCH 2/3] fix(skills): reject manifest paths that resolve to directories --- crates/rmcp/src/model/skill.rs | 155 +++++++----------- crates/rmcp/tests/test_skills.rs | 90 +++++----- crates/rmcp/tests/test_skills_capabilities.rs | 79 --------- 3 files changed, 107 insertions(+), 217 deletions(-) delete mode 100644 crates/rmcp/tests/test_skills_capabilities.rs diff --git a/crates/rmcp/src/model/skill.rs b/crates/rmcp/src/model/skill.rs index bae3be358..97ae05173 100644 --- a/crates/rmcp/src/model/skill.rs +++ b/crates/rmcp/src/model/skill.rs @@ -328,14 +328,19 @@ fn resource_is_within_skill(skill_uri: &str, resource_uri: &str) -> bool { return false; }; let path = relative.split(['?', '#']).next().unwrap_or_default(); - if path.is_empty() || path.ends_with('/') { + // Decode unreserved dots without turning a reserved %2F into a separator. + let mut segments = path + .split('/') + .map(|segment| segment.replace("%2e", ".").replace("%2E", ".")); + // An empty or dot-segment leaf resolves to a directory. + if matches!( + segments.next_back().as_deref(), + None | Some("" | "." | "..") + ) { return false; } let mut depth = 0usize; - for segment in path.split('/') { - // Dots are unreserved URI characters. Recognize their escaped form, - // but leave reserved delimiters such as %2F and query data untouched. - let segment = segment.replace("%2e", ".").replace("%2E", "."); + for segment in segments { match segment.as_str() { "." => {} ".." => { @@ -347,7 +352,7 @@ fn resource_is_within_skill(skill_uri: &str, resource_uri: &str) -> bool { _ => depth += 1, } } - depth > 0 + true } /// Whether `digest` is `sha256:` followed by 64 lowercase hex characters. @@ -381,7 +386,7 @@ pub enum SkillValidationError { MissingSkillMd(String), #[error("skill manifest lists {0} twice")] DuplicateResource(String), - #[error("skill file {uri} is not within the directory of {skill_uri}")] + #[error("resource {uri} does not identify a file within the directory of {skill_uri}")] ResourceOutsideSkill { uri: String, skill_uri: String }, #[error("skill file {uri} has a malformed digest {digest}")] MalformedDigest { uri: String, digest: String }, @@ -609,6 +614,16 @@ mod tests { assert_eq!(skill_name_from_uri("skill://acme/billing/README.md"), None); } + #[test] + fn ignores_skill_md_in_a_query_or_fragment() { + for uri in [ + "skill://acme/billing?reference=/billing/SKILL.md", + "skill://acme/billing#reference/billing/SKILL.md", + ] { + assert_eq!(skill_name_from_uri(uri), None, "{uri}"); + } + } + #[test] fn returns_none_for_skill_md_at_the_root() { assert_eq!(skill_name_from_uri("skill:///SKILL.md"), None); @@ -639,6 +654,43 @@ mod tests { } } + #[test] + fn manifest_paths_must_resolve_to_files_within_the_skill_directory() { + let skill_uri = "custom:skills/billing/SKILL.md"; + for (path, valid) in [ + ("refunds/SKILL.md", true), + ("refs/./guide.md", true), + ("refs/../invoice.md", true), + ("refs/%2e%2E/invoice.md", true), + ("refs%2F..%2F..%2Fguide.md?ref=/../../other", true), + ("invoice.md#ref/../../other", true), + ("refs/../../invoice.md", false), + (".%2E/invoice.md", false), + ("refs/", false), + ("refs/.", false), + ("refs/sub/..", false), + ("refs/%2E?file=invoice.md", false), + ("refs/sub/.%2e#invoice.md", false), + ] { + let uri = format!("custom:skills/billing/{path}"); + assert_eq!(resource_is_within_skill(skill_uri, &uri), valid, "{path}"); + } + } + + #[test] + fn rejects_resources_with_a_different_root() { + for uri in [ + "skill://acme/billing-other/invoice.md", + "skill://other/billing/invoice.md", + "github://acme/billing/invoice.md", + ] { + assert!( + !resource_is_within_skill("skill://acme/billing/SKILL.md", uri), + "{uri}" + ); + } + } + mod skill_validate { use super::*; @@ -647,95 +699,6 @@ mod tests { assert_eq!(billing_skill().validate(), Ok(())); } - fn with_supporting_file(skill_uri: &str, resource_uri: &str) -> Skill { - Skill::new( - skill_uri, - SkillFrontmatter::new("billing", "Issue invoices"), - SkillResources::Files(vec![ - SkillResource::from_bytes(skill_uri, b"skill"), - SkillResource::from_bytes(resource_uri, b"supporting file"), - ]), - ) - } - - #[test] - fn accepts_supporting_files_within_the_skill_directory() { - for uri in [ - "skill://acme/billing/invoice.md", - "skill://acme/billing/refunds/SKILL.md", - "skill://acme/billing/references/../invoice.md", - "skill://acme/billing/references/%2e%2E/invoice.md", - "skill://acme/billing/templates/.../invoice.md", - "skill://acme/billing/references%2Fguide.md", - "skill://acme/billing/invoice.md?reference=/../../other", - "skill://acme/billing/invoice.md#reference/../../other", - ] { - let skill = with_supporting_file("skill://acme/billing/SKILL.md", uri); - assert_eq!(skill.validate(), Ok(()), "{uri}"); - } - } - - #[test] - fn accepts_supporting_files_under_other_uri_schemes() { - for (skill_uri, resource_uri) in [ - ( - "github://owner/repo/skills/billing/SKILL.md", - "github://owner/repo/skills/billing/references/guide.md", - ), - ( - "file:///skills/billing/SKILL.md", - "file:///skills/billing/templates/invoice.md", - ), - ( - "custom:skills/billing/SKILL.md", - "custom:skills/billing/invoice.md", - ), - ] { - assert_eq!( - with_supporting_file(skill_uri, resource_uri).validate(), - Ok(()), - "{resource_uri}" - ); - } - } - - #[test] - fn rejects_supporting_files_outside_the_skill_directory() { - for uri in [ - "skill://other/billing/invoice.md", - "github://acme/billing/invoice.md", - "skill://acme/billing-other/invoice.md", - "skill://acme/invoice.md", - "skill://acme/billing/../invoice.md", - "skill://acme/billing/references/../../invoice.md", - "skill://acme/billing/%2e%2e/invoice.md", - "skill://acme/billing/.%2E/invoice.md", - "skill://acme/billing/%2E./invoice.md", - "skill://acme/billing/references/%2E%2e/%2e%2E/invoice.md", - "skill://acme/billing", - "skill://acme/billing/", - "skill://acme/billing/references/..", - "skill://acme/billing/references/../", - "skill://acme/billing/references/../?reference=invoice.md", - ] { - let skill = with_supporting_file("skill://acme/billing/SKILL.md", uri); - assert!(skill.validate().is_err(), "accepted {uri}"); - } - } - - #[test] - fn rejects_a_skill_md_suffix_in_a_query_or_fragment() { - for uri in [ - "skill://acme/billing?reference=/billing/SKILL.md", - "skill://acme/billing#reference/billing/SKILL.md", - ] { - assert!( - with_supporting_file(uri, uri).validate().is_err(), - "accepted {uri}" - ); - } - } - #[test] fn accepts_a_dynamic_entry_without_files() { let skill = Skill::new( diff --git a/crates/rmcp/tests/test_skills.rs b/crates/rmcp/tests/test_skills.rs index 9378c8b68..f61b8caf2 100644 --- a/crates/rmcp/tests/test_skills.rs +++ b/crates/rmcp/tests/test_skills.rs @@ -19,7 +19,7 @@ use rmcp::{ }, service::{RequestContext, RunningService, ServiceError, serve_directly}, }; -use tokio::task::JoinHandle; +use tokio::{io::AsyncReadExt, task::JoinHandle}; const SKILL_MD: &str = "---\nname: pdf-processing\ndescription: Extract, fill, and assemble PDF documents\n---\n\n# PDF processing\n\nChoose the matching template from `templates/`.\n"; const PDF_SKILL_URI: &str = "skill://pdf-processing/SKILL.md"; @@ -28,7 +28,7 @@ const TEMPLATES_DIR_URI: &str = "skill://pdf-processing/templates"; const REGIONAL_DIR_URI: &str = "skill://pdf-processing/templates/regional"; const DYNAMIC_SKILL_URI: &str = "skill://live-metrics/SKILL.md"; -fn pdf_skill() -> Skill { +fn pdf_skill(invoice_uri: &str) -> Skill { Skill::new( PDF_SKILL_URI, SkillFrontmatter::new( @@ -37,7 +37,7 @@ fn pdf_skill() -> Skill { ), SkillResources::Files(vec![ SkillResource::from_bytes(PDF_SKILL_URI, SKILL_MD.as_bytes()), - SkillResource::from_bytes(INVOICE_URI, b"# Invoice\n"), + SkillResource::from_bytes(invoice_uri, b"# Invoice\n"), ]), ) } @@ -57,7 +57,7 @@ struct SkillServer { declare_skills: bool, declare_directory_read: bool, omit_resources: bool, - include_outside_resource: bool, + invoice_uri: Option<&'static str>, answer_get_with_other_uri: bool, directory_calls: Arc, } @@ -79,17 +79,7 @@ impl SkillServer { } fn pdf_skill(&self) -> Skill { - let mut skill = pdf_skill(); - if self.include_outside_resource { - let SkillResources::Files(files) = &mut skill.resources else { - unreachable!() - }; - files.push(SkillResource::from_bytes( - "skill://other-skill/private.md", - b"outside the skill directory", - )); - } - skill + pdf_skill(self.invoice_uri.unwrap_or(INVOICE_URI)) } } @@ -298,7 +288,7 @@ async fn get_skill_should_return_the_entry_for_a_served_uri() { .expect("skills/get"); disconnect(client, server_task).await; - assert_eq!(result.skill, pdf_skill()); + assert_eq!(result.skill, pdf_skill(INVOICE_URI)); } #[tokio::test] @@ -351,35 +341,51 @@ async fn get_skill_should_fail_with_internal_error_when_the_handler_answers_anot } #[tokio::test] -async fn skills_methods_should_be_method_not_found_when_the_extension_is_undeclared() { - let (client, server_task) = connect(SkillServer::default(), ProtocolVersion::V_2026_07_28); - - let error = client.list_skills(None).await.expect_err("undeclared"); - disconnect(client, server_task).await; - - assert_eq!(error_code(error), ErrorCode::METHOD_NOT_FOUND); +async fn skills_methods_should_not_send_without_a_declared_extension() { + let capabilities = ServerCapabilities::builder().enable_resources().build(); + for peer_info in [None, Some(InitializeResult::new(capabilities).into())] { + let (transport, mut wire) = tokio::io::duplex(1024); + // Drop the peer on the first byte so an unexpected request fails immediately. + let first_read = tokio::spawn(async move { wire.read(&mut [0]).await }); + let client = serve_directly::((), transport, peer_info); + + for error in [ + client.list_skills(None).await.unwrap_err(), + client + .get_skill(GetSkillRequestParams::new(PDF_SKILL_URI)) + .await + .unwrap_err(), + client.list_all_skills().await.unwrap_err(), + ] { + assert_eq!(error_code(error), ErrorCode::METHOD_NOT_FOUND); + } + client.cancel().await.unwrap(); + assert_eq!(first_read.await.unwrap().unwrap(), 0, "unexpected request"); + } } #[tokio::test] -async fn skills_methods_should_reject_handler_manifests_with_outside_resources() { - let server = SkillServer { - include_outside_resource: true, - ..SkillServer::with_directory_read() - }; - let (client, server_task) = connect(server, ProtocolVersion::V_2026_07_28); - - let list_error = client - .list_skills(None) - .await - .expect_err("invalid manifest"); - let get_error = client - .get_skill(GetSkillRequestParams::new(PDF_SKILL_URI)) - .await - .expect_err("invalid manifest"); - disconnect(client, server_task).await; - - assert_eq!(error_code(list_error), ErrorCode::INTERNAL_ERROR); - assert_eq!(error_code(get_error), ErrorCode::INTERNAL_ERROR); +async fn skills_methods_should_reject_invalid_manifest_files() { + for uri in [ + "skill://other-skill/private.md", + "skill://pdf-processing/templates/.", + ] { + let server = SkillServer { + invoice_uri: Some(uri), + ..SkillServer::with_directory_read() + }; + let (client, server_task) = connect(server, ProtocolVersion::V_2026_07_28); + for error in [ + client.list_skills(None).await.unwrap_err(), + client + .get_skill(GetSkillRequestParams::new(PDF_SKILL_URI)) + .await + .unwrap_err(), + ] { + assert_eq!(error_code(error), ErrorCode::INTERNAL_ERROR, "{uri}"); + } + disconnect(client, server_task).await; + } } #[tokio::test] diff --git a/crates/rmcp/tests/test_skills_capabilities.rs b/crates/rmcp/tests/test_skills_capabilities.rs deleted file mode 100644 index d2e3a14ad..000000000 --- a/crates/rmcp/tests/test_skills_capabilities.rs +++ /dev/null @@ -1,79 +0,0 @@ -//! Skills methods must stay local until the server declares the extension. -#![cfg(not(feature = "local"))] -#![cfg(all(feature = "client", feature = "server"))] - -use rmcp::{ - RoleClient, - model::{ - ErrorCode, GetSkillRequestParams, InitializeResult, ServerCapabilities, ServerPeerInfo, - }, - service::{ServiceError, serve_directly}, -}; -use serde_json::{Value, json}; -use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; - -async fn assert_no_skills_requests(peer_info: Option) { - let (client_transport, raw_transport) = tokio::io::duplex(4096); - // Use a raw peer instead of a ServerHandler: the SDK's server-side - // capability check would otherwise hide requests sent by the client. - let raw_peer = tokio::spawn(async move { - let (read, mut write) = tokio::io::split(raw_transport); - let mut lines = BufReader::new(read).lines(); - let mut requests = Vec::new(); - while let Some(line) = lines.next_line().await.expect("read request") { - let request: Value = serde_json::from_str(&line).expect("JSON request"); - requests.push( - request["method"] - .as_str() - .expect("request method") - .to_owned(), - ); - let response = json!({ - "jsonrpc": "2.0", - "id": request["id"], - "error": { "code": -32601, "message": "Method not found" }, - }); - let mut bytes = serde_json::to_vec(&response).expect("JSON response"); - bytes.push(b'\n'); - write.write_all(&bytes).await.expect("write response"); - } - requests - }); - let client = serve_directly::((), client_transport, peer_info); - - let errors = [ - client - .list_skills(None) - .await - .expect_err("skills undeclared"), - client - .get_skill(GetSkillRequestParams::new("skill://example/SKILL.md")) - .await - .expect_err("skills undeclared"), - client - .list_all_skills() - .await - .expect_err("skills undeclared"), - ]; - client.cancel().await.expect("cancel client"); - let requests = raw_peer.await.expect("raw peer task"); - - for error in errors { - assert!(matches!( - error, - ServiceError::McpError(error) if error.code == ErrorCode::METHOD_NOT_FOUND - )); - } - assert!(requests.is_empty(), "unexpected requests: {requests:?}"); -} - -#[tokio::test] -async fn skills_methods_do_not_send_without_peer_info() { - assert_no_skills_requests(None).await; -} - -#[tokio::test] -async fn skills_methods_do_not_send_without_extension_declaration() { - let capabilities = ServerCapabilities::builder().enable_resources().build(); - assert_no_skills_requests(Some(InitializeResult::new(capabilities).into())).await; -} From 36203c3624bfb4ef3d1efa042d2f34514e81ce2b Mon Sep 17 00:00:00 2001 From: thomas <291887306+tsarlandie-oai@users.noreply.github.com> Date: Tue, 3 Nov 2026 00:16:42 +0000 Subject: [PATCH 3/3] fix(skills): normalize manifest paths before checking for duplicates --- crates/rmcp/src/model/skill.rs | 135 ++++++++++++++++++++++----------- 1 file changed, 90 insertions(+), 45 deletions(-) diff --git a/crates/rmcp/src/model/skill.rs b/crates/rmcp/src/model/skill.rs index 97ae05173..edd111e91 100644 --- a/crates/rmcp/src/model/skill.rs +++ b/crates/rmcp/src/model/skill.rs @@ -285,13 +285,13 @@ impl Skill { } let mut seen = std::collections::HashSet::with_capacity(files.len()); for file in files { - if !resource_is_within_skill(&self.uri, &file.uri) { + let Some(key) = manifest_resource_key(&self.uri, &file.uri) else { return Err(SkillValidationError::ResourceOutsideSkill { uri: file.uri.clone(), skill_uri: self.uri.clone(), }); - } - if !seen.insert(file.uri.as_str()) { + }; + if !seen.insert(key) { return Err(SkillValidationError::DuplicateResource(file.uri.clone())); } if !is_sha256_digest(&file.digest) { @@ -318,41 +318,59 @@ pub fn skill_name_from_uri(uri: &str) -> Option<&str> { (!name.is_empty()).then_some(name) } -fn resource_is_within_skill(skill_uri: &str, resource_uri: &str) -> bool { +/// Comparison key for a file under this skill's root. Reads use the advertised URI. +fn manifest_resource_key(skill_uri: &str, resource_uri: &str) -> Option { // Keep the trailing slash to distinguish a child from a sibling whose // directory name merely starts with the skill's name, under any scheme. - let Some(relative) = skill_uri - .strip_suffix(SKILL_MD) - .and_then(|root| resource_uri.strip_prefix(root)) - else { - return false; - }; - let path = relative.split(['?', '#']).next().unwrap_or_default(); - // Decode unreserved dots without turning a reserved %2F into a separator. - let mut segments = path - .split('/') - .map(|segment| segment.replace("%2e", ".").replace("%2E", ".")); + let root = skill_uri.strip_suffix(SKILL_MD)?; + let relative = resource_uri.strip_prefix(root)?; + let (path, suffix) = relative.split_at(relative.find(['?', '#']).unwrap_or(relative.len())); + let path = normalize_path_encoding(path); + let mut segments = path.split('/'); // An empty or dot-segment leaf resolves to a directory. - if matches!( - segments.next_back().as_deref(), - None | Some("" | "." | "..") - ) { - return false; + let file = segments.next_back()?; + if matches!(file, "" | "." | "..") { + return None; } - let mut depth = 0usize; + let mut resolved = Vec::new(); for segment in segments { - match segment.as_str() { + match segment { "." => {} ".." => { - let Some(parent) = depth.checked_sub(1) else { - return false; - }; - depth = parent; + resolved.pop()?; } - _ => depth += 1, + _ => resolved.push(segment), } } - true + resolved.push(file); + let mut key = resolved.join("/"); + key.push_str(suffix); + Some(key) +} + +// RFC 3986 ยง6.2.2 decodes unreserved bytes and uppercases the hex digits in +// other escapes. Reserved delimiters therefore stay encoded in the key. +fn normalize_path_encoding(path: &str) -> String { + let mut parts = path.split('%'); + let mut normalized = parts.next().unwrap_or_default().to_owned(); + for part in parts { + if let Some((hex, rest)) = part.split_at_checked(2) + && hex.bytes().all(|b| b.is_ascii_hexdigit()) + { + let byte = u8::from_str_radix(hex, 16).expect("two hex digits"); + if byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'.' | b'_' | b'~') { + normalized.push(char::from(byte)); + } else { + normalized.push('%'); + normalized.push_str(&hex.to_ascii_uppercase()); + } + normalized.push_str(rest); + } else { + normalized.push('%'); + normalized.push_str(part); + } + } + normalized } /// Whether `digest` is `sha256:` followed by 64 lowercase hex characters. @@ -657,23 +675,34 @@ mod tests { #[test] fn manifest_paths_must_resolve_to_files_within_the_skill_directory() { let skill_uri = "custom:skills/billing/SKILL.md"; - for (path, valid) in [ - ("refunds/SKILL.md", true), - ("refs/./guide.md", true), - ("refs/../invoice.md", true), - ("refs/%2e%2E/invoice.md", true), - ("refs%2F..%2F..%2Fguide.md?ref=/../../other", true), - ("invoice.md#ref/../../other", true), - ("refs/../../invoice.md", false), - (".%2E/invoice.md", false), - ("refs/", false), - ("refs/.", false), - ("refs/sub/..", false), - ("refs/%2E?file=invoice.md", false), - ("refs/sub/.%2e#invoice.md", false), + for (path, key) in [ + ("refunds/SKILL.md", Some("refunds/SKILL.md")), + ("refs/./guide.md", Some("refs/guide.md")), + ("refs/../invoice.md", Some("invoice.md")), + ("refs/%2e%2E/invoice.md", Some("invoice.md")), + ("refs//invoice.md", Some("refs//invoice.md")), + ( + "refs%2fguide.md?ref=/../../other", + Some("refs%2Fguide.md?ref=/../../other"), + ), + ( + "invoice.md#ref/../../other", + Some("invoice.md#ref/../../other"), + ), + ("refs/../../invoice.md", None), + (".%2E/invoice.md", None), + ("refs/", None), + ("refs/.", None), + ("refs/sub/..", None), + ("refs/%2E?file=invoice.md", None), + ("refs/sub/.%2e#invoice.md", None), ] { let uri = format!("custom:skills/billing/{path}"); - assert_eq!(resource_is_within_skill(skill_uri, &uri), valid, "{path}"); + assert_eq!( + manifest_resource_key(skill_uri, &uri).as_deref(), + key, + "{path}" + ); } } @@ -684,8 +713,9 @@ mod tests { "skill://other/billing/invoice.md", "github://acme/billing/invoice.md", ] { - assert!( - !resource_is_within_skill("skill://acme/billing/SKILL.md", uri), + assert_eq!( + manifest_resource_key("skill://acme/billing/SKILL.md", uri), + None, "{uri}" ); } @@ -742,6 +772,21 @@ mod tests { )); } + #[test] + fn rejects_two_spellings_of_the_same_manifest_file() { + let file = + |path: &str| SkillResource::from_bytes(format!("skill://acme/billing/{path}"), b""); + let mut skill = billing_skill(); + let entry = file("SKILL.md"); + let alias = file("refs/../%69nvoice.md"); + skill.resources = SkillResources::Files(vec![entry.clone(), alias.clone()]); + assert_eq!(skill.validate(), Ok(())); + + let duplicate = SkillValidationError::DuplicateResource(alias.uri.clone()); + skill.resources = SkillResources::Files(vec![entry, file("invoice.md"), alias]); + assert_eq!(skill.validate(), Err(duplicate)); + } + #[test] fn rejects_a_malformed_digest() { let mut skill = billing_skill();