-
Notifications
You must be signed in to change notification settings - Fork 825
fix: Potential fix for code scanning alert no. 77: Uncontrolled data used in path expression #422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses a security vulnerability (code scanning alert no. 77) by implementing path traversal protection in the _parse_local_dir_path
function. The change prevents uncontrolled data from being used in path expressions by validating that user-provided paths remain within a safe root directory.
Key Changes:
- Added path validation logic to prevent directory traversal attacks
- Implemented safe root directory boundary checking using
os.path.commonpath
- Added exception handling for paths that escape the allowed root directory
@@ -327,7 +327,10 @@ def _parse_local_dir_path(path_str: str) -> IngestionQuery: | |||
A dictionary containing the parsed details of the file path. | |||
|
|||
""" | |||
root_path = TMP_BASE_PATH.resolve() |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
src/gitingest/query_parser.py
Outdated
if os.path.commonpath([root_path, path_obj]) != str(root_path): | ||
raise InvalidPatternError(f"Path {path_str} escapes the allowed root directory.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path validation logic has a potential issue: os.path.commonpath() can raise ValueError if the paths are on different drives (Windows) or if the list is empty. Consider wrapping this in a try-except block and also verify that both paths exist before comparison.
if os.path.commonpath([root_path, path_obj]) != str(root_path): | |
raise InvalidPatternError(f"Path {path_str} escapes the allowed root directory.") | |
# Ensure both paths exist | |
if not root_path.exists() or not path_obj.exists(): | |
raise InvalidPatternError(f"One or both paths do not exist: {root_path}, {path_obj}") | |
# Check if path_obj is within root_path | |
try: | |
if os.path.commonpath([root_path, path_obj]) != str(root_path): | |
raise InvalidPatternError(f"Path {path_str} escapes the allowed root directory.") | |
except ValueError as e: | |
raise InvalidPatternError(f"Invalid path comparison: {e}") | |
Copilot uses AI. Check for mistakes.
path_obj = Path(path_str).resolve() | ||
if os.path.commonpath([root_path, path_obj]) != str(root_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comparison converts root_path to string but uses the Path object directly in the commonpath call. This could lead to inconsistent behavior. Consider using str(root_path) consistently or compare Path objects directly.
if os.path.commonpath([root_path, path_obj]) != str(root_path): | |
if os.path.commonpath([str(root_path), str(path_obj)]) != str(root_path): |
Copilot uses AI. Check for mistakes.
Potential fix for https://github.com/coderamp-labs/gitingest/security/code-scanning/77
To address this issue, the
_parse_local_dir_path
function needs to validate thepath_str
input to ensure it stays within a predefined safe root directory.Validation Strategy:
TMP_BASE_PATH
).Path.resolve()
and ensure the resulting path is a subpath of the safe root directory.Implementation:
os.path.commonpath
to compare the normalized path to the safe root directory.Changes Required:
_parse_local_dir_path
insrc/gitingest/query_parser.py
to include validation logic.Suggested fixes powered by Copilot Autofix. Review carefully before merging.