🛡️ Sentinel: [CRITICAL/HIGH] Fix path validation vulnerability in SafeStaticFiles#96
🛡️ Sentinel: [CRITICAL/HIGH] Fix path validation vulnerability in SafeStaticFiles#96socialawy-dev wants to merge 1 commit into
Conversation
Updated SafeStaticFiles to correctly block access to all hidden files and directories, securely handle null inputs, and fail closed by catching unexpected exceptions. 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 enhances the security of the SafeStaticFiles middleware by preventing access to all hidden files and directories, handling None path inputs, and wrapping the validation logic in a try...except block to ensure the application fails closed. It also includes updates to the mypy and ruff development dependencies and adds a corresponding entry to the security log. A review comment suggests simplifying the path validation condition by removing a redundant check for the .git directory, which is already covered by the more general hidden file validation logic.
| try: | ||
| # Normalize path for check | ||
| p = Path(str(path).lower()) | ||
| if "00_config" in p.parts or ".git" in p.parts or any(part.startswith(".") for part in p.parts): |
There was a problem hiding this comment.
The check ".git" in p.parts is redundant because the subsequent any(part.startswith(".") for part in p.parts) already covers any file or directory starting with a dot, including .git. Removing it simplifies the condition without changing the logic.
| if "00_config" in p.parts or ".git" in p.parts or any(part.startswith(".") for part in p.parts): | |
| if "00_config" in p.parts or any(part.startswith(".") for part in p.parts): |
1 participant
🚨 Severity: HIGH
💡 Vulnerability: SafeStaticFiles didn't prevent access to hidden files and directories because it only checked if the file name started with ".env". It also did not handle None values or unexpected exceptions properly, violating fail-closed security principles.
🎯 Impact: This could allow an attacker to bypass directory/file restrictions and read sensitive files (such as .hidden_file) and potentially fail open if unhandled exceptions were encountered when serving static paths.
🔧 Fix: Updated SafeStaticFiles to check if the path is None, wrap the path parsing logic in a try...except block to fail securely with 400 or 403 on errors, and ensure it actively checks whether any part of the requested path starts with a dot
.indicating a hidden file/directory.✅ Verification: Tested against paths including 00_CONFIG, .env, hidden files, valid files, and None paths using Python unittests, and the app fails closed for invalid ones.
PR created automatically by Jules for task 4614833920971284354 started by @socialawy