From 8196b38e3887fc399fd38b11e8532eb86c30b01e Mon Sep 17 00:00:00 2001 From: Muhammad Akewukanwo Date: Sat, 3 Jan 2026 11:22:38 +0100 Subject: [PATCH 1/8] fix ExtractZip function, so all folders inside zip are created in current /tmp sub folder instead of cwd Signed-off-by: Muhammad Akewukanwo --- utils/unarchive.go | 60 +++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/utils/unarchive.go b/utils/unarchive.go index 4e729cae..c657fd24 100644 --- a/utils/unarchive.go +++ b/utils/unarchive.go @@ -69,52 +69,52 @@ func readData(name string) (buffer []byte, err error) { } return } - func ExtractZip(path, artifactPath string) error { zipReader, err := zip.OpenReader(artifactPath) - defer func() { - _ = zipReader.Close() - }() - if err != nil { return ErrExtractZip(err, path) } - buffer := make([]byte, 1<<4) - for _, file := range zipReader.File { + defer zipReader.Close() - fd, err := file.Open() - defer func() { - _ = fd.Close() - }() + for _, file := range zipReader.File { + err := func() error { + fd, err := file.Open() + if err != nil { + return err + } + defer fd.Close() - if err != nil { - return ErrExtractZip(err, path) - } + filePath := filepath.Join(path, file.Name) - filePath := filepath.Join(path, file.Name) + if file.FileInfo().IsDir() { + // this will create the folders in the filepath(/temp ) instead of cwd, which was a bug + return os.MkdirAll(filePath, file.Mode()) + } + // Skip macOS resource fork files + if strings.HasPrefix(filepath.Base(filePath), "._") { + return nil + } - if file.FileInfo().IsDir() { - err := os.Mkdir(file.Name, file.Mode()) - if err != nil { - return ErrExtractZip(err, path) + if err := os.MkdirAll(filepath.Dir(filePath), 0755); err != nil { + return err } - } else { + openedFile, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode()) if err != nil { - return ErrExtractZip(err, path) - } - _, err = io.CopyBuffer(openedFile, fd, buffer) - if err != nil { - return ErrExtractZip(err, path) + return err } - defer func() { - _ = openedFile.Close() - }() - } + defer openedFile.Close() + _, err = io.Copy(openedFile, fd) + + return err + }() + // we capture the closure error here and wrap it with ErrExtractZip + if err != nil { + return ErrExtractZip(err, path) + } } return nil - } func ExtractTarGz(path, downloadfilePath string) error { From 067a69f54064c0d5b7cf79851f023b81b8397713 Mon Sep 17 00:00:00 2001 From: Muhammad Akewukanwo Date: Sat, 3 Jan 2026 11:37:08 +0100 Subject: [PATCH 2/8] check for files to skip before creating the dir and add __MACOSX to skipped file Signed-off-by: Muhammad Akewukanwo --- utils/unarchive.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/utils/unarchive.go b/utils/unarchive.go index c657fd24..1da294de 100644 --- a/utils/unarchive.go +++ b/utils/unarchive.go @@ -85,15 +85,14 @@ func ExtractZip(path, artifactPath string) error { defer fd.Close() filePath := filepath.Join(path, file.Name) + // CHECK for files to skip + if strings.HasPrefix(filepath.Base(filePath), "._") || filepath.Base(filePath) == "__MACOSX" { + return nil + } if file.FileInfo().IsDir() { - // this will create the folders in the filepath(/temp ) instead of cwd, which was a bug return os.MkdirAll(filePath, file.Mode()) } - // Skip macOS resource fork files - if strings.HasPrefix(filepath.Base(filePath), "._") { - return nil - } if err := os.MkdirAll(filepath.Dir(filePath), 0755); err != nil { return err @@ -109,7 +108,6 @@ func ExtractZip(path, artifactPath string) error { return err }() - // we capture the closure error here and wrap it with ErrExtractZip if err != nil { return ErrExtractZip(err, path) } From 494fe1e2890e3f0a7ca27f8572ef74aeec2c7d78 Mon Sep 17 00:00:00 2001 From: Muhammad Akewukanwo Date: Sat, 3 Jan 2026 12:11:50 +0100 Subject: [PATCH 3/8] fix Zip Slip attack and add test Signed-off-by: Muhammad Akewukanwo --- utils/unarchive.go | 32 +++++++++++++++------- utils/zip_test.go | 66 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 10 deletions(-) create mode 100644 utils/zip_test.go diff --git a/utils/unarchive.go b/utils/unarchive.go index 1da294de..037e4190 100644 --- a/utils/unarchive.go +++ b/utils/unarchive.go @@ -4,6 +4,7 @@ import ( "archive/tar" "archive/zip" "compress/gzip" + "fmt" "io" "net/http" "os" @@ -69,43 +70,54 @@ func readData(name string) (buffer []byte, err error) { } return } + func ExtractZip(path, artifactPath string) error { zipReader, err := zip.OpenReader(artifactPath) if err != nil { return ErrExtractZip(err, path) } defer zipReader.Close() - + destDir, err := filepath.Abs(path) + if err != nil { + return err + } for _, file := range zipReader.File { err := func() error { - fd, err := file.Open() + targetPath, err := filepath.Abs(filepath.Join(destDir, file.Name)) if err != nil { return err } - defer fd.Close() - filePath := filepath.Join(path, file.Name) - // CHECK for files to skip - if strings.HasPrefix(filepath.Base(filePath), "._") || filepath.Base(filePath) == "__MACOSX" { + if !strings.HasPrefix(targetPath, destDir+string(os.PathSeparator)) && targetPath != destDir { + return fmt.Errorf("zipslip: illegal file path: %s", file.Name) + } + + // CHECK for files to skip (macOS metadata) + if strings.HasPrefix(filepath.Base(targetPath), "._") || filepath.Base(targetPath) == "__MACOSX" { return nil } + fd, err := file.Open() + if err != nil { + return err + } + defer fd.Close() + if file.FileInfo().IsDir() { - return os.MkdirAll(filePath, file.Mode()) + return os.MkdirAll(targetPath, file.Mode()) } - if err := os.MkdirAll(filepath.Dir(filePath), 0755); err != nil { + if err := os.MkdirAll(filepath.Dir(targetPath), 0755); err != nil { return err } - openedFile, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode()) + openedFile, err := os.OpenFile(targetPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode()) if err != nil { return err } defer openedFile.Close() _, err = io.Copy(openedFile, fd) - return err }() if err != nil { diff --git a/utils/zip_test.go b/utils/zip_test.go new file mode 100644 index 00000000..b821f6cb --- /dev/null +++ b/utils/zip_test.go @@ -0,0 +1,66 @@ +package utils + +import ( + "archive/zip" + "os" + "testing" +) + +func TestExtractZip(t *testing.T) { + tests := []struct { + name string + files map[string]string // fileName: content + wantErr bool + errMatch string + }{ + { + name: "Valid Extraction", + files: map[string]string{ + "test.txt": "hello world", + "subdir/file.txt": "nested content", + }, + wantErr: false, + }, + { + name: "Zip Slip Attack Attempt", + files: map[string]string{ + "../outside.txt": "malicious content", + }, + wantErr: true, + errMatch: "zipslip: illegal file path", + }, + { + name: "Skip macOS Metadata", + files: map[string]string{ + "__MACOSX/secret": "should skip", + "._metadata": "should skip", + "realfile.txt": "should keep", + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir, _ := os.MkdirTemp("", "extract-test-*") + defer os.RemoveAll(tmpDir) + + zipFile, _ := os.CreateTemp("", "test-*.zip") + defer os.Remove(zipFile.Name()) + + writer := zip.NewWriter(zipFile) + for name, content := range tt.files { + f, _ := writer.Create(name) + f.Write([]byte(content)) + } + writer.Close() + zipFile.Close() + + err := ExtractZip(tmpDir, zipFile.Name()) + + if (err != nil) != tt.wantErr { + t.Errorf("ExtractZip() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} From f177806143ce18b1f4ed7ac916e1209b1c3354f7 Mon Sep 17 00:00:00 2001 From: Muhammad Akewukanwo Date: Sat, 3 Jan 2026 12:14:43 +0100 Subject: [PATCH 4/8] add test that fail for old function Signed-off-by: Muhammad Akewukanwo --- utils/zip_test.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/utils/zip_test.go b/utils/zip_test.go index b821f6cb..087150cb 100644 --- a/utils/zip_test.go +++ b/utils/zip_test.go @@ -3,6 +3,7 @@ package utils import ( "archive/zip" "os" + "path/filepath" "testing" ) @@ -64,3 +65,37 @@ func TestExtractZip(t *testing.T) { }) } } + +func TestExtractZip_Destination(t *testing.T) { + destDir, _ := os.MkdirTemp("", "correct-dest-*") + defer os.RemoveAll(destDir) + + cwd, _ := os.Getwd() + + subDirName := "target-subfolder" + zipFile, _ := os.CreateTemp("", "test-*.zip") + writer := zip.NewWriter(zipFile) + + _, _ = writer.Create(subDirName + "/") + f, _ := writer.Create(filepath.Join(subDirName, "file.txt")) + f.Write([]byte("content")) + writer.Close() + zipFile.Close() + defer os.Remove(zipFile.Name()) + + err := ExtractZip(destDir, zipFile.Name()) + if err != nil { + t.Fatalf("Extraction failed: %v", err) + } + + wrongPath := filepath.Join(cwd, subDirName) + if _, err := os.Stat(wrongPath); err == nil { + t.Errorf("BUG FOUND: Folder was created in CWD: %s", wrongPath) + os.RemoveAll(wrongPath) // Cleanup the mess made by the bug + } + + rightPath := filepath.Join(destDir, subDirName) + if _, err := os.Stat(rightPath); os.IsNotExist(err) { + t.Errorf("FAILURE: Folder was NOT created in destination: %s", rightPath) + } +} From 9002212ce8a1eed44434965ea3c9e28ddc459b22 Mon Sep 17 00:00:00 2001 From: Muhammad Akewukanwo Date: Sat, 3 Jan 2026 12:19:47 +0100 Subject: [PATCH 5/8] add test that fail for old function Signed-off-by: Muhammad Akewukanwo --- utils/zip_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/utils/zip_test.go b/utils/zip_test.go index 087150cb..30646889 100644 --- a/utils/zip_test.go +++ b/utils/zip_test.go @@ -76,9 +76,16 @@ func TestExtractZip_Destination(t *testing.T) { zipFile, _ := os.CreateTemp("", "test-*.zip") writer := zip.NewWriter(zipFile) - _, _ = writer.Create(subDirName + "/") + header := &zip.FileHeader{ + Name: subDirName + "/", + Method: zip.Store, + } + header.SetMode(0755) + _, _ = writer.CreateHeader(header) + f, _ := writer.Create(filepath.Join(subDirName, "file.txt")) f.Write([]byte("content")) + writer.Close() zipFile.Close() defer os.Remove(zipFile.Name()) @@ -91,7 +98,7 @@ func TestExtractZip_Destination(t *testing.T) { wrongPath := filepath.Join(cwd, subDirName) if _, err := os.Stat(wrongPath); err == nil { t.Errorf("BUG FOUND: Folder was created in CWD: %s", wrongPath) - os.RemoveAll(wrongPath) // Cleanup the mess made by the bug + os.RemoveAll(wrongPath) } rightPath := filepath.Join(destDir, subDirName) From d3cd80e598e45223265207533949d7c7d77dc459 Mon Sep 17 00:00:00 2001 From: Muhammad Akewukanwo Date: Sat, 3 Jan 2026 12:25:53 +0100 Subject: [PATCH 6/8] add test that fail for old function Signed-off-by: Muhammad Akewukanwo --- utils/zip_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/utils/zip_test.go b/utils/zip_test.go index 30646889..145cac3a 100644 --- a/utils/zip_test.go +++ b/utils/zip_test.go @@ -90,11 +90,7 @@ func TestExtractZip_Destination(t *testing.T) { zipFile.Close() defer os.Remove(zipFile.Name()) - err := ExtractZip(destDir, zipFile.Name()) - if err != nil { - t.Fatalf("Extraction failed: %v", err) - } - + extractionErr := ExtractZip(destDir, zipFile.Name()) wrongPath := filepath.Join(cwd, subDirName) if _, err := os.Stat(wrongPath); err == nil { t.Errorf("BUG FOUND: Folder was created in CWD: %s", wrongPath) @@ -105,4 +101,8 @@ func TestExtractZip_Destination(t *testing.T) { if _, err := os.Stat(rightPath); os.IsNotExist(err) { t.Errorf("FAILURE: Folder was NOT created in destination: %s", rightPath) } + if extractionErr != nil { + t.Fatalf("Extraction failed: %v", extractionErr) + } + } From fb4ff3d4774174fdf2f1278a535245dabb7d3759 Mon Sep 17 00:00:00 2001 From: Muhammad Akewukanwo Date: Sat, 3 Jan 2026 12:49:31 +0100 Subject: [PATCH 7/8] make err a mesherror Signed-off-by: Muhammad Akewukanwo --- utils/unarchive.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/unarchive.go b/utils/unarchive.go index 037e4190..fe359f45 100644 --- a/utils/unarchive.go +++ b/utils/unarchive.go @@ -79,7 +79,7 @@ func ExtractZip(path, artifactPath string) error { defer zipReader.Close() destDir, err := filepath.Abs(path) if err != nil { - return err + return ErrExtractZip(err, path) } for _, file := range zipReader.File { err := func() error { From 65937b72dff0e3e8dacd1e073a4f909300ed8a8f Mon Sep 17 00:00:00 2001 From: Muhammad Akewukanwo Date: Fri, 9 Jan 2026 19:19:12 +0100 Subject: [PATCH 8/8] update test Signed-off-by: Muhammad Akewukanwo --- utils/zip_test.go | 53 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/utils/zip_test.go b/utils/zip_test.go index 145cac3a..a827034c 100644 --- a/utils/zip_test.go +++ b/utils/zip_test.go @@ -4,6 +4,7 @@ import ( "archive/zip" "os" "path/filepath" + "strings" "testing" ) @@ -43,25 +44,55 @@ func TestExtractZip(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tmpDir, _ := os.MkdirTemp("", "extract-test-*") + tmpDir, err := os.MkdirTemp("", "extract-test-*") + if err != nil { + t.Fatal(err) + } defer os.RemoveAll(tmpDir) + zipPath := filepath.Join(t.TempDir(), "test.zip") + f, err := os.Create(zipPath) + if err != nil { + t.Fatal(err) + } - zipFile, _ := os.CreateTemp("", "test-*.zip") - defer os.Remove(zipFile.Name()) - - writer := zip.NewWriter(zipFile) + writer := zip.NewWriter(f) for name, content := range tt.files { - f, _ := writer.Create(name) - f.Write([]byte(content)) + w, err := writer.Create(name) + if err != nil { + continue + } + w.Write([]byte(content)) } writer.Close() - zipFile.Close() - - err := ExtractZip(tmpDir, zipFile.Name()) + f.Close() + err = ExtractZip(tmpDir, zipPath) if (err != nil) != tt.wantErr { - t.Errorf("ExtractZip() error = %v, wantErr %v", err, tt.wantErr) + t.Fatalf("ExtractZip() error = %v, wantErr %v", err, tt.wantErr) } + if tt.wantErr && tt.errMatch != "" { + if !strings.Contains(err.Error(), tt.errMatch) { + t.Errorf("error %q does not match %q", err.Error(), tt.errMatch) + } + return + } + + for name, expectedContent := range tt.files { + if strings.HasPrefix(filepath.Base(name), "._") || filepath.Base(name) == "__MACOSX" { + continue + } + + path := filepath.Join(tmpDir, name) + gotContent, err := os.ReadFile(path) + if err != nil { + t.Errorf("Expected file %s missing: %v", name, err) + continue + } + if string(gotContent) != expectedContent { + t.Errorf("File %s: got %q, want %q", name, string(gotContent), expectedContent) + } + } + }) } }