-
Notifications
You must be signed in to change notification settings - Fork 0
π‘οΈ Sentinel: [CRITICAL] Fix TOCTOU and DoS vulnerability in file reading #75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -233,9 +233,16 @@ def _to_int(value: object, default: int) -> int: | |
| def _read_json(path: Path) -> dict[str, object]: | ||
| if not path.exists(): | ||
| return {} | ||
| if path.stat().st_size > 10 * 1024 * 1024: | ||
| 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 {} | ||
|
Comment on lines
+236
to
246
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new secure file reading logic is a great improvement. However, this pattern is now duplicated across four different files:
This duplication introduces a few issues:
To address this, I recommend refactoring this logic into a single, shared utility function. This |
||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,13 +105,19 @@ def record_attempts(self, attempts: list[Attempt]) -> None: | |
| def _load_storage(self) -> dict[str, object]: | ||
| if not self._file_path.exists(): | ||
| return {"items": [], "attempts": []} | ||
| if self._file_path.stat().st_size > 10 * 1024 * 1024: | ||
| raise ValueError( | ||
| f"Practice repository file {self._file_path} exceeds 10MB size limit" | ||
| ) | ||
| if not self._file_path.is_file(): | ||
| return {"items": [], "attempts": []} | ||
|
|
||
| try: | ||
| content = 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( | ||
|
Comment on lines
+113
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This bounded-read check is done in text mode, so Useful? React with πΒ / π. |
||
| f"Practice repository file {self._file_path} exceeds " | ||
| "10MB size limit" | ||
| ) | ||
|
|
||
| if not content.strip(): | ||
| return {"items": [], "attempts": []} | ||
| parsed = json.loads(content) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,12 +36,19 @@ def save(self, snapshot: ProgressSnapshot) -> None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _load_payload(self) -> dict[str, object]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not self._file_path.exists(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._file_path.stat().st_size > 10 * 1024 * 1024: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"Progress snapshot file {self._file_path} exceeds 10MB size limit" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not self._file_path.is_file(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
42
to
54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The structure of this Moving the successful return path inside the
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call to
json.loads(content)is not wrapped in atry...exceptblock and does not handle empty or whitespace-only files. Ifcontentis empty or not valid JSON, this will raise ajson.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.