diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index c1e615bde6..7b9e49d4f2 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -5,6 +5,8 @@ from contextlib import contextmanager from pathlib import Path from unittest.mock import patch +import pytest + from tools.skill_manager_tool import ( _validate_name, _validate_category, @@ -330,6 +332,25 @@ word word result = _patch_skill("nonexistent", "old", "new") assert result["success"] is False + def test_patch_supporting_file_symlink_escape_blocked(self, tmp_path): + outside_file = tmp_path / "outside.txt" + outside_file.write_text("old text here") + + with _skill_dir(tmp_path): + _create_skill("my-skill", VALID_SKILL_CONTENT) + link = tmp_path / "my-skill" / "references" / "evil.md" + link.parent.mkdir(parents=True, exist_ok=True) + try: + link.symlink_to(outside_file) + except OSError: + pytest.skip("Symlinks not supported") + + result = _patch_skill("my-skill", "old text", "new text", file_path="references/evil.md") + + assert result["success"] is False + assert "boundary" in result["error"].lower() + assert outside_file.read_text() == "old text here" + class TestDeleteSkill: def test_delete_existing(self, tmp_path): @@ -375,6 +396,25 @@ class TestWriteFile: result = _write_file("my-skill", "secret/evil.py", "malicious") assert result["success"] is False + def test_write_symlink_escape_blocked(self, tmp_path): + outside_dir = tmp_path / "outside" + outside_dir.mkdir() + + with _skill_dir(tmp_path): + _create_skill("my-skill", VALID_SKILL_CONTENT) + link = tmp_path / "my-skill" / "references" / "escape" + link.parent.mkdir(parents=True, exist_ok=True) + try: + link.symlink_to(outside_dir, target_is_directory=True) + except OSError: + pytest.skip("Symlinks not supported") + + result = _write_file("my-skill", "references/escape/owned.md", "malicious") + + assert result["success"] is False + assert "boundary" in result["error"].lower() + assert not (outside_dir / "owned.md").exists() + class TestRemoveFile: def test_remove_existing_file(self, tmp_path): @@ -391,6 +431,27 @@ class TestRemoveFile: result = _remove_file("my-skill", "references/nope.md") assert result["success"] is False + def test_remove_symlink_escape_blocked(self, tmp_path): + outside_dir = tmp_path / "outside" + outside_dir.mkdir() + outside_file = outside_dir / "keep.txt" + outside_file.write_text("content") + + with _skill_dir(tmp_path): + _create_skill("my-skill", VALID_SKILL_CONTENT) + link = tmp_path / "my-skill" / "references" / "escape" + link.parent.mkdir(parents=True, exist_ok=True) + try: + link.symlink_to(outside_dir, target_is_directory=True) + except OSError: + pytest.skip("Symlinks not supported") + + result = _remove_file("my-skill", "references/escape/keep.txt") + + assert result["success"] is False + assert "boundary" in result["error"].lower() + assert outside_file.exists() + # --------------------------------------------------------------------------- # skill_manage dispatcher diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 97a4bf5aa5..8a513c69d1 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -40,7 +40,7 @@ import shutil import tempfile from pathlib import Path from hermes_constants import get_hermes_home -from typing import Dict, Any, Optional +from typing import Dict, Any, Optional, Tuple logger = logging.getLogger(__name__) @@ -240,6 +240,20 @@ def _validate_file_path(file_path: str) -> Optional[str]: return None +def _resolve_skill_target(skill_dir: Path, file_path: str) -> Tuple[Optional[Path], Optional[str]]: + """Resolve a supporting-file path and ensure it stays within the skill directory.""" + target = skill_dir / file_path + try: + resolved = target.resolve(strict=False) + skill_dir_resolved = skill_dir.resolve() + resolved.relative_to(skill_dir_resolved) + except ValueError: + return None, "Path escapes skill directory boundary." + except OSError as e: + return None, f"Invalid file path '{file_path}': {e}" + return target, None + + def _atomic_write_text(file_path: Path, content: str, encoding: str = "utf-8") -> None: """ Atomically write text content to a file. @@ -394,7 +408,9 @@ def _patch_skill( err = _validate_file_path(file_path) if err: return {"success": False, "error": err} - target = skill_dir / file_path + target, err = _resolve_skill_target(skill_dir, file_path) + if err: + return {"success": False, "error": err} else: # Patching SKILL.md target = skill_dir / "SKILL.md" @@ -500,7 +516,9 @@ def _write_file(name: str, file_path: str, file_content: str) -> Dict[str, Any]: if not existing: return {"success": False, "error": f"Skill '{name}' not found. Create it first with action='create'."} - target = existing["path"] / file_path + target, err = _resolve_skill_target(existing["path"], file_path) + if err: + return {"success": False, "error": err} target.parent.mkdir(parents=True, exist_ok=True) # Back up for rollback original_content = target.read_text(encoding="utf-8") if target.exists() else None @@ -533,7 +551,9 @@ def _remove_file(name: str, file_path: str) -> Dict[str, Any]: return {"success": False, "error": f"Skill '{name}' not found."} skill_dir = existing["path"] - target = skill_dir / file_path + target, err = _resolve_skill_target(skill_dir, file_path) + if err: + return {"success": False, "error": err} if not target.exists(): # List what's actually there for the model to see available = []