mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-27 08:00:17 +00:00
feat: Secure prompt-based skills system (Phases 1-4) (#51)
* feat: Add secure prompt-based skills system (Phase 1 MVP) Implement a skills system that extends the agent with prompt-level instructions from local directories. Skills declare activation criteria, tool permissions, and trust tiers that determine authority attenuation. Core security model: the minimum trust level of any active skill determines a tool ceiling -- tools above the ceiling are removed from the LLM's tool list entirely at the API level, preventing prompt-based manipulation. New modules: - skills/mod.rs: Core types (SkillTrust, SkillManifest, LoadedSkill) - skills/scanner.rs: Content scanner for manipulation detection - skills/registry.rs: Filesystem discovery and manifest parsing - skills/selector.rs: Deterministic two-phase prefilter (no LLM) - skills/attenuation.rs: Trust-based tool filtering Integration: - Agent loop selects skills per-turn and applies tool attenuation - Reasoning engine injects skill context with structural isolation - Config supports SKILLS_ENABLED, SKILLS_DIR, SKILLS_MAX_ACTIVE, SKILLS_MAX_CONTEXT_TOKENS environment variables - Disabled by default (SKILLS_ENABLED=false) 41 new tests covering all modules. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: Address all adversarial review findings for skills system Security fixes: - Escape skill name/version in XML attributes to prevent trust spoofing - Escape prompt content to prevent </skill> tag breakout - Require integrity hash for Verified/Community tier skills - Validate skill names against [a-zA-Z0-9][a-zA-Z0-9._-]{0,63} - Add 64 KiB file size limit on prompt.md Bug fixes: - Use actual SkillsConfig from AgentDeps instead of SkillsConfig::default() - Add skills_config field to AgentDeps, wired through from main.rs Performance: - Pre-compile regex patterns at load time (cached on LoadedSkill) - Selector uses pre-compiled patterns instead of recompiling per message - Switch all std::fs to tokio::fs for non-blocking async I/O Hardening: - Cap keyword score at 30 points to prevent keyword stuffing attacks - Enforce max 20 keywords and 5 patterns per skill - Normalize line endings (CRLF/CR to LF) before hashing - Also includes cargo fmt formatting fixes for adjacent code Tests: 54 skills tests pass (up from 41), zero new clippy warnings. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: Address medium/low severity findings from adversarial review Fixes all 18 medium/low severity findings identified by the security review: - mod.rs: Add MAX_TAGS_PER_SKILL cap (10) in enforce_limits(); use RegexBuilder with 64 KiB size_limit to prevent ReDoS; replace case-enumerated escape_skill_content with regex matching all case variants plus whitespace/null byte injection between </ and skill; document allowed_patterns as unenforced until Phase 2; document Marketplace URL validation as Phase 3 concern - registry.rs: Add MAX_MANIFEST_FILE_SIZE (16 KiB) check before reading; add symlink detection via symlink_metadata to reject symlinks in discover_local; add MAX_DISCOVERED_SKILLS (100) cap; validate prompt_hash format (sha256: + 64 hex chars); warn on name collision before overwriting; accept SkillSource parameter in load_skill instead of always using Local; add InvalidHashFormat, ManifestTooLarge, SymlinkDetected error variants - selector.rs: Add MAX_TAG_SCORE (15) cap parallel to keyword cap; warn when declared max_context_tokens diverges >2x from actual prompt size - scanner.rs: Add mixed-script homoglyph detection (Cyrillic, Greek, Armenian unicode ranges); document token-boundary bypass and semantic paraphrasing as known limitations - attenuation.rs: Document READ_ONLY_TOOLS maintenance requirements - agent_loop.rs: Surface scan warnings via structured tracing; add structured audit events for skill activation and tool attenuation 61 tests pass, 0 new clippy warnings. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: Address 12 findings from second adversarial security review HIGH: - Escape opening <skill tags in prompt content (prevents fake skill block injection) - Scan manifest metadata fields (description, author, tags, reasons) not just prompt - Block trust downgrade on name collision (existing Local can't be replaced by Community) MEDIUM: - Eliminate TOCTOU gap: read files then check size instead of metadata-then-read - Reject file-level symlinks in load_skill (prompt.md, skill.toml) - Truncate and filter manifest.skill.tags (prevent unlimited tag scoring) - Cap regex pattern score at 40 (prevent 5x20=100 dominating keyword+tag) - Add doc comment about skill_list tool exposing metadata (sanitization required) - Move Community disclaimer inside <skill> tags (not outside structural boundary) - Filter keywords/tags shorter than 3 chars (prevent broad matching) LOW: - Enforce minimum token_cost of 1 (max_context_tokens=0 can't bypass budget) - Remove redundant try_exists checks in discover_local (let load_skill handle errors) 70 skills tests passing. Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: Add HTTP endpoint scoping for skills (Phase 1) Skills that declare an [http] section in skill.toml now have their HTTP requests constrained to declared endpoints at runtime. This addresses the gap where allowed_patterns was parsed but never enforced -- once the http tool was visible via attenuation, the LLM could reach any URL. Enforcement reuses EndpointPattern/AllowlistValidator from the WASM capability system. Semantics: if no active skill declares [http], all requests pass through (backward compat). If any skill declares [http], URLs must match at least one skill's allowlist (union). Community skills' [http] declarations are silently ignored (defense in depth). Shell commands using curl/wget are also validated against scopes. Scanner gains detection for known exfiltration domains (webhook.site, ngrok.io, etc.), overly broad wildcards, and credential/host mismatches. Closes #38 Co-Authored-By: Claude Opus 4.6 <[email protected]> * style: Apply cargo fmt to http_scoping.rs Co-Authored-By: Claude Opus 4.6 <[email protected]> * style: Apply cargo fmt across codebase Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: Add parameter-level permission enforcement for skills (Phase 2) Activates enforcement of `allowed_patterns` in skill.toml permissions. Previously these patterns were parsed but not enforced -- a Verified skill declaring `permissions.shell` with `allowed_patterns = [{command = "cargo *"}]` could still run any shell command. Now the enforcer validates tool parameters against declared glob patterns before execution. Key changes: - New `enforcer.rs` module with `SkillPermissionEnforcer`, `glob_to_regex()`, and `validate_tool_call()` with union semantics across active skills - Typed pattern enums (`ShellPattern`, `FilePathPattern`, `MemoryTargetPattern`) replace the previous `Vec<serde_json::Value>` in `ToolPermissionDeclaration` - Scanner gains `scan_permission_patterns()` detecting dangerous patterns (rm, sudo, curl, bare wildcards, command chaining, sensitive paths, identity files) - Registry blocks non-Local skills with critical permission pattern warnings - Agent loop threads enforcer into `execute_chat_tool` alongside HTTP scoping Trust interaction: Community patterns ignored, Verified enforced, Local without patterns unrestricted, Local with patterns enforced as guidance. Union semantics across skills -- tool call allowed if ANY skill's patterns permit it. 34 new tests. All 818 library tests pass. Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: Add worker permission enforcement and LLM behavioral analysis (Phase 3+4) Phase 3 - Worker-side permission enforcement: - Add SerializedToolPermission/SerializedPattern DTOs for HTTP boundary crossing - Extend JobDescription, ContainerHandle, and orchestrator API to carry permissions - CreateJobTool snapshots and forwards skill permissions to spawned workers - Worker runtime builds SkillPermissionEnforcer and checks before tool execution - Load-time token budget enforcement rejects prompts exceeding 2x declared budget - Deduplicate enforcer construction: from_active_skills() delegates to from_serialized() Phase 4 - LLM behavioral analysis: - BehavioralAnalyzer with cached, LLM-based semantic content analysis - Structured output parsing (FINDING|CATEGORY|SEVERITY|DESCRIPTION or CLEAN) - Content-hash caching with bounded size (MAX_CACHE_ENTRIES=256) - Graceful degradation when LLM unavailable - Integrated into load_skill() for non-Local skills; critical findings block loading Review fixes: - Real cache tests with CountingLlm mock (test_cache_hit, test_cache_miss, test_cache_bounded) - UTF-8-safe truncate() in worker runtime - Few-shot examples in behavioral analysis prompt - Documented max_context_tokens=0 opt-out and create_job() permission gap 848 tests passing, no new clippy warnings. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: Address review feedback from serrrfirat on skills-phase2 - Fix truncate_cmd UTF-8 panic: use char-boundary-aware slicing - Remove redundant effective_tools branching in reasoning.rs - Document cache eviction as known limitation (arbitrary, not LRU) - Add safety comment on SkillTrust enum ordering (security-critical) - Simplify active_skills selection (prefilter_skills handles empty input) Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address remaining skills review feedback * refactor: replace skills system with OpenClaw SKILL.md format + 2-state trust Replace the 5-gate, 3-tier trust hierarchy (scanner, behavioral analyzer, parameter-level enforcer, HTTP endpoint scoping) with a simplified 3-layer security model: gating -> attenuation -> Docker confinement. Key changes: - SKILL.md format (YAML frontmatter + markdown prompt) replaces skill.toml + prompt.md - 2-state trust (Installed/Trusted) replaces 3-tier (Community/Verified/Local) - New parser.rs for SKILL.md parsing with serde_yaml - New gating.rs for requirements checking (bins/env/config) - Simplified registry with 2-location discovery (workspace + user dirs) - Removed scanner, behavioral_analyzer, enforcer, http_scoping (~4,100 lines) - Removed skill_permissions propagation through job/orchestrator/worker pipeline - Added serde_yaml dependency for YAML frontmatter parsing Net: -5,298 lines, 59 skills tests pass, 907 total tests pass. Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: add in-app skill management tools and ClawHub catalog integration Add 4 chat-callable tools (skill_list, skill_search, skill_install, skill_remove) plus matching web gateway endpoints for managing skills at runtime. The catalog fetches from ClawHub's public registry API at runtime rather than bundling entries at compile time. Key changes: - SkillRegistry gains mutation methods (install_skill, remove_skill, reload, find_by_name) with Arc<RwLock> for concurrent access - New catalog module queries ClawHub /api/v1/search with in-memory caching (5-min TTL, configurable via CLAWHUB_REGISTRY env var) - skill_list and skill_search added to READ_ONLY_TOOLS for safe use under Installed trust ceiling - Web gateway gets /api/skills, /api/skills/search, /api/skills/install, and /api/skills/{name} DELETE endpoints Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address PR #51 review feedback from ilblackdragon Security: - Add SSRF protection to fetch_skill_content: require HTTPS, reject private/loopback/link-local IPs and internal hostnames, disable redirects. Gateway install handler now reuses the same validation. - URL-encode slug in skill_download_url to prevent query injection. - Require X-Confirm-Action header on gateway skill install/remove endpoints (equivalent to chat tool requires_approval gate). Correctness: - Eliminate all block_in_place/block_on usage in skill tools and gateway handlers. Split install into prepare_install_to_disk (static async, no lock) + commit_install (sync, brief write lock). Same pattern for remove: validate_remove + delete_skill_files + commit_remove. - Write normalized content to disk in install_skill (was writing original un-normalized content, causing hash mismatch on re-read). - Fix token estimation from 0.75 to 0.25 tokens/byte (~4 chars per token) in registry.rs, selector.rs, and standalone loader. Dependencies: - Replace deprecated serde_yaml 0.9 with serde_yml 0.0.12. - Remove unused toml dependency. Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
This commit is contained in:
Zaki Manian
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
8e6e84a08d
commit
bac2d75713
@@ -51,7 +51,7 @@ impl CreateJobTool {
|
||||
self
|
||||
}
|
||||
|
||||
fn sandbox_enabled(&self) -> bool {
|
||||
pub fn sandbox_enabled(&self) -> bool {
|
||||
self.job_manager.is_some()
|
||||
}
|
||||
|
||||
|
||||
@@ -9,6 +9,7 @@ mod json;
|
||||
mod memory;
|
||||
pub mod routine;
|
||||
pub(crate) mod shell;
|
||||
pub mod skill_tools;
|
||||
mod time;
|
||||
|
||||
pub use echo::EchoTool;
|
||||
@@ -24,4 +25,5 @@ pub use routine::{
|
||||
RoutineCreateTool, RoutineDeleteTool, RoutineHistoryTool, RoutineListTool, RoutineUpdateTool,
|
||||
};
|
||||
pub use shell::ShellTool;
|
||||
pub use skill_tools::{SkillInstallTool, SkillListTool, SkillRemoveTool, SkillSearchTool};
|
||||
pub use time::TimeTool;
|
||||
|
||||
@@ -0,0 +1,662 @@
|
||||
//! Agent-callable tools for managing skills (prompt-level extensions).
|
||||
//!
|
||||
//! Four tools for discovering, installing, listing, and removing skills
|
||||
//! entirely through conversation, following the extension_tools pattern.
|
||||
|
||||
use std::sync::Arc;
|
||||
|
||||
use async_trait::async_trait;
|
||||
|
||||
use crate::context::JobContext;
|
||||
use crate::skills::catalog::SkillCatalog;
|
||||
use crate::skills::registry::SkillRegistry;
|
||||
use crate::tools::tool::{Tool, ToolError, ToolOutput, require_str};
|
||||
|
||||
// ── skill_list ──────────────────────────────────────────────────────────
|
||||
|
||||
pub struct SkillListTool {
|
||||
registry: Arc<std::sync::RwLock<SkillRegistry>>,
|
||||
}
|
||||
|
||||
impl SkillListTool {
|
||||
pub fn new(registry: Arc<std::sync::RwLock<SkillRegistry>>) -> Self {
|
||||
Self { registry }
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl Tool for SkillListTool {
|
||||
fn name(&self) -> &str {
|
||||
"skill_list"
|
||||
}
|
||||
|
||||
fn description(&self) -> &str {
|
||||
"List all loaded skills with their trust level, source, and activation keywords."
|
||||
}
|
||||
|
||||
fn parameters_schema(&self) -> serde_json::Value {
|
||||
serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"verbose": {
|
||||
"type": "boolean",
|
||||
"description": "Include extra detail (tags, content_hash, version)",
|
||||
"default": false
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
async fn execute(
|
||||
&self,
|
||||
params: serde_json::Value,
|
||||
_ctx: &JobContext,
|
||||
) -> Result<ToolOutput, ToolError> {
|
||||
let start = std::time::Instant::now();
|
||||
let verbose = params
|
||||
.get("verbose")
|
||||
.and_then(|v| v.as_bool())
|
||||
.unwrap_or(false);
|
||||
|
||||
let guard = self
|
||||
.registry
|
||||
.read()
|
||||
.map_err(|e| ToolError::ExecutionFailed(format!("Lock poisoned: {}", e)))?;
|
||||
|
||||
let skills: Vec<serde_json::Value> = guard
|
||||
.skills()
|
||||
.iter()
|
||||
.map(|s| {
|
||||
let mut entry = serde_json::json!({
|
||||
"name": s.manifest.name,
|
||||
"description": s.manifest.description,
|
||||
"trust": s.trust.to_string(),
|
||||
"source": format!("{:?}", s.source),
|
||||
"keywords": s.manifest.activation.keywords,
|
||||
});
|
||||
|
||||
if verbose && let Some(obj) = entry.as_object_mut() {
|
||||
obj.insert(
|
||||
"version".to_string(),
|
||||
serde_json::Value::String(s.manifest.version.clone()),
|
||||
);
|
||||
obj.insert(
|
||||
"tags".to_string(),
|
||||
serde_json::json!(s.manifest.activation.tags),
|
||||
);
|
||||
obj.insert(
|
||||
"content_hash".to_string(),
|
||||
serde_json::Value::String(s.content_hash.clone()),
|
||||
);
|
||||
obj.insert(
|
||||
"max_context_tokens".to_string(),
|
||||
serde_json::json!(s.manifest.activation.max_context_tokens),
|
||||
);
|
||||
}
|
||||
|
||||
entry
|
||||
})
|
||||
.collect();
|
||||
|
||||
let output = serde_json::json!({
|
||||
"skills": skills,
|
||||
"count": skills.len(),
|
||||
});
|
||||
|
||||
Ok(ToolOutput::success(output, start.elapsed()))
|
||||
}
|
||||
}
|
||||
|
||||
// ── skill_search ────────────────────────────────────────────────────────
|
||||
|
||||
pub struct SkillSearchTool {
|
||||
registry: Arc<std::sync::RwLock<SkillRegistry>>,
|
||||
catalog: Arc<SkillCatalog>,
|
||||
}
|
||||
|
||||
impl SkillSearchTool {
|
||||
pub fn new(
|
||||
registry: Arc<std::sync::RwLock<SkillRegistry>>,
|
||||
catalog: Arc<SkillCatalog>,
|
||||
) -> Self {
|
||||
Self { registry, catalog }
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl Tool for SkillSearchTool {
|
||||
fn name(&self) -> &str {
|
||||
"skill_search"
|
||||
}
|
||||
|
||||
fn description(&self) -> &str {
|
||||
"Search for skills in the ClawHub catalog and among locally loaded skills."
|
||||
}
|
||||
|
||||
fn parameters_schema(&self) -> serde_json::Value {
|
||||
serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"query": {
|
||||
"type": "string",
|
||||
"description": "Search query (name, keyword, or description fragment)"
|
||||
}
|
||||
},
|
||||
"required": ["query"]
|
||||
})
|
||||
}
|
||||
|
||||
async fn execute(
|
||||
&self,
|
||||
params: serde_json::Value,
|
||||
_ctx: &JobContext,
|
||||
) -> Result<ToolOutput, ToolError> {
|
||||
let start = std::time::Instant::now();
|
||||
let query = require_str(¶ms, "query")?;
|
||||
|
||||
// Search the ClawHub catalog (async, best-effort)
|
||||
let catalog_results = self.catalog.search(query).await;
|
||||
|
||||
// Search locally loaded skills
|
||||
let installed_names: Vec<String> = {
|
||||
let guard = self
|
||||
.registry
|
||||
.read()
|
||||
.map_err(|e| ToolError::ExecutionFailed(format!("Lock poisoned: {}", e)))?;
|
||||
guard
|
||||
.skills()
|
||||
.iter()
|
||||
.map(|s| s.manifest.name.clone())
|
||||
.collect()
|
||||
};
|
||||
|
||||
// Mark catalog entries that are already installed
|
||||
let catalog_json: Vec<serde_json::Value> = catalog_results
|
||||
.iter()
|
||||
.map(|entry| {
|
||||
let is_installed = installed_names.iter().any(|n| {
|
||||
// Match by slug suffix or exact name
|
||||
entry.slug.ends_with(n.as_str()) || entry.name == *n
|
||||
});
|
||||
serde_json::json!({
|
||||
"slug": entry.slug,
|
||||
"name": entry.name,
|
||||
"description": entry.description,
|
||||
"version": entry.version,
|
||||
"score": entry.score,
|
||||
"installed": is_installed,
|
||||
})
|
||||
})
|
||||
.collect();
|
||||
|
||||
// Find matching local skills (simple substring match)
|
||||
let query_lower = query.to_lowercase();
|
||||
let local_matches: Vec<serde_json::Value> = {
|
||||
let guard = self
|
||||
.registry
|
||||
.read()
|
||||
.map_err(|e| ToolError::ExecutionFailed(format!("Lock poisoned: {}", e)))?;
|
||||
guard
|
||||
.skills()
|
||||
.iter()
|
||||
.filter(|s| {
|
||||
s.manifest.name.to_lowercase().contains(&query_lower)
|
||||
|| s.manifest.description.to_lowercase().contains(&query_lower)
|
||||
|| s.manifest
|
||||
.activation
|
||||
.keywords
|
||||
.iter()
|
||||
.any(|k| k.to_lowercase().contains(&query_lower))
|
||||
})
|
||||
.map(|s| {
|
||||
serde_json::json!({
|
||||
"name": s.manifest.name,
|
||||
"description": s.manifest.description,
|
||||
"trust": s.trust.to_string(),
|
||||
})
|
||||
})
|
||||
.collect()
|
||||
};
|
||||
|
||||
let output = serde_json::json!({
|
||||
"catalog": catalog_json,
|
||||
"catalog_count": catalog_json.len(),
|
||||
"installed": local_matches,
|
||||
"installed_count": local_matches.len(),
|
||||
"registry_url": self.catalog.registry_url(),
|
||||
});
|
||||
|
||||
Ok(ToolOutput::success(output, start.elapsed()))
|
||||
}
|
||||
}
|
||||
|
||||
// ── skill_install ───────────────────────────────────────────────────────
|
||||
|
||||
pub struct SkillInstallTool {
|
||||
registry: Arc<std::sync::RwLock<SkillRegistry>>,
|
||||
catalog: Arc<SkillCatalog>,
|
||||
}
|
||||
|
||||
impl SkillInstallTool {
|
||||
pub fn new(
|
||||
registry: Arc<std::sync::RwLock<SkillRegistry>>,
|
||||
catalog: Arc<SkillCatalog>,
|
||||
) -> Self {
|
||||
Self { registry, catalog }
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl Tool for SkillInstallTool {
|
||||
fn name(&self) -> &str {
|
||||
"skill_install"
|
||||
}
|
||||
|
||||
fn description(&self) -> &str {
|
||||
"Install a skill from SKILL.md content, a URL, or by name from the ClawHub catalog."
|
||||
}
|
||||
|
||||
fn parameters_schema(&self) -> serde_json::Value {
|
||||
serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"name": {
|
||||
"type": "string",
|
||||
"description": "Skill name or slug (from search results)"
|
||||
},
|
||||
"url": {
|
||||
"type": "string",
|
||||
"description": "Direct URL to a SKILL.md file"
|
||||
},
|
||||
"content": {
|
||||
"type": "string",
|
||||
"description": "Raw SKILL.md content to install directly"
|
||||
}
|
||||
},
|
||||
"required": ["name"]
|
||||
})
|
||||
}
|
||||
|
||||
async fn execute(
|
||||
&self,
|
||||
params: serde_json::Value,
|
||||
_ctx: &JobContext,
|
||||
) -> Result<ToolOutput, ToolError> {
|
||||
let start = std::time::Instant::now();
|
||||
let name = require_str(¶ms, "name")?;
|
||||
|
||||
let content = if let Some(raw) = params.get("content").and_then(|v| v.as_str()) {
|
||||
// Direct content provided
|
||||
raw.to_string()
|
||||
} else if let Some(url) = params.get("url").and_then(|v| v.as_str()) {
|
||||
// Fetch from explicit URL
|
||||
fetch_skill_content(url).await?
|
||||
} else {
|
||||
// Look up in catalog and fetch
|
||||
let download_url =
|
||||
crate::skills::catalog::skill_download_url(self.catalog.registry_url(), name);
|
||||
fetch_skill_content(&download_url).await?
|
||||
};
|
||||
|
||||
// Check for duplicates and get user_dir under a brief read lock.
|
||||
let (user_dir, skill_name_from_parse) = {
|
||||
let guard = self
|
||||
.registry
|
||||
.read()
|
||||
.map_err(|e| ToolError::ExecutionFailed(format!("Lock poisoned: {}", e)))?;
|
||||
|
||||
// Parse to extract the name (cheap, in-memory)
|
||||
let normalized = crate::skills::normalize_line_endings(&content);
|
||||
let parsed = crate::skills::parser::parse_skill_md(&normalized)
|
||||
.map_err(|e| ToolError::ExecutionFailed(e.to_string()))?;
|
||||
let skill_name = parsed.manifest.name.clone();
|
||||
|
||||
if guard.has(&skill_name) {
|
||||
return Err(ToolError::ExecutionFailed(format!(
|
||||
"Skill '{}' already exists",
|
||||
skill_name
|
||||
)));
|
||||
}
|
||||
|
||||
(guard.user_dir().to_path_buf(), skill_name)
|
||||
};
|
||||
|
||||
// Perform async I/O (write to disk, validate round-trip) with no lock held.
|
||||
let (skill_name, loaded_skill) =
|
||||
crate::skills::registry::SkillRegistry::prepare_install_to_disk(
|
||||
&user_dir,
|
||||
&skill_name_from_parse,
|
||||
&crate::skills::normalize_line_endings(&content),
|
||||
)
|
||||
.await
|
||||
.map_err(|e| ToolError::ExecutionFailed(e.to_string()))?;
|
||||
|
||||
// Commit the in-memory addition under a brief write lock.
|
||||
let installed_name = {
|
||||
let mut guard = self
|
||||
.registry
|
||||
.write()
|
||||
.map_err(|e| ToolError::ExecutionFailed(format!("Lock poisoned: {}", e)))?;
|
||||
guard
|
||||
.commit_install(&skill_name, loaded_skill)
|
||||
.map_err(|e| ToolError::ExecutionFailed(e.to_string()))?;
|
||||
skill_name
|
||||
};
|
||||
|
||||
let output = serde_json::json!({
|
||||
"name": installed_name,
|
||||
"status": "installed",
|
||||
"trust": "installed",
|
||||
"message": format!(
|
||||
"Skill '{}' installed successfully. It will activate when matching keywords are detected.",
|
||||
installed_name
|
||||
),
|
||||
});
|
||||
|
||||
Ok(ToolOutput::success(output, start.elapsed()))
|
||||
}
|
||||
|
||||
fn requires_approval(&self) -> bool {
|
||||
true
|
||||
}
|
||||
}
|
||||
|
||||
/// Validate that a URL is safe to fetch (SSRF prevention).
|
||||
///
|
||||
/// Rejects:
|
||||
/// - Non-HTTPS URLs (except in tests)
|
||||
/// - URLs pointing to private, loopback, or link-local IP addresses
|
||||
/// - URLs without a host
|
||||
pub fn validate_fetch_url(url_str: &str) -> Result<(), ToolError> {
|
||||
let parsed = url::Url::parse(url_str)
|
||||
.map_err(|e| ToolError::ExecutionFailed(format!("Invalid URL '{}': {}", url_str, e)))?;
|
||||
|
||||
// Require HTTPS
|
||||
if parsed.scheme() != "https" {
|
||||
return Err(ToolError::ExecutionFailed(format!(
|
||||
"Only HTTPS URLs are allowed for skill fetching, got scheme '{}'",
|
||||
parsed.scheme()
|
||||
)));
|
||||
}
|
||||
|
||||
let host = parsed
|
||||
.host_str()
|
||||
.ok_or_else(|| ToolError::ExecutionFailed("URL has no host".to_string()))?;
|
||||
|
||||
// Check if host is an IP address and reject private ranges
|
||||
if let Ok(ip) = host.parse::<std::net::IpAddr>()
|
||||
&& (ip.is_loopback() || ip.is_unspecified() || is_private_ip(&ip) || is_link_local_ip(&ip))
|
||||
{
|
||||
return Err(ToolError::ExecutionFailed(format!(
|
||||
"URL points to a private/loopback/link-local address: {}",
|
||||
host
|
||||
)));
|
||||
}
|
||||
|
||||
// Reject common internal hostnames
|
||||
let host_lower = host.to_lowercase();
|
||||
if host_lower == "localhost"
|
||||
|| host_lower == "metadata.google.internal"
|
||||
|| host_lower.ends_with(".internal")
|
||||
|| host_lower.ends_with(".local")
|
||||
{
|
||||
return Err(ToolError::ExecutionFailed(format!(
|
||||
"URL points to an internal hostname: {}",
|
||||
host
|
||||
)));
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn is_private_ip(ip: &std::net::IpAddr) -> bool {
|
||||
match ip {
|
||||
std::net::IpAddr::V4(v4) => {
|
||||
// 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16
|
||||
v4.is_private() || v4.is_link_local()
|
||||
}
|
||||
std::net::IpAddr::V6(v6) => {
|
||||
// Unique local (fc00::/7)
|
||||
let segments = v6.segments();
|
||||
(segments[0] & 0xfe00) == 0xfc00
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn is_link_local_ip(ip: &std::net::IpAddr) -> bool {
|
||||
match ip {
|
||||
std::net::IpAddr::V4(v4) => v4.is_link_local(),
|
||||
std::net::IpAddr::V6(v6) => {
|
||||
// fe80::/10
|
||||
let segments = v6.segments();
|
||||
(segments[0] & 0xffc0) == 0xfe80
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Fetch SKILL.md content from a URL with SSRF protection.
|
||||
pub async fn fetch_skill_content(url: &str) -> Result<String, ToolError> {
|
||||
validate_fetch_url(url)?;
|
||||
|
||||
let client = reqwest::Client::builder()
|
||||
.timeout(std::time::Duration::from_secs(15))
|
||||
.user_agent("ironclaw/0.1")
|
||||
.redirect(reqwest::redirect::Policy::none())
|
||||
.build()
|
||||
.map_err(|e| ToolError::ExecutionFailed(format!("HTTP client error: {}", e)))?;
|
||||
|
||||
let response = client.get(url).send().await.map_err(|e| {
|
||||
ToolError::ExecutionFailed(format!("Failed to fetch skill from {}: {}", url, e))
|
||||
})?;
|
||||
|
||||
if !response.status().is_success() {
|
||||
return Err(ToolError::ExecutionFailed(format!(
|
||||
"Skill fetch returned HTTP {}: {}",
|
||||
response.status(),
|
||||
url
|
||||
)));
|
||||
}
|
||||
|
||||
let content = response
|
||||
.text()
|
||||
.await
|
||||
.map_err(|e| ToolError::ExecutionFailed(format!("Failed to read response body: {}", e)))?;
|
||||
|
||||
// Basic size check
|
||||
if content.len() as u64 > crate::skills::MAX_PROMPT_FILE_SIZE {
|
||||
return Err(ToolError::ExecutionFailed(format!(
|
||||
"Skill content too large: {} bytes (max {} bytes)",
|
||||
content.len(),
|
||||
crate::skills::MAX_PROMPT_FILE_SIZE
|
||||
)));
|
||||
}
|
||||
|
||||
Ok(content)
|
||||
}
|
||||
|
||||
// ── skill_remove ────────────────────────────────────────────────────────
|
||||
|
||||
pub struct SkillRemoveTool {
|
||||
registry: Arc<std::sync::RwLock<SkillRegistry>>,
|
||||
}
|
||||
|
||||
impl SkillRemoveTool {
|
||||
pub fn new(registry: Arc<std::sync::RwLock<SkillRegistry>>) -> Self {
|
||||
Self { registry }
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl Tool for SkillRemoveTool {
|
||||
fn name(&self) -> &str {
|
||||
"skill_remove"
|
||||
}
|
||||
|
||||
fn description(&self) -> &str {
|
||||
"Remove an installed skill by name. Only user-installed skills can be removed."
|
||||
}
|
||||
|
||||
fn parameters_schema(&self) -> serde_json::Value {
|
||||
serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"name": {
|
||||
"type": "string",
|
||||
"description": "Name of the skill to remove"
|
||||
}
|
||||
},
|
||||
"required": ["name"]
|
||||
})
|
||||
}
|
||||
|
||||
async fn execute(
|
||||
&self,
|
||||
params: serde_json::Value,
|
||||
_ctx: &JobContext,
|
||||
) -> Result<ToolOutput, ToolError> {
|
||||
let start = std::time::Instant::now();
|
||||
let name = require_str(¶ms, "name")?;
|
||||
|
||||
// Validate removal and get the filesystem path under a brief read lock.
|
||||
let skill_path = {
|
||||
let guard = self
|
||||
.registry
|
||||
.read()
|
||||
.map_err(|e| ToolError::ExecutionFailed(format!("Lock poisoned: {}", e)))?;
|
||||
guard
|
||||
.validate_remove(name)
|
||||
.map_err(|e| ToolError::ExecutionFailed(e.to_string()))?
|
||||
};
|
||||
|
||||
// Delete files from disk (async I/O, no lock held).
|
||||
crate::skills::registry::SkillRegistry::delete_skill_files(&skill_path)
|
||||
.await
|
||||
.map_err(|e| ToolError::ExecutionFailed(e.to_string()))?;
|
||||
|
||||
// Remove from in-memory registry under a brief write lock.
|
||||
{
|
||||
let mut guard = self
|
||||
.registry
|
||||
.write()
|
||||
.map_err(|e| ToolError::ExecutionFailed(format!("Lock poisoned: {}", e)))?;
|
||||
guard
|
||||
.commit_remove(name)
|
||||
.map_err(|e| ToolError::ExecutionFailed(e.to_string()))?;
|
||||
}
|
||||
|
||||
let output = serde_json::json!({
|
||||
"name": name,
|
||||
"status": "removed",
|
||||
"message": format!("Skill '{}' has been removed.", name),
|
||||
});
|
||||
|
||||
Ok(ToolOutput::success(output, start.elapsed()))
|
||||
}
|
||||
|
||||
fn requires_approval(&self) -> bool {
|
||||
true
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
fn test_registry() -> Arc<std::sync::RwLock<SkillRegistry>> {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
// Keep the tempdir so it lives for the test duration
|
||||
let path = dir.keep();
|
||||
Arc::new(std::sync::RwLock::new(SkillRegistry::new(path)))
|
||||
}
|
||||
|
||||
fn test_catalog() -> Arc<SkillCatalog> {
|
||||
Arc::new(SkillCatalog::with_url("http://127.0.0.1:1"))
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_skill_list_schema() {
|
||||
let tool = SkillListTool::new(test_registry());
|
||||
assert_eq!(tool.name(), "skill_list");
|
||||
assert!(!tool.requires_approval());
|
||||
let schema = tool.parameters_schema();
|
||||
assert!(schema.get("properties").is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_skill_search_schema() {
|
||||
let tool = SkillSearchTool::new(test_registry(), test_catalog());
|
||||
assert_eq!(tool.name(), "skill_search");
|
||||
assert!(!tool.requires_approval());
|
||||
let schema = tool.parameters_schema();
|
||||
assert!(schema["properties"].get("query").is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_skill_install_schema() {
|
||||
let tool = SkillInstallTool::new(test_registry(), test_catalog());
|
||||
assert_eq!(tool.name(), "skill_install");
|
||||
assert!(tool.requires_approval());
|
||||
let schema = tool.parameters_schema();
|
||||
assert!(schema["properties"].get("name").is_some());
|
||||
assert!(schema["properties"].get("url").is_some());
|
||||
assert!(schema["properties"].get("content").is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_skill_remove_schema() {
|
||||
let tool = SkillRemoveTool::new(test_registry());
|
||||
assert_eq!(tool.name(), "skill_remove");
|
||||
assert!(tool.requires_approval());
|
||||
let schema = tool.parameters_schema();
|
||||
assert!(schema["properties"].get("name").is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_validate_fetch_url_allows_https() {
|
||||
assert!(super::validate_fetch_url("https://clawhub.ai/api/v1/download?slug=foo").is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_validate_fetch_url_rejects_http() {
|
||||
let err = super::validate_fetch_url("http://example.com/skill.md").unwrap_err();
|
||||
assert!(err.to_string().contains("Only HTTPS"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_validate_fetch_url_rejects_private_ip() {
|
||||
let err = super::validate_fetch_url("https://192.168.1.1/skill.md").unwrap_err();
|
||||
assert!(err.to_string().contains("private"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_validate_fetch_url_rejects_loopback() {
|
||||
let err = super::validate_fetch_url("https://127.0.0.1/skill.md").unwrap_err();
|
||||
assert!(err.to_string().contains("private"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_validate_fetch_url_rejects_localhost() {
|
||||
let err = super::validate_fetch_url("https://localhost/skill.md").unwrap_err();
|
||||
assert!(err.to_string().contains("internal hostname"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_validate_fetch_url_rejects_metadata_endpoint() {
|
||||
let err =
|
||||
super::validate_fetch_url("https://169.254.169.254/latest/meta-data/").unwrap_err();
|
||||
assert!(err.to_string().contains("private"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_validate_fetch_url_rejects_internal_domain() {
|
||||
let err =
|
||||
super::validate_fetch_url("https://metadata.google.internal/something").unwrap_err();
|
||||
assert!(err.to_string().contains("internal hostname"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_validate_fetch_url_rejects_file_scheme() {
|
||||
let err = super::validate_fetch_url("file:///etc/passwd").unwrap_err();
|
||||
assert!(err.to_string().contains("Only HTTPS"));
|
||||
}
|
||||
}
|
||||
+30
-2
@@ -12,12 +12,15 @@ use crate::llm::{LlmProvider, ToolDefinition};
|
||||
use crate::orchestrator::job_manager::ContainerJobManager;
|
||||
use crate::safety::SafetyLayer;
|
||||
use crate::secrets::SecretsStore;
|
||||
use crate::skills::catalog::SkillCatalog;
|
||||
use crate::skills::registry::SkillRegistry;
|
||||
use crate::tools::builder::{BuildSoftwareTool, BuilderConfig, LlmSoftwareBuilder};
|
||||
use crate::tools::builtin::{
|
||||
ApplyPatchTool, CancelJobTool, CreateJobTool, EchoTool, HttpTool, JobStatusTool, JsonTool,
|
||||
ListDirTool, ListJobsTool, MemoryReadTool, MemorySearchTool, MemoryTreeTool, MemoryWriteTool,
|
||||
ReadFileTool, ShellTool, TimeTool, ToolActivateTool, ToolAuthTool, ToolInstallTool,
|
||||
ToolListTool, ToolRemoveTool, ToolSearchTool, WriteFileTool,
|
||||
ReadFileTool, ShellTool, SkillInstallTool, SkillListTool, SkillRemoveTool, SkillSearchTool,
|
||||
TimeTool, ToolActivateTool, ToolAuthTool, ToolInstallTool, ToolListTool, ToolRemoveTool,
|
||||
ToolSearchTool, WriteFileTool,
|
||||
};
|
||||
use crate::tools::tool::{Tool, ToolDomain};
|
||||
use crate::tools::wasm::{
|
||||
@@ -59,6 +62,10 @@ const PROTECTED_TOOL_NAMES: &[&str] = &[
|
||||
"routine_update",
|
||||
"routine_delete",
|
||||
"routine_history",
|
||||
"skill_list",
|
||||
"skill_search",
|
||||
"skill_install",
|
||||
"skill_remove",
|
||||
];
|
||||
|
||||
/// Registry of available tools.
|
||||
@@ -271,6 +278,27 @@ impl ToolRegistry {
|
||||
tracing::info!("Registered 6 extension management tools");
|
||||
}
|
||||
|
||||
/// Register skill management tools (list, search, install, remove).
|
||||
///
|
||||
/// These allow the LLM to manage prompt-level skills through conversation.
|
||||
pub fn register_skill_tools(
|
||||
&self,
|
||||
registry: Arc<std::sync::RwLock<SkillRegistry>>,
|
||||
catalog: Arc<SkillCatalog>,
|
||||
) {
|
||||
self.register_sync(Arc::new(SkillListTool::new(Arc::clone(®istry))));
|
||||
self.register_sync(Arc::new(SkillSearchTool::new(
|
||||
Arc::clone(®istry),
|
||||
Arc::clone(&catalog),
|
||||
)));
|
||||
self.register_sync(Arc::new(SkillInstallTool::new(
|
||||
Arc::clone(®istry),
|
||||
Arc::clone(&catalog),
|
||||
)));
|
||||
self.register_sync(Arc::new(SkillRemoveTool::new(registry)));
|
||||
tracing::info!("Registered 4 skill management tools");
|
||||
}
|
||||
|
||||
/// Register routine management tools.
|
||||
///
|
||||
/// These allow the LLM to create, list, update, delete, and view history
|
||||
|
||||
Reference in New Issue
Block a user