diff --git a/crates/rmcp/Cargo.toml b/crates/rmcp/Cargo.toml index b0e77c72a..3409fdbfb 100644 --- a/crates/rmcp/Cargo.toml +++ b/crates/rmcp/Cargo.toml @@ -57,6 +57,9 @@ 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"] } +unicode-general-category = "1" +unicode-normalization = { version = "0.1", default-features = false } 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..9abf0780f 100644 --- a/crates/rmcp/src/model/skill.rs +++ b/crates/rmcp/src/model/skill.rs @@ -5,10 +5,12 @@ //! `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 unicode_general_category::{GeneralCategory, get_general_category}; +use unicode_normalization::UnicodeNormalization; use super::{ CacheScope, JsonObject, MetaObject, Resource, ResultType, deserialize_cache_scope, @@ -267,11 +269,23 @@ impl Skill { /// Check the rules the spec puts on a skill entry that can be checked /// without reading any file. + /// + /// The 1–64 character limit counts Unicode scalar values after NFKC + /// normalization. This same form is used to compare the frontmatter name + /// with the decoded URI segment. Use the advertised URI for reads and + /// pair it with the originating server for cache and approval keys. 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 !is_valid_skill_name(name) { + return Err(SkillValidationError::InvalidName(name.to_owned())); + } + if !percent_decode_str(encoded_name) + .decode_utf8() + .is_ok_and(|segment| segment.nfkc().eq(name.nfkc())) + { return Err(SkillValidationError::NameMismatch { uri: self.uri.clone(), name: self.frontmatter.name.clone(), @@ -318,6 +332,28 @@ pub fn skill_name_from_uri(uri: &str) -> Option<&str> { (!name.is_empty()).then_some(name) } +/// Validate the NFKC form, as in the Agent Skills reference validator. +/// Letters from scripts without case are valid in a skill name. +fn is_valid_skill_name(name: &str) -> bool { + let mut needs_alphanumeric = true; + for (index, ch) in name.nfkc().enumerate() { + if index >= 64 { + return false; + } + match get_general_category(ch) { + GeneralCategory::LowercaseLetter + | GeneralCategory::ModifierLetter + | GeneralCategory::OtherLetter + | GeneralCategory::DecimalNumber + | GeneralCategory::LetterNumber + | GeneralCategory::OtherNumber => needs_alphanumeric = false, + _ if ch == '-' && !needs_alphanumeric => needs_alphanumeric = true, + _ => return false, + } + } + !needs_alphanumeric +} + /// 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 @@ -325,7 +361,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()?; @@ -348,12 +384,16 @@ fn manifest_resource_key(skill_uri: &str, resource_uri: &str) -> Option 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 { +// First percent-encode literal Unicode and ASCII control characters. Then apply +// RFC 3986 §6.2.2: decode unreserved bytes and uppercase the other escapes, +// keeping reserved delimiters encoded in the comparison key. +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 +410,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 +425,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())); } } @@ -398,6 +438,10 @@ pub fn validate_skills(skills: &[Skill]) -> Result<(), SkillValidationError> { pub enum SkillValidationError { #[error("skill uri {0} does not end in /SKILL.md")] UriNotSkillMd(String), + #[error( + "invalid skill name {0:?}: after NFKC normalization, use 1–64 characters with lowercase letters, numbers, and single internal hyphens" + )] + InvalidName(String), #[error("skill uri {uri} does not end in the frontmatter name {name}")] NameMismatch { uri: String, name: String }, #[error("skill manifest {0} does not list its own SKILL.md")] @@ -739,6 +783,73 @@ 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 enforces_the_agent_skills_name_rules() { + let longest = "界".repeat(64); + let too_long = "界".repeat(65); + let decomposed_accents = "e\u{301}".repeat(64); // 128 scalars normalize to 64. + let ffi_ligatures = "\u{fb03}".repeat(22); // 22 scalars normalize to 66. + for (name, valid) in [ + ("a", true), + ("東京-٢", true), + (longest.as_str(), true), + (too_long.as_str(), false), + (decomposed_accents.as_str(), true), + (ffi_ligatures.as_str(), false), + ("", false), + ("PDF", false), + ("École", false), + ("two_words", false), + ("two.words", false), + ("two words", false), + ("coffee/tea", false), + ("-two", false), + ("two-", false), + ("two--words", false), + ("☕", false), + ] { + let skill = Skill::new( + format!( + "skill://team/{}/SKILL.md", + utf8_percent_encode(name, percent_encoding::NON_ALPHANUMERIC) + ), + SkillFrontmatter::new(name, "Example skill"), + SkillResources::Dynamic, + ); + assert_eq!(skill.validate().is_ok(), valid, "{name:?}"); + } + } + + #[test] + fn compares_unicode_names_in_normalized_form() { + for (segment, name) in [ + ("re%CC%81sume%CC%81", "résumé"), + ("r%C3%A9sum%C3%A9", "re\u{301}sume\u{301}"), + ] { + let skill = Skill::new( + format!("skill://team/{segment}/SKILL.md"), + SkillFrontmatter::new(name, "Résumé templates"), + SkillResources::Dynamic, + ); + assert_eq!(skill.validate(), Ok(()), "{segment}"); + } + } + #[test] fn rejects_a_name_that_differs_from_the_uri() { let mut skill = billing_skill(); @@ -749,6 +860,27 @@ 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é"), + ] { + 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(); @@ -804,6 +936,18 @@ mod tests { mod validate_skills { use super::*; + #[test] + fn compatibility_equivalent_names_can_identify_distinct_resources() { + let skills = ["skill://ｃａｆｅ/SKILL.md", "skill://cafe/SKILL.md"].map(|uri| { + Skill::new( + uri, + SkillFrontmatter::new("cafe", "Coffee recipes"), + SkillResources::Files(vec![SkillResource::from_bytes(uri, b"")]), + ) + }); + assert_eq!(validate_skills(&skills), Ok(())); + } + #[test] fn rejects_the_same_skill_listed_twice() { let skills = [billing_skill(), billing_skill()]; @@ -812,6 +956,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 {