fix(security): prevent path traversal in skill_view

tests/tools/test_skills_tool_path_traversal.py

@@ -0,0 +1,167 @@
+"""Tests for skill_view path traversal prevention.
+
+Verifies that skill_view blocks path traversal attempts that could
+expose sensitive files like ~/.hermes/.env or ~/.ssh/id_rsa.
+
+Fixes: https://github.com/NousResearch/hermes-agent/issues/220
+"""
+
+import json
+import pytest
+from pathlib import Path
+from unittest.mock import patch, MagicMock
+
+
+class TestSkillViewPathTraversal:
+    """Test path traversal prevention in skill_view."""
+
+    def test_double_dot_blocked(self):
+        """Path traversal with .. components should be rejected."""
+        from tools.skills_tool import skill_view
+
+        # Mock _find_all_skills to return a fake skill
+        with patch('tools.skills_tool._find_all_skills') as mock_find:
+            mock_find.return_value = [{
+                "name": "test-skill",
+                "md": Path("/tmp/skills/test-skill/SKILL.md"),
+                "dir": Path("/tmp/skills/test-skill")
+            }]
+
+            # Attempt path traversal to read .env
+            result = skill_view("test-skill", file_path="../../.env")
+            data = json.loads(result)
+
+            assert data["success"] is False
+            assert "traversal" in data["error"].lower()
+
+    def test_double_dot_nested_blocked(self):
+        """Nested path traversal should also be rejected."""
+        from tools.skills_tool import skill_view
+
+        with patch('tools.skills_tool._find_all_skills') as mock_find:
+            mock_find.return_value = [{
+                "name": "test-skill",
+                "md": Path("/tmp/skills/test-skill/SKILL.md"),
+                "dir": Path("/tmp/skills/test-skill")
+            }]
+
+            # Try deeper traversal
+            result = skill_view("test-skill", file_path="references/../../../etc/passwd")
+            data = json.loads(result)
+
+            assert data["success"] is False
+            assert "traversal" in data["error"].lower()
+
+    def test_valid_path_allowed(self):
+        """Valid relative paths within skill directory should work."""
+        from tools.skills_tool import skill_view
+        import tempfile
+        import os
+
+        # Create a temporary skill directory with a file
+        with tempfile.TemporaryDirectory() as tmpdir:
+            skill_dir = Path(tmpdir) / "test-skill"
+            skill_dir.mkdir()
+
+            skill_md = skill_dir / "SKILL.md"
+            skill_md.write_text("# Test Skill")
+
+            ref_dir = skill_dir / "references"
+            ref_dir.mkdir()
+            ref_file = ref_dir / "test.md"
+            ref_file.write_text("Test reference content")
+
+            with patch('tools.skills_tool._find_all_skills') as mock_find:
+                mock_find.return_value = [{
+                    "name": "test-skill",
+                    "md": skill_md,
+                    "dir": skill_dir
+                }]
+
+                result = skill_view("test-skill", file_path="references/test.md")
+                data = json.loads(result)
+
+                assert data["success"] is True
+                assert data["content"] == "Test reference content"
+
+    def test_symlink_escape_blocked(self):
+        """Symlinks that escape the skill directory should be blocked."""
+        from tools.skills_tool import skill_view
+        import tempfile
+        import os
+
+        with tempfile.TemporaryDirectory() as tmpdir:
+            skill_dir = Path(tmpdir) / "test-skill"
+            skill_dir.mkdir()
+
+            skill_md = skill_dir / "SKILL.md"
+            skill_md.write_text("# Test Skill")
+
+            # Create a symlink pointing outside the skill directory
+            secret_file = Path(tmpdir) / "secret.txt"
+            secret_file.write_text("SECRET DATA")
+
+            symlink = skill_dir / "evil-link"
+            try:
+                symlink.symlink_to(secret_file)
+            except OSError:
+                pytest.skip("Symlinks not supported on this system")
+
+            with patch('tools.skills_tool._find_all_skills') as mock_find:
+                mock_find.return_value = [{
+                    "name": "test-skill",
+                    "md": skill_md,
+                    "dir": skill_dir
+                }]
+
+                result = skill_view("test-skill", file_path="evil-link")
+                data = json.loads(result)
+
+                # Should either fail or be blocked
+                # The resolve() check should catch this
+                if data["success"]:
+                    # If it succeeded, the content should NOT be the secret
+                    assert data.get("content") != "SECRET DATA"
+
+
+class TestSkillViewEdgeCases:
+    """Edge cases for path validation."""
+
+    def test_absolute_path_rejected(self):
+        """Absolute paths should not escape the skill directory."""
+        from tools.skills_tool import skill_view
+
+        with patch('tools.skills_tool._find_all_skills') as mock_find:
+            mock_find.return_value = [{
+                "name": "test-skill",
+                "md": Path("/tmp/skills/test-skill/SKILL.md"),
+                "dir": Path("/tmp/skills/test-skill")
+            }]
+
+            # Try absolute path
+            result = skill_view("test-skill", file_path="/etc/passwd")
+            data = json.loads(result)
+
+            # Should fail - either not found or blocked
+            # (Path("/tmp/skills/test-skill") / "/etc/passwd" normalizes weirdly on some systems)
+            assert data["success"] is False or "passwd" not in data.get("content", "")
+
+    def test_url_encoded_traversal_blocked(self):
+        """URL-encoded path traversal should be blocked."""
+        from tools.skills_tool import skill_view
+
+        with patch('tools.skills_tool._find_all_skills') as mock_find:
+            mock_find.return_value = [{
+                "name": "test-skill",
+                "md": Path("/tmp/skills/test-skill/SKILL.md"),
+                "dir": Path("/tmp/skills/test-skill")
+            }]
+
+            # %2e%2e is URL encoding for ..
+            # Python's pathlib will NOT decode this, so it should be safe
+            # But we test anyway to ensure the behavior is correct
+            result = skill_view("test-skill", file_path="%2e%2e/.env")
+            data = json.loads(result)
+
+            # Should fail (file not found since %2e%2e is literal)
+            assert data["success"] is False

tools/skills_tool.py

@@ -443,7 +443,34 @@ def skill_view(name: str, file_path: str = None, task_id: str = None) -> str:
 
         # If a specific file path is requested, read that instead
         if file_path and skill_dir:
+            # Security: Prevent path traversal attacks
+            normalized_path = Path(file_path)
+            if ".." in normalized_path.parts:
+                return json.dumps({
+                    "success": False,
+                    "error": "Path traversal ('..') is not allowed.",
+                    "hint": "Use a relative path within the skill directory"
+                }, ensure_ascii=False)
+
             target_file = skill_dir / file_path
+
+            # Security: Verify resolved path is still within skill directory
+            try:
+                resolved = target_file.resolve()
+                skill_dir_resolved = skill_dir.resolve()
+                if not str(resolved).startswith(str(skill_dir_resolved) + "/") and resolved != skill_dir_resolved:
+                    return json.dumps({
+                        "success": False,
+                        "error": "Path escapes skill directory boundary.",
+                        "hint": "Use a relative path within the skill directory"
+                    }, ensure_ascii=False)
+            except (OSError, ValueError):
+                return json.dumps({
+                    "success": False,
+                    "error": f"Invalid file path: '{file_path}'",
+                    "hint": "Use a valid relative path within the skill directory"
+                }, ensure_ascii=False)
+
             if not target_file.exists():
                 # List available files in the skill directory, organized by type
                 available_files = {