Repository navigation
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: reject escaping ZIP member paths
The core change is correct and well-targeted. Gating extraction on filepath.IsLocal(file.Name) is the right primitive for closing the zip-slip hole — Go guarantees that when IsLocal(p) is true, filepath.Join(base, p) stays inside base, so .. traversal, absolute paths, Windows drive-relative/UNC paths, and reserved names are all rejected. Closing the reader returned alongside zip.ErrInsecurePath is a genuine fd-leak fix. Test coverage is strong: it exercises both the custom guard (GODEBUG=zipinsecurepath=1) and the stdlib rejection path (=0), covers Windows-specific vectors behind a runtime.GOOS gate, and asserts the security property (outside file unchanged, outside dir not created) rather than just the error string.
A few non-blocking observations below.
Notes (non-blocking)
- Traversal-guard idiom now diverges from
extractTarGz. After this PR,extractZipusesfilepath.IsLocal, while the siblingextractTarGz(internal/crosscompile/fetch.go:337) still uses the olderstrings.HasPrefix(target, filepath.Clean(dest)+os.PathSeparator)pattern with a different error message (illegal file path). Not a regression, but converging both extractors on thefilepath.IsLocalidiom in a follow-up would remove two parallel hardening patterns from the same file.
Nothing here blocks merge; the inline items are a latent robustness gap and a minor wording nit.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
cpunion
changed the title
visualfc
approved these changes
Nov 12, 2026
visualfc
left a comment
visualfc
left a comment
Collaborator
There was a problem hiding this comment.
The filepath.IsLocal gate is the right primitive: Go 1.27 still accepts non-local ZIP/TAR names by default, and Join(dest, name) stays inside dest once IsLocal is true. Closing the reader returned with zip.ErrInsecurePath is required on Windows. Creating parent directories covers missing and unordered ZIP directory records.
Tests check the security property (files outside dest are unchanged) for traversal, absolute names, relative destinations, and Windows path forms. CI is green.
A few non-blocking nits below. The .tar.xz path still goes through system tar -xf / 7-Zip; that is outside this PR, but downloadAndExtractArchive uses it for toolchains too.
zhouguangyuan0718
approved these changes
Nov 12, 2026
LLGo baseline benchmarks
String equality benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
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.
Reject nonlocal TAR/ZIP member names such as
../outside.txtand absolute paths usingfilepath.IsLocal. Create parent directories for ZIP files with missing or unordered directory records, and close readers returned alongsidezip.ErrInsecurePath.Follow up the extraction finding in the review of #2942. Tests cover traversal, relative destinations, Windows path forms and unordered entries. The ZIP traversal regression fails on the previous implementation. The complete
internal/crosscompilesuite passes.