security: Fix V-006 MCP OAuth Deserialization (CVSS 8.8 CRITICAL)

- Replace pickle with JSON + HMAC-SHA256 state serialization
- Add constant-time signature verification
- Implement replay attack protection with nonce expiration
- Add comprehensive security test suite (54 tests)
- Harden token storage with integrity verification

Resolves: V-006 (CVSS 8.8)

tests/agent/test_skill_security.py

@@ -0,0 +1,391 @@
+"""Security tests for skill loading and validation.
+
+Tests for V-011: Skills Guard Bypass via Path Traversal
+Ensures skill names are properly validated to prevent path traversal attacks.
+"""
+
+import json
+import pytest
+from pathlib import Path
+from unittest.mock import patch
+
+from agent.skill_security import (
+    validate_skill_name,
+    resolve_skill_path,
+    sanitize_skill_identifier,
+    is_safe_skill_path,
+    SkillSecurityError,
+    PathTraversalError,
+    InvalidSkillNameError,
+    VALID_SKILL_NAME_PATTERN,
+    MAX_SKILL_NAME_LENGTH,
+)
+
+
+class TestValidateSkillName:
+    """Tests for validate_skill_name function."""
+
+    def test_valid_simple_name(self):
+        """Simple alphanumeric names should be valid."""
+        validate_skill_name("my-skill")  # Should not raise
+        validate_skill_name("my_skill")  # Should not raise
+        validate_skill_name("mySkill")   # Should not raise
+        validate_skill_name("skill123")  # Should not raise
+
+    def test_valid_with_path_separator(self):
+        """Names with path separators should be valid when allowed."""
+        validate_skill_name("mlops/axolotl", allow_path_separator=True)
+        validate_skill_name("category/my-skill", allow_path_separator=True)
+
+    def test_valid_with_dots(self):
+        """Names with dots should be valid."""
+        validate_skill_name("skill.v1")
+        validate_skill_name("my.skill.name")
+
+    def test_invalid_path_traversal_dotdot(self):
+        """Path traversal with .. should be rejected."""
+        # When path separator is NOT allowed, '/' is rejected by character validation first
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("../../../etc/passwd")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("../secret")
+        # When path separator IS allowed, '..' is caught by traversal check
+        with pytest.raises(PathTraversalError):
+            validate_skill_name("skill/../../etc/passwd", allow_path_separator=True)
+
+    def test_invalid_absolute_path(self):
+        """Absolute paths should be rejected (by character validation or traversal check)."""
+        # '/' is not in the allowed character set, so InvalidSkillNameError is raised
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("/etc/passwd")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("/root/.ssh/id_rsa")
+
+    def test_invalid_home_directory(self):
+        """Home directory expansion should be rejected (by character validation)."""
+        # '~' is not in the allowed character set
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("~/.hermes/.env")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("~root/.bashrc")
+
+    def test_invalid_protocol_handlers(self):
+        """Protocol handlers should be rejected (by character validation)."""
+        # ':' and '/' are not in the allowed character set
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("file:///etc/passwd")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("http://evil.com/skill")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("https://evil.com/skill")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("javascript:alert(1)")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("data:text/plain,evil")
+
+    def test_invalid_windows_path(self):
+        """Windows-style paths should be rejected (by character validation)."""
+        # ':' and '\\' are not in the allowed character set
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("C:\\Windows\\System32\\config")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("\\\\server\\share\\secret")
+
+    def test_invalid_null_bytes(self):
+        """Null bytes should be rejected."""
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("skill\x00hidden")
+
+    def test_invalid_control_characters(self):
+        """Control characters should be rejected."""
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("skill\x01test")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("skill\x1ftest")
+
+    def test_invalid_special_characters(self):
+        """Special shell characters should be rejected."""
+        with pytest.raises((InvalidSkillNameError, PathTraversalError)):
+            validate_skill_name("skill;rm -rf /")
+        with pytest.raises((InvalidSkillNameError, PathTraversalError)):
+            validate_skill_name("skill|cat /etc/passwd")
+        with pytest.raises((InvalidSkillNameError, PathTraversalError)):
+            validate_skill_name("skill&&evil")
+
+    def test_invalid_too_long(self):
+        """Names exceeding max length should be rejected."""
+        long_name = "a" * (MAX_SKILL_NAME_LENGTH + 1)
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name(long_name)
+
+    def test_invalid_empty(self):
+        """Empty names should be rejected."""
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name(None)
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("   ")
+
+    def test_path_separator_not_allowed_by_default(self):
+        """Path separators should not be allowed by default."""
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("mlops/axolotl", allow_path_separator=False)
+
+
+class TestResolveSkillPath:
+    """Tests for resolve_skill_path function."""
+
+    def test_resolve_valid_skill(self, tmp_path):
+        """Valid skill paths should resolve correctly."""
+        skills_dir = tmp_path / "skills"
+        skill_dir = skills_dir / "my-skill"
+        skill_dir.mkdir(parents=True)
+
+        resolved, error = resolve_skill_path("my-skill", skills_dir)
+        assert error is None
+        assert resolved == skill_dir.resolve()
+
+    def test_resolve_valid_nested_skill(self, tmp_path):
+        """Valid nested skill paths should resolve correctly."""
+        skills_dir = tmp_path / "skills"
+        skill_dir = skills_dir / "mlops" / "axolotl"
+        skill_dir.mkdir(parents=True)
+
+        resolved, error = resolve_skill_path("mlops/axolotl", skills_dir, allow_path_separator=True)
+        assert error is None
+        assert resolved == skill_dir.resolve()
+
+    def test_resolve_traversal_blocked(self, tmp_path):
+        """Path traversal should be blocked."""
+        skills_dir = tmp_path / "skills"
+        skills_dir.mkdir()
+
+        # Create a file outside skills dir
+        secret_file = tmp_path / "secret.txt"
+        secret_file.write_text("secret data")
+
+        # resolve_skill_path returns (path, error_message) on validation failure
+        resolved, error = resolve_skill_path("../secret.txt", skills_dir)
+        assert error is not None
+        assert "traversal" in error.lower() or ".." in error
+
+    def test_resolve_traversal_nested_blocked(self, tmp_path):
+        """Nested path traversal should be blocked."""
+        skills_dir = tmp_path / "skills"
+        skill_dir = skills_dir / "category" / "skill"
+        skill_dir.mkdir(parents=True)
+
+        # resolve_skill_path returns (path, error_message) on validation failure
+        resolved, error = resolve_skill_path("category/skill/../../../etc/passwd", skills_dir, allow_path_separator=True)
+        assert error is not None
+        assert "traversal" in error.lower() or ".." in error
+
+    def test_resolve_absolute_path_blocked(self, tmp_path):
+        """Absolute paths should be blocked."""
+        skills_dir = tmp_path / "skills"
+        skills_dir.mkdir()
+
+        # resolve_skill_path raises PathTraversalError for absolute paths that escape the boundary
+        with pytest.raises(PathTraversalError):
+            resolve_skill_path("/etc/passwd", skills_dir)
+
+
+class TestSanitizeSkillIdentifier:
+    """Tests for sanitize_skill_identifier function."""
+
+    def test_sanitize_traversal(self):
+        """Path traversal sequences should be removed."""
+        result = sanitize_skill_identifier("../../../etc/passwd")
+        assert ".." not in result
+        assert result == "/etc/passwd" or result == "etc/passwd"
+
+    def test_sanitize_home_expansion(self):
+        """Home directory expansion should be removed."""
+        result = sanitize_skill_identifier("~/.hermes/.env")
+        assert not result.startswith("~")
+        assert ".hermes" in result or ".env" in result
+
+    def test_sanitize_protocol(self):
+        """Protocol handlers should be removed."""
+        result = sanitize_skill_identifier("file:///etc/passwd")
+        assert "file:" not in result.lower()
+
+    def test_sanitize_null_bytes(self):
+        """Null bytes should be removed."""
+        result = sanitize_skill_identifier("skill\x00hidden")
+        assert "\x00" not in result
+
+    def test_sanitize_backslashes(self):
+        """Backslashes should be converted to forward slashes."""
+        result = sanitize_skill_identifier("path\\to\\skill")
+        assert "\\" not in result
+        assert "/" in result
+
+
+class TestIsSafeSkillPath:
+    """Tests for is_safe_skill_path function."""
+
+    def test_safe_within_directory(self, tmp_path):
+        """Paths within allowed directories should be safe."""
+        allowed = [tmp_path / "skills", tmp_path / "external"]
+        for d in allowed:
+            d.mkdir()
+
+        safe_path = tmp_path / "skills" / "my-skill"
+        safe_path.mkdir()
+
+        assert is_safe_skill_path(safe_path, allowed) is True
+
+    def test_unsafe_outside_directory(self, tmp_path):
+        """Paths outside allowed directories should be unsafe."""
+        allowed = [tmp_path / "skills"]
+        allowed[0].mkdir()
+
+        unsafe_path = tmp_path / "secret" / "file.txt"
+        unsafe_path.parent.mkdir()
+        unsafe_path.touch()
+
+        assert is_safe_skill_path(unsafe_path, allowed) is False
+
+    def test_symlink_escape_blocked(self, tmp_path):
+        """Symlinks pointing outside allowed directories should be unsafe."""
+        allowed = [tmp_path / "skills"]
+        skills_dir = allowed[0]
+        skills_dir.mkdir()
+
+        # Create target outside allowed dir
+        target = tmp_path / "secret.txt"
+        target.write_text("secret")
+
+        # Create symlink inside allowed dir
+        symlink = skills_dir / "evil-link"
+        try:
+            symlink.symlink_to(target)
+        except OSError:
+            pytest.skip("Symlinks not supported on this platform")
+
+        assert is_safe_skill_path(symlink, allowed) is False
+
+
+class TestSkillSecurityIntegration:
+    """Integration tests for skill security with actual skill loading."""
+
+    def test_skill_view_blocks_traversal_in_name(self, tmp_path):
+        """skill_view should block path traversal in skill name."""
+        from tools.skills_tool import skill_view
+
+        skills_dir = tmp_path / "skills"
+        skills_dir.mkdir(parents=True)
+
+        # Create secret file outside skills dir
+        secret_file = tmp_path / ".env"
+        secret_file.write_text("SECRET_KEY=12345")
+
+        with patch("tools.skills_tool.SKILLS_DIR", skills_dir):
+            result = json.loads(skill_view("../.env"))
+            assert result["success"] is False
+            assert "security_error" in result or "traversal" in result.get("error", "").lower()
+
+    def test_skill_view_blocks_absolute_path(self, tmp_path):
+        """skill_view should block absolute paths."""
+        from tools.skills_tool import skill_view
+
+        skills_dir = tmp_path / "skills"
+        skills_dir.mkdir(parents=True)
+
+        with patch("tools.skills_tool.SKILLS_DIR", skills_dir):
+            result = json.loads(skill_view("/etc/passwd"))
+            assert result["success"] is False
+            # Error could be from validation or path resolution - either way it's blocked
+            error_msg = result.get("error", "").lower()
+            assert "security_error" in result or "invalid" in error_msg or "non-relative" in error_msg or "boundary" in error_msg
+
+    def test_load_skill_payload_blocks_traversal(self, tmp_path):
+        """_load_skill_payload should block path traversal attempts."""
+        from agent.skill_commands import _load_skill_payload
+
+        skills_dir = tmp_path / "skills"
+        skills_dir.mkdir(parents=True)
+
+        with patch("tools.skills_tool.SKILLS_DIR", skills_dir):
+            # These should all return None (blocked)
+            assert _load_skill_payload("../../../etc/passwd") is None
+            assert _load_skill_payload("~/.hermes/.env") is None
+            assert _load_skill_payload("/etc/passwd") is None
+            assert _load_skill_payload("../secret") is None
+
+    def test_legitimate_skill_still_works(self, tmp_path):
+        """Legitimate skill loading should still work."""
+        from agent.skill_commands import _load_skill_payload
+        from tools.skills_tool import skill_view
+
+        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("""\
+---
+name: test-skill
+description: A test skill
+---
+
+# Test Skill
+
+This is a test skill.
+""")
+
+        with patch("tools.skills_tool.SKILLS_DIR", skills_dir):
+            # Test skill_view
+            result = json.loads(skill_view("test-skill"))
+            assert result["success"] is True
+            assert "test-skill" in result.get("name", "")
+
+            # Test _load_skill_payload
+            payload = _load_skill_payload("test-skill")
+            assert payload is not None
+            loaded_skill, skill_dir_result, skill_name = payload
+            assert skill_name == "test-skill"
+
+
+class TestEdgeCases:
+    """Edge case tests for skill security."""
+
+    def test_unicode_in_skill_name(self):
+        """Unicode characters should be handled appropriately."""
+        # Most unicode should be rejected as invalid
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("skill\u0000")
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("skill<script>")
+
+    def test_url_encoding_in_skill_name(self):
+        """URL-encoded characters should be rejected."""
+        with pytest.raises((InvalidSkillNameError, PathTraversalError)):
+            validate_skill_name("skill%2F..%2Fetc%2Fpasswd")
+
+    def test_double_encoding_in_skill_name(self):
+        """Double-encoded characters should be rejected."""
+        with pytest.raises((InvalidSkillNameError, PathTraversalError)):
+            validate_skill_name("skill%252F..%252Fetc%252Fpasswd")
+
+    def test_case_variations_of_protocols(self):
+        """Case variations of protocol handlers should be caught."""
+        # These should be caught by the '/' check or pattern validation
+        with pytest.raises((PathTraversalError, InvalidSkillNameError)):
+            validate_skill_name("FILE:///etc/passwd")
+        with pytest.raises((PathTraversalError, InvalidSkillNameError)):
+            validate_skill_name("HTTP://evil.com")
+
+    def test_null_byte_injection(self):
+        """Null byte injection attempts should be blocked."""
+        with pytest.raises(InvalidSkillNameError):
+            validate_skill_name("skill.txt\x00.php")
+
+    def test_very_long_traversal(self):
+        """Very long traversal sequences should be blocked (by length or pattern)."""
+        traversal = "../" * 100 + "etc/passwd"
+        # Should be blocked either by length limit or by traversal pattern
+        with pytest.raises((PathTraversalError, InvalidSkillNameError)):
+            validate_skill_name(traversal)