diff --git a/crates/rmcp/Cargo.toml b/crates/rmcp/Cargo.toml index b0e77c72a..1d04fa889 100644 --- a/crates/rmcp/Cargo.toml +++ b/crates/rmcp/Cargo.toml @@ -57,6 +57,7 @@ indexmap = { version = "2", features = ["serde"] } tracing = { version = "0.1" } tokio-util = { version = "0.7" } pin-project-lite = "0.2" +percent-encoding = { version = "2", default-features = false, features = ["alloc"] } pastey = { version = "0.2.0", optional = true } # oauth2 support oauth2 = { version = "5.0", optional = true, default-features = false } diff --git a/crates/rmcp/src/model/skill.rs b/crates/rmcp/src/model/skill.rs index edd111e91..24e8fc3bf 100644 --- a/crates/rmcp/src/model/skill.rs +++ b/crates/rmcp/src/model/skill.rs @@ -5,9 +5,9 @@ //! `resources/read` serves the file bytes. The request and result wiring //! lives in the parent module next to the other methods. -#[cfg(feature = "schemars")] use std::borrow::Cow; +use percent_encoding::{CONTROLS, percent_decode_str, utf8_percent_encode}; use serde::{Deserialize, Serialize}; use super::{ @@ -268,10 +268,13 @@ impl Skill { /// Check the rules the spec puts on a skill entry that can be checked /// without reading any file. pub fn validate(&self) -> Result<(), SkillValidationError> { - let Some(name) = skill_name_from_uri(&self.uri) else { + let Some(encoded_name) = skill_name_from_uri(&self.uri) else { return Err(SkillValidationError::UriNotSkillMd(self.uri.clone())); }; - if name != self.frontmatter.name { + let name = self.name(); + if name.contains(['/', '\\', '?', '#']) + || !percent_decode_str(encoded_name).eq(name.bytes()) + { return Err(SkillValidationError::NameMismatch { uri: self.uri.clone(), name: self.frontmatter.name.clone(), @@ -325,7 +328,7 @@ fn manifest_resource_key(skill_uri: &str, resource_uri: &str) -> Option 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 path = normalize_percent_encoding(path); let mut segments = path.split('/'); // An empty or dot-segment leaf resolves to a directory. let file = segments.next_back()?; @@ -350,10 +353,13 @@ fn manifest_resource_key(skill_uri: &str, resource_uri: &str) -> Option // 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 { +fn normalize_percent_encoding(value: &str) -> Cow<'_, str> { + let encoded: Cow<'_, str> = utf8_percent_encode(value, CONTROLS).into(); + let Some((prefix, escaped)) = encoded.split_once('%') else { + return encoded; + }; + let mut normalized = prefix.to_owned(); + for part in escaped.split('%') { if let Some((hex, rest)) = part.split_at_checked(2) && hex.bytes().all(|b| b.is_ascii_hexdigit()) { @@ -370,7 +376,7 @@ fn normalize_path_encoding(path: &str) -> String { normalized.push_str(part); } } - normalized + Cow::Owned(normalized) } /// Whether `digest` is `sha256:` followed by 64 lowercase hex characters. @@ -385,7 +391,7 @@ pub fn validate_skills(skills: &[Skill]) -> Result<(), SkillValidationError> { let mut seen = std::collections::HashSet::with_capacity(skills.len()); for skill in skills { skill.validate()?; - if !seen.insert(skill.uri.as_str()) { + if !seen.insert(normalize_percent_encoding(&skill.uri)) { return Err(SkillValidationError::DuplicateSkill(skill.uri.clone())); } } @@ -739,6 +745,21 @@ mod tests { assert_eq!(skill.validate(), Ok(())); } + #[test] + fn accepts_a_percent_encoded_unicode_name() { + for uri in [ + "skill://r%C3%A9sum%C3%A9/SKILL.md", + "skill://team/r%c3%a9sum%c3%a9/SKILL.md", + ] { + let skill = Skill::new( + uri, + SkillFrontmatter::new("résumé", "Résumé templates"), + SkillResources::Files(vec![SkillResource::from_bytes(uri, b"")]), + ); + assert_eq!(skill.validate(), Ok(()), "{uri}"); + } + } + #[test] fn rejects_a_name_that_differs_from_the_uri() { let mut skill = billing_skill(); @@ -749,6 +770,28 @@ mod tests { )); } + #[test] + fn rejects_invalid_encoded_name_matches() { + for (segment, name) in [ + ("r%FFsum%C3%A9", "résumé"), + ("r%25C3%25A9sum%25C3%25A9", "résumé"), + ("coffee%2Ftea", "coffee/tea"), + ] { + let skill = Skill::new( + format!("skill://team/{segment}/SKILL.md"), + SkillFrontmatter::new(name, "Drinks"), + SkillResources::Dynamic, + ); + assert!( + matches!( + skill.validate(), + Err(SkillValidationError::NameMismatch { .. }) + ), + "{segment}" + ); + } + } + #[test] fn rejects_a_manifest_without_skill_md() { let mut skill = billing_skill(); @@ -812,6 +855,28 @@ mod tests { Err(SkillValidationError::DuplicateSkill(_)) )); } + + #[test] + fn rejects_equivalent_percent_encodings_of_a_skill_uri() { + let skill = |uri| { + Skill::new( + uri, + SkillFrontmatter::new("résumé", "Résumé templates"), + SkillResources::Dynamic, + ) + }; + let uri = "skill://team/r%C3%A9sum%C3%A9/SKILL.md"; + for alias in [ + "skill://team/r%c3%a9sum%c3%a9/SKILL.md", + "skill://team/%72%C3%A9sum%C3%A9/SKILL.md", + "skill://team/résumé/SKILL.md", + ] { + assert_eq!( + validate_skills(&[skill(uri), skill(alias)]), + Err(SkillValidationError::DuplicateSkill(alias.into())) + ); + } + } } mod get_skill_result {