- Notifications
You must be signed in to change notification settings - Fork825
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…in path expressionCo-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.
Uh oh!
There was an error while loading.Please reload this page.
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.
| ifos.path.commonpath([root_path,path_obj])!=str(root_path): | |
| raiseInvalidPatternError(f"Path{path_str} escapes the allowed root directory.") | |
| # Ensure both paths exist | |
| ifnotroot_path.exists()ornotpath_obj.exists(): | |
| raiseInvalidPatternError(f"One or both paths do not exist:{root_path},{path_obj}") | |
| # Check if path_obj is within root_path | |
| try: | |
| ifos.path.commonpath([root_path,path_obj])!=str(root_path): | |
| raiseInvalidPatternError(f"Path{path_str} escapes the allowed root directory.") | |
| exceptValueErrorase: | |
| raiseInvalidPatternError(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.
| ifos.path.commonpath([root_path,path_obj])!=str(root_path): | |
| ifos.path.commonpath([str(root_path),str(path_obj)])!=str(root_path): |
Copilot uses AI. Check for mistakes.
Potential fix forhttps://github.com/coderamp-labs/gitingest/security/code-scanning/77
To address this issue, the
_parse_local_dir_pathfunction needs to validate thepath_strinput 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.commonpathto compare the normalized path to the safe root directory.Changes Required:
_parse_local_dir_pathinsrc/gitingest/query_parser.pyto include validation logic.Suggested fixes powered by Copilot Autofix. Review carefully before merging.