Merge pull request #353 from DragonFSKY/fix/path-traversal-security

fix: path traversal vulnerability in is_dangerous_path()

tests/conftest.py

@@ -6,10 +6,18 @@ import asyncio
 import importlib
 import os
 import sys
+import tempfile
 from pathlib import Path
 
 import pytest
 
+# On macOS, the default pytest temp dir is typically under /var (e.g. /private/var/folders/...).
+# If /var is considered a dangerous system path, tests must use a safe temp root (like /tmp).
+if sys.platform == "darwin":
+    os.environ["TMPDIR"] = "/tmp"
+    # tempfile caches the temp dir after first lookup; clear it so pytest fixtures pick up TMPDIR.
+    tempfile.tempdir = None
+
 # Ensure the parent directory is in the Python path for imports
 parent_dir = Path(__file__).resolve().parent.parent
 if str(parent_dir) not in sys.path:

tests/test_path_traversal_security.py

@@ -0,0 +1,149 @@
+"""
+Test path traversal security fix.
+
+Fixes vulnerability reported in:
+- https://github.com/BeehiveInnovations/zen-mcp-server/issues/293
+- https://github.com/BeehiveInnovations/zen-mcp-server/issues/312
+
+The vulnerability: is_dangerous_path() only did exact string matching,
+so /etc was blocked but /etc/passwd was allowed.
+
+Additionally, this fix properly handles home directory containers:
+- /home and C:\\Users are blocked (exact match only)
+- /home/user/project paths are allowed through is_dangerous_path()
+  and handled by is_home_directory_root() in resolve_and_validate_path()
+"""
+
+from pathlib import Path
+
+from utils.security_config import is_dangerous_path
+
+
+class TestPathTraversalFix:
+    """Test that subdirectories of dangerous system paths are blocked."""
+
+    def test_exact_match_still_works(self):
+        """Test that exact dangerous paths are still blocked."""
+        assert is_dangerous_path(Path("/etc")) is True
+        assert is_dangerous_path(Path("/usr")) is True
+        assert is_dangerous_path(Path("/var")) is True
+
+    def test_subdirectory_now_blocked(self):
+        """Test that subdirectories of system paths are blocked (the fix)."""
+        # These were allowed before the fix
+        assert is_dangerous_path(Path("/etc/passwd")) is True
+        assert is_dangerous_path(Path("/etc/shadow")) is True
+        assert is_dangerous_path(Path("/etc/hosts")) is True
+        assert is_dangerous_path(Path("/var/log/auth.log")) is True
+
+    def test_deeply_nested_blocked(self):
+        """Test that deeply nested system paths are blocked."""
+        assert is_dangerous_path(Path("/etc/ssh/sshd_config")) is True
+        assert is_dangerous_path(Path("/usr/local/bin/python")) is True
+
+    def test_root_blocked(self):
+        """Test that root directory is blocked."""
+        assert is_dangerous_path(Path("/")) is True
+
+    def test_safe_paths_allowed(self):
+        """Test that safe paths are still allowed."""
+        # User project directories should be allowed
+        assert is_dangerous_path(Path("/tmp/test")) is False
+        assert is_dangerous_path(Path("/tmp/myproject/src")) is False
+
+    def test_similar_names_not_blocked(self):
+        """Test that paths with similar names are not blocked."""
+        # /etcbackup should NOT be blocked (it's not under /etc)
+        assert is_dangerous_path(Path("/tmp/etcbackup")) is False
+        assert is_dangerous_path(Path("/tmp/my_etc_files")) is False
+
+
+class TestHomeDirectoryHandling:
+    """Test that home directory containers are handled correctly.
+
+    Home containers (/home, C:\\Users) should only block the exact path,
+    not subdirectories. Subdirectory access control is delegated to
+    is_home_directory_root() in resolve_and_validate_path().
+    """
+
+    def test_home_container_blocked(self):
+        """Test that /home itself is blocked."""
+        assert is_dangerous_path(Path("/home")) is True
+
+    def test_home_subdirectories_allowed(self):
+        """Test that /home subdirectories pass through is_dangerous_path().
+
+        These paths should NOT be blocked by is_dangerous_path() because:
+        1. /home/user/project is a valid user workspace
+        2. Access control for /home/username is handled by is_home_directory_root()
+        """
+        # User home directories should pass is_dangerous_path()
+        # (they are handled by is_home_directory_root() separately)
+        assert is_dangerous_path(Path("/home/user")) is False
+        assert is_dangerous_path(Path("/home/user/project")) is False
+        assert is_dangerous_path(Path("/home/user/project/src/main.py")) is False
+
+    def test_home_deeply_nested_allowed(self):
+        """Test that deeply nested home paths are allowed."""
+        assert is_dangerous_path(Path("/home/user/documents/work/project/src")) is False
+
+
+class TestRegressionPrevention:
+    """Regression tests for the specific vulnerability."""
+
+    def test_etc_passwd_blocked(self):
+        """Test /etc/passwd is blocked (common attack target)."""
+        assert is_dangerous_path(Path("/etc/passwd")) is True
+
+    def test_etc_shadow_blocked(self):
+        """Test /etc/shadow is blocked (password hashes)."""
+        assert is_dangerous_path(Path("/etc/shadow")) is True
+
+
+class TestWindowsPathHandling:
+    """Test Windows path handling with trailing backslash.
+
+    Fixes issue reported in PR #353: Windows paths like C:\\ have trailing
+    backslash which caused double separator issues with string prefix matching.
+    Using Path.is_relative_to() resolves this correctly.
+    """
+
+    def test_windows_root_drive_blocked(self):
+        """Test that Windows root drive C:\\ is blocked."""
+        from pathlib import PureWindowsPath
+
+        # Simulate Windows path behavior using PureWindowsPath
+        # On Linux, we test the logic with PureWindowsPath to verify cross-platform correctness
+        c_root = PureWindowsPath("C:\\")
+        assert c_root.parent == c_root  # Root check works
+
+    def test_windows_dangerous_subdirectory_detection(self):
+        """Test that Windows subdirectories are correctly detected as dangerous.
+
+        This verifies the fix for the double backslash issue:
+        - Before fix: "C:\\" + "\\" = "C:\\\\" which doesn't match "C:\\Users"
+        - After fix: Path.is_relative_to() handles this correctly
+        """
+        from pathlib import PureWindowsPath
+
+        # Verify is_relative_to works correctly for Windows paths
+        c_users = PureWindowsPath("C:\\Users")
+        c_root = PureWindowsPath("C:\\")
+
+        # This is the key test - subdirectory detection must work
+        assert c_users.is_relative_to(c_root) is True
+
+        # Deeper paths should also work
+        c_users_admin = PureWindowsPath("C:\\Users\\Admin")
+        assert c_users_admin.is_relative_to(c_root) is True
+        assert c_users_admin.is_relative_to(c_users) is True
+
+    def test_windows_path_not_relative_to_different_drive(self):
+        """Test that paths on different drives are not related."""
+        from pathlib import PureWindowsPath
+
+        d_path = PureWindowsPath("D:\\Data")
+        c_root = PureWindowsPath("C:\\")
+
+        # D: drive paths should not be relative to C:
+        assert d_path.is_relative_to(c_root) is False

tests/test_utils.py

@@ -29,14 +29,12 @@ class TestFileUtils:
         assert "Error: File does not exist" in content
         assert tokens > 0
 
-    def test_read_file_content_safe_files_allowed(self):
-        """Test that safe files outside the original project root are now allowed"""
-        # In the new security model, safe files like /etc/passwd
-        # can be read as they're not in the dangerous paths list
+    def test_read_file_content_dangerous_files_blocked(self):
+        """Test that dangerous system files are blocked"""
+        # /etc/passwd should be blocked as it's under /etc (dangerous path)
         content, tokens = read_file_content("/etc/passwd")
-        # Should successfully read the file (with timestamp in header)
-        assert "--- BEGIN FILE: /etc/passwd (Last modified:" in content
-        assert "--- END FILE: /etc/passwd ---" in content
+        assert "--- ERROR ACCESSING FILE:" in content
+        assert "Access to system directory denied" in content
         assert tokens > 0
 
     def test_read_file_content_relative_path_rejected(self):

utils/security_config.py

@@ -7,22 +7,30 @@ for file access control.
 
 from pathlib import Path
 
-# Dangerous paths that should never be scanned
-# These would give overly broad access and pose security risks
-DANGEROUS_PATHS = {
+# Dangerous system paths - block these AND all their subdirectories
+# These are system directories where user code should never reside
+DANGEROUS_SYSTEM_PATHS = {
     "/",
     "/etc",
     "/usr",
     "/bin",
     "/var",
     "/root",
-    "/home",
-    "C:\\",
     "C:\\Windows",
     "C:\\Program Files",
+}
+
+# User home container paths - block ONLY the exact path, not subdirectories
+# Subdirectory access (e.g., /home/user/project) is controlled by is_home_directory_root()
+# This allows users to work in their home subdirectories while blocking overly broad access
+DANGEROUS_HOME_CONTAINERS = {
+    "/home",
     "C:\\Users",
 }
 
+# Combined set for backward compatibility
+DANGEROUS_PATHS = DANGEROUS_SYSTEM_PATHS | DANGEROUS_HOME_CONTAINERS
+
 # Directories to exclude from recursive file search
 # These typically contain generated code, dependencies, or build artifacts
 EXCLUDED_DIRS = {
@@ -89,16 +97,67 @@ EXCLUDED_DIRS = {
 
 def is_dangerous_path(path: Path) -> bool:
     """
-    Check if a path is in the dangerous paths list.
+    Check if a path is in or under a dangerous directory.
+
+    This function handles two categories of dangerous paths differently:
+
+    1. System paths (DANGEROUS_SYSTEM_PATHS): Block the path AND all subdirectories.
+       Example: /etc is dangerous, so /etc/passwd is also blocked.
+
+    2. Home containers (DANGEROUS_HOME_CONTAINERS): Block ONLY the exact path.
+       Example: /home is blocked, but /home/user/project is allowed.
+       Subdirectory access control is delegated to is_home_directory_root().
 
     Args:
         path: Path to check
 
     Returns:
         True if the path is dangerous and should not be accessed
+
+    Security:
+        Fixes path traversal vulnerability (CWE-22) while preserving
+        user access to home subdirectories.
     """
     try:
         resolved = path.resolve()
-        return str(resolved) in DANGEROUS_PATHS or resolved.parent == resolved
+
+        def _dangerous_variants(p: Path) -> set[Path]:
+            variants = {p}
+            # Only resolve paths that are absolute on the current platform.
+            # This avoids turning Windows-style strings into nonsense absolute paths on POSIX.
+            if p.is_absolute():
+                try:
+                    variants.add(p.resolve())
+                except Exception:
+                    pass
+            return variants
+
+        # Check 1: Root directory (filesystem root)
+        if resolved.parent == resolved:
+            return True
+
+        # Check 2: System paths - block exact match AND all subdirectories
+        for dangerous in DANGEROUS_SYSTEM_PATHS:
+            # Skip root "/" - already handled above
+            if dangerous == "/":
+                continue
+
+            for dangerous_path in _dangerous_variants(Path(dangerous)):
+                # is_relative_to() correctly handles both exact matches and subdirectories.
+                # Resolving the dangerous base path also handles platform symlinks
+                # (e.g., macOS /etc -> /private/etc, /var -> /private/var).
+                if resolved == dangerous_path or resolved.is_relative_to(dangerous_path):
+                    return True
+
+        # Check 3: Home containers - block ONLY exact match
+        # Subdirectories like /home/user/project should pass through here
+        # and be handled by is_home_directory_root() in resolve_and_validate_path()
+        for container in DANGEROUS_HOME_CONTAINERS:
+            for container_path in _dangerous_variants(Path(container)):
+                if resolved == container_path:
+                    return True
+
+        return False
+
     except Exception:
         return True  # If we can't resolve, consider it dangerous