🛡️ Sentinel: [CRITICAL] Fix 500 Server Error due to Path.lower() AttributeError causing SafeStaticFiles bypass#59
Conversation
Modified SafeStaticFiles.get_response to correctly lower-case the path string before instantiating the Path object. This prevents an AttributeError when the server attempts to validate static file requests against the security blocklist. 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 critical AttributeError in the SafeStaticFiles middleware by ensuring path strings are normalized before being converted to Path objects. It also documents this vulnerability in the security log. Feedback was provided regarding a missing newline in the documentation file which disrupts the Markdown 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.## 2025-02-21 - SafeStaticFiles Bypass via AttributeError |
There was a problem hiding this comment.
The new vulnerability entry is appended directly to the end of the previous line without a newline. This breaks the markdown structure and makes the security log difficult to read. Please ensure there is a blank line between entries.
| **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-21 - SafeStaticFiles Bypass via AttributeError | |
| **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-21 - SafeStaticFiles Bypass via AttributeError |
socialawy
deleted the
sentinel/fix-safestaticfiles-attribute-error-17310240228911556860
branch
Understood. Acknowledging that this work is obsolete because the root |
2 participants
🚨 Severity: CRITICAL
💡 Vulnerability:
The
SafeStaticFilesclass, which serves as a security boundary preventing access to sensitive directories (like00_configor.env), had a bug where it called.lower()directly on apathlib.Pathobject (Path(path).lower()). BecausePosixPathhas no.lower()method, this resulted in a 500AttributeError, causing the file server to crash or the security check to fail closed, effectively breaking the static file serving capability and the intended security boundary.🎯 Impact:
Any attempt to access static files through the
SafeStaticFilesmount would result in an immediate 500 Internal Server Error, rendering the dashboard and project serving offline. Furthermore, an exception in the security control could lead to unpredictable bypasses depending on middleware exception handling.🔧 Fix:
Corrected the normalization logic to lower-case the string before passing it to
Path():Path(path.lower()). This ensures the string is safely normalized for the blocklist check while the unmodified original path is still securely passed to the underlyingStaticFilesresponse handler.✅ Verification:
uv run pytest tests/test_server_routes.pysuccessfully.uv run ruff checkanduv run ruff formatto ensure adherence to linting standards.Path('test').lower()fails whilePath('test'.lower())succeeds.PR created automatically by Jules for task 17310240228911556860 started by @socialawy