🛡️ Sentinel: [CRITICAL] Fix Path Traversal validation and DoS in SafeStaticFiles#60
Conversation
…StaticFiles 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. |
|
|
||
| return False | ||
| except (ValueError, RuntimeError, OSError): | ||
| resolved_root = root.resolve() |
| return False | ||
| except (ValueError, RuntimeError, OSError): | ||
| resolved_root = root.resolve() | ||
| resolved_path = path.resolve() |
There was a problem hiding this comment.
Code Review
This pull request fixes a path traversal bypass and type confusion vulnerability by correcting a type error in SafeStaticFiles and refactoring validate_path_within to use pathlib's is_relative_to method for more robust validation. The sentinel log was also updated to document the issue. Feedback was provided regarding a missing newline in the Markdown documentation that affects header rendering.
| **Vulnerability:** The `/projects/{project_id}/mix` API endpoint in `src/audioformation/server/routes.py` accepted a `music` parameter (meant to specify a filename within the `05_MUSIC/generated` directory) but directly passed it to `mix_project` without sanitization. This allowed directory traversal payloads like `../../../etc/passwd` to be used for background music resolution. | ||
| **Learning:** Even internal API inputs that map strictly to filenames inside an expected directory must be sanitized. A simple check for file existence (`if not bg_music_path.exists():`) is insufficient as it confirms existence but allows looking outside the bounded directory. | ||
| **Prevention:** Always use established sanitization helpers (like `sanitize_filename`) or bound checks (like `validate_path_within`) for any user-supplied string that forms part of a filesystem path. Ensure bypass parameters like `FORCE_NO_MUSIC` are handled before and mutually exclusively from sanitization. No newline at end of file | ||
| **Prevention:** Always use established sanitization helpers (like `sanitize_filename`) or bound checks (like `validate_path_within`) for any user-supplied string that forms part of a filesystem path. Ensure bypass parameters like `FORCE_NO_MUSIC` are handled before and mutually exclusively from sanitization.## 2025-02-23 - Path Traversal Bypass and File Disclosure via Type Confusion |
There was a problem hiding this comment.
The new vulnerability entry header (## 2025-02-23) is appended directly to the end of the previous line. This will prevent it from being rendered as a header in Markdown. Please add a newline before the new entry to maintain proper document structure.
| **Prevention:** Always use established sanitization helpers (like `sanitize_filename`) or bound checks (like `validate_path_within`) for any user-supplied string that forms part of a filesystem path. Ensure bypass parameters like `FORCE_NO_MUSIC` are handled before and mutually exclusively from sanitization.## 2025-02-23 - Path Traversal Bypass and File Disclosure via Type Confusion | |
| **Prevention:** Always use established sanitization helpers (like `sanitize_filename`) or bound checks (like `validate_path_within`) for any user-supplied string that forms part of a filesystem path. Ensure bypass parameters like `FORCE_NO_MUSIC` are handled before and mutually exclusively from sanitization. | |
| ## 2025-02-23 - Path Traversal Bypass and File Disclosure via Type Confusion |
There was a problem hiding this comment.
Code Review
This pull request addresses path traversal and type confusion vulnerabilities by fixing a bug in SafeStaticFiles where .lower() was incorrectly called on a Path object and by simplifying the validate_path_within utility to use pathlib's is_relative_to method. Additionally, the security log in .jules/sentinel.md was updated, though a missing newline between entries was identified that affects Markdown rendering.
| **Vulnerability:** The `/projects/{project_id}/mix` API endpoint in `src/audioformation/server/routes.py` accepted a `music` parameter (meant to specify a filename within the `05_MUSIC/generated` directory) but directly passed it to `mix_project` without sanitization. This allowed directory traversal payloads like `../../../etc/passwd` to be used for background music resolution. | ||
| **Learning:** Even internal API inputs that map strictly to filenames inside an expected directory must be sanitized. A simple check for file existence (`if not bg_music_path.exists():`) is insufficient as it confirms existence but allows looking outside the bounded directory. | ||
| **Prevention:** Always use established sanitization helpers (like `sanitize_filename`) or bound checks (like `validate_path_within`) for any user-supplied string that forms part of a filesystem path. Ensure bypass parameters like `FORCE_NO_MUSIC` are handled before and mutually exclusively from sanitization. No newline at end of file | ||
| **Prevention:** Always use established sanitization helpers (like `sanitize_filename`) or bound checks (like `validate_path_within`) for any user-supplied string that forms part of a filesystem path. Ensure bypass parameters like `FORCE_NO_MUSIC` are handled before and mutually exclusively from sanitization.## 2025-02-23 - Path Traversal Bypass and File Disclosure via Type Confusion |
There was a problem hiding this comment.
The new entry for 2025-02-23 is appended directly to the end of the previous entry's 'Prevention' section without a newline. This will cause the Markdown renderer to treat the new header as plain text on the same line. Please add a newline between the entries to ensure correct formatting.
| **Prevention:** Always use established sanitization helpers (like `sanitize_filename`) or bound checks (like `validate_path_within`) for any user-supplied string that forms part of a filesystem path. Ensure bypass parameters like `FORCE_NO_MUSIC` are handled before and mutually exclusively from sanitization.## 2025-02-23 - Path Traversal Bypass and File Disclosure via Type Confusion | |
| **Prevention:** Always use established sanitization helpers (like `sanitize_filename`) or bound checks (like `validate_path_within`) for any user-supplied string that forms part of a filesystem path. Ensure bypass parameters like `FORCE_NO_MUSIC` are handled before and mutually exclusively from sanitization. | |
| ## 2025-02-23 - Path Traversal Bypass and File Disclosure via Type Confusion |
Understood. Acknowledging that this work is superseded by #90 and stopping work on this task. |
🚨 Severity: CRITICAL
💡 Vulnerability: The path validation function
validate_path_withinrelied on string-based comparison (os.path.abspath().startswith()) which can be bypassed using complex symlinks. Additionally,SafeStaticFilesin the FastAPI app contained a type error (Path.lower()) that triggered anAttributeError, resulting in a Denial of Service (500 Internal Server Error) and failing the sensitive resource blocklist checks.🎯 Impact: Allowed potential bypass of path traversal protections and caused DoS on static file serving.
🔧 Fix:
validate_path_withinto exclusively usePath.resolve().is_relative_to()and broadened exception handling to fail securely.SafeStaticFilesby lowercasing the string before instantiating thePathobject.✅ Verification: Verified by unit tests and manually confirmed
Path.lower()bug is resolved.PR created automatically by Jules for task 8328123637300980948 started by @socialawy