Return the file error instead of the URL error for a missing artifact path (#2872)

validateFileOrURL fell through to URL validation when the file path
failed, so a nonexistent --artifact/--entry file returned only
"'<v>' is not a valid url" and masked the real error (e.g. "no such
file or directory"). Return the file validation error when the value is
not an http(s) URL so the message is actionable, and add
TestValidateFileOrURL.

Addresses #2138

Signed-off-by: Nikhil Jathar <22786232+mailnike@users.noreply.github.com>

Author: mailnike

cmd/rekor-cli/app/pflags.go

@@ -280,12 +280,23 @@ func validateSHAValue(v string) error {
 
 // validateFileOrURL ensures the provided string is either a valid file path that can be opened or a valid URL
 func validateFileOrURL(v string) error {
-	valGen := pflagValueFuncMap[fileFlag]
-	if valGen().Set(v) == nil {
+	fileErr := pflagValueFuncMap[fileFlag]().Set(v)
+	if fileErr == nil {
 		return nil
 	}
-	valGen = pflagValueFuncMap[urlFlag]
-	return valGen().Set(v)
+	urlErr := pflagValueFuncMap[urlFlag]().Set(v)
+	if urlErr == nil {
+		return nil
+	}
+	// Neither a readable file nor a valid URL. Return the error for whichever
+	// the input was most likely intended to be, so the message is actionable:
+	// a value with an http(s) scheme is treated as a URL, anything else as a
+	// file path. Previously the file error (e.g. "no such file or directory")
+	// was always masked by the "is not a valid url" error.
+	if strings.HasPrefix(v, "http://") || strings.HasPrefix(v, "https://") {
+		return urlErr
+	}
+	return fileErr
 }
 
 // validateID ensures the ID is either an EntryID (TreeID + UUID) or a UUID

cmd/rekor-cli/app/pflags_test.go

@@ -21,6 +21,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/spf13/cobra"
@@ -926,3 +927,34 @@ func TestParseTypeFlag(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateFileOrURL(t *testing.T) {
+	tests := []struct {
+		caseDesc  string
+		value     string
+		expectErr bool
+		errSubstr string
+	}{
+		{caseDesc: "valid local file", value: "tests/test_file.txt", expectErr: false},
+		{caseDesc: "valid https url", value: "https://example.com/artifact", expectErr: false},
+		{caseDesc: "missing file reports the file error, not the url error", value: "tests/not_a_file", expectErr: true, errSubstr: "not_a_file"},
+		{caseDesc: "malformed http url reports the url error", value: "http://", expectErr: true, errSubstr: "is not a valid url"},
+	}
+	for _, tc := range tests {
+		err := validateFileOrURL(tc.value)
+		if tc.expectErr {
+			if err == nil {
+				t.Errorf("%s: expected an error, got nil", tc.caseDesc)
+				continue
+			}
+			if tc.errSubstr != "" && !strings.Contains(err.Error(), tc.errSubstr) {
+				t.Errorf("%s: expected error containing %q, got %q", tc.caseDesc, tc.errSubstr, err.Error())
+			}
+			if !strings.HasPrefix(tc.value, "http") && strings.Contains(err.Error(), "is not a valid url") {
+				t.Errorf("%s: file error was masked by the url error: %q", tc.caseDesc, err.Error())
+			}
+		} else if err != nil {
+			t.Errorf("%s: expected no error, got %q", tc.caseDesc, err.Error())
+		}
+	}
+}