test: fix some coverage issues, refactored some of the pagination logic to accomplish this (#3674)

Signed-off-by: Andrei Aaron <[email protected]>

Author: andaaron

pkg/api/htpasswd_test.go

@@ -538,5 +538,45 @@ func TestHTPasswdWatcher(t *testing.T) {
 			So(ok, ShouldBeTrue)
 			So(present, ShouldBeTrue)
 		})
+
+		Convey("Test htpasswd file with zero users warning", func() {
+			// Create a buffer to capture log output
+			logBuffer, multiWriter := test.CreateLogCapturingWriter(os.Stdout)
+			capturingLogger := log.NewLoggerWithWriter("debug", multiWriter)
+
+			username, _ := test.GenerateRandomString()
+			password, _ := test.GenerateRandomString()
+
+			htp := api.NewHTPasswd(capturingLogger)
+
+			// Create an empty htpasswd file (zero users)
+			emptyPath := test.MakeHtpasswdFileFromString(t, "")
+
+			// Reload the empty file
+			err := htp.Reload(emptyPath)
+			So(err, ShouldBeNil)
+
+			// Verify the warning message is logged
+			So(test.WaitForLogMessages(logBuffer, "loaded htpasswd file appears to have zero users", 1, 5*time.Second),
+				ShouldBeTrue)
+
+			// Verify store is empty
+			_, present := htp.Get(username)
+			So(present, ShouldBeFalse)
+
+			// Now load a file with a user and verify the info message instead
+			userPath := test.MakeHtpasswdFileFromString(t, test.GetBcryptCredString(username, password))
+
+			err = htp.Reload(userPath)
+			So(err, ShouldBeNil)
+
+			// Verify the info message is logged
+			So(test.WaitForLogMessages(logBuffer, "loaded htpasswd file", 1, 5*time.Second), ShouldBeTrue)
+
+			// Verify user is present
+			ok, present := htp.Authenticate(username, password)
+			So(ok, ShouldBeTrue)
+			So(present, ShouldBeTrue)
+		})
 	})
 }

pkg/cli/server/root.go

@@ -324,6 +324,11 @@ func validateStorageConfig(cfg *config.Config, logger zlog.Logger) error {
 		})
 	}
 
+	// Sort stores by route to ensure deterministic ordering
+	slices.SortFunc(allStores, func(a, b storeInfo) int {
+		return strings.Compare(a.route, b.route)
+	})
+
 	// Validate each store
 	for _, store := range allStores {
 		route := store.route

pkg/cli/server/root_test.go

@@ -1266,6 +1266,44 @@ storage:
 		os.Args = []string{"cli_test", "verify", tmpfile}
 		err = cli.NewServerRootCmd().Execute()
 		So(err, ShouldBeNil)
+
+		// Default local store is inside substore (should be rejected)
+		// default is at /tmp/zot-parent/subdir, /a is at /tmp/zot-parent
+		content = `{"storage":{"rootDirectory":"/tmp/zot-parent/subdir",
+							"subPaths": {"/a": {"rootDirectory": "/tmp/zot-parent"}}},
+							"http":{"address":"127.0.0.1","port":"8080","realm":"zot",
+							"auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`
+		err = os.WriteFile(tmpfile, []byte(content), 0o0600)
+		So(err, ShouldBeNil)
+
+		os.Args = []string{"cli_test", "verify", tmpfile}
+		err = cli.NewServerRootCmd().Execute()
+		So(err, ShouldNotBeNil)
+		// default storage is inside /a, validation reports this conflict
+		So(err.Error(), ShouldContainSubstring,
+			"invalid storage config, default storage root directory cannot be inside substore (route: /a) root directory")
+
+		// Default S3 store is inside substore, with S3, (should be rejected)
+		// default is at /zot-parent/subdir, /a is at /zot-parent
+		content = `{"storage":{"rootDirectory":"/zot-parent/subdir",
+							"storageDriver":{"name":"s3","rootdirectory":"/zot-parent/subdir","region":"us-east-2",
+							"bucket":"zot-storage","secure":true,"skipverify":false},
+							"dedupe":false,
+							"subPaths": {"/a": {"rootDirectory": "/zot-parent",
+							"storageDriver":{"name":"s3","rootdirectory":"/zot-parent","region":"us-east-2",
+							"bucket":"zot-storage","secure":true,"skipverify":false},
+							"dedupe":false}}},
+							"http":{"address":"127.0.0.1","port":"8080","realm":"zot",
+							"auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`
+		err = os.WriteFile(tmpfile, []byte(content), 0o0600)
+		So(err, ShouldBeNil)
+
+		os.Args = []string{"cli_test", "verify", tmpfile}
+		err = cli.NewServerRootCmd().Execute()
+		So(err, ShouldNotBeNil)
+		// default storage is inside /a, validation reports this conflict
+		So(err.Error(), ShouldContainSubstring,
+			"invalid storage config, default storage root directory cannot be inside substore (route: /a) root directory")
 	})
 
 	Convey("Test verify w/ authorization and w/o authentication", t, func(c C) {

pkg/extensions/search/cve/cve.go

@@ -339,21 +339,29 @@ func getConfigAndDigest(metaDB mTypes.MetaDB, manifestDigestStr string) (ispec.I
 	return manifestData.Manifests[0].Config, manifestDigest, err
 }
 
+func shouldIncludeCVE(cve cvemodel.CVE, searchedCVE, excludedCVE, severity string) bool {
+	if severity != "" && (cvemodel.CompareSeverities(cve.Severity, severity) != 0) {
+		return false
+	}
+
+	if excludedCVE != "" && cve.ContainsStr(excludedCVE) {
+		return false
+	}
+
+	if !cve.ContainsStr(searchedCVE) {
+		return false
+	}
+
+	return true
+}
+
 func filterCVEMap(cveMap map[string]cvemodel.CVE, searchedCVE, excludedCVE, severity string,
 	pageFinder *CvePageFinder,
 ) {
 	searchedCVE = strings.ToUpper(searchedCVE)
 
 	for _, cve := range cveMap {
-		if severity != "" && (cvemodel.CompareSeverities(cve.Severity, severity) != 0) {
-			continue
-		}
-
-		if excludedCVE != "" && cve.ContainsStr(excludedCVE) {
-			continue
-		}
-
-		if cve.ContainsStr(searchedCVE) {
+		if shouldIncludeCVE(cve, searchedCVE, excludedCVE, severity) {
 			pageFinder.Add(cve)
 		}
 	}
@@ -363,15 +371,7 @@ func filterCVEList(cveList []cvemodel.CVE, searchedCVE, excludedCVE, severity st
 	searchedCVE = strings.ToUpper(searchedCVE)
 
 	for _, cve := range cveList {
-		if severity != "" && (cvemodel.CompareSeverities(cve.Severity, severity) != 0) {
-			continue
-		}
-
-		if excludedCVE != "" && cve.ContainsStr(excludedCVE) {
-			continue
-		}
-
-		if cve.ContainsStr(searchedCVE) {
+		if shouldIncludeCVE(cve, searchedCVE, excludedCVE, severity) {
 			pageFinder.Add(cve)
 		}
 	}

pkg/extensions/search/cve/cve_internal_test.go

@@ -136,5 +136,57 @@ func TestUtils(t *testing.T) {
 				})
 			So(tags, ShouldBeEmpty)
 		})
+
+		Convey("shouldIncludeCVE filtering logic", func() {
+			baseCVE := cvemodel.CVE{
+				ID:          "CVE-2024-0001",
+				Severity:    "HIGH",
+				Title:       "Test CVE 1",
+				Description: "Description contains keyword",
+			}
+
+			Convey("includes CVE when all filters pass", func() {
+				// No filters
+				So(shouldIncludeCVE(baseCVE, "", "", ""), ShouldBeTrue)
+
+				// Matching searchedCVE
+				So(shouldIncludeCVE(baseCVE, "CVE-2024", "", ""), ShouldBeTrue)
+				So(shouldIncludeCVE(baseCVE, "keyword", "", ""), ShouldBeTrue)
+
+				// Matching severity
+				So(shouldIncludeCVE(baseCVE, "", "", "HIGH"), ShouldBeTrue)
+			})
+
+			Convey("excludes CVE when severity doesn't match", func() {
+				So(shouldIncludeCVE(baseCVE, "", "", "LOW"), ShouldBeFalse)
+				So(shouldIncludeCVE(baseCVE, "", "", "MEDIUM"), ShouldBeFalse)
+				So(shouldIncludeCVE(baseCVE, "", "", "CRITICAL"), ShouldBeFalse)
+			})
+
+			Convey("excludes CVE when it contains excluded string", func() {
+				So(shouldIncludeCVE(baseCVE, "", "keyword", ""), ShouldBeFalse)
+				So(shouldIncludeCVE(baseCVE, "", "CVE-2024", ""), ShouldBeFalse)
+				So(shouldIncludeCVE(baseCVE, "", "Test CVE", ""), ShouldBeFalse)
+			})
+
+			Convey("excludes CVE when searchedCVE doesn't match", func() {
+				So(shouldIncludeCVE(baseCVE, "CVE-2023", "", ""), ShouldBeFalse)
+				So(shouldIncludeCVE(baseCVE, "notfound", "", ""), ShouldBeFalse)
+			})
+
+			Convey("handles multiple filters combined", func() {
+				// All filters match - should include
+				So(shouldIncludeCVE(baseCVE, "CVE-2024", "", "HIGH"), ShouldBeTrue)
+
+				// Severity matches but excluded - should exclude
+				So(shouldIncludeCVE(baseCVE, "", "keyword", "HIGH"), ShouldBeFalse)
+
+				// Searched matches but severity doesn't - should exclude
+				So(shouldIncludeCVE(baseCVE, "CVE-2024", "", "LOW"), ShouldBeFalse)
+
+				// Everything matches but excluded - should exclude
+				So(shouldIncludeCVE(baseCVE, "CVE-2024", "Test", "HIGH"), ShouldBeFalse)
+			})
+		})
 	})
 }

pkg/extensions/search/resolver.go

@@ -1440,34 +1440,7 @@ func expandedRepoInfo(ctx context.Context, repo string, metaDB mTypes.MetaDB, cv
 		dateSortedImages = append(dateSortedImages, imgSummary)
 	}
 
-	//nolint:varnamelen // standard comparison func signature
-	slices.SortFunc(dateSortedImages, func(a, b *gql_generated.ImageSummary) int {
-		// Handle nil and zero time cases: both are treated as oldest (come last in descending sort)
-		aIsZero := a.LastUpdated == nil || (a.LastUpdated != nil && a.LastUpdated.IsZero())
-		bIsZero := b.LastUpdated == nil || (b.LastUpdated != nil && b.LastUpdated.IsZero())
-
-		if aIsZero && bIsZero {
-			return 0
-		}
-
-		if aIsZero {
-			return 1 // a is zero/nil, b is not - a comes after b
-		}
-
-		if bIsZero {
-			return -1 // b is zero/nil, a is not - a comes before b
-		}
-
-		if a.LastUpdated.After(*b.LastUpdated) {
-			return -1
-		}
-
-		if a.LastUpdated.Equal(*b.LastUpdated) {
-			return 0
-		}
-
-		return 1
-	})
+	slices.SortFunc(dateSortedImages, pagination.ImgSortByUpdateTime)
 
 	return &gql_generated.RepoInfo{Summary: repoSummary, Images: dateSortedImages}, nil
 }