Preserve symlinks in ZIP directory transfers - #329
Open
rgarcia wants to merge 4 commits into
Open
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Symlink extract skips path checks
- Unzip now validates relative symlink targets by resolving them against the link directory and rejecting any target that escapes the destination root.
- ✅ Fixed: Symlink create fails over existing paths
- Unzip now removes an existing destination entry before creating a symlink so same-path extractions overwrite consistently like regular files.
Or push these changes by commenting:
@cursor push f2155909f6
Preview (f2155909f6)
diff --git a/server/lib/ziputil/ziputil.go b/server/lib/ziputil/ziputil.go
--- a/server/lib/ziputil/ziputil.go
+++ b/server/lib/ziputil/ziputil.go
@@ -104,13 +104,15 @@
if err := os.MkdirAll(destDir, 0755); err != nil {
return fmt.Errorf("failed to create destination directory: %w", err)
}
+ cleanDestDir := filepath.Clean(destDir)
+
// Extract each file
for _, file := range reader.File {
// Create the full destination path
destPath := filepath.Join(destDir, file.Name)
// Check for directory traversal vulnerabilities
- if !strings.HasPrefix(destPath, filepath.Clean(destDir)+string(os.PathSeparator)) {
+ if !strings.HasPrefix(filepath.Clean(destPath), cleanDestDir+string(os.PathSeparator)) {
return fmt.Errorf("illegal file path: %s", file.Name)
}
@@ -139,7 +141,22 @@
if err != nil {
return fmt.Errorf("failed to read symlink target: %w", err)
}
- if err := os.Symlink(string(target), destPath); err != nil {
+ targetPath := string(target)
+
+ // Relative symlink targets must not escape destDir.
+ // Absolute symlinks are allowed to preserve archive behavior.
+ if !filepath.IsAbs(targetPath) {
+ symlinkDir := filepath.Dir(destPath)
+ resolvedTarget := filepath.Clean(filepath.Join(symlinkDir, targetPath))
+ if resolvedTarget != cleanDestDir && !strings.HasPrefix(resolvedTarget, cleanDestDir+string(os.PathSeparator)) {
+ return fmt.Errorf("illegal symlink target (escapes destination): %s -> %s", file.Name, targetPath)
+ }
+ }
+
+ if err := os.Remove(destPath); err != nil && !os.IsNotExist(err) {
+ return fmt.Errorf("failed to remove existing path for symlink: %w", err)
+ }
+ if err := os.Symlink(targetPath, destPath); err != nil {
return fmt.Errorf("failed to create symlink: %w", err)
}
continue
diff --git a/server/lib/ziputil/ziputil_test.go b/server/lib/ziputil/ziputil_test.go
--- a/server/lib/ziputil/ziputil_test.go
+++ b/server/lib/ziputil/ziputil_test.go
@@ -36,6 +36,67 @@
assert.Equal(t, "target.txt", target)
}
+func TestUnzipRejectsEscapingRelativeSymlink(t *testing.T) {
+ zipPath := filepath.Join(t.TempDir(), "escape-symlink.zip")
+ zipFile, err := os.Create(zipPath)
+ require.NoError(t, err)
+
+ zipWriter := zip.NewWriter(zipFile)
+ symlinkHeader := &zip.FileHeader{
+ Name: "link.txt",
+ Method: zip.Store,
+ }
+ symlinkHeader.SetMode(os.ModeSymlink | 0777)
+ linkWriter, err := zipWriter.CreateHeader(symlinkHeader)
+ require.NoError(t, err)
+ _, err = linkWriter.Write([]byte(".."))
+ require.NoError(t, err)
+ require.NoError(t, zipWriter.Close())
+ require.NoError(t, zipFile.Close())
+
+ err = Unzip(zipPath, t.TempDir())
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "illegal symlink target")
+}
+
+func TestUnzipSymlinkOverwritesExistingPath(t *testing.T) {
+ zipPath := filepath.Join(t.TempDir(), "overwrite-symlink.zip")
+ zipFile, err := os.Create(zipPath)
+ require.NoError(t, err)
+
+ zipWriter := zip.NewWriter(zipFile)
+ targetWriter, err := zipWriter.Create("target.txt")
+ require.NoError(t, err)
+ _, err = targetWriter.Write([]byte("target contents"))
+ require.NoError(t, err)
+
+ symlinkHeader := &zip.FileHeader{
+ Name: "link.txt",
+ Method: zip.Store,
+ }
+ symlinkHeader.SetMode(os.ModeSymlink | 0777)
+ linkWriter, err := zipWriter.CreateHeader(symlinkHeader)
+ require.NoError(t, err)
+ _, err = linkWriter.Write([]byte("target.txt"))
+ require.NoError(t, err)
+
+ require.NoError(t, zipWriter.Close())
+ require.NoError(t, zipFile.Close())
+
+ destDir := t.TempDir()
+ linkPath := filepath.Join(destDir, "link.txt")
+ require.NoError(t, os.WriteFile(linkPath, []byte("old contents"), 0644))
+
+ require.NoError(t, Unzip(zipPath, destDir))
+
+ info, err := os.Lstat(linkPath)
+ require.NoError(t, err)
+ assert.True(t, info.Mode()&os.ModeSymlink != 0)
+ target, err := os.Readlink(linkPath)
+ require.NoError(t, err)
+ assert.Equal(t, "target.txt", target)
+}
+
func TestUnzipFile(t *testing.T) {
// Create a temporary directory for test files
sourceDir, err := os.MkdirTemp("", "zip-source-*")You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Symlink escape check bypassable
- I fixed Unzip to resolve entry and symlink targets through existing filesystem symlinks before containment checks and added a regression test for the link->'.'/escape->'link/..' chain escape case.
Or push these changes by commenting:
@cursor push 41fcd54292
Preview (41fcd54292)
diff --git a/server/lib/ziputil/ziputil.go b/server/lib/ziputil/ziputil.go
--- a/server/lib/ziputil/ziputil.go
+++ b/server/lib/ziputil/ziputil.go
@@ -108,13 +108,22 @@
// Extract each file
for _, file := range reader.File {
+ entryPath := filepath.FromSlash(file.Name)
+
// Create the full destination path
- destPath := filepath.Join(destDir, file.Name)
+ destPath := filepath.Join(cleanDestDir, entryPath)
// Check for directory traversal vulnerabilities
- if !strings.HasPrefix(destPath, cleanDestDir+string(os.PathSeparator)) {
+ if !isPathWithinDir(cleanDestDir, destPath) {
return fmt.Errorf("illegal file path: %s", file.Name)
}
+ resolvedDestPath, err := resolvePathWithSymlinks(cleanDestDir, entryPath)
+ if err != nil {
+ return fmt.Errorf("failed to resolve destination path %s: %w", file.Name, err)
+ }
+ if !isPathWithinDir(cleanDestDir, resolvedDestPath) {
+ return fmt.Errorf("illegal file path: %s", file.Name)
+ }
// Handle directories
if file.FileInfo().IsDir() {
@@ -143,8 +152,15 @@
}
targetPath := string(target)
if !filepath.IsAbs(targetPath) {
- resolvedTarget := filepath.Clean(filepath.Join(filepath.Dir(destPath), targetPath))
- if resolvedTarget != cleanDestDir && !strings.HasPrefix(resolvedTarget, cleanDestDir+string(os.PathSeparator)) {
+ resolvedParentPath, err := resolvePathWithSymlinks(cleanDestDir, filepath.Dir(entryPath))
+ if err != nil {
+ return fmt.Errorf("failed to resolve symlink parent path: %w", err)
+ }
+ resolvedTarget, err := resolvePathWithSymlinks(resolvedParentPath, targetPath)
+ if err != nil {
+ return fmt.Errorf("failed to resolve symlink target: %w", err)
+ }
+ if !isPathWithinDir(cleanDestDir, resolvedTarget) {
return fmt.Errorf("illegal symlink target: %s -> %s", file.Name, targetPath)
}
}
@@ -172,3 +188,34 @@
return nil
}
+
+func isPathWithinDir(baseDir, path string) bool {
+ return path == baseDir || strings.HasPrefix(path, baseDir+string(os.PathSeparator))
+}
+
+func resolvePathWithSymlinks(baseDir, relPath string) (string, error) {
+ currentPath := filepath.Clean(baseDir)
+ for _, part := range strings.Split(filepath.FromSlash(relPath), string(os.PathSeparator)) {
+ switch part {
+ case "", ".":
+ continue
+ case "..":
+ currentPath = filepath.Dir(currentPath)
+ continue
+ }
+
+ nextPath := filepath.Join(currentPath, part)
+ resolvedPath, err := filepath.EvalSymlinks(nextPath)
+ if err == nil {
+ currentPath = resolvedPath
+ continue
+ }
+ if !os.IsNotExist(err) {
+ return "", fmt.Errorf("evaluate symlinks for %s: %w", nextPath, err)
+ }
+
+ currentPath = nextPath
+ }
+
+ return filepath.Clean(currentPath), nil
+}
diff --git a/server/lib/ziputil/ziputil_test.go b/server/lib/ziputil/ziputil_test.go
--- a/server/lib/ziputil/ziputil_test.go
+++ b/server/lib/ziputil/ziputil_test.go
@@ -44,6 +44,20 @@
assert.Contains(t, err.Error(), "illegal symlink target")
}
+func TestUnzipRejectsSymlinkChainEscape(t *testing.T) {
+ zipPath := createSymlinkChainEscapeZip(t)
+ destParent := t.TempDir()
+ destDir := filepath.Join(destParent, "extract")
+ outsideFile := filepath.Join(destParent, "pwned.txt")
+
+ err := Unzip(zipPath, destDir)
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "illegal symlink target")
+ _, statErr := os.Stat(outsideFile)
+ require.Error(t, statErr)
+ assert.True(t, os.IsNotExist(statErr))
+}
+
func TestUnzipOverwritesFileWithSymlink(t *testing.T) {
zipPath := createSymlinkZip(t, "target.txt")
destDir := t.TempDir()
@@ -80,6 +94,40 @@
return zipPath
}
+func createSymlinkChainEscapeZip(t *testing.T) string {
+ t.Helper()
+
+ zipPath := filepath.Join(t.TempDir(), "chain-escape.zip")
+ zipFile, err := os.Create(zipPath)
+ require.NoError(t, err)
+
+ zipWriter := zip.NewWriter(zipFile)
+
+ linkHeader := &zip.FileHeader{Name: "link", Method: zip.Store}
+ linkHeader.SetMode(os.ModeSymlink | 0777)
+ linkWriter, err := zipWriter.CreateHeader(linkHeader)
+ require.NoError(t, err)
+ _, err = linkWriter.Write([]byte("."))
+ require.NoError(t, err)
+
+ escapeHeader := &zip.FileHeader{Name: "escape", Method: zip.Store}
+ escapeHeader.SetMode(os.ModeSymlink | 0777)
+ escapeWriter, err := zipWriter.CreateHeader(escapeHeader)
+ require.NoError(t, err)
+ _, err = escapeWriter.Write([]byte("link/.."))
+ require.NoError(t, err)
+
+ fileWriter, err := zipWriter.Create("escape/pwned.txt")
+ require.NoError(t, err)
+ _, err = fileWriter.Write([]byte("pwned"))
+ require.NoError(t, err)
+
+ require.NoError(t, zipWriter.Close())
+ require.NoError(t, zipFile.Close())
+
+ return zipPath
+}
+
func TestUnzipFile(t *testing.T) {
// Create a temporary directory for test files
sourceDir, err := os.MkdirTemp("", "zip-source-*")You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit ede3c18. Configure here.
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
1 participant
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.
summary
tests
go test -race $(go list ./... | grep -v /e2e$)go vet ./...