Repository navigation
Limit zip file entries to 100MB by default - #406
Conversation
This comment has been minimized.
This comment has been minimized.
|
@claude, the CI is failing with: I'm not sure how |
|
Claude finished @Enkidu93's task in 4m 36s —— View job Pyright errors fixedI pushed
How the first run passed: I can't say for certain, and I didn't investigate. The pyright version here (v1.1.411) or the platform may differ from CI, which ran on macOS. The 868 passed figure in the PR body is unchanged. The review findings on corpus-level tests (F1) are not addressed here, since you didn't ask for them. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
==========================================
+ Coverage 92.17% 92.18% +0.01%
==========================================
Files 396 398 +2
Lines 25225 25267 +42
==========================================
+ Hits 23251 23293 +42
Misses 1974 1974 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
@claude Please address the comments on this PR.
@Enkidu93 reviewed 6 files and all commit messages, made 4 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on claude[bot]).
|
@claude Please address the comments on this PR. |
|
Claude finished @Enkidu93's task in 4m 2s —— View job Review comments addressed
What I found. I built a zip with a deflated 5000-byte entry and patched the central-directory What I changed. I kept
|
|
Alright, @claude, it looks like we don't need the bounded stream implementation then. Just check the size from the zip file's header. |
| """Opens a zip entry for reading, refusing entries that are too large or too highly compressed. | ||
|
|
||
| Raises `BadZipFile` if the entry's declared size or compression ratio exceeds the limits. `ZipFile` never yields | ||
| more than the declared size, so an entry whose header understates its size fails the CRC check with `BadZipFile`. |
There was a problem hiding this comment.
FYI: F4. The new docstring promises more than CPython delivers. ZipExtFile checks the CRC against the central-directory CRC over the bytes it has yielded, so if a crafted entry sets file_size and CRC to match a truncated prefix, the entry is silently cut short and no BadZipFile is raised. The size bound still holds. Saying "normally fails the CRC check" would be accurate. Unverified: I could not run Python here.
There was a problem hiding this comment.
F4 still applies at bdce2ab: the docstring now says outright that an understated header "fails the CRC check with BadZipFile", and that claim is the reason BoundedStream was dropped. An entry whose CRC matches the truncated prefix is still cut short without any error. F5 covers the memory side of the same gap.
This comment has been minimized.
This comment has been minimized.
|
Claude finished @Enkidu93's task in 3m 40s —— View job BoundedStream removed
I pushed
Please update the PR description. It still says |
| if entry.compress_size > 0 and entry.file_size / entry.compress_size > max_compression_ratio: | ||
| raise BadZipFile("Compression ratio exceeds safe threshold.") | ||
|
|
||
| return archive.open(entry, "r") |
There was a problem hiding this comment.
Minor: F5. With BoundedStream gone, a single read of a crafted entry no longer has a memory bound. ZipEntryStreamContainer.open_stream and ZipParatextProjectFileHandler.open call stream.read(), and ZipExtFile.read(-1) calls _read1(MAX_N). That decompresses up to 2 GiB in one call before cutting the output to file_size, so file_size=1000, compress_size=2MB passes both checks. Before bdce2ab, RawIOBase.readall read in 8 KB chunks. Reading in chunks at those two sites (e.g. shutil.copyfileobj) restores the bound. Unverified: I reasoned this from CPython zipfile and could not run it.
This comment has been minimized.
This comment has been minimized.
Ports sillsdev/machine#481. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-authored-by: Eli C. Lowry <83078660+Enkidu93@users.noreply.github.com>
Type the inner stream as IO[bytes] and wrap the bounded stream in a BufferedReader before handing it to TextIOWrapper. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-authored-by: Eli C. Lowry <83078660+Enkidu93@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-authored-by: Eli C. Lowry <83078660+Enkidu93@users.noreply.github.com>
ZipFile never yields more than the declared size, so the runtime limit could not fire. open_bounded_stream now returns the zip entry stream. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-authored-by: Eli C. Lowry <83078660+Enkidu93@users.noreply.github.com>
bdce2ab to
ba37fcb
Compare
| if entry.compress_size > 0 and entry.file_size / entry.compress_size > max_compression_ratio: | ||
| raise BadZipFile("Compression ratio exceeds safe threshold.") | ||
|
|
||
| return archive.open(entry, "r") |
There was a problem hiding this comment.
FYI: F6. A bzip2 or LZMA entry is not bounded by these checks, even if the reads are chunked. For those types, ZipExtFile._read1 calls decompress(data) with no max_length, so one compressed chunk can expand without limit before the output is cut to file_size. This means F5's chunked-read fix covers only deflate. C# ZipArchive cannot read these types, so refusing anything except ZIP_STORED/ZIP_DEFLATED here would match C#. Unverified: I reasoned this from CPython zipfile and could not run Python here.
|
Review summary (round 5)
Surface changed: Findings:
Reviewed at ba37fcb |
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions.
Zip entries read through the Paratext and DBL corpora are now refused when they are larger than 100MB or compressed more than 100:1. Ports machine PR #481.
Changes
open_bounded_streaminmachine/utils/zip_entry_utils.pyis the counterpart ofOpenBoundedStream. It raisesBadZipFilewhen the declared size or compression ratio exceeds the limits.BoundedStreaminmachine/utils/bounded_stream.pyraisesOSErrorif more than the limit is actually read, which covers entries whose headers understate their size.ZipEntryStreamContainer,ZipParatextProjectFileHandlerandDblBundleTextCorpusnow open entries through it. Before, they usedarchive.readorarchive.openwith no limit.Write,SetLengthand seek handling is not ported. The Python stream is read-only, since only reads are used.Tests
tests/utils/test_zip_entry_utils.pyports the four C# tests and adds an empty-entry case../local_check.sh --agent-strict: 868 passed, 3 skipped.Closes #358
🤖 Generated with Claude Code
This change is