fix(skills): prevent path traversal in skill remove command

- Fix URL validation to check for https:// or http:// prefixes instead of partial string matching which could be bypassed
- Add path traversal protection in skill remove command to reject .., /, and verify canonical path is inside the skills directory
This commit is contained in:
Argenis 2026-02-15 08:15:41 -05:00 committed by GitHub
parent da453f0b4b
commit 641a5bf917
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -499,7 +499,7 @@ pub fn handle_command(command: crate::SkillCommands, workspace_dir: &Path) -> Re
let skills_path = skills_dir(workspace_dir); let skills_path = skills_dir(workspace_dir);
std::fs::create_dir_all(&skills_path)?; std::fs::create_dir_all(&skills_path)?;
if source.starts_with("http") || source.contains("github.com") { if source.starts_with("https://") || source.starts_with("http://") {
// Git clone // Git clone
let output = std::process::Command::new("git") let output = std::process::Command::new("git")
.args(["clone", "--depth", "1", &source]) .args(["clone", "--depth", "1", &source])
@ -585,7 +585,23 @@ pub fn handle_command(command: crate::SkillCommands, workspace_dir: &Path) -> Re
Ok(()) Ok(())
} }
crate::SkillCommands::Remove { name } => { crate::SkillCommands::Remove { name } => {
// Reject path traversal attempts
if name.contains("..") || name.contains('/') || name.contains('\\') {
anyhow::bail!("Invalid skill name: {name}");
}
let skill_path = skills_dir(workspace_dir).join(&name); let skill_path = skills_dir(workspace_dir).join(&name);
// Verify the resolved path is actually inside the skills directory
let canonical_skills = skills_dir(workspace_dir)
.canonicalize()
.unwrap_or_else(|_| skills_dir(workspace_dir));
if let Ok(canonical_skill) = skill_path.canonicalize() {
if !canonical_skill.starts_with(&canonical_skills) {
anyhow::bail!("Skill path escapes skills directory: {name}");
}
}
if !skill_path.exists() { if !skill_path.exists() {
anyhow::bail!("Skill not found: {name}"); anyhow::bail!("Skill not found: {name}");
} }