Fix weave init#206
Closed
Benzbeeb wants to merge 4 commits into
Closed
Conversation
- Improved error handling during file creation by ensuring the target directory exists before creating files. - Added support for symlink extraction, allowing the creation of symbolic links as specified in the tar.gz archive. - Streamlined the file handling logic to ensure proper closure of file descriptors, enhancing resource management.
- Modified the FindBinaryDir function to allow non-executable files to be recognized, changing the behavior to return the current directory when a non-executable binary is found. - Updated the corresponding test case to reflect this change, ensuring that non-executable files are now correctly identified in the binary search process.
Contributor
WalkthroughFindBinaryDir now matches binaries by filename regardless of executable bits; ExtractTarGz canonicalizes destination, prevents path traversal, ensures parent directories, preserves file modes, closes files on copy errors, and adds symlink extraction with root-bound validation. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Caller
end
rect rgba(200,255,200,0.5)
participant Extractor
end
rect rgba(255,200,200,0.5)
participant Filesystem
end
rect rgba(255,255,200,0.5)
participant PathValidator
end
Caller->>Extractor: Call ExtractTarGz(src, dest)
Extractor->>Filesystem: Resolve dest -> Abs + EvalSymlinks
Extractor->>Extractor: Read next tar header
alt header.Type == TypeReg
Extractor->>Filesystem: MkdirAll(parent of target)
Extractor->>Filesystem: OpenFile(target, mode)
Extractor->>Filesystem: io.Copy(content -> file)
Extractor->>Filesystem: Close(file) on success or error
else header.Type == TypeSymlink
Extractor->>PathValidator: Validate link target within dest root
PathValidator-->>Extractor: allowed / rejected
alt allowed
Extractor->>Filesystem: MkdirAll(parent of symlink)
Extractor->>Filesystem: os.Symlink(linkname, target)
else rejected
Extractor-->>Caller: return error
end
else
Extractor-->>Caller: handle other types / continue
end
Extractor-->>Caller: return nil on completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
io/filesystem.go (1)
63-92:⚠️ Potential issue | 🔴 CriticalBlock archive entries from escaping
destbefore creating files or symlinks.A crafted tar archive can escape the destination directory through two vectors: (1)
header.Namewith path traversal sequences like../that, when joined and resolved by the filesystem, point outsidedest, and (2)header.Linknamecreating symlinks to targets outsidedest, which subsequent entries then write through. Validate the resolved target path is withindestbefore creating directories, files, or symlinks. For symlinks, also resolve the linkname relative to its directory and confirm it stays within bounds.🔒 Hardening sketch
+func withinRoot(root, path string) bool { + rel, err := filepath.Rel(root, path) + return err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) +} + func ExtractTarGz(src string, dest string) error { ... - target := filepath.Join(dest, header.Name) + target := filepath.Clean(filepath.Join(dest, header.Name)) + if !withinRoot(dest, target) { + return fmt.Errorf("archive entry escapes destination: %q", header.Name) + } switch header.Typeflag { case tar.TypeReg: + resolvedParent, err := filepath.EvalSymlinks(filepath.Dir(target)) + if err != nil { + return err + } + if !withinRoot(dest, resolvedParent) { + return fmt.Errorf("archive entry follows symlink outside destination: %q", header.Name) + } ... case tar.TypeSymlink: + linkTarget := header.Linkname + if !filepath.IsAbs(linkTarget) { + linkTarget = filepath.Join(filepath.Dir(target), linkTarget) + } + linkTarget = filepath.Clean(linkTarget) + if !withinRoot(dest, linkTarget) { + return fmt.Errorf("symlink escapes destination: %q -> %q", header.Name, header.Linkname) + } if err := os.Symlink(header.Linkname, target); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@io/filesystem.go` around lines 63 - 92, Validate that the resolved path of each archive entry stays inside dest before creating any dirs/files/symlinks: for entries using filepath.Join(dest, header.Name) (in the tar.TypeDir and tar.TypeReg branches) compute cleaned := filepath.Clean(target) and use filepath.Rel(dest, cleaned) (or equivalent) to ensure the result does not begin with ".." (return an error if it does) before calling os.MkdirAll or os.OpenFile; for tar.TypeSymlink, resolve the symlink target relative to the symlink's parent directory (e.g., filepath.Join(filepath.Dir(target), header.Linkname) then Clean + Rel against dest) and refuse to create the symlink if the resolved link target would point outside dest (also refuse creation if header.Name itself escapes); apply these checks right before each os.MkdirAll, os.OpenFile, and os.Symlink call in the extraction loop.
🧹 Nitpick comments (1)
cosmosutils/binary.go (1)
262-274: UpdateFindBinaryDir’s contract to match the new behavior.The doc comment still promises the directory containing the named executable, but Line 272 now accepts any non-directory entry with that name. Either restore the executability filter or document the broader match so callers do not assume the returned path is already runnable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cosmosutils/binary.go` around lines 262 - 274, The comment for FindBinaryDir no longer matches its implementation because the filepath.Walk callback accepts any non-directory entry named binaryName (it no longer checks executability); update the function to either reintroduce an executability check in the walk callback (e.g., test file mode for executable bits on info.Mode() before setting result) or change the doc comment to state it returns the directory of any matching file name (not necessarily executable). Locate the closure passed to filepath.Walk inside FindBinaryDir and apply the chosen fix (restore the exec bit check on info or revise the doc comment) so callers have a correct contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@io/filesystem.go`:
- Around line 63-92: Validate that the resolved path of each archive entry stays
inside dest before creating any dirs/files/symlinks: for entries using
filepath.Join(dest, header.Name) (in the tar.TypeDir and tar.TypeReg branches)
compute cleaned := filepath.Clean(target) and use filepath.Rel(dest, cleaned)
(or equivalent) to ensure the result does not begin with ".." (return an error
if it does) before calling os.MkdirAll or os.OpenFile; for tar.TypeSymlink,
resolve the symlink target relative to the symlink's parent directory (e.g.,
filepath.Join(filepath.Dir(target), header.Linkname) then Clean + Rel against
dest) and refuse to create the symlink if the resolved link target would point
outside dest (also refuse creation if header.Name itself escapes); apply these
checks right before each os.MkdirAll, os.OpenFile, and os.Symlink call in the
extraction loop.
---
Nitpick comments:
In `@cosmosutils/binary.go`:
- Around line 262-274: The comment for FindBinaryDir no longer matches its
implementation because the filepath.Walk callback accepts any non-directory
entry named binaryName (it no longer checks executability); update the function
to either reintroduce an executability check in the walk callback (e.g., test
file mode for executable bits on info.Mode() before setting result) or change
the doc comment to state it returns the directory of any matching file name (not
necessarily executable). Locate the closure passed to filepath.Walk inside
FindBinaryDir and apply the chosen fix (restore the exec bit check on info or
revise the doc comment) so callers have a correct contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c3541242-e032-4825-a38b-4d26235842b2
📒 Files selected for processing (3)
cosmosutils/binary.gocosmosutils/binary_test.goio/filesystem.go
…lution - Added absolute path resolution for the extraction destination to improve reliability. - Implemented symlink validation to ensure extracted links do not escape the designated extraction root, enhancing security. - Introduced a new helper function, isWithinRoot, to check if a candidate path is within the specified root after resolving symlinks.
- Added a destination prefix check to ensure extracted files and symlinks do not escape the specified extraction root, enhancing security. - Removed the isWithinRoot helper function, streamlining the validation logic for symlinks during extraction.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@io/filesystem.go`:
- Around line 73-85: The current extraction checks only the lexical path
(destPrefix) but does not reject existing symlinked path components, so before
creating directories or opening files (when handling tar.TypeDir and tar.TypeReg
for the variables target and header), walk each path component of target with
os.Lstat (including existing leaf) and if any component is a symlink
(FileMode&os.ModeSymlink != 0) return an error (e.g., "tar entry %q has
symlinked path component") to prevent following symlinks; perform this check
immediately after computing target/validation against destPrefix and before
calling os.MkdirAll or os.OpenFile.
Comment on lines
+73
to
+85
| if !strings.HasPrefix(target, destPrefix) && target != dest { | ||
| return fmt.Errorf("tar entry %q escapes extraction root", header.Name) | ||
| } | ||
| switch header.Typeflag { | ||
| case tar.TypeDir: | ||
| if err := os.MkdirAll(target, os.ModePerm); err != nil { | ||
| return err | ||
| } | ||
| case tar.TypeReg: | ||
| file, err := os.Create(target) | ||
| if err := os.MkdirAll(filepath.Dir(target), os.ModePerm); err != nil { | ||
| return err | ||
| } | ||
| file, err := os.OpenFile(target, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, header.FileInfo().Mode()) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "filesystem.go" -path "*/io/*" | head -5Repository: initia-labs/weave
Length of output: 79
🏁 Script executed:
cat -n io/filesystem.go | head -120Repository: initia-labs/weave
Length of output: 3673
🏁 Script executed:
# Check lines 73-85 specifically and surrounding context
cat -n io/filesystem.go | sed -n '50,110p'Repository: initia-labs/weave
Length of output: 2055
🌐 Web query:
Go os.MkdirAll os.OpenFile symlink resolution behavior path traversal
💡 Result:
os.Open,os.OpenFile,os.Create,os.WriteFile: these use normal OS pathname resolution, meaning symlinks in the path are followed (so a symlink can redirect the operation somewhere else). This is one of the core ways symlink-based path traversal bugs happen. [1][2]os.MkdirAll(path, perm): creates the directory tree for the path you pass it (likemkdir -p) and does not provide any special protection against path traversal / symlink tricks in attacker-controlled paths; it’s just a convenience for recursive directory creation. Ifpathis already a directory, it returns nil. [4]filepath.EvalSymlinks: resolves symlinks in a path, but using it as “resolve then validate then open” is TOCTOU-racy if an attacker can change the filesystem between your check and your use. [1][3]- Traversal-resistant alternative (Go 1.24+): use
os.Root(viaos.OpenRoot) oros.OpenInRoot(dir, name). These APIs reject any..or symlink traversal that would escape the chosen root directory (portable across platforms). [1][2]
Sources: [1] Go blog: Traversal-resistant file APIs (Damien Neil, Mar 12 2025). [2] Go os package docs (OpenFile, OpenInRoot, Root). [3] Go path/filepath docs (EvalSymlinks). [4] Go os package docs (MkdirAll).
Reject symlinked components in target before extraction.
Line 73 only validates the lexical path. If any parent directory of target already exists as a symlink (e.g., dest/bin → /usr/bin), the subsequent os.MkdirAll and os.OpenFile calls on lines 78, 82, and 85 will follow the symlink and write outside the extraction root. Walk every existing component of target with os.Lstat—including the leaf if it already exists—and reject symlinks before extraction. This applies to both directory and regular file extraction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@io/filesystem.go` around lines 73 - 85, The current extraction checks only
the lexical path (destPrefix) but does not reject existing symlinked path
components, so before creating directories or opening files (when handling
tar.TypeDir and tar.TypeReg for the variables target and header), walk each path
component of target with os.Lstat (including existing leaf) and if any component
is a symlink (FileMode&os.ModeSymlink != 0) return an error (e.g., "tar entry %q
has symlinked path component") to prevent following symlinks; perform this check
immediately after computing target/validation against destPrefix and before
calling os.MkdirAll or os.OpenFile.
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
2 participants
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.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Bug Fixes
Tests