fix: use Path.is_relative_to() for cross-platform dangerous path detection
Replace string prefix matching with Path.is_relative_to() to correctly handle Windows paths like "C:\" where trailing backslash caused double separator issues (e.g., "C:\\" instead of "C:\"). Changes: - Use Path.is_relative_to() for subdirectory detection (requires Python 3.9+) - Add Windows path handling tests using PureWindowsPath - Update test_utils.py to expect /etc/passwd to be blocked (security fix)
This commit is contained in:
@@ -63,3 +63,52 @@ class TestRegressionPrevention:
|
|||||||
def test_etc_shadow_blocked(self):
|
def test_etc_shadow_blocked(self):
|
||||||
"""Test /etc/shadow is blocked (password hashes)."""
|
"""Test /etc/shadow is blocked (password hashes)."""
|
||||||
assert is_dangerous_path(Path("/etc/shadow")) is True
|
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
|
||||||
|
|||||||
@@ -29,14 +29,12 @@ class TestFileUtils:
|
|||||||
assert "Error: File does not exist" in content
|
assert "Error: File does not exist" in content
|
||||||
assert tokens > 0
|
assert tokens > 0
|
||||||
|
|
||||||
def test_read_file_content_safe_files_allowed(self):
|
def test_read_file_content_dangerous_files_blocked(self):
|
||||||
"""Test that safe files outside the original project root are now allowed"""
|
"""Test that dangerous system files are blocked"""
|
||||||
# In the new security model, safe files like /etc/passwd
|
# /etc/passwd should be blocked as it's under /etc (dangerous path)
|
||||||
# can be read as they're not in the dangerous paths list
|
|
||||||
content, tokens = read_file_content("/etc/passwd")
|
content, tokens = read_file_content("/etc/passwd")
|
||||||
# Should successfully read the file (with timestamp in header)
|
assert "--- ERROR ACCESSING FILE:" in content
|
||||||
assert "--- BEGIN FILE: /etc/passwd (Last modified:" in content
|
assert "Access to system directory denied" in content
|
||||||
assert "--- END FILE: /etc/passwd ---" in content
|
|
||||||
assert tokens > 0
|
assert tokens > 0
|
||||||
|
|
||||||
def test_read_file_content_relative_path_rejected(self):
|
def test_read_file_content_relative_path_rejected(self):
|
||||||
|
|||||||
@@ -91,7 +91,7 @@ def is_dangerous_path(path: Path) -> bool:
|
|||||||
"""
|
"""
|
||||||
Check if a path is in or under a dangerous directory.
|
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"
|
For example, if "/etc" is in DANGEROUS_PATHS, both "/etc" and "/etc/passwd"
|
||||||
will be blocked.
|
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
|
True if the path is dangerous and should not be accessed
|
||||||
|
|
||||||
Security:
|
Security:
|
||||||
Fixes path traversal vulnerability (CWE-22) reported in:
|
Fixes path traversal vulnerability (CWE-22)
|
||||||
- https://github.com/BeehiveInnovations/zen-mcp-server/issues/293
|
|
||||||
- https://github.com/BeehiveInnovations/zen-mcp-server/issues/312
|
|
||||||
"""
|
"""
|
||||||
try:
|
try:
|
||||||
resolved = path.resolve()
|
resolved = path.resolve()
|
||||||
resolved_str = str(resolved)
|
|
||||||
|
|
||||||
# Check 1: Root directory (filesystem root)
|
# Check 1: Root directory (filesystem root)
|
||||||
if resolved.parent == resolved:
|
if resolved.parent == resolved:
|
||||||
return True
|
return True
|
||||||
|
|
||||||
# Check 2: Exact match or subdirectory of dangerous paths
|
# 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:
|
for dangerous in DANGEROUS_PATHS:
|
||||||
# Skip root "/" - already handled above
|
# Skip root "/" - already handled above
|
||||||
if dangerous == "/":
|
if dangerous == "/":
|
||||||
continue
|
continue
|
||||||
|
|
||||||
# Exact match
|
dangerous_path = Path(dangerous)
|
||||||
if resolved_str == dangerous:
|
# is_relative_to() correctly handles both exact matches and subdirectories
|
||||||
return True
|
# Works properly on Windows with paths like "C:\" and "C:\Users"
|
||||||
|
if resolved == dangerous_path or resolved.is_relative_to(dangerous_path):
|
||||||
# Subdirectory check: path starts with dangerous + separator
|
|
||||||
# Use os.sep for platform-appropriate separator
|
|
||||||
if resolved_str.startswith(dangerous + "/") or resolved_str.startswith(dangerous + "\\"):
|
|
||||||
return True
|
return True
|
||||||
|
|
||||||
return False
|
return False
|
||||||
|
|||||||
Reference in New Issue
Block a user