diff --git a/tests/test_path_traversal_security.py b/tests/test_path_traversal_security.py index 1345398..f281c58 100644 --- a/tests/test_path_traversal_security.py +++ b/tests/test_path_traversal_security.py @@ -63,3 +63,52 @@ class TestRegressionPrevention: 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 9623113..34140b1 100644 --- a/utils/security_config.py +++ b/utils/security_config.py @@ -91,7 +91,7 @@ def is_dangerous_path(path: Path) -> bool: """ Check if a path is in or under a dangerous directory. - Uses PREFIX MATCHING to block dangerous directories AND their subdirectories. + Uses Path.is_relative_to() to block dangerous directories AND their subdirectories. For example, if "/etc" is in DANGEROUS_PATHS, both "/etc" and "/etc/passwd" will be blocked. @@ -102,31 +102,26 @@ def is_dangerous_path(path: Path) -> bool: True if the path is dangerous and should not be accessed Security: - Fixes path traversal vulnerability (CWE-22) reported in: - - https://github.com/BeehiveInnovations/zen-mcp-server/issues/293 - - https://github.com/BeehiveInnovations/zen-mcp-server/issues/312 + Fixes path traversal vulnerability (CWE-22) """ try: resolved = path.resolve() - resolved_str = str(resolved) # Check 1: Root directory (filesystem root) if resolved.parent == resolved: return True # Check 2: Exact match or subdirectory of dangerous paths + # Use Path.is_relative_to() for correct cross-platform path comparison for dangerous in DANGEROUS_PATHS: # Skip root "/" - already handled above if dangerous == "/": continue - # Exact match - if resolved_str == dangerous: - return True - - # Subdirectory check: path starts with dangerous + separator - # Use os.sep for platform-appropriate separator - if resolved_str.startswith(dangerous + "/") or resolved_str.startswith(dangerous + "\\"): + dangerous_path = Path(dangerous) + # is_relative_to() correctly handles both exact matches and subdirectories + # Works properly on Windows with paths like "C:\" and "C:\Users" + if resolved == dangerous_path or resolved.is_relative_to(dangerous_path): return True return False