diff --git a/src/appliers/base.py b/src/appliers/base.py index 8c1d0ba..af18c38 100644 --- a/src/appliers/base.py +++ b/src/appliers/base.py @@ -70,6 +70,12 @@ class BaseApplier(ABC): # with a description of how the tool expects its memory files. MEMORY_SCHEMA: str = "" + # Subclasses MUST override this with the directory the LLM is allowed to + # write memory files into. apply_memory_via_llm() rejects any path that + # does not resolve inside this directory. Defaults to Path.home() as a + # minimum guard; narrow it in each applier. + MEMORY_ALLOWED_BASE: Optional[Path] = None + def get_manifest(self) -> ToolManifest: """Return (or create) the manifest for this tool.""" return ToolManifest(self.TOOL_NAME) @@ -95,7 +101,15 @@ def link_skills(self, skills: List[Dict], source_dir: Path, manifest: ToolManife count = 0 for skill in skills: - name = skill.get("name", "unnamed") + raw_name = skill.get("name", "unnamed") + try: + from skills import sanitize_skill_name + + name = sanitize_skill_name(raw_name) + except (ValueError, ImportError): + warning(f"Skipping skill with invalid name: {raw_name!r}") + continue + source = source_dir / name if not source.exists(): continue @@ -212,6 +226,10 @@ def apply_memory_via_llm(self, collected_memory: List[Dict], manifest: ToolManif warning(f"Raw LLM response: {response[:500]}") return 0 + # Determine the allowed write root for this applier. + # Resolving at call-time so tests can monkeypatch Path.home(). + allowed_base = (self.MEMORY_ALLOWED_BASE or Path.home()).resolve() + # Write files count = 0 for op in file_ops: @@ -222,11 +240,21 @@ def apply_memory_via_llm(self, collected_memory: List[Dict], manifest: ToolManif if not file_path or content is None: continue - path = Path(file_path) - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(content, encoding="utf-8") + # Security: resolve the path (collapses `..`) and assert it lands + # inside the allowed base directory. Rejects prompt-injection or + # hallucinated paths like /etc/cron.d/evil. + resolved = Path(file_path).resolve() + if not str(resolved).startswith(str(allowed_base) + "/") and resolved != allowed_base: + warning( + f"[security] Rejecting LLM-suggested write outside allowed path: " + f"{file_path!r} (resolved: {resolved}, allowed base: {allowed_base})" + ) + continue + + resolved.parent.mkdir(parents=True, exist_ok=True) + resolved.write_text(content, encoding="utf-8") manifest.record_memory( - file_path=str(path), + file_path=str(resolved), content=content, entry_ids=[e.get("entry_id") or e.get("id", "") for e in collected_memory], ) diff --git a/src/appliers/claude.py b/src/appliers/claude.py index 0f1e345..6bc022d 100644 --- a/src/appliers/claude.py +++ b/src/appliers/claude.py @@ -50,6 +50,10 @@ def SKILL_DIR(self, value): TOOL_NAME = "claude-code" MEMORY_SCHEMA = CLAUDE_MEMORY_SCHEMA + @property # type: ignore[override] + def MEMORY_ALLOWED_BASE(self) -> "Path": # noqa: N802 + return _claude_dir() + def apply_skills(self, skills: List[Dict], manifest: ToolManifest) -> int: _claude_commands_dir().mkdir(parents=True, exist_ok=True) count = 0 diff --git a/src/appliers/copilot.py b/src/appliers/copilot.py index 1a2e707..e6cd239 100644 --- a/src/appliers/copilot.py +++ b/src/appliers/copilot.py @@ -72,6 +72,11 @@ class CopilotApplier(BaseApplier): TOOL_NAME = "github-copilot" MEMORY_SCHEMA = COPILOT_MEMORY_SCHEMA + @property # type: ignore[override] + def MEMORY_ALLOWED_BASE(self) -> "Path": # noqa: N802 + # Copilot writes to .github/ in the current project directory. + return Path.cwd() + def apply_skills(self, skills: List[Dict], manifest: ToolManifest) -> int: count = 0 for skill in skills: diff --git a/src/appliers/cursor.py b/src/appliers/cursor.py index 2383074..a6e9e2f 100644 --- a/src/appliers/cursor.py +++ b/src/appliers/cursor.py @@ -77,6 +77,10 @@ class CursorApplier(BaseApplier): TOOL_NAME = "cursor" MEMORY_SCHEMA = CURSOR_MEMORY_SCHEMA + @property # type: ignore[override] + def MEMORY_ALLOWED_BASE(self) -> "Path": # noqa: N802 + return _cursor_dir() + @property def SKILL_DIR(self) -> Path: # type: ignore[override] return _cursor_rules_dir() diff --git a/src/appliers/gemini.py b/src/appliers/gemini.py index 53eeaa1..5650245 100644 --- a/src/appliers/gemini.py +++ b/src/appliers/gemini.py @@ -75,6 +75,10 @@ class GeminiApplier(BaseApplier): TOOL_NAME = "gemini-cli" MEMORY_SCHEMA = GEMINI_MEMORY_SCHEMA + @property # type: ignore[override] + def MEMORY_ALLOWED_BASE(self) -> "Path": # noqa: N802 + return _gemini_dir() + def apply_skills(self, skills: List[Dict], manifest: ToolManifest) -> int: return 0 # Gemini doesn't have a skills format diff --git a/src/appliers/openclaw.py b/src/appliers/openclaw.py index 5bf19ac..720bad0 100644 --- a/src/appliers/openclaw.py +++ b/src/appliers/openclaw.py @@ -64,6 +64,10 @@ def SKILL_DIR(self, value): TOOL_NAME = "openclaw" MEMORY_SCHEMA = OPENCLAW_MEMORY_SCHEMA + @property # type: ignore[override] + def MEMORY_ALLOWED_BASE(self) -> "Path": # noqa: N802 + return _openclaw_workspace() + def apply_skills(self, skills: List[Dict], manifest: ToolManifest) -> int: _openclaw_skills_dir().mkdir(parents=True, exist_ok=True) count = 0 diff --git a/src/appliers/windsurf.py b/src/appliers/windsurf.py index 75c7caa..e02d785 100644 --- a/src/appliers/windsurf.py +++ b/src/appliers/windsurf.py @@ -91,6 +91,10 @@ class WindsurfApplier(BaseApplier): TOOL_NAME = "windsurf" MEMORY_SCHEMA = WINDSURF_MEMORY_SCHEMA + @property # type: ignore[override] + def MEMORY_ALLOWED_BASE(self) -> "Path": # noqa: N802 + return _windsurf_dir() + def apply_skills(self, skills: List[Dict], manifest: ToolManifest) -> int: return 0 diff --git a/src/install.py b/src/install.py index 49fe984..8ad3485 100644 --- a/src/install.py +++ b/src/install.py @@ -10,7 +10,7 @@ from appliers import get_applier from cache import load_skills, merge_skills, save_skills from extractors import detect_installed_tools -from skills import fetch_skill_from_repo, list_skills_in_repo, save_skill_file +from skills import fetch_skill_from_repo, list_skills_in_repo, sanitize_skill_name, save_skill_file _AGENTS = ["claude-code", "cursor", "gemini-cli", "github-copilot", "openclaw", "windsurf"] @@ -176,6 +176,13 @@ def install(repo, skills, install_all, targets, branch, list_only, yes): click.echo(f" not found in {repo}") continue + # Validate name once more before writing to disk (save_skill_file also validates) + try: + sanitize_skill_name(skill["name"]) + except ValueError as exc: + click.echo(f" skipped — invalid name: {exc}", err=True) + continue + # Save to ~/.apc/skills//SKILL.md raw_content = skill.pop("_raw_content", skill.get("body", "")) save_skill_file(skill["name"], raw_content) diff --git a/src/skills.py b/src/skills.py index d612a79..f51b5f7 100644 --- a/src/skills.py +++ b/src/skills.py @@ -4,6 +4,7 @@ tool's skill directory on sync. """ +import re from pathlib import Path from typing import Any, Dict, List, Optional @@ -16,6 +17,29 @@ _GITHUB_TREE_API = "https://api.github.com/repos/{repo}/git/trees/{branch}?recursive=1" _GITHUB_RAW = "https://raw.githubusercontent.com/{repo}/{branch}/skills/{skill}/SKILL.md" +_SKILL_NAME_SAFE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9_\-]{0,63}$") + + +def sanitize_skill_name(name: str) -> str: + """Return a safe skill name or raise ValueError. + + Rules: + - Basename only (strips directory components). + - No path separators, no '..' traversal. + - Must match [A-Za-z0-9][A-Za-z0-9_\\-]{0,63}. + """ + # Take basename — eliminates leading paths like "../../etc" + safe = Path(name).name + # Reject anything that still looks path-like after basename + if not safe or safe in (".", ".."): + raise ValueError(f"Invalid skill name (empty or dot): {name!r}") + if not _SKILL_NAME_SAFE.match(safe): + raise ValueError( + f"Skill name {safe!r} contains invalid characters. " + "Only letters, digits, hyphens, and underscores are allowed." + ) + return safe + # --------------------------------------------------------------------------- # Skills directory @@ -31,6 +55,7 @@ def get_skills_dir() -> Path: def save_skill_file(skill_name: str, raw_content: str) -> Path: """Save raw SKILL.md to ~/.apc/skills//SKILL.md. Returns the path.""" + skill_name = sanitize_skill_name(skill_name) # raises ValueError on traversal skill_dir = get_skills_dir() / skill_name skill_dir.mkdir(exist_ok=True) path = skill_dir / "SKILL.md" @@ -86,8 +111,15 @@ def fetch_skill_from_repo( return None metadata, body = parse_frontmatter(resp.text) + # Sanitize the name from frontmatter — it comes from an untrusted source. + raw_name = metadata.get("name", skill_name) + try: + safe_name = sanitize_skill_name(raw_name) + except ValueError: + # Fall back to the URL-path component (already validated by list_skills_in_repo) + safe_name = sanitize_skill_name(skill_name) return { - "name": metadata.get("name", skill_name), + "name": safe_name, "description": metadata.get("description", ""), "body": body.strip(), "tags": metadata.get("tags", []),