From 56f833efa427ccb444c0f9ad1759af1012f2124d Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 6 Jun 2026 18:29:52 -0700 Subject: [PATCH] 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 --- tests/tools/test_skill_view_traversal.py | 38 ++++++++++++++++ tools/skills_tool.py | 58 +++++++++++++++++++++++- 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_skill_view_traversal.py b/tests/tools/test_skill_view_traversal.py index 426ebb111b7..cfa23e077eb 100644 --- a/tests/tools/test_skill_view_traversal.py +++ b/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")) diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 04bf35abe69..48274ccfec5 100644 --- a/tools/skills_tool.py +++ b/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():