diff --git a/tests/conftest.py b/tests/conftest.py index b8d82b3..6772f26 100644 --- a/tests/conftest.py +++ b/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: diff --git a/tests/test_path_traversal_security.py b/tests/test_path_traversal_security.py new file mode 100644 index 0000000..57509dd --- /dev/null +++ b/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 diff --git a/tests/test_utils.py b/tests/test_utils.py index f3d1f92..a2c49fb 100644 --- a/tests/test_utils.py +++ b/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): diff --git a/utils/security_config.py b/utils/security_config.py index ce8fb29..8ee779b 100644 --- a/utils/security_config.py +++ b/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