From c61aa75f1cbece2eeea8b12559ac0e8e0a89b4fa Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Tue, 21 Apr 2026 17:57:33 +0530 Subject: [PATCH 1/2] REFACTOR: Path traversal vulnerability in logging --- mssql_python/logging.py | 42 ++++++++++++++++++++++++++++++++------- tests/test_007_logging.py | 22 ++++++++++++++++++-- 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/mssql_python/logging.py b/mssql_python/logging.py index a16bf7734..2130f3a88 100644 --- a/mssql_python/logging.py +++ b/mssql_python/logging.py @@ -273,17 +273,44 @@ def _cleanup_handlers(self): except: pass - def _validate_log_file_extension(self, file_path: str) -> None: + def _validate_log_file_path(self, file_path: str) -> str: """ - Validate that the log file has an allowed extension. + Validate and sanitize the log file path. + + Resolves the path to its canonical form, checks for path traversal + outside the current working directory (for relative paths), and + validates the file extension. Args: file_path: Path to the log file + Returns: + The resolved canonical path. + Raises: - ValueError: If the file extension is not allowed + ValueError: If the path contains traversal outside the allowed + base directory, or if the file extension is not allowed """ - _, ext = os.path.splitext(file_path) + resolved = os.path.realpath(os.path.abspath(file_path)) + + # For relative paths, ensure the resolved path stays under cwd + if not os.path.isabs(file_path): + base = os.path.realpath(os.path.abspath(os.getcwd())) + # os.path.commonpath raises ValueError if paths are on different drives + try: + common = os.path.commonpath([base, resolved]) + except ValueError: + raise ValueError( + "log_file_path resolves outside the current working directory. " + "Path traversal is not permitted for relative paths." + ) + if common != base: + raise ValueError( + "log_file_path resolves outside the current working directory. " + "Path traversal is not permitted for relative paths." + ) + + _, ext = os.path.splitext(resolved) ext_lower = ext.lower() if ext_lower not in ALLOWED_LOG_EXTENSIONS: @@ -292,6 +319,8 @@ def _validate_log_file_extension(self, file_path: str) -> None: f"Invalid log file extension '{ext}'. " f"Allowed extensions: {allowed}" ) + return resolved + def _write_log_header(self): """ Write CSV header and metadata to the log file. @@ -483,10 +512,9 @@ def _setLevel( ) self._output_mode = output - # Store custom log file path if provided + # Store custom log file path if provided (sanitized) if log_file_path is not None: - self._validate_log_file_extension(log_file_path) - self._custom_log_path = log_file_path + self._custom_log_path = self._validate_log_file_path(log_file_path) # Setup handlers if not yet initialized or if output mode/path changed # Handler setup is protected by _handler_lock inside _setup_handlers() diff --git a/tests/test_007_logging.py b/tests/test_007_logging.py index daafc6fc2..363855226 100644 --- a/tests/test_007_logging.py +++ b/tests/test_007_logging.py @@ -150,8 +150,9 @@ def test_custom_log_file_path(self, cleanup_logger, temp_log_dir): setup_logging(log_file_path=custom_path) logger.debug("Test message") - assert logger.log_file == custom_path - assert os.path.exists(custom_path) + resolved = os.path.realpath(os.path.abspath(custom_path)) + assert logger.log_file == resolved + assert os.path.exists(resolved) def test_custom_log_file_path_creates_directory(self, cleanup_logger, temp_log_dir): """Custom log file path should create parent directories""" @@ -179,6 +180,23 @@ def test_log_file_extension_validation_invalid(self, cleanup_logger, temp_log_di with pytest.raises(ValueError, match="Invalid log file extension"): setup_logging(log_file_path=custom_path) + def test_path_traversal_rejected(self, cleanup_logger): + """Relative paths with traversal outside cwd should be rejected""" + with pytest.raises(ValueError, match="Path traversal is not permitted"): + setup_logging(log_file_path="../../etc/cron.d/evil.log") + + def test_path_traversal_dot_dot_rejected(self, cleanup_logger): + """Relative paths using ../ to escape cwd should be rejected""" + with pytest.raises(ValueError, match="Path traversal is not permitted"): + setup_logging(log_file_path="../../../tmp/evil.log") + + def test_absolute_path_allowed(self, cleanup_logger, temp_log_dir): + """Absolute paths should be allowed (developer-controlled)""" + custom_path = os.path.join(temp_log_dir, "absolute_test.log") + setup_logging(log_file_path=custom_path) + logger.debug("Test message") + assert os.path.exists(os.path.realpath(os.path.abspath(custom_path))) + class TestCSVFormat: """Test CSV output format""" From cbdf47674bd399082804a9de4fa1a9ed94723906 Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Tue, 21 Apr 2026 18:39:54 +0530 Subject: [PATCH 2/2] Fixing comments --- tests/test_007_logging.py | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/tests/test_007_logging.py b/tests/test_007_logging.py index 363855226..cb6eefd55 100644 --- a/tests/test_007_logging.py +++ b/tests/test_007_logging.py @@ -180,15 +180,28 @@ def test_log_file_extension_validation_invalid(self, cleanup_logger, temp_log_di with pytest.raises(ValueError, match="Invalid log file extension"): setup_logging(log_file_path=custom_path) - def test_path_traversal_rejected(self, cleanup_logger): - """Relative paths with traversal outside cwd should be rejected""" + def test_path_traversal_rejected(self, cleanup_logger, monkeypatch, tmp_path): + """Relative paths with traversal outside a controlled cwd should be rejected""" + monkeypatch.chdir(tmp_path) with pytest.raises(ValueError, match="Path traversal is not permitted"): - setup_logging(log_file_path="../../etc/cron.d/evil.log") + setup_logging(log_file_path="../evil.log") - def test_path_traversal_dot_dot_rejected(self, cleanup_logger): - """Relative paths using ../ to escape cwd should be rejected""" + def test_path_traversal_dot_dot_rejected(self, cleanup_logger, monkeypatch, tmp_path): + """Relative paths using ../ to escape a controlled cwd should be rejected""" + monkeypatch.chdir(tmp_path) with pytest.raises(ValueError, match="Path traversal is not permitted"): - setup_logging(log_file_path="../../../tmp/evil.log") + setup_logging(log_file_path="../nested/evil.log") + + def test_path_traversal_different_drive(self, cleanup_logger, monkeypatch, tmp_path): + """commonpath ValueError (e.g. different drives on Windows) should be rejected""" + monkeypatch.chdir(tmp_path) + + def mock_commonpath(paths): + raise ValueError("Can't mix absolute and relative paths") + + monkeypatch.setattr(os.path, "commonpath", mock_commonpath) + with pytest.raises(ValueError, match="Path traversal is not permitted"): + setup_logging(log_file_path="subdir/test.log") def test_absolute_path_allowed(self, cleanup_logger, temp_log_dir): """Absolute paths should be allowed (developer-controlled)"""