diff --git a/tests/tools/test_skill_view_traversal.py b/tests/tools/test_skill_view_traversal.py new file mode 100644 index 0000000000000..fea2096296f5f --- /dev/null +++ b/tests/tools/test_skill_view_traversal.py @@ -0,0 +1,58 @@ +"""Tests for path traversal prevention in skill_view.""" + +import json +import sys +import types +import pytest +from pathlib import Path +from unittest.mock import patch + +# Stub out heavy optional dependencies so tools/__init__.py doesn't blow up +from unittest.mock import MagicMock +for mod_name in ("firecrawl", "fal_client", "tavily"): + sys.modules.setdefault(mod_name, MagicMock()) + +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-12345") + + 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): + 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): + 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): + 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): + result = json.loads(skill_view("test-skill")) + assert result["success"] is True diff --git a/tools/skills_tool.py b/tools/skills_tool.py index a0121f30c761f..eb1cda2cca57f 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -443,7 +443,19 @@ 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: - target_file = skill_dir / file_path + # Prevent path traversal + normalized = Path(file_path) + if ".." in normalized.parts: + return json.dumps({ + "success": False, + "error": "Path traversal ('..') is not allowed.", + }, ensure_ascii=False) + target_file = (skill_dir / file_path).resolve() + if not str(target_file).startswith(str(skill_dir.resolve())): + return json.dumps({ + "success": False, + "error": "Path traversal is not allowed.", + }, ensure_ascii=False) if not target_file.exists(): # List available files in the skill directory, organized by type available_files = {