Use mholt/archives instead of mholt/archiver (#3128)

* Use mholt/archives instead of mholt/archiver
* Fix validations
* Fix iszip function
* Fix directory
* Add better path sanitization
* ensure safe dir is passed
* Fix file permissions
* Fix config path
* Sanitize builder source path

---------

Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>

pkg/builder/builder.go

@@ -121,16 +121,20 @@ func (builder *Builder) Handler(w http.ResponseWriter, r *http.Request) {
 	}
 	logger.Info("builder received request", zap.Any("request", req))
 
-	if !utils.ValidateFilePathComponent(req.SrcPkgFilename) {
-		e := "invalid source package filename"
-		logger.Error(e, zap.String("filename", req.SrcPkgFilename))
-		builder.reply(r.Context(), w, "", e, http.StatusBadRequest)
+	logger.Debug("starting build")
+	srcPkgPath, err := utils.SanitizeFilePath(filepath.Join(builder.sharedVolumePath, req.SrcPkgFilename), builder.sharedVolumePath)
+	if err != nil {
+		logger.Error(err.Error(), zap.String("filename", req.SrcPkgFilename))
+		builder.reply(r.Context(), w, "", err.Error(), http.StatusBadRequest)
 		return
 	}
-	logger.Debug("starting build")
-	srcPkgPath := filepath.Join(builder.sharedVolumePath, req.SrcPkgFilename)
 	deployPkgFilename := fmt.Sprintf("%s-%s", req.SrcPkgFilename, strings.ToLower(uniuri.NewLen(6)))
-	deployPkgPath := filepath.Join(builder.sharedVolumePath, deployPkgFilename)
+	deployPkgPath, err := utils.SanitizeFilePath(filepath.Join(builder.sharedVolumePath, deployPkgFilename), builder.sharedVolumePath)
+	if err != nil {
+		logger.Error(err.Error(), zap.String("filename", req.SrcPkgFilename))
+		builder.reply(r.Context(), w, "", err.Error(), http.StatusBadRequest)
+		return
+	}
 
 	var buildArgs []string
 	buildCmd := req.BuildCommand

pkg/fetcher/fetcher.go

@@ -29,7 +29,6 @@ import (
 	"time"
 
 	"github.com/google/uuid"
-	"github.com/mholt/archiver/v3"
 	"github.com/pkg/errors"
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
 	"go.uber.org/zap"
@@ -253,15 +252,14 @@ func (fetcher *Fetcher) SpecializeHandler(w http.ResponseWriter, r *http.Request
 func (fetcher *Fetcher) Fetch(ctx context.Context, pkg *fv1.Package, req FunctionFetchRequest) (int, error) {
 	logger := otelUtils.LoggerWithTraceID(ctx, fetcher.logger)
 
-	// check that the requested filename is not an empty string and doest not contain any path traversal
-	if !utils.ValidateFilePathComponent(req.Filename) {
-		e := "fetch request received for an invalid file name"
-		logger.Error(e, zap.Any("request", req))
-		return http.StatusBadRequest, errors.New(fmt.Sprintf("%s, request: %v", e, req))
+	storePath, err := utils.SanitizeFilePath(filepath.Join(fetcher.sharedVolumePath, req.Filename), fetcher.sharedVolumePath)
+	if err != nil {
+		logger.Error(err.Error(), zap.String("filename", req.Filename))
+		return http.StatusBadRequest, errors.New(fmt.Sprintf("%s, request: %v", err, req))
 	}
 
 	// verify first if the file already exists.
-	if _, err := os.Stat(filepath.Join(fetcher.sharedVolumePath, req.Filename)); err == nil {
+	if _, err := os.Stat(storePath); err == nil {
 		logger.Info("requested file already exists at shared volume - skipping fetch",
 			zap.String("requested_file", req.Filename),
 			zap.String("shared_volume_path", fetcher.sharedVolumePath))
@@ -269,8 +267,11 @@ func (fetcher *Fetcher) Fetch(ctx context.Context, pkg *fv1.Package, req Functio
 		return http.StatusOK, nil
 	}
 
-	tmpFile := req.Filename + ".tmp"
-	tmpPath := filepath.Join(fetcher.sharedVolumePath, tmpFile)
+	tmpPath, err := utils.SanitizeFilePath(storePath+".tmp", fetcher.sharedVolumePath)
+	if err != nil {
+		logger.Error(err.Error(), zap.String("filename", req.Filename))
+		return http.StatusBadRequest, errors.New(fmt.Sprintf("%s, request: %v", err, req))
+	}
 
 	if req.FetchType == fv1.FETCH_URL {
 		otelUtils.SpanTrackEvent(ctx, "fetch_url", otelUtils.MapToAttributes(map[string]string{
@@ -350,10 +351,10 @@ func (fetcher *Fetcher) Fetch(ctx context.Context, pkg *fv1.Package, req Functio
 	}
 
 	// checking if file is a zip
-	if match, _ := utils.IsZip(tmpPath); match && !req.KeepArchive {
+	if match, _ := utils.IsZip(ctx, tmpPath); match && !req.KeepArchive {
 		// unarchive tmp file to a tmp unarchive path
 		tmpUnarchivePath := filepath.Join(fetcher.sharedVolumePath, uuid.NewString())
-		err := fetcher.unarchive(tmpPath, tmpUnarchivePath)
+		err := utils.Unarchive(ctx, tmpPath, tmpUnarchivePath)
 		if err != nil {
 			logger.Error("error unarchive",
 				zap.Error(err),
@@ -366,18 +367,17 @@ func (fetcher *Fetcher) Fetch(ctx context.Context, pkg *fv1.Package, req Functio
 	}
 
 	// move tmp file to requested filename
-	renamePath := filepath.Join(fetcher.sharedVolumePath, req.Filename)
-	err := fetcher.rename(tmpPath, renamePath)
+	err = fetcher.rename(tmpPath, storePath)
 	if err != nil {
 		logger.Error("error renaming file",
 			zap.Error(err),
 			zap.String("original_path", tmpPath),
-			zap.String("rename_path", renamePath))
+			zap.String("rename_path", storePath))
 		return http.StatusInternalServerError, err
 	}
 
 	otelUtils.SpanTrackEvent(ctx, "packageFetched", otelUtils.GetAttributesForPackage(pkg)...)
-	logger.Info("successfully placed", zap.String("location", renamePath))
+	logger.Info("successfully placed", zap.String("location", storePath))
 	return http.StatusOK, nil
 }
 
@@ -406,13 +406,12 @@ func (fetcher *Fetcher) FetchSecretsAndCfgMaps(ctx context.Context, secrets []fv
 				return httpCode, errors.New(e)
 			}
 
-			if !utils.ValidateFilePathComponent(secret.Namespace) && !utils.ValidateFilePathComponent(secret.Name) {
-				e := "fetch request received for an invalid secret name or namespace"
-				logger.Error(e, zap.Any("request", secret))
-				return http.StatusBadRequest, errors.New(fmt.Sprintf("%s, request: %v", e, secret))
+			secretDir, err := utils.SanitizeFilePath(filepath.Join(fetcher.sharedSecretPath, secret.Namespace, secret.Name), fetcher.sharedSecretPath)
+			if err != nil {
+				logger.Error(err.Error(), zap.String("directory", secretDir), zap.String("secret_name", secret.Name), zap.String("secret_namespace", secret.Namespace))
+				return http.StatusBadRequest, errors.New(fmt.Sprintf("%s, request: %v", err, secret))
 			}
-			secretPath := filepath.Join(secret.Namespace, secret.Name)
-			secretDir := filepath.Join(fetcher.sharedSecretPath, secretPath)
+
 			err = os.MkdirAll(secretDir, os.ModeDir|0750)
 			if err != nil {
 				e := "failed to create directory for secret"
@@ -459,14 +458,13 @@ func (fetcher *Fetcher) FetchSecretsAndCfgMaps(ctx context.Context, secrets []fv
 				return httpCode, errors.New(e)
 			}
 
-			if !utils.ValidateFilePathComponent(config.Namespace) && !utils.ValidateFilePathComponent(config.Name) {
-				e := "fetch request received for an invalid configmap name or namespace"
-				logger.Error(e, zap.Any("request", config))
-				return http.StatusBadRequest, errors.New(fmt.Sprintf("%s, request: %v", e, config))
+			configDir, err := utils.SanitizeFilePath(filepath.Join(fetcher.sharedConfigPath, config.Namespace, config.Name), fetcher.sharedConfigPath)
+			if err != nil {
+				logger.Error(err.Error(), zap.String("directory", configDir), zap.String("config_map_name", config.Name), zap.String("config_map_namespace", config.Namespace))
+				return http.StatusBadRequest, errors.New(fmt.Sprintf("%s, request: %v", err,
+					config))
 			}
 
-			configPath := filepath.Join(config.Namespace, config.Name)
-			configDir := filepath.Join(fetcher.sharedConfigPath, configPath)
 			err = os.MkdirAll(configDir, os.ModeDir|0750)
 			if err != nil {
 				e := "failed to create directory for configmap"
@@ -531,17 +529,20 @@ func (fetcher *Fetcher) UploadHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if !utils.ValidateFilePathComponent(req.Filename) {
-		logger.Error("invalid filename in request", zap.String("filename", req.Filename))
-		http.Error(w, "Invalid file name", http.StatusBadRequest)
-		return
-	}
-
 	logger.Info("fetcher received upload request", zap.Any("request", req))
 
-	zipFilename := req.Filename + ".zip"
-	srcFilepath := filepath.Join(fetcher.sharedVolumePath, req.Filename)
-	dstFilepath := filepath.Join(fetcher.sharedVolumePath, zipFilename)
+	srcFilepath, err := utils.SanitizeFilePath(filepath.Join(fetcher.sharedVolumePath, req.Filename), fetcher.sharedVolumePath)
+	if err != nil {
+		logger.Error("error sanitizing file path", zap.Error(err))
+		http.Error(w, fmt.Sprintf("%s: %v", err, req.Filename), http.StatusBadRequest)
+		return
+	}
+	dstFilepath, err := utils.SanitizeFilePath(filepath.Join(fetcher.sharedVolumePath, req.Filename+".zip"), fetcher.sharedVolumePath)
+	if err != nil {
+		logger.Error("error sanitizing file path", zap.Error(err))
+		http.Error(w, fmt.Sprintf("%s: %v", err, req.Filename), http.StatusBadRequest)
+		return
+	}
 
 	defer func() {
 		errC := utils.DeleteOldPackages(srcFilepath, "DEPLOY_PKG")
@@ -552,7 +553,7 @@ func (fetcher *Fetcher) UploadHandler(w http.ResponseWriter, r *http.Request) {
 	}()
 
 	if req.ArchivePackage {
-		err = fetcher.archive(srcFilepath, dstFilepath)
+		err = utils.Archive(ctx, srcFilepath, dstFilepath)
 		if err != nil {
 			e := "error archiving zip file"
 			logger.Error(e, zap.Error(err), zap.String("source", srcFilepath), zap.String("destination", dstFilepath))
@@ -621,39 +622,6 @@ func (fetcher *Fetcher) rename(src string, dst string) error {
 	return nil
 }
 
-// archive zips the contents of directory at src into a new zip file
-// at dst (note that the contents are zipped, not the directory itself).
-func (fetcher *Fetcher) archive(src string, dst string) error {
-	var files []string
-	target, err := os.Stat(src)
-	if err != nil {
-		return errors.Wrap(err, "failed to zip file")
-	}
-	if target.IsDir() {
-		// list all
-		fs, _ := os.ReadDir(src)
-		for _, f := range fs {
-			files = append(files, filepath.Join(src, f.Name()))
-		}
-	} else {
-		files = append(files, src)
-	}
-	zip := archiver.NewZip()
-	defer zip.Close()
-	return zip.Archive(files, dst)
-}
-
-// unarchive is a function that unzips a zip file to destination
-func (fetcher *Fetcher) unarchive(src string, dst string) error {
-	zip := archiver.NewZip()
-	defer zip.Close()
-	err := zip.Unarchive(src, dst)
-	if err != nil {
-		return fmt.Errorf("failed to unzip file: %w", err)
-	}
-	return nil
-}
-
 // getPkgInformation gets package information from k8s api server.
 func (fetcher *Fetcher) getPkgInformation(ctx context.Context, req FunctionFetchRequest) (pkg *fv1.Package, err error) {
 	logger := otelUtils.LoggerWithTraceID(ctx, fetcher.logger)

pkg/fission-cli/cmd/package/package.go

@@ -185,7 +185,7 @@ func CreateArchive(client cmd.Client, input cli.Input, includeFiles []string, no
 		return &archive, nil
 	}
 
-	archivePath, err := makeArchiveFile("", includeFiles, noZip)
+	archivePath, err := makeArchiveFile(input.Context(), "", includeFiles, noZip)
 	if err != nil {
 		return nil, err
 	}
@@ -200,7 +200,7 @@ func CreateArchive(client cmd.Client, input cli.Input, includeFiles []string, no
 // returned as-is with no zipping.  (This is used for compatibility
 // with v1 envs.)  noZip is IGNORED if there is more than one input
 // file.
-func makeArchiveFile(archiveNameHint string, archiveInput []string, noZip bool) (string, error) {
+func makeArchiveFile(ctx context.Context, archiveNameHint string, archiveInput []string, noZip bool) (string, error) {
 
 	// Unique name for the archive
 	archiveFileName := archiveName(archiveNameHint, archiveInput) + ".zip"
@@ -219,7 +219,7 @@ func makeArchiveFile(archiveNameHint string, archiveInput []string, noZip bool)
 		}
 
 		// if it's an existing zip file OR we're not supposed to zip it, don't do anything
-		if match, _ := utils.IsZip(files[0]); match || noZip {
+		if match, _ := utils.IsZip(ctx, files[0]); match || noZip {
 			return files[0], nil
 		}
 	}
@@ -230,7 +230,7 @@ func makeArchiveFile(archiveNameHint string, archiveInput []string, noZip bool)
 		return "", errors.Wrap(err, "error create temporary archive directory")
 	}
 
-	archivePath, err := utils.MakeZipArchive(filepath.Join(tmpDir, archiveFileName), archiveInput...)
+	archivePath, err := utils.MakeZipArchiveWithGlobs(ctx, filepath.Join(tmpDir, archiveFileName), archiveInput...)
 	if err != nil {
 		return "", errors.Wrap(err, "create archive file")
 	}

pkg/fission-cli/cmd/spec/apply.go

@@ -27,7 +27,6 @@ import (
 
 	"github.com/fsnotify/fsnotify"
 	"github.com/go-git/go-git/v5"
-	"github.com/mholt/archiver/v3"
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -358,7 +357,7 @@ func applyArchives(input cli.Input, fclient cmd.Client, specDir string, fr *Fiss
 
 	// create archives locally and calculate checksums
 	for _, aus := range fr.ArchiveUploadSpecs {
-		ar, err := localArchiveFromSpec(specDir, &aus)
+		ar, err := localArchiveFromSpec(input.Context(), specDir, &aus)
 		if err != nil {
 			return err
 		}
@@ -501,7 +500,7 @@ func applyResources(input cli.Input, fclient cmd.Client, specDir string, fr *Fis
 
 // localArchiveFromSpec creates an archive on the local filesystem from the given spec,
 // and returns its path and checksum.
-func localArchiveFromSpec(specDir string, aus *spectypes.ArchiveUploadSpec) (*fv1.Archive, error) {
+func localArchiveFromSpec(ctx context.Context, specDir string, aus *spectypes.ArchiveUploadSpec) (*fv1.Archive, error) {
 	// get root dir
 	var rootDir string
 
@@ -518,7 +517,7 @@ func localArchiveFromSpec(specDir string, aus *spectypes.ArchiveUploadSpec) (*fv
 	files := make([]string, 0)
 
 	// checking if file is a zip
-	if match, _ := utils.IsZip(aus.IncludeGlobs[0]); match && len(aus.IncludeGlobs) == 1 {
+	if match, _ := utils.IsZip(ctx, aus.IncludeGlobs[0]); match && len(aus.IncludeGlobs) == 1 {
 		files = append(files, aus.IncludeGlobs[0])
 	} else {
 		for _, relativeGlob := range aus.IncludeGlobs {
@@ -560,9 +559,7 @@ func localArchiveFromSpec(specDir string, aus *spectypes.ArchiveUploadSpec) (*fv
 		}
 		archiveFileName = archiveFile.Name()
 
-		// This instance is required to allow overwriting and not changing DefaultZip
-		zipOverwrite := archiver.Zip{OverwriteExisting: true}
-		err = zipOverwrite.Archive(files, archiveFileName)
+		_, err = utils.MakeZipArchiveWithGlobs(ctx, archiveFileName, files...)
 		if err != nil {
 			return nil, err
 		}

pkg/fission-cli/cmd/spec/destroy.go

@@ -176,7 +176,7 @@ func (opts *DestroySubCommand) insertNSToResource(input cli.Input, fr *FissionRe
 	return result.ErrorOrNil()
 }
 
-func deleteResources(ctx context.Context, fclient cmd.Client, fr *FissionResources, forceDelete bool) error {
+func deleteResources(ctx context.Context, fclient cmd.Client, fr *FissionResources, _ bool) error {
 
 	var err error
 

pkg/fission-cli/cmd/support/dump.go

@@ -142,7 +142,7 @@ func (opts *DumpSubCommand) do(input cli.Input) error {
 	if !nozip {
 		defer os.RemoveAll(tempDir)
 		path := filepath.Join(outputDir, fmt.Sprintf("%v.zip", dumpName))
-		_, err := utils.MakeZipArchive(path, tempDir)
+		_, err := utils.MakeZipArchiveWithGlobs(input.Context(), path, tempDir)
 		if err != nil {
 			fmt.Printf("Error creating archive for dump files: %v", err)
 			return err

pkg/utils/utils.go

@@ -29,7 +29,6 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/mholt/archiver/v3"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context/ctxhttp"
 	apiv1 "k8s.io/api/core/v1"
@@ -88,21 +87,6 @@ func FindAllGlobs(paths ...string) ([]string, error) {
 	return files, nil
 }
 
-func MakeZipArchive(targetName string, globs ...string) (string, error) {
-	files, err := FindAllGlobs(globs...)
-	if err != nil {
-		return "", err
-	}
-
-	// zip up the file list
-	err = archiver.DefaultZip.Archive(files, targetName)
-	if err != nil {
-		return "", err
-	}
-
-	return filepath.Abs(targetName)
-}
-
 // RemoveZeroBytes remove empty byte(\x00) from input byte slice and return a new byte slice
 // This function is trying to fix the problem that empty byte will fail os.Openfile
 // For more information, please visit:
@@ -219,15 +203,6 @@ func DownloadUrl(ctx context.Context, httpClient *http.Client, url string, local
 	return nil
 }
 
-func IsZip(filename string) (bool, error) {
-	f, err := os.Open(filename)
-	if err != nil {
-		return false, nil
-	}
-	defer f.Close()
-	return archiver.DefaultZip.Match(f)
-}
-
 func GetStringValueFromEnv(envVar string) (string, error) {
 	v := os.Getenv(envVar)
 	if v == "" {
@@ -310,17 +285,22 @@ func IsOwnerReferencesEnabled() bool {
 	return !disableOwnerReference
 }
 
-// ValidateFilePathComponent checks if the filename is valid to prevent directory traversal attacks.
-func ValidateFilePathComponent(filename string) bool {
-	return len(filename) > 0 && !containsInvalidChars(filename)
-}
-
-func containsInvalidChars(filename string) bool {
-	invalidChars := []string{"/", "\\", ".."}
-	for _, char := range invalidChars {
-		if strings.Contains(filename, char) {
-			return true
-		}
+// SanitizeFilePath checks if the path is valid to prevent directory traversal attacks.
+func SanitizeFilePath(path string, safedir string) (string, error) {
+	if len(path) == 0 {
+		return "", errors.New("invalid path")
 	}
-	return false
+	if len(safedir) == 0 {
+		return "", errors.New("invalid safe directory")
+	}
+	// get normalized path and check for directory traversal attacks
+	normalizedPath := filepath.Clean(path)
+	if normalizedPath != path {
+		return "", errors.New("invalid path")
+	}
+	// check if the path is under the safe directory
+	if !strings.HasPrefix(normalizedPath, safedir) {
+		return "", fmt.Errorf("path %s is not under the safe directory %s", normalizedPath, safedir)
+	}
+	return normalizedPath, nil
 }

pkg/utils/zip.go

@@ -0,0 +1,120 @@
+package utils
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"os"
+	"path/filepath"
+
+	"github.com/mholt/archives"
+)
+
+func IsZip(ctx context.Context, filename string) (bool, error) {
+	f, err := os.Open(filename)
+	if err != nil {
+		return false, nil
+	}
+	result, err := archives.Zip{}.Match(ctx, filename, f)
+	if err != nil {
+		return false, err
+	}
+	if result.ByName || result.ByStream {
+		return true, nil
+	}
+	return false, nil
+}
+
+func MakeZipArchiveWithGlobs(ctx context.Context, targetName string, globs ...string) (string, error) {
+	globFiles, err := FindAllGlobs(globs...)
+	if err != nil {
+		return "", err
+	}
+	if len(globFiles) == 0 {
+		return "", fmt.Errorf("no files found for globs: %v", globs)
+	}
+	files := make(map[string]string, len(globFiles))
+	for _, file := range globFiles {
+		files[file] = ""
+	}
+
+	archiveFiles, err := archives.FilesFromDisk(ctx, nil, files)
+	if err != nil {
+		return "", fmt.Errorf("failed to read files from disk: %w", err)
+	}
+	out, err := os.Create(targetName)
+	if err != nil {
+		return "", fmt.Errorf("failed to create archive file: %w", err)
+	}
+	defer out.Close()
+	zip := archives.CompressedArchive{
+		Archival: archives.Zip{},
+	}
+	if err := zip.Archive(ctx, out, archiveFiles); err != nil {
+		return "", fmt.Errorf("failed to create archive: %w", err)
+	}
+	return filepath.Abs(targetName)
+}
+
+// Archive zips the contents of directory at src into a new zip file
+// at dst (note that the contents are zipped, not the directory itself).
+func Archive(ctx context.Context, src string, dst string) error {
+	srcInfo, err := os.Stat(src)
+	if err != nil {
+		return fmt.Errorf("failed to get source directory info: %w", err)
+	}
+	if srcInfo.IsDir() {
+		src = src + "/*"
+	}
+	_, err = MakeZipArchiveWithGlobs(ctx, dst, src)
+	return err
+}
+
+// Unarchive is a function that unzips a zip file to destination
+func Unarchive(ctx context.Context, src string, dst string) error {
+	var format archives.Zip
+	file, err := os.Open(src)
+	if err != nil {
+		return fmt.Errorf("failed to open file: %w", err)
+	}
+	defer file.Close()
+
+	return format.Extract(ctx, file, func(ctx context.Context, f archives.FileInfo) error {
+		destPath := filepath.Join(dst, f.NameInArchive)
+		// check if the file is a directory
+		if f.IsDir() {
+			return os.MkdirAll(destPath, f.Mode())
+		}
+
+		// check if parent directory exists for the file
+		if err := os.MkdirAll(filepath.Dir(destPath), os.ModeDir|0755); err != nil {
+			return fmt.Errorf("failed to create parent directory: %w", err)
+		}
+
+		// Open file in archive
+		rc, err := f.Open()
+		if err != nil {
+			return fmt.Errorf("failed to open file in archive: %w", err)
+		}
+		defer rc.Close()
+
+		// Create file in destination
+		destFile, err := os.Create(destPath)
+		if err != nil {
+			return fmt.Errorf("failed to create file in destination: %w", err)
+		}
+		defer destFile.Close()
+		err = destFile.Chmod(f.Mode())
+		if err != nil {
+			return fmt.Errorf("failed to set file permissions: %w", err)
+		}
+
+		// Copy file contents
+		_, err = io.Copy(destFile, rc)
+		if err != nil {
+			return fmt.Errorf("failed to copy file contents: %w", err)
+		}
+
+		return nil
+	})
+}

pkg/utils/zip_test.go

@@ -0,0 +1,354 @@
+package utils
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestIsZip(t *testing.T) {
+	tests := []struct {
+		name    string
+		setupFn func() string
+		want    bool
+		wantErr bool
+		cleanup bool
+	}{
+		{
+			name: "valid zip file",
+			setupFn: func() string {
+				return "testdata/test.zip"
+			},
+			want:    true,
+			wantErr: false,
+			cleanup: false,
+		},
+		{
+			name: "non-existent file",
+			setupFn: func() string {
+				return "testdata/non-existent.zip"
+			},
+			want:    false,
+			wantErr: false,
+			cleanup: true,
+		},
+		{
+			name: "text file",
+			setupFn: func() string {
+				f, err := os.CreateTemp("", "test-*.txt")
+				if err != nil {
+					t.Fatal(err)
+				}
+				defer f.Close()
+				if _, err := f.WriteString("hello world"); err != nil {
+					t.Fatal(err)
+				}
+				return f.Name()
+			},
+			want:    false,
+			wantErr: false,
+			cleanup: true,
+		},
+		{
+			name: "corrupt zip file",
+			setupFn: func() string {
+				f, err := os.CreateTemp("", "corrupt-*.zip")
+				if err != nil {
+					t.Fatal(err)
+				}
+				defer f.Close()
+				if _, err := f.WriteString("corrupted content"); err != nil {
+					t.Fatal(err)
+				}
+				return f.Name()
+			},
+			want:    true,
+			wantErr: false,
+			cleanup: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			filename := tt.setupFn()
+			if tt.cleanup && !filepath.IsAbs(filename) {
+				// Cleanup only temp files
+				t.Cleanup(func() {
+					os.Remove(filename)
+				})
+			}
+
+			got, err := IsZip(context.Background(), filename)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("IsZip() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got != tt.want {
+				t.Errorf("IsZip() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestArchiveUnarchive(t *testing.T) {
+	ctx := context.Background()
+
+	// Create temp test directories
+	sourceDir, err := os.MkdirTemp("", "zip-test-source-*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(sourceDir)
+
+	// Create test files and directories
+	files := map[string][]byte{
+		"file1.txt":           []byte("hello world"),
+		"file2.txt":           []byte("test content"),
+		"dir1/file3.txt":      []byte("nested file"),
+		"dir1/dir2/file4.txt": []byte("deeply nested"),
+	}
+
+	for path, content := range files {
+		fullPath := filepath.Join(sourceDir, path)
+		err := os.MkdirAll(filepath.Dir(fullPath), 0755)
+		if err != nil {
+			t.Fatal(err)
+		}
+		err = os.WriteFile(fullPath, content, 0644)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	// Create empty directory
+	emptyDir := filepath.Join(sourceDir, "empty-dir")
+	if err := os.Mkdir(emptyDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+
+	tests := []struct {
+		name    string
+		srcPath string
+		wantErr bool
+	}{
+		{
+			name:    "archive and unarchive directory",
+			srcPath: sourceDir,
+			wantErr: false,
+		},
+		{
+			name:    "archive and unarchive single file",
+			srcPath: filepath.Join(sourceDir, "file1.txt"),
+			wantErr: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create temp zip file
+			zipFile, err := os.CreateTemp("", "test-*.zip")
+			if err != nil {
+				t.Fatal(err)
+			}
+			zipFile.Close()
+			defer os.Remove(zipFile.Name())
+
+			// Create temp extract directory
+			extractDir, err := os.MkdirTemp("", "zip-test-extract-*")
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer os.RemoveAll(extractDir)
+
+			// Test Archive
+			err = Archive(ctx, tt.srcPath, zipFile.Name())
+			if (err != nil) != tt.wantErr {
+				t.Errorf("Archive() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+
+			// Test is valid zip file
+			isZip, err := IsZip(ctx, zipFile.Name())
+			if err != nil {
+				t.Fatal(err)
+			}
+			if !isZip {
+				t.Errorf("Archive() did not create a valid zip file")
+				return
+			}
+
+			// Test Unarchive
+			err = Unarchive(ctx, zipFile.Name(), extractDir)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("Unarchive() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+
+			// Validate extracted content
+			err = filepath.Walk(tt.srcPath, func(path string, info os.FileInfo, err error) error {
+				if err != nil {
+					return err
+				}
+
+				relPath, err := filepath.Rel(tt.srcPath, path)
+				if err != nil {
+					return err
+				}
+				if relPath == "." {
+					return nil
+				}
+
+				extractedPath := filepath.Join(extractDir, relPath)
+
+				extractedInfo, err := os.Stat(extractedPath)
+				if err != nil {
+					t.Errorf("Expected file %s not found in extracted directory", relPath)
+					return nil
+				}
+
+				if info.Mode().Perm() != extractedInfo.Mode().Perm() {
+					t.Errorf("File %s permissions mismatch: got %v, want %v",
+						relPath, extractedInfo.Mode().Perm(), info.Mode().Perm())
+				}
+
+				if !info.IsDir() {
+					originalContent, err := os.ReadFile(path)
+					if err != nil {
+						return err
+					}
+					extractedContent, err := os.ReadFile(extractedPath)
+					if err != nil {
+						return err
+					}
+					if string(originalContent) != string(extractedContent) {
+						t.Errorf("File %s content mismatch", relPath)
+					}
+				}
+				return nil
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+		})
+	}
+}
+
+func TestArchiveOverwrite(t *testing.T) {
+	ctx := context.Background()
+
+	// Create initial source directory
+	sourceDir, err := os.MkdirTemp("", "zip-test-source-*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(sourceDir)
+
+	// Create initial files
+	initialFiles := map[string][]byte{
+		"old1.txt": []byte("old content 1"),
+		"old2.txt": []byte("old content 2"),
+	}
+	for path, content := range initialFiles {
+		fullPath := filepath.Join(sourceDir, path)
+		if err := os.WriteFile(fullPath, content, 0644); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	// Create zip file
+	zipFile, err := os.CreateTemp("", "test-*.zip")
+	if err != nil {
+		t.Fatal(err)
+	}
+	zipFile.Close()
+	defer os.Remove(zipFile.Name())
+
+	// Create initial zip
+	if err := Archive(ctx, sourceDir, zipFile.Name()); err != nil {
+		t.Fatal(err)
+	}
+
+	// Create new source directory with different content
+	newSourceDir, err := os.MkdirTemp("", "zip-test-new-source-*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(newSourceDir)
+
+	// Create new files
+	newFiles := map[string][]byte{
+		"new1.txt": []byte("new content 1"),
+		"new2.txt": []byte("new content 2"),
+	}
+	for path, content := range newFiles {
+		fullPath := filepath.Join(newSourceDir, path)
+		if err := os.WriteFile(fullPath, content, 0644); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	// Overwrite existing zip
+	if err := Archive(ctx, newSourceDir, zipFile.Name()); err != nil {
+		t.Fatal(err)
+	}
+
+	// Create extraction directory
+	extractDir, err := os.MkdirTemp("", "zip-test-extract-*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(extractDir)
+
+	// Extract overwritten zip
+	if err := Unarchive(ctx, zipFile.Name(), extractDir); err != nil {
+		t.Fatal(err)
+	}
+
+	// Validate extracted content
+	files, err := os.ReadDir(extractDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Verify only new files exist
+	expectedFiles := map[string]bool{
+		"new1.txt": false,
+		"new2.txt": false,
+	}
+
+	for _, f := range files {
+		if _, ok := expectedFiles[f.Name()]; !ok {
+			t.Errorf("Unexpected file found: %s", f.Name())
+			continue
+		}
+		expectedFiles[f.Name()] = true
+
+		// Verify content
+		content, err := os.ReadFile(filepath.Join(extractDir, f.Name()))
+		if err != nil {
+			t.Fatal(err)
+		}
+		expected := newFiles[f.Name()]
+		if string(content) != string(expected) {
+			t.Errorf("File %s content mismatch: got %s, want %s",
+				f.Name(), string(content), string(expected))
+		}
+	}
+
+	// Verify old files do not exist
+	oldFiles := []string{"old1.txt", "old2.txt"}
+	for _, oldFile := range oldFiles {
+		_, err := os.Stat(filepath.Join(extractDir, oldFile))
+		if !os.IsNotExist(err) {
+			t.Errorf("Old file %s should not exist in zip", oldFile)
+		}
+	}
+
+	// Verify all expected files were found
+	for name, found := range expectedFiles {
+		if !found {
+			t.Errorf("Expected file not found: %s", name)
+		}
+	}
+}