🛡️ Sentinel: [HIGH] Fix Path Traversal Symlink Bypass & File Server Exceptions#91
Conversation
- Replaces string-based `os.path.abspath` comparisons with robust `Path.resolve().is_relative_to()` check in `validate_path_within` to prevent symlink traversal. - Adds comprehensive exception handling in `validate_path_within` to catch TypeError/None inputs securely. - Wraps `SafeStaticFiles.get_response` in a try-except block to gracefully catch malformed/None paths and return HTTP 400 instead of failing open or 500ing. 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. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if path is None or root is None: | ||
| return False | ||
|
|
||
| resolved_root = root.resolve() |
| return False | ||
|
|
||
| resolved_root = root.resolve() | ||
| resolved_path = path.resolve() |
![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 addresses path traversal and symlink bypass vulnerabilities by refactoring path validation logic to use Path.resolve() and is_relative_to in the security utilities. It also enhances the SafeStaticFiles middleware with a fail-closed error handling strategy and explicit input validation. Additionally, development dependencies for mypy and ruff were updated. Review feedback highlights that the blacklist check in the middleware remains lexical and could still be bypassed via symlinks, suggesting a more robust check for .env files and path resolution before validation.
| if "00_config" in p.parts or p.name.startswith(".env") or ".git" in p.parts: | ||
| raise HTTPException( | ||
| status_code=403, detail="Access denied to sensitive resource" | ||
| ) |
There was a problem hiding this comment.
The blacklist check in SafeStaticFiles remains purely lexical and does not account for symlinks, which contradicts the pull request's stated goal of fixing symlink bypasses. An attacker could potentially bypass these restrictions by using a symlink that points to a sensitive directory but does not contain the blacklisted strings in its path (e.g., ln -s 00_config public_link). To fully address this, the middleware should resolve the requested path against the filesystem root before checking. Additionally, the current .env check only validates the last component of the path (p.name); using any() on p.parts would more robustly block access to any directory or file starting with .env.
| if "00_config" in p.parts or p.name.startswith(".env") or ".git" in p.parts: | |
| raise HTTPException( | |
| status_code=403, detail="Access denied to sensitive resource" | |
| ) | |
| if "00_config" in p.parts or ".git" in p.parts or any(part.startswith(".env") for part in p.parts): | |
| raise HTTPException( | |
| status_code=403, detail="Access denied to sensitive resource" | |
| ) |
2 participants
🚨 Severity: HIGH
💡 Vulnerability: Path traversal via symlink bypass and potential denial-of-service/fail-open from unhandled path resolution exceptions in SafeStaticFiles.
🎯 Impact: Attackers could potentially bypass static directory restrictions using maliciously crafted symlinks, or cause unhandled 500 server errors (DoS) / fail open states by passing invalid (e.g. None) paths to the SafeStaticFiles class.
🔧 Fix: Upgraded
validate_path_withinto strictly usePath.resolve().is_relative_to()and broadened exception handling. Added strict type checks and a try-catch block around the static file access gateway.✅ Verification: Ran full pytest test suite (including test_security.py and test_server_routes.py) to ensure all tests passed successfully.
PR created automatically by Jules for task 12193734970889885254 started by @socialawy