Merge pull request #1 from Rahul-2k4/copilot/review-pull-75-and-suggest-fixes

fix(skills-hub): path traversal in `remove()` + add `/skill remove` Discord command

Author: Rahul-2k4

core/src/channels/discord/mod.rs

@@ -915,7 +915,8 @@ async fn tmux(
         "skill_view",
         "skill_search",
         "skill_install",
-        "skill_hub_list"
+        "skill_hub_list",
+        "skill_remove"
     )
 )]
 async fn skill(
@@ -1170,6 +1171,42 @@ async fn skill_hub_list(
     Ok(())
 }
 
+/// Remove an installed hub skill (member role required)
+#[poise::command(slash_command)]
+async fn skill_remove(
+    ctx: poise::Context<'_, Data, DiscordError>,
+    #[description = "skill name to remove"] name: String,
+) -> std::result::Result<(), DiscordError> {
+    let _ = ctx.channel_id().start_typing(&ctx.serenity_context().http);
+
+    let roles = DiscordChannel::get_user_roles(ctx.guild_id(), ctx.http(), ctx.author().id).await;
+    let channel = DiscordChannel::from_context(&ctx);
+
+    if !channel.is_member(&roles) {
+        ctx.say("error: remove skill requires member role").await?;
+        return Ok(());
+    }
+
+    match ctx.data().hub_client.remove(name.trim()) {
+        Ok(true) => {
+            ctx.say(format!("✓ removed hub skill `{}`", name.trim()))
+                .await?;
+        }
+        Ok(false) => {
+            ctx.say(format!(
+                "hub skill `{}` was not found. use `/skill hub-list` to see installed skills",
+                name.trim()
+            ))
+            .await?;
+        }
+        Err(e) => {
+            ctx.say(format!("error removing skill: {}", e)).await?;
+        }
+    }
+
+    Ok(())
+}
+
 /// Parse skill name and version from "name@version" format
 fn parse_skill_name_version(input: &str) -> (&str, Option<String>) {
     let trimmed = input.trim();

core/src/skills/hub.rs

@@ -399,6 +399,7 @@ impl SkillHubClient {
 
     /// Remove an installed skill
     pub fn remove(&self, name: &str) -> Result<bool> {
+        validate_skill_name(name)?;
         let mut removed = false;
         let skill_dir = self.config.managed_root.join(name);
         if skill_dir.exists() {
@@ -654,4 +655,37 @@ mod tests {
             "partial install directory should be removed on failure"
         );
     }
+
+    #[test]
+    fn remove_rejects_path_traversal_name() {
+        let cache_root = TempDir::new().unwrap();
+        let managed_root = TempDir::new().unwrap();
+        let config = SkillHubClientConfig::for_mofaclaw()
+            .with_managed_root(managed_root.path())
+            .with_cache_root(cache_root.path());
+        let client = SkillHubClient::new(config).unwrap();
+
+        let err = client.remove("../outside").unwrap_err();
+        assert!(
+            err.to_string().contains("invalid skill name"),
+            "unexpected error: {err}"
+        );
+        assert!(
+            !managed_root.path().join("outside").exists(),
+            "path traversal should not remove files outside managed root"
+        );
+    }
+
+    #[test]
+    fn remove_returns_false_for_nonexistent_skill() {
+        let cache_root = TempDir::new().unwrap();
+        let managed_root = TempDir::new().unwrap();
+        let config = SkillHubClientConfig::for_mofaclaw()
+            .with_managed_root(managed_root.path())
+            .with_cache_root(cache_root.path());
+        let client = SkillHubClient::new(config).unwrap();
+
+        let removed = client.remove("no-such-skill").unwrap();
+        assert!(!removed, "expected false when skill does not exist");
+    }
 }

core/tests/skills_hub.rs

@@ -311,4 +311,38 @@ mod skills_hub_tests {
         );
     }
 
+    #[tokio::test]
+    async fn test_hub_client_remove_uninstalls_skill() {
+        let _guard = env_lock().lock().unwrap_or_else(|e| e.into_inner());
+        let home_dir = TempDir::new().unwrap();
+        let _home_guard = HomeEnvGuard::set(home_dir.path());
+
+        let (catalog_url, server_handle) =
+            spawn_test_hub_server("removable-skill", "A skill to be removed");
+        let hub_client = SkillHubClient::new(
+            SkillHubClientConfig::for_mofaclaw().with_catalog_url(catalog_url),
+        )
+        .unwrap();
+
+        // Install then remove
+        let record = hub_client
+            .install("removable-skill", None)
+            .await
+            .unwrap();
+        assert!(record.installed_path.exists(), "skill should be installed");
+
+        let removed = hub_client.remove("removable-skill").unwrap();
+        assert!(removed, "remove should return true for an installed skill");
+        assert!(
+            !record.installed_path.exists(),
+            "skill directory should be deleted after remove"
+        );
+        assert!(
+            hub_client.get_installed("removable-skill").unwrap().is_none(),
+            "registry record should be deleted after remove"
+        );
+
+        server_handle.join().unwrap();
+    }
+
 }