From 1cb2311bad5d10ce7de66f6c0ac5e91956a3ce34 Mon Sep 17 00:00:00 2001 From: teknium1 Date: Mon, 2 Mar 2026 02:00:09 -0800 Subject: [PATCH] fix(security): block path traversal in skill_view file_path (fixes #220) skill_view accepted arbitrary file_path values like '../../.env' and would read files outside the skill directory, exposing API keys and other sensitive data. Added two layers of defense: 1. Reject paths with '..' components (fast, catches obvious traversal) 2. resolve() containment check with trailing '/' to prevent prefix collisions (catches symlinks and edge cases) Fix approach from PR #242 (@Bartok9). Vulnerability reported by @Farukest (#220, PR #221). Tests rewritten to properly mock SKILLS_DIR. Closes #220 --- tests/tools/test_skill_view_traversal.py | 83 ++++++++++++++++++++++++ tools/skills_tool.py | 26 ++++++++ 2 files changed, 109 insertions(+) create mode 100644 tests/tools/test_skill_view_traversal.py diff --git a/tests/tools/test_skill_view_traversal.py b/tests/tools/test_skill_view_traversal.py new file mode 100644 index 00000000..55d84d8c --- /dev/null +++ b/tests/tools/test_skill_view_traversal.py @@ -0,0 +1,83 @@ +"""Tests for path traversal prevention in skill_view. + +Regression tests for issue #220: skill_view file_path parameter allowed +reading arbitrary files (e.g., ~/.hermes/.env) via path traversal. +""" + +import json +import pytest +from pathlib import Path +from unittest.mock import patch + +from tools.skills_tool import skill_view + + +@pytest.fixture() +def fake_skills(tmp_path): + """Create a fake skills directory with one skill and a sensitive file outside.""" + skills_dir = tmp_path / "skills" + skill_dir = skills_dir / "test-skill" + skill_dir.mkdir(parents=True) + + # Create SKILL.md + (skill_dir / "SKILL.md").write_text("# Test Skill\nA test skill.") + + # Create a legitimate file inside the skill + refs = skill_dir / "references" + refs.mkdir() + (refs / "api.md").write_text("API docs here") + + # Create a sensitive file outside skills dir (simulating .env) + (tmp_path / ".env").write_text("SECRET_API_KEY=sk-do-not-leak") + + with patch("tools.skills_tool.SKILLS_DIR", skills_dir): + yield {"skills_dir": skills_dir, "skill_dir": skill_dir, "tmp_path": tmp_path} + + +class TestPathTraversalBlocked: + def test_dotdot_in_file_path(self, fake_skills): + """Direct .. traversal should be rejected.""" + result = json.loads(skill_view("test-skill", file_path="../../.env")) + assert result["success"] is False + assert "traversal" in result["error"].lower() + + def test_dotdot_nested(self, fake_skills): + """Nested .. traversal should also be rejected.""" + result = json.loads(skill_view("test-skill", file_path="references/../../../.env")) + assert result["success"] is False + assert "traversal" in result["error"].lower() + + def test_legitimate_file_still_works(self, fake_skills): + """Valid paths within the skill directory should work normally.""" + result = json.loads(skill_view("test-skill", file_path="references/api.md")) + assert result["success"] is True + assert "API docs here" in result["content"] + + def test_no_file_path_shows_skill(self, fake_skills): + """Calling skill_view without file_path should return the SKILL.md.""" + result = json.loads(skill_view("test-skill")) + assert result["success"] is True + + def test_symlink_escape_blocked(self, fake_skills): + """Symlinks pointing outside the skill directory should be blocked.""" + skill_dir = fake_skills["skill_dir"] + secret = fake_skills["tmp_path"] / "secret.txt" + secret.write_text("TOP SECRET DATA") + + symlink = skill_dir / "evil-link" + try: + symlink.symlink_to(secret) + except OSError: + pytest.skip("Symlinks not supported") + + result = json.loads(skill_view("test-skill", file_path="evil-link")) + # The resolve() check should catch the symlink escaping + assert result["success"] is False + assert "escapes" in result["error"].lower() or "boundary" in result["error"].lower() + + def test_sensitive_file_not_leaked(self, fake_skills): + """Even if traversal somehow passes, sensitive content must not leak.""" + result = json.loads(skill_view("test-skill", file_path="../../.env")) + assert result["success"] is False + assert "sk-do-not-leak" not in result.get("content", "") + assert "sk-do-not-leak" not in json.dumps(result) diff --git a/tools/skills_tool.py b/tools/skills_tool.py index a0121f30..f118b203 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -443,7 +443,33 @@ 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 = {