Add ExtractAll with os.Root confinement - #27
Merged
Merged
Conversation
ExtractAll(r Reader, dir string) writes every entry under dir using os.OpenRoot so no operation can escape the target through a symlink. Entry names are validated with filepath.Localize and traversal attempts return ErrUnsafePath naming the offending entry. Existing objects at a destination path are removed before an O_EXCL create so in-root symlinks and hard links are replaced rather than followed or modified in place. File permissions are applied via fchmod on the open descriptor when the archive recorded a mode; directory permissions are applied deepest-first after all entries are written. FileInfo gains HasMode to distinguish a recorded mode from a synthesised or absent one; zip entries set it only when the creator system is Unix or macOS and a Unix st_mode is present. tarReader now derives Mode from header.FileInfo().Mode() so symlink and device typeflags surface as fs.ModeType bits, and marks TypeLink entries irregular. Both readers build a path index at open time so Extract is a map lookup and ExtractAll is linear in the entry count. Closes #22
There was a problem hiding this comment.
Pull request overview
This PR adds a new disk-extraction API to the archives package, allowing callers to materialize an archive’s contents under a target directory while attempting to prevent path traversal and symlink-escape writes via os.OpenRoot confinement. It also extends per-entry metadata to distinguish “mode recorded vs synthesized” and improves extraction performance by indexing entries for O(1) lookup.
Changes:
- Add
ExtractAll(r Reader, dir string) errorwith path validation andos.Rootconfinement, plus deferred directory chmod handling. - Extend
FileInfowithHasModeand update ZIP/TAR readers to populate it appropriately. - Add comprehensive tests for extraction behavior (traversal rejection, root confinement, replacing existing objects, mode handling), and document the new API in the README.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
zip.go |
Adds an entry index for O(1) extraction and implements HasMode detection for Unix/macOS-created ZIP entries. |
tar.go |
Adds an entry index for O(1) extraction and improves mode/typeflag mapping (including marking hardlinks irregular). |
archives.go |
Extends FileInfo with HasMode to distinguish recorded modes from synthesized defaults. |
extract.go |
Introduces ExtractAll with os.Root confinement, path validation, replacement semantics, and deferred directory chmod. |
extract_test.go |
Adds tests covering traversal protection, confinement vs symlinks, replacement behavior, and mode preservation logic. |
README.md |
Documents ExtractAll usage and its safety/mode behavior at a high level. |
Suppressed comments (1)
extract.go:131
- When applying recorded file modes,
out.Chmod(perm)usesperm := fs.FileMode(entry.Mode).Perm(), which drops sticky/setuid/setgid bits from the stored mode. If the intent is to preserve the recorded mode (excluding type bits), chmod should use the full recorded mode rather than only.Perm().
if entry.HasMode {
// The mode passed to OpenFile is subject to the process umask;
// restore the recorded permissions on the open descriptor so no
// path lookup is involved.
if err := out.Chmod(perm); err != nil {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
ExtractAll(r Reader, dir string) erroras a package function that writes every entry underdir. Implemented on top ofListandExtractso it works for zip, tar, gem, and prefix-stripped readers with one code path.All per-entry filesystem operations go through
os.OpenRoot(dir), so a symlink underdircannot redirect a write outside it. Entry names are also validated withfilepath.Localize; absolute paths and..escapes returnErrUnsafePathnaming the offending entry. Any existing object at a destination path is removed before anO_EXCLcreate so pre-existing in-root symlinks and hard links are replaced with a fresh regular file rather than followed or modified in place.Permissions are preserved when the archive records them: file modes are applied via
File.Chmodon the open descriptor before close, and directory modes are applied deepest-first after all entries are written. setuid/setgid/sticky bits are dropped since extracted archives are untrusted.FileInfogains aHasModefield so a recorded0o000is distinguishable from an absent mode; for zip it is set only when the creator system is Unix or macOS and a Unixst_modeis present inExternalAttrs, so the0666thatarchive/zipsynthesises for FAT/NTFS entries is not applied on disk.Supporting changes:
tarReadernow derivesModefromheader.FileInfo().Mode()so symlink/device typeflags surface asfs.ModeTypebits, marksTypeLinkentries asfs.ModeIrregular, and both readers build a path index at open time soExtractis O(1) andExtractAllis linear in the entry count. This changesFileInfo.Modefor tar directories and special entries (they now carryfs.ModeTypebits, matching what zip already returned); regular-file modes are unchanged and no known consumer readsModeon non-regular entries.Closes #22.