REFACTOR: Add path canonicalization and traversal guard to setup_logging log_file_path validation#530
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens mssql_python’s custom log file handling by canonicalizing the configured log path and preventing relative-path traversal outside the current working directory, while keeping extension validation in place. It also updates unit tests to assert the canonicalized path behavior and adds traversal-related test cases.
Changes:
- Replace
_validate_log_file_extensionwith_validate_log_file_pathto resolve/canonicalize paths, restrict relative-path traversal, and validate extensions. - Update
_setLevelto store and use the sanitized (resolved) log file path. - Update/add tests to assert canonical paths and to verify traversal rejection vs. absolute path allowance.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
mssql_python/logging.py |
Adds canonicalization + relative-path traversal protection and returns the sanitized path for downstream use. |
tests/test_007_logging.py |
Updates assertions to match canonicalized paths and adds traversal/absolute-path behavior tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 67.8%
mssql_python.row.py: 70.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 74.5%
mssql_python.pybind.connection.connection.cpp: 75.8%
mssql_python.__init__.py: 77.3%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 85.2%🔗 Quick Links
|
|
This PR fixed a path traversal (CWE-22) that I originally reported via MSRC (VULN-183589). Would it be possible to also create a GitHub Security Advisory and request a CVE-ID for this fix? It would help with tracking and downstream security tools. Thanks! |
Work Item / Issue Reference
Summary
This pull request strengthens the validation and sanitization of custom log file paths in the logging system. The main improvement is a new method that ensures log file paths are canonicalized, prevents path traversal for relative paths, and validates file extensions. The tests are updated to reflect these changes and to verify that unsafe paths are correctly rejected.
Log file path validation and sanitization:
_validate_log_file_extensionwith_validate_log_file_pathinmssql_python/logging.py, which now resolves the canonical path, checks for path traversal outside the current working directory for relative paths, and validates the file extension. The method returns the resolved path._setLevelto use the sanitized log file path returned by_validate_log_file_pathinstead of the raw input.Testing improvements:
tests/test_007_logging.pyto check that the logger uses the resolved canonical path and to verify that log files are created at the correct location.These changes improve the security and correctness of log file handling by preventing directory traversal attacks and ensuring log files are created only in intended locations.