🛡️ Sentinel: [CRITICAL] Fix Path Traversal Prevention Bypass#110
🛡️ Sentinel: [CRITICAL] Fix Path Traversal Prevention Bypass#110socialawy-dev wants to merge 1 commit into
Conversation
Replaces string-based prefix checking with secure symlink resolution in `validate_path_within`. Also expanded exception handling to catch null inputs and other edge case errors during validation. Co-authored-by: socialawy <24765060+socialawy@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request simplifies the path validation logic in validate_path_within to prevent path traversal and symlink bypass attacks, while also documenting the vulnerability in the sentinel log. The feedback highlights a compatibility concern where is_relative_to (introduced in Python 3.9) might fail on older Python versions, and suggests using Path.relative_to() along with defensive Path() wrapping as a robust alternative.
| if path is None or root is None: | ||
| return False | ||
| except (ValueError, RuntimeError, OSError): | ||
|
|
||
| try: | ||
| resolved_root = root.resolve() | ||
| resolved_path = path.resolve() | ||
| return resolved_path.is_relative_to(resolved_root) | ||
| except (TypeError, ValueError, RuntimeError, AttributeError, OSError): | ||
| return False |
There was a problem hiding this comment.
The PurePath.is_relative_to method was introduced in Python 3.9. If this application is run on Python 3.8 or lower, calling is_relative_to will raise an AttributeError, which is caught by the except block and causes the function to always return False. This would break all path validation checks (e.g., file uploads and previews).
To ensure backward compatibility with Python 3.4+, you can use Path.relative_to() instead. Since ValueError is already caught in the except block, this is a safe and robust drop-in replacement. Additionally, wrapping the inputs in Path() defensively ensures the function works correctly even if strings are passed.
| if path is None or root is None: | |
| return False | |
| except (ValueError, RuntimeError, OSError): | |
| try: | |
| resolved_root = root.resolve() | |
| resolved_path = path.resolve() | |
| return resolved_path.is_relative_to(resolved_root) | |
| except (TypeError, ValueError, RuntimeError, AttributeError, OSError): | |
| return False | |
| if path is None or root is None: | |
| return False | |
| try: | |
| resolved_root = Path(root).resolve() | |
| resolved_path = Path(path).resolve() | |
| resolved_path.relative_to(resolved_root) | |
| return True | |
| except (TypeError, ValueError, RuntimeError, AttributeError, OSError): | |
| return False |
| except (ValueError, RuntimeError, OSError): | ||
|
|
||
| try: | ||
| resolved_root = root.resolve() |
|
|
||
| try: | ||
| resolved_root = root.resolve() | ||
| resolved_path = path.resolve() |
🚨 Severity: CRITICAL
💡 Vulnerability: The custom
validate_path_withinfunction used string manipulation (os.path.abspath) andstartswith()for its preliminary path check. This allows path traversal payload bypasses through malicious symlinks, because string operations do not resolve physical filesystem links, enabling access outside the intended root directory.🎯 Impact: Potential bypass of sandboxed file operations throughout the application where user input interacts with the filesystem.
🔧 Fix: Refactored
validate_path_withinto exclusively usePath.resolve()on both target and root, followed byPath.is_relative_to(). Added defensive null input checks and broad exception handling to ensure it always fails securely closed.✅ Verification: Verified by passing the
pytesttest suite, including dedicated security tests. Ensure standard testing and linting (ruff) commands succeed.PR created automatically by Jules for task 14109035593781470194 started by @socialawy