Add WithMaxBytes limit to ExtractAll - #35
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The current limit enforcement can write past the configured cap and the option implementation is stateful across calls when an ExtractOption is reused.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds an optional extraction budget to ExtractAll to cap the total decompressed bytes written across all entries, intended to protect callers extracting untrusted archives.
Changes:
- Add
ExtractOption/WithMaxBytesand plumb a shared “remaining bytes” budget through extraction. - Introduce
ErrExtractLimitand return it (wrapped) when the extraction would exceed the configured budget. - Add tests covering the max-bytes behavior for tar and zip inputs.
File summaries
| File | Description |
|---|---|
| extract.go | Adds the options API and enforces a running decompressed-bytes budget during entry extraction. |
| extract_test.go | Adds tests for budget enforcement and for ignoring header-declared/compressed sizes. |
Review details
Suppressed comments (1)
extract.go:211
- copyWithLimit currently uses io.LimitReader(src, *remaining+1) and copies that extra byte into dst, so extraction can write past the configured cap (and the file on disk can exceed the limit). Also, *remaining+1 can overflow when remaining is math.MaxInt64, causing the limit reader to behave incorrectly.
// Read one byte past the budget so an entry that would exceed it is
// detected without draining the whole stream.
n, err := io.Copy(dst, io.LimitReader(src, *remaining+1))
if err != nil {
return n, err
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+39
to
+45
| func WithMaxBytes(n int64) ExtractOption { | ||
| return func(c *extractConfig) { | ||
| if n > 0 { | ||
| c.remaining = &n | ||
| } | ||
| } | ||
| } |
Comment on lines
+453
to
+455
| if info.Size() > 41 { | ||
| t.Fatalf("wrote %d bytes past the limit", info.Size()) | ||
| } |
Caps total decompressed bytes written, enforced against actual bytes read rather than header-declared sizes.
Store the limit as a value on extractConfig and derive the mutable budget inside ExtractAll, so reusing one ExtractOption across calls starts each with a fresh counter. Saturate the +1 sentinel read at MaxInt64 so the limit reader cannot wrap negative.
Copy exactly the remaining budget then read one byte from src to detect overflow, so nothing past the cap reaches dst. Drops the +1 and with it the MaxInt64 saturation guard.
andrew
force-pushed
the
extract-byte-limit
branch
from
September 4, 2026 10:22
1716a51 to
b6c9b38
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a
WithMaxBytesoption toExtractAllthat caps the total decompressed bytes written across all entries. The limit is enforced by wrapping each entry copy inio.LimitReaderagainst a running budget, so an archive whose headers under-report uncompressed sizes still cannot write past it. Exceeding the limit returnsErrExtractLimitnaming the entry that tripped it.Needed by git-pkgs/git-pkgs#328, which extracts untrusted registry archives to a temp directory and currently only checks header-declared sizes.