Path traversal vulnerability fixes in skill_view command

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #40566 fix(skills): block path traversal via skill_view name argument

  merged to main

  files: tests/tools/test_skill_view_traversal.py, tools/skills_tool.py

  Show the diff

  • tests/tools/test_skill_view_traversal.py

    @@ -34,6 +34,44 @@ def fake_skills(tmp_path):
     
     
     class TestPathTraversalBlocked:
    +    def test_dotdot_in_skill_name_blocked(self, fake_skills):
    +        """A traversal skill name must not escape the skills search root.
    +
    +        Regression: `name` was joined onto each search dir to build the lookup
    +        path with no `..`/absolute guard (while file_path WAS validated), so
    +        name="../outside-skill" could select a sibling dir outside SKILLS_DIR.
    +        """
    +        tmp_path = fake_skills["tmp_path"]
    +        outside_skill = tmp_path / "outside-skill"
    +        outside_skill.mkdir()
    +        (outside_skill / "SKILL.md").write_text("# Outside Skill\n")
    +        (outside_skill / ".env").write_text("ESCAPED_SECRET=[redacted:secret-assignment]")
    +
    +        result = json.loads(skill_view("../outside-skill", file_path=".env"))
    +
    +        assert result["success"] is False
    +        assert "traversal" in result["error"].lower()
    +        assert "do-not-leak" not in json.dumps(result)
    +
    +    def test_absolute_skill_name_blocked(self, fake_skills):
    +        """An absolute skill name must not bypass the trusted search root."""
    +        tmp_path = fake_skills["tmp_path"]
    +        outside_skill = tmp_path / "outside-absolute"
    +        outside_skill.mkdir()
    +        (outside_skill / "SKILL.md").write_text("# Outside Absolute\n")
    +        (outside_skill / ".env").write_text("ABSOLUTE_SECRET=[redacted:secret-assignment]")
    +
    +        result = json.loads(skill_view(str(outside_skill), file_path=".env"))
    +
    +        assert result["success"] is False
    +        assert "relative path" in result["error"].lower()
    +        assert "do-not-leak" not in json.dumps(result)
    +
    +    def test_legit_skill_name_still_works(self, fake_skills):
    +        """A normal skill name must still resolve after the name guard."""
    +        result = json.loads(skill_view("test-skill"))
    +        assert result["success"] is True
    +
         def test_dotdot_in_file_path(self, fake_skills):
             """Direct .. traversal should be rejected."""
             result = json.loads(skill_view("test-skill", file_path="../../.env"))

  • tools/skills_tool.py

    @@ -73,7 +73,7 @@
     import os
     import re
     from enum import Enum
    -from pathlib import Path
    +from pathlib import Path, PurePosixPath, PureWindowsPath
     from typing import Dict, Any, List, Optional, Set, Tuple
     
     from tools.registry import registry, tool_error
    @@ -108,6 +108,33 @@
     _secret_capture_callback = None
     
     
    +def _skill_lookup_path_error(name: str) -> Optional[str]:
    +    """Return an error if a local skill lookup *name* can escape search roots.
    +
    +    The skill ``name`` is joined onto each trusted search dir to build the
    +    on-disk lookup path, so it must stay relative and free of ``..`` segments —
    +    otherwise ``name="../outside"`` or an absolute path could select a skill
    +    (and read files) outside the skills directory. Mirrors the ``file_path``
    +    validation done later via ``tools.path_security``. We also reject Windows
    +    drive paths (e.g. ``C:\\skills``), whose ``:`` would otherwise be misread as
    +    a plugin namespace separator.
    +    """
    +    from tools.path_security import has_traversal_component
    +
    +    if not isinstance(name, str):
    +        return "Skill name must be a string."
    +    candidate = name.strip()
    +    if (
    +        PurePosixPath(candidate).is_absolute()
    +        or PureWindowsPath(candidate).is_absolute()
    +        or PureWindowsPath(candidate).drive
    +    ):
    +        return "Skill name must be a relative path within the skills directory."
    +    if has_traversal_component(candidate):
    +        return "Skill name cannot contain '..' path traversal components."
    +    return None
    +
    +
     def load_env() -> Dict[str, str]:
         """Load profile-scoped environment variables from HERMES_HOME/.env."""
         env_path = get_hermes_home() / ".env"
    @@ -847,6 +874,21 @@ def skill_view(
             JSON string with skill content or error message
         """
         try:
    +        # Validate before the ':' qualified-name dispatch so a Windows drive
    +        # path (e.g. C:\skills\foo) can't be reinterpreted as a plugin
    +        # namespace, and so a traversal/absolute name never reaches the
    +        # search-dir join that builds direct_path below.
    +        lookup_error = _skill_lookup_path_error(name)
    +        if lookup_error:
    +            return json.dumps(
    +                {
    +                    "success": False,
    +                    "error": lookup_error,
    +                    "hint": "Use a skill name or relative path within the skills directory.",
    +                },
    +                ensure_ascii=False,
    +            )
    +
             local_category_name: str | None = None
             # ── Qualified name dispatch (plugin skills) ──────────────────
             # Names containing ':' are routed to the plugin skill registry.
    @@ -917,6 +959,20 @@ def skill_view(
     
             from agent.skill_utils import get_external_skills_dirs
     
    +        # The categorized fall-through form (namespace/bare) joins onto each
    +        # search dir too; re-validate it since `bare` is not namespace-checked.
    +        if local_category_name:
    +            lookup_error = _skill_lookup_path_error(local_category_name)
    +            if lookup_error:
    +                return json.dumps(
    +                    {
    +                        "success": False,
    +                        "error": lookup_error,
    +                        "hint": "Use a skill name or relative path within the skills directory.",
    +                    },
    +                    ensure_ascii=False,
    +                )
    +
             # Build list of all skill directories to search
             all_dirs = []
             if SKILLS_DIR.exists():