From 9ed15f405a9462b4db7aa44ca2d989e092c008e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B0=A2=E6=A0=8B=E6=A2=81?= Date: Wed, 3 Dec 2025 15:17:07 +0800 Subject: [PATCH 1/4] fix: path traversal vulnerability - use prefix matching in is_dangerous_path() The is_dangerous_path() function only did exact string matching, allowing attackers to bypass protection by accessing subdirectories: - /etc was blocked but /etc/passwd was allowed - C:\Windows was blocked but C:\Windows\System32\... was allowed This minimal fix changes is_dangerous_path() to use PREFIX MATCHING: - Now blocks dangerous directories AND all their subdirectories - Paths like /etcbackup are still allowed (not under /etc) - No changes to DANGEROUS_PATHS list Security: - Fixes CWE-22: Path Traversal vulnerability - Reported by: Team off-course (K-Shield.Jr 15th) Fixes #312 Fixes #293 --- tests/test_path_traversal_security.py | 65 +++++++++++++++++++++++++++ utils/security_config.py | 35 ++++++++++++++- 2 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 tests/test_path_traversal_security.py diff --git a/tests/test_path_traversal_security.py b/tests/test_path_traversal_security.py new file mode 100644 index 0000000..1345398 --- /dev/null +++ b/tests/test_path_traversal_security.py @@ -0,0 +1,65 @@ +""" +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. +""" + +from pathlib import Path + +from utils.security_config import is_dangerous_path + + +class TestPathTraversalFix: + """Test that subdirectories of dangerous paths are now 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 are now 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 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 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 diff --git a/utils/security_config.py b/utils/security_config.py index ce8fb29..9623113 100644 --- a/utils/security_config.py +++ b/utils/security_config.py @@ -89,16 +89,47 @@ 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. + + Uses PREFIX MATCHING to block dangerous directories AND their subdirectories. + For example, if "/etc" is in DANGEROUS_PATHS, both "/etc" and "/etc/passwd" + will be blocked. Args: path: Path to check Returns: 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 """ try: resolved = path.resolve() - return str(resolved) in DANGEROUS_PATHS or resolved.parent == resolved + 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 + 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 + "\\"): + return True + + return False + except Exception: return True # If we can't resolve, consider it dangerous From 91ffb51564e5655ec91111938039ed81e0d8e4c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B0=A2=E6=A0=8B=E6=A2=81?= Date: Fri, 5 Dec 2025 13:53:39 +0800 Subject: [PATCH 2/4] 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) --- tests/test_path_traversal_security.py | 49 +++++++++++++++++++++++++++ tests/test_utils.py | 12 +++---- utils/security_config.py | 19 ++++------- 3 files changed, 61 insertions(+), 19 deletions(-) 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 From e5548acb984ca4f8b2ae8381f879a0285094257f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B0=A2=E6=A0=8B=E6=A2=81?= Date: Mon, 15 Dec 2025 20:24:44 +0800 Subject: [PATCH 3/4] fix: allow home subdirectories through is_dangerous_path() Split DANGEROUS_PATHS into two categories: 1. DANGEROUS_SYSTEM_PATHS: Block path AND all subdirectories (e.g., /etc, /etc/passwd, /var/log/auth.log) 2. DANGEROUS_HOME_CONTAINERS: Block ONLY exact match (e.g., /home is blocked but /home/user/project passes through) This fixes the issue where /home/user/project was incorrectly blocked by is_dangerous_path(). Subdirectory access control for home directories is properly delegated to is_home_directory_root() in resolve_and_validate_path(). Addresses review feedback from @chatgpt-codex-connector about blocking all home directory subpaths. --- tests/test_path_traversal_security.py | 41 ++++++++++++++++++++++-- utils/security_config.py | 46 ++++++++++++++++++++------- 2 files changed, 72 insertions(+), 15 deletions(-) diff --git a/tests/test_path_traversal_security.py b/tests/test_path_traversal_security.py index f281c58..57509dd 100644 --- a/tests/test_path_traversal_security.py +++ b/tests/test_path_traversal_security.py @@ -7,6 +7,11 @@ Fixes vulnerability reported in: 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 @@ -15,7 +20,7 @@ from utils.security_config import is_dangerous_path class TestPathTraversalFix: - """Test that subdirectories of dangerous paths are now blocked.""" + """Test that subdirectories of dangerous system paths are blocked.""" def test_exact_match_still_works(self): """Test that exact dangerous paths are still blocked.""" @@ -24,7 +29,7 @@ class TestPathTraversalFix: assert is_dangerous_path(Path("/var")) is True def test_subdirectory_now_blocked(self): - """Test that subdirectories are now blocked (the fix).""" + """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 @@ -32,7 +37,7 @@ class TestPathTraversalFix: assert is_dangerous_path(Path("/var/log/auth.log")) is True def test_deeply_nested_blocked(self): - """Test that deeply nested paths are blocked.""" + """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 @@ -53,6 +58,36 @@ class TestPathTraversalFix: 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.""" diff --git a/utils/security_config.py b/utils/security_config.py index 34140b1..4f42b49 100644 --- a/utils/security_config.py +++ b/utils/security_config.py @@ -7,22 +7,31 @@ 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 = { @@ -91,9 +100,14 @@ def is_dangerous_path(path: Path) -> bool: """ Check if a path is in or under a dangerous directory. - 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. + 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 @@ -102,7 +116,8 @@ 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) + Fixes path traversal vulnerability (CWE-22) while preserving + user access to home subdirectories. """ try: resolved = path.resolve() @@ -111,19 +126,26 @@ def is_dangerous_path(path: Path) -> bool: 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: + # Check 2: System paths - block exact match AND all subdirectories + for dangerous in DANGEROUS_SYSTEM_PATHS: # Skip root "/" - already handled above if dangerous == "/": continue 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" + # Works properly on Windows with paths like "C:\" and "C:\Windows" 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: + container_path = Path(container) + if resolved == container_path: + return True + return False except Exception: From ba08308a23d1c1491099c5d0eae548077bd88f9f Mon Sep 17 00:00:00 2001 From: Fahad Date: Mon, 15 Dec 2025 17:02:24 +0000 Subject: [PATCH 4/4] fix(security): handle macOS symlinked system dirs Follow-up on PR #353 to keep dangerous-path blocking correct on macOS (/etc -> /private/etc) while avoiding overblocking Windows workspaces (C:\). --- tests/conftest.py | 8 ++++++++ utils/security_config.py | 29 ++++++++++++++++++++--------- 2 files changed, 28 insertions(+), 9 deletions(-) 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/utils/security_config.py b/utils/security_config.py index 4f42b49..8ee779b 100644 --- a/utils/security_config.py +++ b/utils/security_config.py @@ -16,7 +16,6 @@ DANGEROUS_SYSTEM_PATHS = { "/bin", "/var", "/root", - "C:\\", "C:\\Windows", "C:\\Program Files", } @@ -122,6 +121,17 @@ def is_dangerous_path(path: Path) -> bool: try: resolved = path.resolve() + 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 @@ -132,19 +142,20 @@ def is_dangerous_path(path: Path) -> bool: if dangerous == "/": continue - dangerous_path = Path(dangerous) - # is_relative_to() correctly handles both exact matches and subdirectories - # Works properly on Windows with paths like "C:\" and "C:\Windows" - if resolved == dangerous_path or resolved.is_relative_to(dangerous_path): - return True + 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: - container_path = Path(container) - if resolved == container_path: - return True + for container_path in _dangerous_variants(Path(container)): + if resolved == container_path: + return True return False