Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/rmcp/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
85 changes: 75 additions & 10 deletions crates/rmcp/src/model/skill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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())
{
Comment on lines +274 to +277

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Agent Skills naming rules already forbid those characters and allow only lowercase letters, numbers, and a single hyphen between them. The spec says the name has to follow those rules. Would you rather enforce them here?

return Err(SkillValidationError::NameMismatch {
uri: self.uri.clone(),
name: self.frontmatter.name.clone(),
Expand Down Expand Up @@ -325,7 +328,7 @@ fn manifest_resource_key(skill_uri: &str, resource_uri: &str) -> Option<String>
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()?;
Expand All @@ -350,10 +353,13 @@ fn manifest_resource_key(skill_uri: &str, resource_uri: &str) -> Option<String>

// RFC 3986 §6.2.2 decodes unreserved bytes and uppercases the hex digits in
// other escapes. Reserved delimiters therefore stay encoded in the key.
Comment on lines 354 to 355

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we mention the new encoding pass in the comment?

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())
{
Expand All @@ -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.
Expand All @@ -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()));
}
}
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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 {
Expand Down
Loading