fix(skills): block category path traversal in skill manager (#3844)
Validate category names in _create_skill() before using them as filesystem path segments. Previously, categories like '../escape' or '/tmp/pwned' could write skill files outside ~/.hermes/skills/. Adds _validate_category() that rejects slashes, backslashes, absolute paths, and non-alphanumeric characters (reuses existing VALID_NAME_RE). Tests: 5 new tests for traversal, absolute paths, and valid categories. Salvaged from PR #1939 by Gutslabs.
This commit is contained in:
@@ -6,6 +6,7 @@ from unittest.mock import patch
|
|||||||
|
|
||||||
from tools.skill_manager_tool import (
|
from tools.skill_manager_tool import (
|
||||||
_validate_name,
|
_validate_name,
|
||||||
|
_validate_category,
|
||||||
_validate_frontmatter,
|
_validate_frontmatter,
|
||||||
_validate_file_path,
|
_validate_file_path,
|
||||||
_find_skill,
|
_find_skill,
|
||||||
@@ -82,6 +83,22 @@ class TestValidateName:
|
|||||||
assert "Invalid skill name 'skill@name'" in err
|
assert "Invalid skill name 'skill@name'" in err
|
||||||
|
|
||||||
|
|
||||||
|
class TestValidateCategory:
|
||||||
|
def test_valid_categories(self):
|
||||||
|
assert _validate_category(None) is None
|
||||||
|
assert _validate_category("") is None
|
||||||
|
assert _validate_category("devops") is None
|
||||||
|
assert _validate_category("mlops-v2") is None
|
||||||
|
|
||||||
|
def test_path_traversal_rejected(self):
|
||||||
|
err = _validate_category("../escape")
|
||||||
|
assert "Invalid category '../escape'" in err
|
||||||
|
|
||||||
|
def test_absolute_path_rejected(self):
|
||||||
|
err = _validate_category("/tmp/escape")
|
||||||
|
assert "Invalid category '/tmp/escape'" in err
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# _validate_frontmatter
|
# _validate_frontmatter
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
@@ -191,6 +208,29 @@ class TestCreateSkill:
|
|||||||
result = _create_skill("my-skill", "no frontmatter here")
|
result = _create_skill("my-skill", "no frontmatter here")
|
||||||
assert result["success"] is False
|
assert result["success"] is False
|
||||||
|
|
||||||
|
def test_create_rejects_category_traversal(self, tmp_path):
|
||||||
|
skills_dir = tmp_path / "skills"
|
||||||
|
skills_dir.mkdir()
|
||||||
|
|
||||||
|
with patch("tools.skill_manager_tool.SKILLS_DIR", skills_dir):
|
||||||
|
result = _create_skill("my-skill", VALID_SKILL_CONTENT, category="../escape")
|
||||||
|
|
||||||
|
assert result["success"] is False
|
||||||
|
assert "Invalid category '../escape'" in result["error"]
|
||||||
|
assert not (tmp_path / "escape").exists()
|
||||||
|
|
||||||
|
def test_create_rejects_absolute_category(self, tmp_path):
|
||||||
|
skills_dir = tmp_path / "skills"
|
||||||
|
skills_dir.mkdir()
|
||||||
|
outside = tmp_path / "outside"
|
||||||
|
|
||||||
|
with patch("tools.skill_manager_tool.SKILLS_DIR", skills_dir):
|
||||||
|
result = _create_skill("my-skill", VALID_SKILL_CONTENT, category=str(outside))
|
||||||
|
|
||||||
|
assert result["success"] is False
|
||||||
|
assert f"Invalid category '{outside}'" in result["error"]
|
||||||
|
assert not (outside / "my-skill" / "SKILL.md").exists()
|
||||||
|
|
||||||
|
|
||||||
class TestEditSkill:
|
class TestEditSkill:
|
||||||
def test_edit_existing_skill(self, tmp_path):
|
def test_edit_existing_skill(self, tmp_path):
|
||||||
|
|||||||
@@ -113,6 +113,31 @@ def _validate_name(name: str) -> Optional[str]:
|
|||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
||||||
|
def _validate_category(category: Optional[str]) -> Optional[str]:
|
||||||
|
"""Validate an optional category name used as a single directory segment."""
|
||||||
|
if category is None:
|
||||||
|
return None
|
||||||
|
if not isinstance(category, str):
|
||||||
|
return "Category must be a string."
|
||||||
|
|
||||||
|
category = category.strip()
|
||||||
|
if not category:
|
||||||
|
return None
|
||||||
|
if "/" in category or "\\" in category:
|
||||||
|
return (
|
||||||
|
f"Invalid category '{category}'. Use lowercase letters, numbers, "
|
||||||
|
"hyphens, dots, and underscores. Categories must be a single directory name."
|
||||||
|
)
|
||||||
|
if len(category) > MAX_NAME_LENGTH:
|
||||||
|
return f"Category exceeds {MAX_NAME_LENGTH} characters."
|
||||||
|
if not VALID_NAME_RE.match(category):
|
||||||
|
return (
|
||||||
|
f"Invalid category '{category}'. Use lowercase letters, numbers, "
|
||||||
|
"hyphens, dots, and underscores. Categories must be a single directory name."
|
||||||
|
)
|
||||||
|
return None
|
||||||
|
|
||||||
|
|
||||||
def _validate_frontmatter(content: str) -> Optional[str]:
|
def _validate_frontmatter(content: str) -> Optional[str]:
|
||||||
"""
|
"""
|
||||||
Validate that SKILL.md content has proper frontmatter with required fields.
|
Validate that SKILL.md content has proper frontmatter with required fields.
|
||||||
@@ -241,6 +266,10 @@ def _create_skill(name: str, content: str, category: str = None) -> Dict[str, An
|
|||||||
if err:
|
if err:
|
||||||
return {"success": False, "error": err}
|
return {"success": False, "error": err}
|
||||||
|
|
||||||
|
err = _validate_category(category)
|
||||||
|
if err:
|
||||||
|
return {"success": False, "error": err}
|
||||||
|
|
||||||
# Validate content
|
# Validate content
|
||||||
err = _validate_frontmatter(content)
|
err = _validate_frontmatter(content)
|
||||||
if err:
|
if err:
|
||||||
|
|||||||
Reference in New Issue
Block a user