fix(skills): enforce manifest boundaries and capability negotiation

crates/rmcp/src/model/skill.rs

@@ -285,7 +285,13 @@ impl Skill {
         }
         let mut seen = std::collections::HashSet::with_capacity(files.len());
         for file in files {
-            if !seen.insert(file.uri.as_str()) {
+            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(key) {
                 return Err(SkillValidationError::DuplicateResource(file.uri.clone()));
             }
             if !is_sha256_digest(&file.digest) {
@@ -304,11 +310,69 @@ 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)
 }
 
+/// 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<String> {
+    // 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 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.
+    let file = segments.next_back()?;
+    if matches!(file, "" | "." | "..") {
+        return None;
+    }
+    let mut resolved = Vec::new();
+    for segment in segments {
+        match segment {
+            "." => {}
+            ".." => {
+                resolved.pop()?;
+            }
+            _ => resolved.push(segment),
+        }
+    }
+    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.
 pub fn is_sha256_digest(digest: &str) -> bool {
     digest.strip_prefix("sha256:").is_some_and(|hex| {
@@ -340,6 +404,8 @@ pub enum SkillValidationError {
     MissingSkillMd(String),
     #[error("skill manifest lists {0} twice")]
     DuplicateResource(String),
+    #[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 },
     #[error("skill {0} is listed twice")]
@@ -566,6 +632,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);
@@ -596,6 +672,55 @@ mod tests {
         }
     }
 
+    #[test]
+    fn manifest_paths_must_resolve_to_files_within_the_skill_directory() {
+        let skill_uri = "custom:skills/billing/SKILL.md";
+        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!(
+                manifest_resource_key(skill_uri, &uri).as_deref(),
+                key,
+                "{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_eq!(
+                manifest_resource_key("skill://acme/billing/SKILL.md", uri),
+                None,
+                "{uri}"
+            );
+        }
+    }
+
     mod skill_validate {
         use super::*;
 
@@ -647,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();

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<RoleClient> {
     /// 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<PaginatedRequestParams>,
     ) -> Result<ListSkillsResult, ServiceError> {
+        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<RoleClient> {
 
     /// 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<GetSkillResult, ServiceError> {
+        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,

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,6 +57,7 @@ struct SkillServer {
     declare_skills: bool,
     declare_directory_read: bool,
     omit_resources: bool,
+    invoice_uri: Option<&'static str>,
     answer_get_with_other_uri: bool,
     directory_calls: Arc<AtomicUsize>,
 }
@@ -76,6 +77,10 @@ impl SkillServer {
             ..Default::default()
         }
     }
+
+    fn pdf_skill(&self) -> Skill {
+        pdf_skill(self.invoice_uri.unwrap_or(INVOICE_URI))
+    }
 }
 
 impl ServerHandler for SkillServer {
@@ -99,7 +104,7 @@ impl ServerHandler for SkillServer {
     ) -> Result<ListSkillsResult, ErrorData> {
         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 +128,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}"),
@@ -283,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]
@@ -336,13 +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;
+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::<RoleClient, _, _, _, _>((), 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");
+    }
+}
 
-    assert_eq!(error_code(error), ErrorCode::METHOD_NOT_FOUND);
+#[tokio::test]
+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]