🛡️ Sentinel: Fix path traversal and telemetry bugs#158
🛡️ Sentinel: Fix path traversal and telemetry bugs#158heidi-dang wants to merge 1 commit intofeat/bootstrap-scaffoldfrom
Conversation
This commit addresses a high-severity path traversal vulnerability in the telemetry and dashboard modules. By sanitizing `run_id` inputs using `Path(run_id).name`, we ensure that file operations are restricted to the intended `runs/` directory, preventing unauthorized access to other parts of the filesystem. Key changes: - Sanitized `run_id` in `heidi_engine/telemetry.py` and `heidi_engine/dashboard.py`. - Fixed a `NameError` in `telemetry.get_state` caused by broken cache logic. - Standardized `AUTOTRAIN_DIR` default value across modules. - Modernized `datetime.utcnow()` calls to use `datetime.now(timezone.utc)`. - Added regression tests in `tests/test_security.py`. - Documented findings in `.jules/sentinel.md`.
|
👋 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, 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 significantly enhances the security and stability of the system by addressing a critical path traversal vulnerability in the telemetry and dashboard modules. It also fixes a 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 in telemetry.py and dashboard.py by sanitizing the run_id. It also fixes a NameError in telemetry.get_state, standardizes the AUTOTRAIN_DIR path for consistency, and modernizes the code by replacing deprecated datetime.utcnow() calls. The addition of regression tests in tests/test_security.py is a great step to prevent future issues. I have one suggestion to improve the new test file to align with standard pytest practices.
| if __name__ == "__main__": | ||
| test_path_traversal_prevention() | ||
| test_name_error_fixed() | ||
| print("Security verification tests passed!") |
There was a problem hiding this comment.
While this if __name__ == '__main__': block allows for direct execution of the script, it's redundant when using a test runner like pytest, which is already imported. The test runner will automatically discover and execute functions prefixed with test_. Relying solely on the test runner is a best practice that ensures tests are run in a consistent environment with proper setup, teardown, and reporting. I suggest removing this block to encourage the use of pytest for test execution.
I have identified and fixed a path traversal vulnerability in the telemetry and dashboard modules. An attacker could previously use a manipulated
run_id(e.g.,../secrets) to access files outside the intendedruns/directory.I have implemented the following improvements:
run_idusingPath(run_id).namein bothheidi_engine/telemetry.pyandheidi_engine/dashboard.pyto ensure all paths remain within theruns/subdirectory.NameErrorintelemetry.get_statethat occurred when attempting to use an undefined variabletarget_run_idduring a cache miss.AUTOTRAIN_DIRto~/.local/heidi_engineto maintain consistency across the entire pipeline.datetime.utcnow()calls to the recommendeddatetime.now(timezone.utc)for improved compatibility and accuracy in Python 3.12+.tests/test_security.pycontaining regression tests for the path traversal and NameError fixes. All 23 tests in the suite are passing.I have also updated the Sentinel security journal at
.jules/sentinel.mdwith these findings.PR created automatically by Jules for task 803352304742846814 started by @heidi-dang