🛡️ Sentinel: [HIGH] Fix SafeStaticFiles path attribute error#58
🛡️ Sentinel: [HIGH] Fix SafeStaticFiles path attribute error#58socialawy-dev wants to merge 1 commit into
Conversation
- Fixed an `AttributeError` in `SafeStaticFiles` where `Path(path).lower()` was called. This crashed the request or bypassed checks designed to block access to sensitive directories like `00_CONFIG` and `.env` files. Replaced with `Path(path.lower())`. - Improved exception handling in `validate_path_within` by catching `TypeError` and `AttributeError` to fail securely when handling malformed paths. 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. |
![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 fixes a security vulnerability in SafeStaticFiles where an AttributeError caused by calling .lower() on a Path object bypassed security checks. The fix ensures the string is lowercased before Path instantiation. Additionally, validate_path_within now handles TypeError and AttributeError to fail securely. A new entry was added to sentinel.md to document the fix, though a review comment correctly identified a missing newline that breaks the markdown header formatting.
| **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.## 2024-05-24 - [Fix Path check bypass in SafeStaticFiles] |
There was a problem hiding this comment.
The new entry header is appended directly to the end of the previous paragraph without a newline. This breaks the markdown formatting for the header and reduces readability.
| **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.## 2024-05-24 - [Fix Path check bypass in SafeStaticFiles] | |
| **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. | |
| ## 2024-05-24 - [Fix Path check bypass in SafeStaticFiles] |
socialawy
deleted the
sentinel-fix-safestaticfiles-check-17652888875589070390
branch
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
2 participants
🚨 Severity: HIGH
💡 Vulnerability: The
SafeStaticFilesmiddleware inapp.py, which is intended to prevent users from accessing sensitive directories like00_CONFIG(containing API keys) and.envfiles, contained a logic error. It called.lower()on apathlib.Pathobject (Path(path).lower()), which raises anAttributeError. This causes the check to fail and can result in application crashes or bypasses to the security mitigation.🎯 Impact: If exploited, attackers might bypass the static file path restrictions or trigger repeated 500 errors.
🔧 Fix: Modified the logic to lowercase the string before path resolution:
Path(path.lower()). Additionally, strengthenedvalidate_path_withininsecurity.pyto catchTypeErrorandAttributeErrorso it fails securely when fed invalid data types.✅ Verification: Ran test suite (
uv run pytest) and verified changes usingread_fileto ensure proper functionality and security.PR created automatically by Jules for task 17652888875589070390 started by @socialawy