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

Closes #38643.

Salvaged from #40521; cleaned up, re-verified against main, tests added.

Co-authored-by: xy200303 <xy200303@users.noreply.github.com>

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=do-not-leak")
+
+        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=do-not-leak")
+
+        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 @@ from hermes_constants import get_hermes_home, display_hermes_home
 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 @@ _REMOTE_ENV_BACKENDS = frozenset(
 _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():