🛡️ Sentinel: [CRITICAL] Fix TOCTOU and DoS vulnerability in file reading - #75
🛡️ Sentinel: [CRITICAL] Fix TOCTOU and DoS vulnerability in file reading#75ivangegovdve-sudo wants to merge 1 commit into
Conversation
Co-authored-by: ivangegovdve-sudo <225339531+ivangegovdve-sudo@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. |
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 addresses critical TOCTOU (Time-of-Check to Time-of-Use) and Denial of Service (DoS) vulnerabilities in the application's JSON file-backed adapters. By replacing an insecure file size check with a robust bounded read mechanism and adding explicit file type validation, the changes prevent potential memory exhaustion and application crashes that could arise from malicious or oversized input files. This significantly enhances the stability and security of file processing within the system. Highlights
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. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e9cb77d87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| content = f.read(10 * 1024 * 1024 + 1) | ||
|
|
||
| if len(content) > 10 * 1024 * 1024: | ||
| raise ValueError( |
There was a problem hiding this comment.
This bounded-read check is done in text mode, so f.read(10 * 1024 * 1024 + 1) and len(content) both operate on characters rather than bytes. A UTF-8 JSON file containing multibyte characters can be larger than 10MB on disk while still passing this guard, which is a regression from the previous byte-based st_size limit and weakens the stated DoS protection for non-ASCII payloads. Please enforce the cap in binary mode (byte count) before decoding.
Useful? React with 👍 / 👎.
![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 addresses TOCTOU vulnerabilities and OOM DoS risks in file-backed JSON repositories and stores by implementing a more robust file reading mechanism. This involves checking if a path is a regular file, performing bounded reads, and verifying content length before JSON parsing. The changes are also documented in .jules/sentinel.md. Feedback includes the need to add try...except blocks for json.loads() and handle empty files in checkpoint_store.py for robustness and consistency, refactor the duplicated secure file reading logic into a shared utility function to improve maintainability, and refine the try...except block structure in json_file_progress_snapshot_store.py for clarity and consistency.
| parsed = json.loads(content) | ||
| return parsed if isinstance(parsed, dict) else {} |
There was a problem hiding this comment.
The call to json.loads(content) is not wrapped in a try...except block and does not handle empty or whitespace-only files. If content is empty or not valid JSON, this will raise a json.JSONDecodeError, causing an unhandled exception. Other adapters in this PR handle this case for robustness. For consistency and to prevent crashes, you should handle this potential error.
| parsed = json.loads(content) | |
| return parsed if isinstance(parsed, dict) else {} | |
| try: | |
| parsed = json.loads(content) if content.strip() else {} | |
| return parsed if isinstance(parsed, dict) else {} | |
| except json.JSONDecodeError: | |
| return {} |
| if not path.is_file(): | ||
| return {} | ||
|
|
||
| with open(path, encoding="utf-8") as f: | ||
| content = f.read(10 * 1024 * 1024 + 1) | ||
|
|
||
| if len(content) > 10 * 1024 * 1024: | ||
| raise ValueError(f"Checkpoint file {path} exceeds 10MB size limit") | ||
| parsed = json.loads(path.read_text(encoding="utf-8")) | ||
|
|
||
| parsed = json.loads(content) | ||
| return parsed if isinstance(parsed, dict) else {} |
There was a problem hiding this comment.
This new secure file reading logic is a great improvement. However, this pattern is now duplicated across four different files:
checkpoint_store.pyjson_file_practice_repository.pyjson_file_progress_repository.pyjson_file_progress_snapshot_store.py
This duplication introduces a few issues:
- Maintainability: If this logic needs to be updated in the future (e.g., changing the size limit), it must be changed in all four places, which is error-prone.
- Magic Number: The size limit
10 * 1024 * 1024is a magic number repeated in each implementation.
To address this, I recommend refactoring this logic into a single, shared utility function. This _read_json function is a great candidate to be moved to a shared utility module. Within that utility, you could define the size limit as a constant (e.g., _MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024) to be used by the function. This would centralize the security-critical code, eliminate duplication, and improve maintainability.
| try: | ||
| parsed = json.loads(self._file_path.read_text(encoding="utf-8")) | ||
| with open(self._file_path, encoding="utf-8") as f: | ||
| content = f.read(10 * 1024 * 1024 + 1) | ||
|
|
||
| if len(content) > 10 * 1024 * 1024: | ||
| raise ValueError( | ||
| f"Progress snapshot file {self._file_path} exceeds 10MB size limit" | ||
| ) | ||
|
|
||
| parsed = json.loads(content) | ||
| except (OSError, json.JSONDecodeError): | ||
| return {} | ||
| return parsed if isinstance(parsed, dict) else {} |
There was a problem hiding this comment.
The structure of this try...except block can be improved for clarity and consistency. The final return statement is outside the try block, which can make the control flow harder to follow. Additionally, unlike other adapters, this doesn't explicitly handle empty files before attempting to parse JSON.
Moving the successful return path inside the try block and adding a check for empty content would make the code more readable and align it better with the other adapters.
| try: | |
| parsed = json.loads(self._file_path.read_text(encoding="utf-8")) | |
| with open(self._file_path, encoding="utf-8") as f: | |
| content = f.read(10 * 1024 * 1024 + 1) | |
| if len(content) > 10 * 1024 * 1024: | |
| raise ValueError( | |
| f"Progress snapshot file {self._file_path} exceeds 10MB size limit" | |
| ) | |
| parsed = json.loads(content) | |
| except (OSError, json.JSONDecodeError): | |
| return {} | |
| return parsed if isinstance(parsed, dict) else {} | |
| try: | |
| with open(self._file_path, encoding="utf-8") as f: | |
| content = f.read(10 * 1024 * 1024 + 1) | |
| if len(content) > 10 * 1024 * 1024: | |
| raise ValueError( | |
| f"Progress snapshot file {self._file_path} exceeds 10MB size limit" | |
| ) | |
| if not content.strip(): | |
| return {} | |
| parsed = json.loads(content) | |
| return parsed if isinstance(parsed, dict) else {} | |
| except (OSError, json.JSONDecodeError): | |
| return {} |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The JSON file-backed adapters (
JsonFilePracticeRepository,CheckpointStore,JsonFileProgressRepository,JsonFileProgressSnapshotStore) previously usedpath.stat().st_sizeto check if a file exceeded 10MB before reading the entire file into memory usingpath.read_text(). This introduced two critical risks:stat()checking the size andread_text()reading the file, leading to unbounded memory consumption./dev/zero),st_sizewould report 0 bytes, bypassing the limit check, butread_text()would stream data infinitely until the process crashed with an Out-Of-Memory error.🎯 Impact: Potential Denial of Service (DoS) and application crash via memory exhaustion.
🔧 Fix:
st_sizecheck andread_text()combination with a secure bounded read pattern..is_file()checks to ensure the path points to a regular file, rejecting directories or block devices gracefully.f.read(10 * 1024 * 1024 + 1)to safely read up to 10MB plus 1 byte.ValueErroras before if it does.✅ Verification:
ValueErrorif the bounded read exceeds 10MB.uv run pytest) and confirmed all tests pass.uv run ruff check .) and type checking (uv run mypy) pass successfully.PR created automatically by Jules for task 7814890576515838238 started by @ivangegovdve-sudo