🛡️ Sentinel: [CRITICAL] Fix Path Traversal in Telemetry#83
🛡️ Sentinel: [CRITICAL] Fix Path Traversal in Telemetry#83heidi-dang wants to merge 1 commit intofeat/bootstrap-scaffoldfrom
Conversation
- Hardened `get_run_dir` by using `Path(run_id).name` to sanitize `run_id` and prevent path traversal. - Added validation in `_rotate_events_log` to ensure the rotation target stays within the `runs/` directory. - Added security-focused documentation comments. - Cleaned up whitespace and linting issues in `telemetry.py`. - Verified fixes with reproduction scripts and existing tests.
|
👋 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. |
Summary of ChangesHello @heidi-dang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical path traversal vulnerability within the telemetry module. It hardens path construction by sanitizing user-provided run IDs and implements robust validation to ensure file operations remain confined to designated directories. These changes significantly enhance the system's security posture against malicious file system manipulation. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical path traversal vulnerability. The two-pronged fix is excellent: sanitizing user input in get_run_dir and adding a defense-in-depth check in _rotate_events_log. I have one suggestion to make the path validation in _rotate_events_log even more robust and idiomatic.
| if runs_root not in resolved_file.parents: | ||
| print(f"[ERROR] Security violation: Attempted to rotate file outside runs directory: {resolved_file}", file=sys.stderr) | ||
| return |
There was a problem hiding this comment.
This is a great defense-in-depth check. For improved robustness and to follow a more idiomatic pathlib approach, consider replacing the if check with Path.relative_to(). This method cleanly handles various edge cases (like symlinks or the path being the root itself) by raising a ValueError if the path is not a sub-path, which can be caught. This is generally considered more reliable than checking the parents attribute.
| if runs_root not in resolved_file.parents: | |
| print(f"[ERROR] Security violation: Attempted to rotate file outside runs directory: {resolved_file}", file=sys.stderr) | |
| return | |
| try: | |
| resolved_file.relative_to(runs_root) | |
| except ValueError: | |
| print(f"[ERROR] Security violation: Attempted to rotate file outside runs directory: {resolved_file}", file=sys.stderr) | |
| return |
🚨 Severity: CRITICAL
💡 Vulnerability: Path Traversal in Telemetry module.
The
get_run_dirfunction was using rawrun_idinput (from environment variables or CLI) to construct paths, allowing an attacker to use..to escape the intendedruns/directory. The_rotate_events_logfunction also lacked validation to ensure it was only operating on files within theruns/hierarchy.🎯 Impact:
An attacker could:
init_telemetry(run_id="../../malicious")).events_logpath was manipulated.🔧 Fix:
get_run_dirto use only the.namepart of the providedrun_id._rotate_events_logthat resolves the path and verifies it is a child of theruns/root.SECURITY:docstring sections to highlight these measures.✅ Verification:
runs/root before the fix.run_idstrings (e.g.,../../maliciousbecomesmalicious)._rotate_events_lognow explicitly blocks and logs attempts to rotate files outside theruns/root.PR created automatically by Jules for task 8137947365068231499 started by @heidi-dang