diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index 97cf3e9d194..245ca934757 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -957,3 +957,74 @@ class TestPinnedGuard: side_effect=RuntimeError("sidecar broken")): result = _delete_skill("my-skill") assert result["success"] is True + + +# --------------------------------------------------------------------------- +# _delete_skill — recursive-delete safety (port of Kilo Code #11240) +# --------------------------------------------------------------------------- + + +class TestDeleteSkillRmtreeGuard: + """Defense-in-depth before ``shutil.rmtree`` in ``_delete_skill``. + + Mirrors the Kilo Code #11227 fix: never let a recursive skill delete + escape the skills tree, target a skills root, or follow a symlink. + """ + + def test_normal_delete_still_works(self, tmp_path): + with _skill_dir(tmp_path): + _create_skill("good-skill", VALID_SKILL_CONTENT) + result = _delete_skill("good-skill", absorbed_into="") + assert result["success"] is True, result + assert not (tmp_path / "good-skill").exists() + + def test_symlinked_skill_dir_refused(self, tmp_path): + """A skill dir that is a symlink must not be rmtree'd — rmtree would + otherwise follow it and delete the link target's contents.""" + victim = tmp_path.parent / "precious_victim" + victim.mkdir() + (victim / "important.txt").write_text("DO NOT DELETE") + skills = tmp_path / "skills" + skills.mkdir() + evil = skills / "evil-skill" + evil.symlink_to(victim, target_is_directory=True) + try: + with patch("tools.skill_manager_tool.SKILLS_DIR", skills), \ + patch("agent.skill_utils.get_all_skills_dirs", return_value=[skills]), \ + patch("tools.skill_manager_tool._find_skill", + return_value={"path": evil}): + result = _delete_skill("evil-skill", absorbed_into="") + assert result["success"] is False + assert "symlink" in result["error"].lower() + assert (victim / "important.txt").exists() + finally: + import shutil as _sh + _sh.rmtree(victim, ignore_errors=True) + + def test_skills_root_itself_refused(self, tmp_path): + """If discovery ever hands back the skills root, refuse — rmtree would + wipe every installed skill.""" + with patch("tools.skill_manager_tool.SKILLS_DIR", tmp_path), \ + patch("agent.skill_utils.get_all_skills_dirs", return_value=[tmp_path]), \ + patch("tools.skill_manager_tool._find_skill", + return_value={"path": tmp_path}): + result = _delete_skill("root-attack", absorbed_into="") + assert result["success"] is False + assert "skills root" in result["error"].lower() + assert tmp_path.exists() + + def test_out_of_tree_path_refused(self, tmp_path): + """A path that resolves outside every known skills root is refused.""" + skills = tmp_path / "skills" + skills.mkdir() + outside = tmp_path / "outside_skill" + outside.mkdir() + (outside / "SKILL.md").write_text("x") + with patch("tools.skill_manager_tool.SKILLS_DIR", skills), \ + patch("agent.skill_utils.get_all_skills_dirs", return_value=[skills]), \ + patch("tools.skill_manager_tool._find_skill", + return_value={"path": outside}): + result = _delete_skill("outside", absorbed_into="") + assert result["success"] is False + assert "skills root" in result["error"].lower() + assert outside.exists() diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 4f96b171e21..020c3a0155a 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -134,6 +134,80 @@ def _containing_skills_root(skill_path: Path) -> Path: return SKILLS_DIR +def _is_path_redirect(path: Path) -> bool: + """True when ``path`` is a symlink or (on Windows) a directory junction. + + Either form lets a poisoned skills tree redirect a subsequent + ``shutil.rmtree`` to content outside the skills root. ``is_junction`` + only exists on Python 3.12+ Windows; gate with ``hasattr``. + """ + try: + return path.is_symlink() or (hasattr(path, "is_junction") and path.is_junction()) + except OSError: + return False + + +def _validate_delete_target(skill_dir: Path) -> Optional[str]: + """Last-line guard before ``shutil.rmtree(skill_dir)`` in ``_delete_skill``. + + ``_find_skill`` already restricts ``skill_dir`` to a real ``SKILL.md`` + parent discovered by walking the skills roots, so the agent cannot inject + an arbitrary path the way Kilo Code's HTTP endpoint could (their issue + #11227: a built-in-skill sentinel resolved to the server cwd and a + recursive delete wiped the user's entire working directory). This is the + matching defense-in-depth for our agent-facing ``skill_manage`` delete + path: even if discovery or a poisoned tree hands us a bad directory, never + recursively delete + + 1. a path that is not strictly *inside* one of the known skills roots, + 2. a skills root itself (would wipe every installed skill), or + 3. a directory reached via a symlink / junction (``rmtree`` would follow + it into content outside the skills tree). + + Returns an error string to refuse on, or ``None`` when the delete is safe. + """ + from agent.skill_utils import get_all_skills_dirs + + # (3) Reject symlink/junction redirects on the skill directory itself. + if _is_path_redirect(skill_dir): + return ( + f"Refusing to delete '{skill_dir}': the skill directory is a " + f"symlink/junction. Remove the link target manually if intended." + ) + + try: + resolved = skill_dir.resolve() + except OSError as exc: + return f"Refusing to delete '{skill_dir}': could not resolve path ({exc})." + + roots = [] + for root in get_all_skills_dirs(): + try: + roots.append(root.resolve()) + except OSError: + continue + + for root in roots: + # (2) Never rmtree a skills root itself. + if resolved == root: + return ( + f"Refusing to delete '{skill_dir}': resolves to the skills root " + f"itself, which would remove every installed skill." + ) + # (1) Must be strictly inside a known root. + try: + rel = resolved.relative_to(root) + except ValueError: + continue + if rel.parts: # at least one component below the root + return None + + return ( + f"Refusing to delete '{skill_dir}': path does not resolve inside any " + f"known skills root." + ) + + def _pinned_guard(name: str) -> Optional[str]: """Return a refusal message if *name* is pinned, else None. @@ -706,6 +780,12 @@ def _delete_skill(name: str, absorbed_into: Optional[str] = None) -> Dict[str, A skill_dir = existing["path"] skills_root = _containing_skills_root(skill_dir) + + # Defense-in-depth before the recursive delete (port of Kilo Code #11240). + unsafe = _validate_delete_target(skill_dir) + if unsafe: + return {"success": False, "error": unsafe} + shutil.rmtree(skill_dir) # Clean up empty category directories (don't remove the skills root itself)