updated per code review

Signed-off-by: Patrick Zheng <[email protected]>

internal/file/file.go

@@ -28,36 +28,37 @@ func IsValidFileName(fileName string) bool {
 }
 
 // CopyToDir copies the src file to dst. Existing file will be overwritten.
-func CopyToDir(src, dst string) (int64, error) {
+func CopyToDir(src, dst string) error {
 	sourceFileStat, err := os.Stat(src)
 	if err != nil {
-		return 0, err
+		return err
 	}
 
 	if !sourceFileStat.Mode().IsRegular() {
-		return 0, fmt.Errorf("%s is not a regular file", src)
+		return fmt.Errorf("%s is not a regular file", src)
 	}
 
 	source, err := os.Open(src)
 	if err != nil {
-		return 0, err
+		return err
 	}
 	defer source.Close()
 
 	if err := os.MkdirAll(dst, 0700); err != nil {
-		return 0, err
+		return err
 	}
 	dstFile := filepath.Join(dst, filepath.Base(src))
 	destination, err := os.Create(dstFile)
 	if err != nil {
-		return 0, err
+		return err
 	}
 	defer destination.Close()
 	err = destination.Chmod(0600)
 	if err != nil {
-		return 0, err
+		return err
 	}
-	return io.Copy(destination, source)
+	_, err = io.Copy(destination, source)
+	return err
 }
 
 // FileNameWithoutExtension returns the file name without extension.

internal/file/file_test.go

@@ -31,7 +31,7 @@ func TestCopyToDir(t *testing.T) {
 		}
 
 		destDir := filepath.Join(tempDir, "b")
-		if _, err := CopyToDir(filename, destDir); err != nil {
+		if err := CopyToDir(filename, destDir); err != nil {
 			t.Fatal(err)
 		}
 	})
@@ -54,15 +54,15 @@ func TestCopyToDir(t *testing.T) {
 		}
 		defer os.Chmod(tempDir, 0700)
 
-		if _, err := CopyToDir(filename, destDir); err == nil {
+		if err := CopyToDir(filename, destDir); err == nil {
 			t.Fatal("should have error")
 		}
 	})
 
 	t.Run("not a regular file", func(t *testing.T) {
 		tempDir := t.TempDir()
 		destDir := t.TempDir()
-		if _, err := CopyToDir(tempDir, destDir); err == nil {
+		if err := CopyToDir(tempDir, destDir); err == nil {
 			t.Fatal("should have error")
 		}
 	})
@@ -85,7 +85,7 @@ func TestCopyToDir(t *testing.T) {
 			t.Fatal(err)
 		}
 		defer os.Chmod(filename, 0600)
-		if _, err := CopyToDir(filename, destDir); err == nil {
+		if err := CopyToDir(filename, destDir); err == nil {
 			t.Fatal("should have error")
 		}
 	})
@@ -108,7 +108,7 @@ func TestCopyToDir(t *testing.T) {
 			t.Fatal(err)
 		}
 		defer os.Chmod(destTempDir, 0700)
-		if _, err := CopyToDir(filename, filepath.Join(destTempDir, "a")); err == nil {
+		if err := CopyToDir(filename, filepath.Join(destTempDir, "a")); err == nil {
 			t.Fatal("should have error")
 		}
 	})
@@ -131,7 +131,7 @@ func TestCopyToDir(t *testing.T) {
 			t.Fatal(err)
 		}
 		defer os.Chmod(destTempDir, 0700)
-		if _, err := CopyToDir(filename, destTempDir); err == nil {
+		if err := CopyToDir(filename, destTempDir); err == nil {
 			t.Fatal("should have error")
 		}
 	})
@@ -145,7 +145,7 @@ func TestCopyToDir(t *testing.T) {
 		}
 
 		destDir := filepath.Join(tempDir, "b")
-		if _, err := CopyToDir(filename, destDir); err != nil {
+		if err := CopyToDir(filename, destDir); err != nil {
 			t.Fatal(err)
 		}
 		validFileContent(t, filepath.Join(destDir, "file.txt"), data)

internal/semver/semver.go

@@ -17,30 +17,35 @@ package semver
 
 import (
 	"fmt"
-	"strings"
+	"regexp"
 
 	"golang.org/x/mod/semver"
 )
 
+// SemVerRegEx is takenfrom https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
+var SemVerRegEx = regexp.MustCompile(`^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$`)
+
+// IsVersionSemverValid returns true if version is a valid semantic version
+func IsVersionSemverValid(version string) bool {
+	return SemVerRegEx.MatchString(version)
+}
+
 // ComparePluginVersion validates and compares two plugin semantic versions.
 //
 // The result will be 0 if v == w, -1 if v < w, or +1 if v > w.
 func ComparePluginVersion(v, w string) (int, error) {
-	// golang.org/x/mod/semver requires semantic version strings must begin
-	// with a leading "v". Adding prefix "v" in case the input plugin version
-	// does not have it.
-	// Reference: https://pkg.go.dev/golang.org/x/mod/semver#pkg-overview
-	if !strings.HasPrefix(v, "v") {
-		v = "v" + v
-	}
-	if !semver.IsValid(v) {
+	// sanity check
+	// a valid semantic version should not have prefix `v`
+	// Reference: https://semver.org/#semantic-versioning-200
+	if !IsVersionSemverValid(v) {
 		return 0, fmt.Errorf("%s is not a valid semantic version", v)
 	}
-	if !strings.HasPrefix(w, "v") {
-		w = "v" + w
-	}
-	if !semver.IsValid(w) {
+	if !IsVersionSemverValid(w) {
 		return 0, fmt.Errorf("%s is not a valid semantic version", w)
 	}
-	return semver.Compare(v, w), nil
+
+	// golang.org/x/mod/semver requires semantic version strings must begin
+	// with a leading "v". Adding prefix "v" to the inputs.
+	// Reference: https://pkg.go.dev/golang.org/x/mod/semver#pkg-overview
+	return semver.Compare("v"+v, "v"+w), nil
 }

plugin/errors.go

@@ -22,13 +22,13 @@ var ErrNotCompliant = errors.New("plugin not compliant")
 // ErrNotRegularFile is returned when the plugin file is not an regular file.
 var ErrNotRegularFile = errors.New("not regular file")
 
-// ErrInstallLowerVersion is returned when installing a plugin with version
+// ErrPluginDowngrade is returned when installing a plugin with version
 // lower than the exisiting plugin version.
-type ErrInstallLowerVersion struct {
+type ErrPluginDowngrade struct {
 	Msg string
 }
 
-func (e ErrInstallLowerVersion) Error() string {
+func (e ErrPluginDowngrade) Error() string {
 	if e.Msg != "" {
 		return e.Msg
 	}

plugin/manager.go

@@ -83,17 +83,19 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) {
 }
 
 // Install installs a plugin at filePath to the system.
-//
-// It returns the new plugin metadata and a nil eror if and only if
+// It returns the new plugin metadata and a nil error if and only if
 // the installation succeeded.
 //
-// If plugin already exists, and overwrite is not set, then the new plugin
+// If plugin does not exist, directly install from filePath.
+// On success, the new plugin metadata is returned.
+//
+// If plugin already exists:
+//
+// If overwrite is not set, then the new plugin
 // version MUST be higher than the existing plugin version.
-// If overwrite is set, version check is skipped.
-// On sucess, existing plugin metadata and new plugin metadata are returned.
 //
-// If plugin does not exist, directly install the plugin from filePath.
-// On success, the new plugin metadata is returned.
+// If overwrite is set, version check is skipped. If existing
+// plugin is malfunctioning, it will be overwritten as well.
 func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite bool) (*proto.GetMetadataResponse, *proto.GetMetadataResponse, error) {
 	// validate and get new plugin metadata
 	pluginFile := filepath.Base(filePath)
@@ -122,7 +124,7 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo
 	} else { // plugin already exists
 		var err error
 		existingPluginMetadata, err = existingPlugin.GetMetadata(ctx, &proto.GetMetadataRequest{})
-		if err != nil {
+		if err != nil && !overwrite { // fail only if overwrite is not set
 			return nil, nil, fmt.Errorf("failed to get metadata of existing plugin: %w", err)
 		}
 		if !overwrite { // overwrite is not set, check version
@@ -132,7 +134,7 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo
 			}
 			switch {
 			case comp < 0:
-				return nil, nil, ErrInstallLowerVersion{Msg: fmt.Sprintf("the installing plugin version %s is lower than the existing plugin version %s", newPluginMetadata.Version, existingPluginMetadata.Version)}
+				return nil, nil, ErrPluginDowngrade{Msg: fmt.Sprintf("the installing plugin version %s is lower than the existing plugin version %s", newPluginMetadata.Version, existingPluginMetadata.Version)}
 			case comp == 0:
 				return nil, nil, ErrInstallEqualVersion{Msg: fmt.Sprintf("plugin %s with version %s already exists", pluginName, existingPluginMetadata.Version)}
 			}
@@ -142,8 +144,7 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err)
 	}
-	_, err = file.CopyToDir(filePath, pluginDirPath)
-	if err != nil {
+	if err := file.CopyToDir(filePath, pluginDirPath); err != nil {
 		return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", filePath, pluginDirPath, err)
 	}
 	// plugin is always executable

plugin/manager_test.go

@@ -273,7 +273,7 @@ func TestManager_Install(t *testing.T) {
 			newPluginStdout:   metadataJSON(validMetadataBar),
 		}
 		_, _, err = mgr.Install(context.Background(), newPluginFilePath, false)
-		expectedErrorMsg := "invalid plugin file name extension. Expecting file notation-bar, but got notation-bar.exe"
+		expectedErrorMsg := "invalid plugin file extension. Expecting file notation-bar, but got notation-bar.exe"
 		if err == nil || err.Error() != expectedErrorMsg {
 			t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err)
 		}
@@ -330,6 +330,19 @@ func TestManager_Install(t *testing.T) {
 			t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err)
 		}
 	})
+
+	t.Run("success to install with overwrite and invalid existing plugin metadata", func(t *testing.T) {
+		executor = testInstallCommander{
+			existedPluginFilePath: existedPluginFilePath,
+			newPluginFilePath:     newPluginFilePath,
+			existedPluginStdout:   metadataJSON(validMetadataBar),
+			newPluginStdout:       metadataJSON(validMetadata),
+		}
+		_, _, err = mgr.Install(context.Background(), newPluginFilePath, true)
+		if err != nil {
+			t.Fatalf("expecting error to be nil, but got %v", err)
+		}
+	})
 }
 
 func TestManager_Uninstall(t *testing.T) {

plugin/plugin.go

@@ -270,10 +270,10 @@ func validate(metadata *proto.GetMetadataResponse) error {
 //
 // On windows, `.exe` extension is required.
 // On other OS, MUST not have the `.exe` extension.
-func validatePluginFileExtensionAgainstOS(fileName string, pluginName string) error {
+func validatePluginFileExtensionAgainstOS(fileName, pluginName string) error {
 	expectedPluginFile := binName(pluginName)
 	if filepath.Ext(fileName) != filepath.Ext(expectedPluginFile) {
-		return fmt.Errorf("invalid plugin file name extension. Expecting file %s, but got %s", expectedPluginFile, fileName)
+		return fmt.Errorf("invalid plugin file extension. Expecting file %s, but got %s", expectedPluginFile, fileName)
 	}
 	return nil
 }

plugin/plugin_test.go

@@ -310,7 +310,7 @@ func TestValidatePluginFileExtensionAgainstOS(t *testing.T) {
 		}
 
 		err = validatePluginFileExtensionAgainstOS("notation-foo", "foo")
-		expectedErrorMsg := "invalid plugin file name extension. Expecting file notation-foo.exe, but got notation-foo"
+		expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo.exe, but got notation-foo"
 		if err == nil || err.Error() != expectedErrorMsg {
 			t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err)
 		}
@@ -322,7 +322,7 @@ func TestValidatePluginFileExtensionAgainstOS(t *testing.T) {
 	}
 
 	err = validatePluginFileExtensionAgainstOS("notation-foo.exe", "foo")
-	expectedErrorMsg := "invalid plugin file name extension. Expecting file notation-foo, but got notation-foo.exe"
+	expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo, but got notation-foo.exe"
 	if err == nil || err.Error() != expectedErrorMsg {
 		t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err)
 	}

verifier/helpers.go

@@ -18,12 +18,12 @@ import (
 	"crypto/x509"
 	"errors"
 	"fmt"
-	"regexp"
 	"strings"
 
 	"github.com/notaryproject/notation-core-go/signature"
 	"github.com/notaryproject/notation-go"
 	set "github.com/notaryproject/notation-go/internal/container"
+	notationsemver "github.com/notaryproject/notation-go/internal/semver"
 	"github.com/notaryproject/notation-go/internal/slices"
 	"github.com/notaryproject/notation-go/verifier/trustpolicy"
 	"github.com/notaryproject/notation-go/verifier/truststore"
@@ -47,9 +47,6 @@ var VerificationPluginHeaders = []string{
 
 var errExtendedAttributeNotExist = errors.New("extended attribute not exist")
 
-// semVerRegEx is takenfrom https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
-var semVerRegEx = regexp.MustCompile(`^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$`)
-
 func loadX509TrustStores(ctx context.Context, scheme signature.SigningScheme, policy *trustpolicy.TrustPolicy, x509TrustStore truststore.X509TrustStore) ([]*x509.Certificate, error) {
 	var typeToLoad truststore.Type
 	switch scheme {
@@ -152,12 +149,8 @@ func getVerificationPluginMinVersion(signerInfo *signature.SignerInfo) (string,
 	if strings.TrimSpace(version) == "" {
 		return "", fmt.Errorf("%v from extended attribute is an empty string", HeaderVerificationPluginMinVersion)
 	}
-	if !isVersionSemverValid(version) {
+	if !notationsemver.IsVersionSemverValid(version) {
 		return "", fmt.Errorf("%v from extended attribute is not a valid SemVer", HeaderVerificationPluginMinVersion)
 	}
 	return version, nil
 }
-
-func isVersionSemverValid(version string) bool {
-	return semVerRegEx.MatchString(version)
-}

verifier/verifier.go

@@ -32,6 +32,7 @@ import (
 	"github.com/notaryproject/notation-go/dir"
 	"github.com/notaryproject/notation-go/internal/envelope"
 	"github.com/notaryproject/notation-go/internal/pkix"
+	notationsemver "github.com/notaryproject/notation-go/internal/semver"
 	"github.com/notaryproject/notation-go/internal/slices"
 	trustpolicyInternal "github.com/notaryproject/notation-go/internal/trustpolicy"
 	"github.com/notaryproject/notation-go/log"
@@ -231,7 +232,7 @@ func (v *verifier) processSignature(ctx context.Context, sigBlob []byte, envelop
 		pluginVersion := metadata.Version
 
 		//checking if the plugin version is in valid semver format
-		if !isVersionSemverValid(pluginVersion) {
+		if !notationsemver.IsVersionSemverValid(pluginVersion) {
 			return notation.ErrorVerificationInconclusive{Msg: fmt.Sprintf("plugin %s has pluginVersion %s which is not in valid semver format", verificationPluginName, pluginVersion)}
 		}