fix: enhance path traversal protection#321
fix: enhance path traversal protection#321LemonZuo wants to merge 3 commits intoBeehiveInnovations:mainfrom
Conversation
Summary of ChangesHello @LemonZuo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly bolsters the application's security posture by addressing a critical path traversal vulnerability. It introduces a sophisticated, multi-faceted validation mechanism for file paths, ensuring that all file system interactions are safe and confined to intended areas, thereby preventing unauthorized access to sensitive system files or directories. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances path traversal protection, addressing a critical security vulnerability. The core logic in is_dangerous_path is now much more robust, using a multi-layered defense that includes pattern matching, null byte checks, and safe relative_to() checks against allow- and denylists. The new security validation is correctly integrated into file reading functions in file_utils.py and image_utils.py. The addition of a comprehensive test suite in test_path_traversal_security.py is excellent and covers a wide range of attack vectors.
I've identified a high-severity issue with the path traversal pattern matching that could lead to false positives, and a medium-severity performance issue due to repeated path resolutions in a loop. My feedback includes suggestions to address these points.
| # Check for path traversal patterns BEFORE resolution | ||
| # This prevents attacks that use symlinks or other techniques | ||
| traversal_patterns = [ | ||
| "..", # Unix/Windows path traversal | ||
| "..%2f", # URL encoded forward slash | ||
| "..%2F", # URL encoded forward slash (uppercase) | ||
| "..%5c", # URL encoded backslash | ||
| "..%5C", # URL encoded backslash (uppercase) | ||
| "%2e%2e", # Double URL encoded dots | ||
| "%252e%252e", # Double URL encoded | ||
| "..;/", # Path traversal with semicolon | ||
| "..\\x2f", # Hex encoded slash | ||
| "..\\x5c", # Hex encoded backslash | ||
| ] | ||
|
|
||
| # Check for any traversal patterns | ||
| for pattern in traversal_patterns: | ||
| if pattern in path_str.lower(): | ||
| return True |
There was a problem hiding this comment.
The current check for path traversal patterns using if ".." in path_str.lower() is too broad and can lead to false positives. For example, a valid filename like notes..txt or a directory version-1.0..beta would be incorrectly flagged as dangerous. The check for .. should be more specific to when it's used as a path component for traversal. A more robust approach is to check for .. in the path's components, reserving the substring search for encoded patterns.
# Check for path traversal patterns BEFORE resolution
# This prevents attacks that use symlinks or other techniques
# Using `path.parts` is more robust for ".." and avoids false positives.
if ".." in path.parts:
return True
traversal_patterns = [
"..%2f", # URL encoded forward slash
"..%2F", # URL encoded forward slash (uppercase)
"..%5c", # URL encoded backslash
"..%5C", # URL encoded backslash (uppercase)
"%2e%2e", # Double URL encoded dots
"%252e%252e", # Double URL encoded
"..;/", # Path traversal with semicolon
"..\\x2f", # Hex encoded slash
"..\\x5c", # Hex encoded backslash
]
# Check for any encoded traversal patterns
for pattern in traversal_patterns:
if pattern in path_str.lower():
return True| for dangerous in DANGEROUS_PATHS: | ||
| try: | ||
| # Skip root directory check if path is in safe locations | ||
| if dangerous == "/": | ||
| # For root, only block if it's actually the root | ||
| if resolved == Path("/").resolve(): | ||
| return True | ||
| # Skip the relative_to check for root | ||
| continue | ||
|
|
||
| # Try to create a relative path from the dangerous directory | ||
| danger_path = Path(dangerous).resolve() | ||
| resolved.relative_to(danger_path) | ||
| # If successful, the path is under a dangerous directory | ||
| return True | ||
| except ValueError: | ||
| # Path is not relative to this dangerous directory | ||
| continue | ||
| except Exception: | ||
| # Handle any other errors safely | ||
| continue |
There was a problem hiding this comment.
The function repeatedly calls Path(dangerous).resolve() inside a loop for every path check. This is inefficient as the set of dangerous paths is constant. These paths can be resolved once at the module level to improve performance, especially since this function may be called frequently. A similar optimization can be applied to the safe_prefixes check.
You could define the resolved paths at the module level like this:
# At module level
_RESOLVED_DANGEROUS_PATHS = set()
for p in DANGEROUS_PATHS:
if p == "/": continue
try:
_RESOLVED_DANGEROUS_PATHS.add(Path(p).resolve())
except Exception:
# Path might not exist on this OS (e.g., C:\\Windows on Linux), so we skip.
pass
# And then use it in the loop:
for danger_path in _RESOLVED_DANGEROUS_PATHS:
try:
resolved.relative_to(danger_path)
return True # It's a subpath of a dangerous one
except ValueError:
continue
2 participants
Description
This PR addresses critical security vulnerabilities reported in #293. It implements comprehensive path traversal protection
using the relative_to() method as recommended by the security researcher.
Changes Made
Testing
Code quality checks passed
./code_quality_checks.sh
✅ All linting and formatting checks passed
Unit tests
python -m unittest tests.test_path_traversal_security -v
✅ 12 tests passed
Security validation tests
✅ Subdirectory blocking validated
✅ Path traversal patterns detected
✅ Windows paths properly blocked
✅ Issue #293 PoC successfully prevented
Related Issues
Additional Notes
Security Improvements:
Test Coverage: