fix: prevent deletion of instance-level skills via agent API - #400
Conversation
SkillSet::remove() now rejects instance-level skills with an error instead of deleting them from disk. The API endpoint returns 403 for this case. Instance skills are shared across all agents and should not be removable through the per-agent endpoint. Also fixes pre-existing clippy collapsible_if lint in signal adapter. Closes spacedriveapp#365
WalkthroughThis PR fixes a security vulnerability where instance-level skills could be permanently deleted via the agent API, affecting all agents. The fix adds source-level verification in the remove method to reject instance-level skill deletions and updates API error handling to return HTTP 403 for such attempts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/skills.rs (1)
182-195: Return a typed error for the instance-skill guard.This policy is currently encoded in an
anyhow!string, which forcessrc/api/skills.rsto detect the 403 case by substring-matching the message. If this text changes, the API will quietly fall back to 500. A dedicated error variant for “instance-level skill cannot be removed here” would make the authorization behavior stable across callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills.rs` around lines 182 - 195, Replace the ad-hoc anyhow::bail string in Skills::remove with a typed error variant: introduce a new error enum variant (e.g., RemoveSkillError::InstanceSkillNotRemovable or add InstanceSkillRemoval to the existing error type used by skills operations) and return that variant when skill.source == SkillSource::Instance from the remove method; update callers (notably the API layer in src/api/skills.rs) to detect this specific error variant instead of substring-matching the message so instance-level removal yields a stable 403 handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/skills.rs`:
- Around line 182-195: Replace the ad-hoc anyhow::bail string in Skills::remove
with a typed error variant: introduce a new error enum variant (e.g.,
RemoveSkillError::InstanceSkillNotRemovable or add InstanceSkillRemoval to the
existing error type used by skills operations) and return that variant when
skill.source == SkillSource::Instance from the remove method; update callers
(notably the API layer in src/api/skills.rs) to detect this specific error
variant instead of substring-matching the message so instance-level removal
yields a stable 403 handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dea34f14-d836-4434-a756-b7f0ef4d65c5
📒 Files selected for processing (3)
src/api/skills.rssrc/skills.rssrc/tools/send_message_to_another_channel.rs
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| @@ -252,8 +252,14 @@ pub(super) async fn remove_skill( | |||
| crate::skills::SkillSet::load(&instance_skills_dir, &workspace_skills_dir).await; | |||
|
|
|||
| let removed_path = skills.remove(&req.name).await.map_err(|error| { | |||
There was a problem hiding this comment.
String-matching on anyhow text to decide 403 feels brittle. Since you already loaded the SkillSet, you can check skill.source directly and keep the status mapping independent of the exact error message.
| let removed_path = skills.remove(&req.name).await.map_err(|error| { | |
| if let Some(skill) = skills.get(&req.name) { | |
| if skill.source == crate::skills::SkillSource::Instance { | |
| tracing::warn!(skill = %req.name, "rejected removal of instance-level skill"); | |
| return Err(StatusCode::FORBIDDEN); | |
| } | |
| } | |
| let removed_path = skills.remove(&req.name).await.map_err(|error| { | |
| tracing::warn!(%error, skill = %req.name, "failed to remove skill"); | |
| StatusCode::INTERNAL_SERVER_ERROR | |
| })?; |
| Skill { | ||
| name: name.into(), | ||
| description: format!("{name} skill"), | ||
| file_path: PathBuf::from(format!("/skills/{name}/SKILL.md")), |
There was a problem hiding this comment.
Minor: hardcoding /tmp makes this test helper less portable (e.g. Windows, sandboxed runners). std::env::temp_dir() keeps it platform-friendly.
| file_path: PathBuf::from(format!("/skills/{name}/SKILL.md")), | |
| base_dir: std::env::temp_dir().join(format!("test-skills-{}", uuid::Uuid::new_v4())), |
2 participants
Summary
DELETE /agents/skills/removewas deleting instance-level skills (shared across all agents) without checking the skill's source. Any agent could permanently remove a skill that all agents depend on.SkillSet::remove()now checksskill.sourceand rejects removal ofInstance-level skills with a clear error message403 Forbiddenwhen attempting to remove an instance skill (previously would have returned 500 or succeeded)Also fixes a pre-existing clippy
collapsible_iflint in the Signal adapter code.Closes #365
Test plan
cargo clippy --all-targets -- -D warningspasses cleanremove_instance_skill_is_rejected- verifies instance skills can't be removedremove_workspace_skill_succeeds- verifies workspace skills still workremove_nonexistent_skill_returns_none- verifies missing skill handlingremove_is_case_insensitive- verifies case-insensitive name matchingNote
Security fix: Prevents accidental deletion of instance-level skills through the agent API. The
SkillSet::remove()method now validates skill source and rejects removal attempts for instance-level skills with a clear error. API endpoint correctly returns 403 Forbidden instead of 500 or allowing removal. Also includes clippy lint cleanup in Signal adapter code.Written by Tembo for commit 6b5347d. This will update automatically on new commits.