🛡️ Sentinel: [CRITICAL/HIGH] Fix SafeStaticFiles path validation AttributeError#76
Conversation
Replaced `Path(path).lower()` with `Path(path.lower())` in `SafeStaticFiles` middleware in `src/audioformation/server/app.py`. `PosixPath` objects do not have a `.lower()` method, leading to 500 errors on requests for static files. Case-insensitivity logic is now properly preserved by lowering the path string prior to creating the Path object. 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 an AttributeError in the SafeStaticFiles class by normalizing the path string to lowercase before creating a Path object, and updates the sentinel log accordingly. Feedback highlights an inconsistency in how sensitive resources are filtered and recommends adding regression tests to verify the security logic.
| async def get_response(self, path: str, scope) -> Response: | ||
| # Normalize path for check | ||
| p = Path(path).lower() | ||
| p = Path(path.lower()) |
There was a problem hiding this comment.
The security logic on line 28 is inconsistent: .env is only checked against the filename (p.name), while 00_config and .git are checked against all path components (p.parts). If .env were a directory (e.g., .env.secrets/), files inside it would not be blocked. To be more robust and consistent, consider checking if any part of the path starts with .env.
| ## 2024-05-24 - Fix SafeStaticFiles path validation AttributeError | ||
| **Vulnerability:** The application was throwing an `AttributeError` during static file path validation because it was trying to call `.lower()` on a `Path` object in `src/audioformation/server/app.py` (`Path(path).lower()`). This resulted in an unhandled exception, causing the application to crash or return a 500 error when serving any static files. | ||
| **Learning:** `PosixPath` objects do not have a `lower()` method. Path strings should be normalized *before* being passed to `Path()`. This code would fail on the first request for a static file and completely break the frontend or block valid operations. | ||
| **Prevention:** Always ensure string operations are performed on string types before casting them to Path objects, and include integration tests that actually make requests to the running application to catch obvious typos in core middleware/routing. |
There was a problem hiding this comment.
While the prevention section correctly identifies the need for integration tests, this PR does not include any new tests, and the verification steps suggest skipping existing server tests. Adding a regression test that specifically verifies the blocking logic of SafeStaticFiles would ensure this critical path remains functional and secure.
socialawy
deleted the
sentinel-fix-safestaticfiles-attributeerror-15630560909867031651
branch
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
2 participants
🚨 Severity: CRITICAL
💡 Vulnerability: The static file serving capability using the custom
SafeStaticFilesclass was completely broken due to attempting to call.lower()on apathlib.Pathobject instead of a string. This resulted in a 500 error for every frontend request.🎯 Impact: Prevents the application dashboard and file streaming from working, breaking overall application functionality.
🔧 Fix: Changed
Path(path).lower()toPath(path.lower()), fixing the syntax error and properly implementing case-insensitive path comparison for blocked sensitive files (like.env).✅ Verification: Verify that all relevant test cases (except incompatible Server/E2E ones) pass via
uv run pytest tests/ --no-cov -k 'not test_e2e_pipeline and not test_server and not test_xtts'.Also, documented this instance in the Sentinel journal
.jules/sentinel.mdas an example of needing to ensure strings are lowered before conversion into paths to avoid application-breaking unhandled exceptions in production.PR created automatically by Jules for task 15630560909867031651 started by @socialawy