Preserve symlinks in ZIP directory transfers - #329
Merged
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.
Contributor
|
reviewed — the symlink round-trip logic looks good, but there are two containment issues worth addressing: Bugs
Tests
|
Contributor
Author
|
addressed in 9ff9d02:
|
Contributor
Author
|
follow-up in 41114af: the ZIP round-trip e2e tests showed that browser profile transfers contain absolute symlinks, so rejecting them breaks the existing transfer contract. I restored absolute-link preservation and added an explicit test documenting that behavior. The containment boundary still prevents archive entries from writing through an absolute symlink, while the |
hiroTamada
approved these changes
Aug 8, 2026
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.
summary
tests
go test -race $(go list ./... | grep -v /e2e$)go vet ./...Note
Medium Risk
Changes affect user-uploaded ZIP extraction (
UploadZip) and directory downloads; symlink handling is security-sensitive though relative targets are validated and tests cover escape cases.Overview
ZIP directory transfers now round-trip symbolic links instead of dropping or emptying them:
ZipDirstores each link’s target in the archive entry, andUnziprecreates symlinks from that payload.Extraction hardening replaces a simple prefix check with a resolved destination root (
Abs+EvalSymlinks), per-entry parent resolution viaresolvePathWithSymlinks, andisPathWithinDir. Relative symlink targets must resolve inside the extract tree; escapes (including chained..via links), root entries, and writes under pre-existing symlinks in the dest are rejected. Regular files can replace an existing symlink at the same path.Regression tests cover round-trip, malicious symlink paths, overwrite behavior, and absolute symlinks (stored as-is).
Reviewed by Cursor Bugbot for commit 41114af. Bugbot is set up for automated code reviews on this repo. Configure here.