Repository navigation
Conversation
WalkthroughAdds comprehensive path validation to Unzip in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Unzip
participant ZipEntry as Zip Entry
participant FS as Filesystem
Caller->>Unzip: Unzip(zipReader, dest)
loop for each ZipEntry
Unzip->>ZipEntry: read f.Name
Unzip->>Unzip: normalize separators (ToSlash) and split into segments
alt any segment invalid ( ".." or 3+ dot-only )
Unzip-->>Caller: return error "invalid file path: %s"
else
Unzip->>Unzip: build target = Join(dest, f.Name)
Unzip->>Unzip: check IsAbs(target) / Windows drive / UNC
alt path invalid (abs/drive/UNC)
Unzip-->>Caller: return error "invalid file path, ... not allowed: %s"
else
Unzip->>Unzip: canonicalize (Clean, Abs if needed) and ensure target is inside dest
alt outside destination
Unzip-->>Caller: return error "invalid file path, outside destination directory: %s"
else
Unzip->>FS: mkdir -p parent(target)
alt entry is directory
Unzip->>FS: ensure directory exists
else entry is file
Unzip->>FS: os.OpenFile(target, O_WRONLY|O_CREATE|O_TRUNC, mode)
Unzip->>ZipEntry: copy contents
end
end
end
end
end
Unzip-->>Caller: nil (on success)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
potofpie
changed the title
potofpie
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sys/io.go (2)
393-398: Avoid defers inside a loop to prevent FD exhaustion.rc is opened per entry and deferred; with large archives this can hit OS fd limits. Close per-iteration instead.
Minimal change:
rc, err := f.Open() if err != nil { return err } ... // after finishing this entry (both dir and file cases) rc.Close()
416-425: Shadowing “f” obscures readability; close the file explicitly.Rename the output file handle and avoid a second defer in the loop.
- f, err := os.OpenFile( - fpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) + out, err := os.OpenFile( + fpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) if err != nil { return err } - defer f.Close() - - _, err = io.Copy(f, rc) + _, err = io.Copy(out, rc) if err != nil { - return err + out.Close() + return err } + out.Close()
🧹 Nitpick comments (1)
sys/io_test.go (1)
47-83: Remove redundant Close calls (double-close) to keep tests clean.You both defer and explicitly Close zf/zw. Pick one style; since you assert on Close errors, keep explicit closes and drop the defers.
- defer zf.Close() ... - zw := zip.NewWriter(zf) - defer zw.Close() + zw := zip.NewWriter(zf)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
sys/io.go(1 hunks)sys/io_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
sys/io_test.go (1)
sys/io.go (2)
Unzip(380-430)Exists(77-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
|
I think I am missing a couple cases here. |
|
@jhaynie added a couple more test cases and changes I found on the cursor found on the internet. Is there any I need to do to release this after I merge? Just redeploy nova right? |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sys/io.go (2)
399-404: Fix FD leaks: don't defer per-entry closes inside the loop; open only for files.
defer rc.Close()anddefer f.Close()inside the loop can exhaust FDs on large archives. Also, you open the entry before knowing you need it (dirs). Move the open/close into the file branch and close explicitly. Rename the output var to avoid shadowingf.Apply:
- rc, err := f.Open() - if err != nil { - return err - } - defer rc.Close() + // Delay opening entry until we know we need to read file content. @@ - f, err := os.OpenFile( - fpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) + out, err := os.OpenFile( + fpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) if err != nil { return err } - defer f.Close() - - _, err = io.Copy(f, rc) - if err != nil { - return err - } + rc, err := f.Open() + if err != nil { + out.Close() + return err + } + _, err = io.Copy(out, rc) + rc.Close() + if cerr := out.Close(); err == nil && cerr != nil { + err = cerr + } + if err != nil { + return err + }Also applies to: 448-456
439-447: Prevent symlink-escape via parent directories before writing the file.If a pre-existing path component under dest is a symlink to outside, writing the file will escape the extraction root. Resolve and verify the parent directory after creation.
err = os.MkdirAll(fdir, os.ModePerm) if err != nil { return err } + // Symlink hardening: ensure parent resolves under destRoot + if resolvedParent, err := filepath.EvalSymlinks(fdir); err == nil { + rp := resolvedParent + if !strings.HasSuffix(rp, string(os.PathSeparator)) { + rp += string(os.PathSeparator) + } + if !strings.HasPrefix(rp, destRoot) { + return fmt.Errorf("invalid file path, parent directory escapes destination: %s", f.Name) + } + }
♻️ Duplicate comments (4)
sys/io_test.go (4)
54-58: Remove redundant defers; we already close explicitly.Double-closing adds noise and hides real close errors.
- defer zf.Close() @@ - defer zw.Close() @@ - zf.Close() + zf.Close()Also applies to: 66-70
92-99: Remove redundant defers; explicit close happens below.- defer zf.Close() @@ - defer zw.Close()Also applies to: 115-119
298-312: Drop the extradefer zf.Close(); you close explicitly afterzw.Close().- defer zf.Close() @@ - zf.Close() + zf.Close()
347-356: Remove redundant defers in special-case tests; explicit close already present.- defer zf.Close() @@ - defer zf.Close() @@ - defer zf.Close()Also applies to: 368-376, 401-411
🧹 Nitpick comments (5)
sys/io.go (3)
388-397: Also block “.” path components.Not critical due to containment check, but cheap defense-in-depth.
- for _, part := range pathParts { + for _, part := range pathParts { // Block path components that could be used for directory traversal // ".." is obvious, but also block pure dot sequences (3+ dots) that might be interpreted as ".." - if part == ".." || (len(part) >= 3 && strings.Trim(part, ".") == "") { + if part == "." || part == ".." || (len(part) >= 3 && strings.Trim(part, ".") == "") { return fmt.Errorf("invalid file path: %s", f.Name) } }
436-444: Skip directory entries entirely when flatten=true.Currently, dir entries fall into the file branch under flatten and may create zero-length files. Bail out early for dirs.
- if f.FileInfo().IsDir() && !flatten { - os.MkdirAll(fpath, os.ModePerm) - } else { + if f.FileInfo().IsDir() { + if !flatten { + os.MkdirAll(fpath, os.ModePerm) + } + continue + } else {
448-456: Avoid variable shadowing off(zip entry) with file handle.Improves readability and prevents mistakes.
- out, err := os.OpenFile( + out, err := os.OpenFile( fpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())sys/io_test.go (2)
173-185: Add a case for Windows drive-relative paths likeC:foo\bar.These bypass the existing
C:\check without the drive-relative guard. Include to lock in behavior.Proposed addition to WindowsPathAttacks:
name: "WindowsPathAttacks", filenames: []string{ + "C:temp\\malicious.exe", + "C:..\\..\\Windows\\System32\\calc.exe",
340-416: Consider a symlink-escape test inside dest (not just symlinked dest).Create
dest/link->outsidesymlink, zip an entrylink/pwn.txt, and assert Unzip errors. This verifies the parent-dir symlink hardening.I can send a ready-to-drop subtest if you want it included.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
sys/io.go(2 hunks)sys/io_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
sys/io_test.go (1)
sys/io.go (2)
Unzip(380-462)Exists(77-82)
| // Validate file name before path construction to catch absolute paths and other attacks | ||
| if filepath.IsAbs(f.Name) { | ||
| return fmt.Errorf("invalid file path, absolute path not allowed: %s", f.Name) | ||
| } | ||
|
|
||
| // Check for Windows-style absolute paths and UNC paths (platform-independent) | ||
| if len(f.Name) >= 3 && f.Name[1] == ':' && (f.Name[2] == '\\' || f.Name[2] == '/') { | ||
| return fmt.Errorf("invalid file path, Windows drive path not allowed: %s", f.Name) | ||
| } | ||
| if strings.HasPrefix(f.Name, "\\\\") { | ||
| return fmt.Errorf("invalid file path, UNC path not allowed: %s", f.Name) | ||
| } | ||
|
|
There was a problem hiding this comment.
Harden Windows path detection: block drive-relative “C:foo” and UNC with // too.
Current check misses C:foo\bar (drive-relative). Also catch UNC prefixed by //.
- // Check for Windows-style absolute paths and UNC paths (platform-independent)
- if len(f.Name) >= 3 && f.Name[1] == ':' && (f.Name[2] == '\\' || f.Name[2] == '/') {
- return fmt.Errorf("invalid file path, Windows drive path not allowed: %s", f.Name)
- }
- if strings.HasPrefix(f.Name, "\\\\") {
- return fmt.Errorf("invalid file path, UNC path not allowed: %s", f.Name)
- }
+ // Check for Windows-style paths (drive-relative like "C:foo" and absolute "C:\..."), and UNC ("\\server\share" or "//server/share")
+ name := f.Name
+ if len(name) >= 2 && ((name[1] == ':' && ((name[0] >= 'A' && name[0] <= 'Z') || (name[0] >= 'a' && name[0] <= 'z')))) {
+ return fmt.Errorf("invalid file path, Windows drive path not allowed: %s", f.Name)
+ }
+ if strings.HasPrefix(name, "\\\\") || strings.HasPrefix(filepath.ToSlash(name), "//") {
+ return fmt.Errorf("invalid file path, UNC path not allowed: %s", f.Name)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Validate file name before path construction to catch absolute paths and other attacks | |
| if filepath.IsAbs(f.Name) { | |
| return fmt.Errorf("invalid file path, absolute path not allowed: %s", f.Name) | |
| } | |
| // Check for Windows-style absolute paths and UNC paths (platform-independent) | |
| if len(f.Name) >= 3 && f.Name[1] == ':' && (f.Name[2] == '\\' || f.Name[2] == '/') { | |
| return fmt.Errorf("invalid file path, Windows drive path not allowed: %s", f.Name) | |
| } | |
| if strings.HasPrefix(f.Name, "\\\\") { | |
| return fmt.Errorf("invalid file path, UNC path not allowed: %s", f.Name) | |
| } | |
| // Validate file name before path construction to catch absolute paths and other attacks | |
| if filepath.IsAbs(f.Name) { | |
| return fmt.Errorf("invalid file path, absolute path not allowed: %s", f.Name) | |
| } | |
| // Check for Windows-style paths (drive-relative like "C:foo" and absolute "C:\\..."), and UNC ("\\\\server\\share" or "//server/share") | |
| name := f.Name | |
| if len(name) >= 2 && (name[1] == ':' && ((name[0] >= 'A' && name[0] <= 'Z') || (name[0] >= 'a' && name[0] <= 'z'))) { | |
| return fmt.Errorf("invalid file path, Windows drive path not allowed: %s", f.Name) | |
| } | |
| if strings.HasPrefix(name, "\\\\") || strings.HasPrefix(filepath.ToSlash(name), "//") { | |
| return fmt.Errorf("invalid file path, UNC path not allowed: %s", f.Name) | |
| } |
🤖 Prompt for AI Agents
In sys/io.go around lines 409 to 421, the Windows path checks miss
drive-relative names like "C:foo" and UNC paths starting with "//"; update the
validation to reject any name where len(f.Name) >= 2 and f.Name[1] == ':'
(covers both "C:\..." and "C:foo") and also treat both backslash- and
forward-slash UNC prefixes as invalid by checking strings.HasPrefix(f.Name,
"\\\\") || strings.HasPrefix(f.Name, "//"); return the same formatted error
messages when these conditions match.
| // Canonicalize paths and validate against directory traversal | ||
| destRoot := filepath.Clean(dest) | ||
| if !strings.HasSuffix(destRoot, string(os.PathSeparator)) { | ||
| destRoot += string(os.PathSeparator) | ||
| } | ||
| cleanedFpath := filepath.Clean(fpath) | ||
|
|
||
| // Ensure the cleaned file path is within the destination directory | ||
| if !strings.HasPrefix(cleanedFpath+string(os.PathSeparator), destRoot) { | ||
| return fmt.Errorf("invalid file path, outside destination directory: %s", f.Name) | ||
| } | ||
|
|
There was a problem hiding this comment.
Symlink-aware containment: resolve dest and paths before prefix compare.
Case-insensitive volumes and symlinks can bypass a pure lexical check. Normalize with Abs and resolve symlinks where possible.
- // Canonicalize paths and validate against directory traversal
- destRoot := filepath.Clean(dest)
+ // Canonicalize paths and validate against directory traversal
+ destRoot := filepath.Clean(dest)
+ if abs, err := filepath.Abs(destRoot); err == nil {
+ destRoot = abs
+ }
+ if resolved, err := filepath.EvalSymlinks(destRoot); err == nil {
+ destRoot = resolved
+ }
if !strings.HasSuffix(destRoot, string(os.PathSeparator)) {
destRoot += string(os.PathSeparator)
}
- cleanedFpath := filepath.Clean(fpath)
+ cleanedFpath := filepath.Clean(fpath)
+ if abs, err := filepath.Abs(cleanedFpath); err == nil {
+ cleanedFpath = abs
+ }
// Ensure the cleaned file path is within the destination directory
if !strings.HasPrefix(cleanedFpath+string(os.PathSeparator), destRoot) {
return fmt.Errorf("invalid file path, outside destination directory: %s", f.Name)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Canonicalize paths and validate against directory traversal | |
| destRoot := filepath.Clean(dest) | |
| if !strings.HasSuffix(destRoot, string(os.PathSeparator)) { | |
| destRoot += string(os.PathSeparator) | |
| } | |
| cleanedFpath := filepath.Clean(fpath) | |
| // Ensure the cleaned file path is within the destination directory | |
| if !strings.HasPrefix(cleanedFpath+string(os.PathSeparator), destRoot) { | |
| return fmt.Errorf("invalid file path, outside destination directory: %s", f.Name) | |
| } | |
| // Canonicalize paths and validate against directory traversal | |
| destRoot := filepath.Clean(dest) | |
| // Prefer absolute path for destRoot | |
| if abs, err := filepath.Abs(destRoot); err == nil { | |
| destRoot = abs | |
| } | |
| // Resolve any symlinks in destRoot | |
| if resolved, err := filepath.EvalSymlinks(destRoot); err == nil { | |
| destRoot = resolved | |
| } | |
| if !strings.HasSuffix(destRoot, string(os.PathSeparator)) { | |
| destRoot += string(os.PathSeparator) | |
| } | |
| cleanedFpath := filepath.Clean(fpath) | |
| // Prefer absolute path for the file being extracted | |
| if abs, err := filepath.Abs(cleanedFpath); err == nil { | |
| cleanedFpath = abs | |
| } | |
| // Ensure the cleaned file path is within the destination directory | |
| if !strings.HasPrefix(cleanedFpath+string(os.PathSeparator), destRoot) { | |
| return fmt.Errorf("invalid file path, outside destination directory: %s", f.Name) | |
| } |
🤖 Prompt for AI Agents
In sys/io.go around lines 424 to 435, the current containment check uses lexical
Clean and prefix compare which can be bypassed by symlinks or case differences;
resolve this by converting dest and fpath to absolute paths and evaluating
symlinks (e.g., using filepath.Abs and filepath.EvalSymlinks) before comparison,
normalize trailing separators, and then verify containment by computing the
relative path (filepath.Rel) and ensuring it does not start with ".." (and
handle case-normalization on case-insensitive filesystems if applicable); return
the same error when the relative path escapes the destination.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests