fix(security): LLM memory write path traversal + skill name sanitization (#17)

* fix(security): path traversal guard on LLM memory writes + skill name sanitization

* style: ruff format

---------

Co-authored-by: Ace <ace@openclaw.ai>

Author: FZ2000

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],
             )

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

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:

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()

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
 

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

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
 

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/<name>/SKILL.md
         raw_content = skill.pop("_raw_content", skill.get("body", ""))
         save_skill_file(skill["name"], raw_content)

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/<name>/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", []),