🛡️ Sentinel: [HIGH] Fix path traversal validation logic and SafeStaticFiles exceptions#97
Conversation
Replaces string-based absolute path checks in `validate_path_within` with strict `Path.resolve().is_relative_to()` to prevent symlink and traversal bypasses. Adds explicit type checking and `try/except` wrapping in `SafeStaticFiles` to ensure malformed inputs securely "fail closed" with HTTP exceptions rather than 500 errors. 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. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| return False | ||
| except (ValueError, RuntimeError, OSError): | ||
| resolved_path = path.resolve() |
| return False | ||
| except (ValueError, RuntimeError, OSError): | ||
| resolved_path = path.resolve() | ||
| resolved_root = root.resolve() |
![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 hardens path validation logic to prevent path traversal and symlink bypass attacks. Key improvements include refactoring validate_path_within to use Path.resolve().is_relative_to() and updating the SafeStaticFiles middleware to fail securely on malformed or null paths. Feedback includes a correction for an invalid ruff version in uv.lock, a recommendation to remove internal plan files from the repository, and a suggestion to use more specific exception handling in the server middleware to avoid masking unrelated issues.
| { name = "python-dotenv", marker = "extra == 'cloud'", specifier = ">=1.0,<2" }, | ||
| { name = "python-multipart", marker = "extra == 'server'", specifier = ">=0.0.27,<1" }, | ||
| { name = "ruff", marker = "extra == 'dev'", specifier = ">=0.1.9" }, | ||
| { name = "ruff", marker = "extra == 'dev'", specifier = ">=0.15.1" }, |
There was a problem hiding this comment.
The version specifier for ruff is set to >=0.15.1, but this version does not exist on PyPI (the current latest version is in the 0.9.x range). This invalid version will cause dependency resolution to fail during installation. Please correct the version number to a valid one.
| 1. Modify `src/audioformation/utils/security.py` using `replace_with_git_merge_diff` to rewrite `validate_path_within` to use `Path.resolve().is_relative_to()` and catch `(TypeError, ValueError, RuntimeError, AttributeError, OSError)`. | ||
| 2. Verify the changes to `src/audioformation/utils/security.py` by using `run_in_bash_session` with `cat src/audioformation/utils/security.py`. | ||
| 3. Modify `src/audioformation/server/app.py` using `replace_with_git_merge_diff` to add `path is not None` check and `try...except` wrapper in `SafeStaticFiles.get_response` method. | ||
| 4. Verify the changes to `src/audioformation/server/app.py` by using `run_in_bash_session` with `cat src/audioformation/server/app.py`. | ||
| 5. Run the required tests by using `run_in_bash_session` with `uv run pytest tests/ -k 'not test_e2e_pipeline and not test_server and not test_xtts'`. | ||
| 6. Run code style checks by using `run_in_bash_session` with `uv run ruff check` and `uv run ruff format`. | ||
| 7. Update `.jules/sentinel.md` by using `run_in_bash_session` to run `cat << 'EOF' >> .jules/sentinel.md` with the new learning entry. | ||
| 8. Complete pre-commit steps to ensure proper testing, verification, review, and reflection are done. | ||
| 9. Submit PR using the `submit` tool with branch name `sentinel-fix-path-traversal` and title `🛡️ Sentinel: [HIGH] Fix path traversal validation logic and SafeStaticFiles exceptions`. |
There was a problem hiding this comment.
This file appears to be an internal execution plan for the automated agent and should not be committed to the repository. Please remove it before merging to keep the codebase clean.
| 1. **Fix path validation in `src/audioformation/utils/security.py`** | ||
| - Replace string-based manipulation (`os.path.abspath` and `startswith`) with strict `Path.resolve().is_relative_to()`. | ||
| - Update the exception catching to handle `(TypeError, ValueError, RuntimeError, AttributeError, OSError)` to fail closed securely on malformed inputs. | ||
| 2. **Hardening `SafeStaticFiles` in `src/audioformation/server/app.py`** | ||
| - Explicitly validate that `path` is not None. | ||
| - Wrap the path normalization in a `try...except` block, raising `HTTPException(status_code=400)` to ensure it fails closed securely on malformed inputs. | ||
| 3. **Verify code changes** | ||
| - Run `uv run pytest tests/` with the required flags to ensure no tests break. | ||
| - Run `uv run ruff check` and `uv run ruff format` to ensure code style is maintained. | ||
| 4. **Update `.jules/sentinel.md`** | ||
| - Add a journal entry documenting the learning about replacing `os.path.abspath` checks with strict `Path.resolve()` for symlink handling, and failing closed on exceptions. | ||
| 5. **Complete pre-commit steps** | ||
| - Complete pre-commit steps to ensure proper testing, verification, review, and reflection are done. | ||
| 6. **Submit PR** | ||
| - Submit the PR with the title `🛡️ Sentinel: [HIGH] Fix path traversal validation logic`. |
There was a problem hiding this comment.
This file appears to be an internal test plan for the automated agent and should not be committed to the repository. Please remove it before merging.
| except Exception: | ||
| raise HTTPException(status_code=400, detail="Invalid path format") |
There was a problem hiding this comment.
For better precision and consistency with the implementation in src/audioformation/utils/security.py, consider catching only the specific exceptions mentioned in the PR description (TypeError, ValueError, RuntimeError, AttributeError, OSError). Catching a broad Exception can mask unrelated bugs or system-level issues that should ideally bubble up.
| except Exception: | |
| raise HTTPException(status_code=400, detail="Invalid path format") | |
| except (TypeError, ValueError, RuntimeError, AttributeError, OSError): | |
| raise HTTPException(status_code=400, detail="Invalid path format") |
2 participants
🚨 Severity: HIGH
💡 Vulnerability: Path Validation Bypass & Fail-Open in Static Files Gate
🎯 Impact: The
validate_path_withincheck usedos.path.abspathand.startswith(), which is susceptible to path traversal via symlinks or specific casing bypasses. Additionally, theSafeStaticFilesrouting threw unhandled exceptions on malformed (NoneType) inputs, creating a fail-open or DoS vector.🔧 Fix: Enforced
Path.resolve().is_relative_to()for robust directory bounding. Caught common error classes(TypeError, ValueError, RuntimeError, AttributeError, OSError)securely to fail closed. Wrapped static file parsing in standard HTTP exceptions.✅ Verification: Tests passing with mocked path boundary traversals proving rejection. Output verified.
PR created automatically by Jules for task 15204369301377167809 started by @socialawy