refactor(vulnfeeds): move GitVersionsToCommits to common dir and refactor to output resolved ranges (#4815)

Move more common functions and remove useless ones.

GitVersionsToCommits now outputs resolved ranges, unresolved ranges and
successful repositories. This function will be more heavily used in the
next PR for the NVD refactor.

It should output resolved/unresolved ranges instead of affected ranges
so that we are able to group the ranges by repository into one set of
ranges/events later on, so as not to have duplicate repository/package
information in the record.

Renamed another function also called GitVersionsToCommits to
VersionInfoToCommits to better reflect what it's actually doing and as
to reduce the confusion around the same name.

Author: jess-lowe

vulnfeeds/conversion/common.go

@@ -14,7 +14,9 @@ import (
 	"strings"
 	"time"
 
+	"github.com/google/osv/vulnfeeds/git"
 	"github.com/google/osv/vulnfeeds/models"
+	"github.com/google/osv/vulnfeeds/utility"
 	"github.com/google/osv/vulnfeeds/utility/logger"
 	"github.com/google/osv/vulnfeeds/vulns"
 	"github.com/ossf/osv-schema/bindings/go/osvschema"
@@ -169,6 +171,90 @@ func WriteMetricsFile(metrics *models.ConversionMetrics, metricsFile *os.File) e
 	return nil
 }
 
+// Examines repos and tries to convert versions to commits by treating them as Git tags.
+func GitVersionsToCommits(versionRanges []*osvschema.Range, repos []string, metrics *models.ConversionMetrics, cache *git.RepoTagsCache) ([]*osvschema.Range, []*osvschema.Range, []string) {
+	var newVersionRanges []*osvschema.Range
+	unresolvedRanges := versionRanges
+	var successfulRepos []string
+
+	for _, repo := range repos {
+		if len(unresolvedRanges) == 0 {
+			break // All ranges have been resolved.
+		}
+
+		normalizedTags, err := git.NormalizeRepoTags(repo, cache)
+		if err != nil {
+			metrics.AddNote("Failed to normalize tags - %s", repo)
+			continue
+		}
+
+		var stillUnresolvedRanges []*osvschema.Range
+		for _, vr := range unresolvedRanges {
+			var introduced, fixed, lastAffected string
+			for _, e := range vr.GetEvents() {
+				if e.GetIntroduced() != "" {
+					introduced = e.GetIntroduced()
+				}
+				if e.GetFixed() != "" {
+					fixed = e.GetFixed()
+				}
+				if e.GetLastAffected() != "" {
+					lastAffected = e.GetLastAffected()
+				}
+			}
+
+			var introducedCommit string
+			if introduced == "0" {
+				introducedCommit = "0"
+			} else {
+				introducedCommit = resolveVersionToCommit(introduced, normalizedTags)
+			}
+			fixedCommit := resolveVersionToCommit(fixed, normalizedTags)
+			lastAffectedCommit := resolveVersionToCommit(lastAffected, normalizedTags)
+
+			if introducedCommit != "" && (fixedCommit != "" || lastAffectedCommit != "") {
+				var newVR *osvschema.Range
+
+				if fixedCommit != "" {
+					newVR = BuildVersionRange(introducedCommit, "", fixedCommit)
+				} else {
+					newVR = BuildVersionRange(introducedCommit, lastAffectedCommit, "")
+				}
+				successfulRepos = append(successfulRepos, repo)
+				newVR.Repo = repo
+				newVR.Type = osvschema.Range_GIT
+				if len(vr.GetEvents()) > 0 {
+					databaseSpecific, err := utility.NewStructpbFromMap(map[string]any{"versions": vr.GetEvents()})
+					if err != nil {
+						metrics.AddNote("failed to make database specific: %v", err)
+					} else {
+						newVR.DatabaseSpecific = databaseSpecific
+					}
+				}
+
+				newVersionRanges = append(newVersionRanges, newVR)
+			} else {
+				stillUnresolvedRanges = append(stillUnresolvedRanges, vr)
+			}
+		}
+		unresolvedRanges = stillUnresolvedRanges
+	}
+
+	if len(newVersionRanges) > 0 {
+		metrics.ResolvedRangesCount += len(newVersionRanges)
+		metrics.Outcome = models.Successful
+	}
+
+	if len(unresolvedRanges) > 0 {
+		metrics.UnresolvedRangesCount += len(unresolvedRanges)
+		if len(newVersionRanges) == 0 {
+			metrics.Outcome = models.NoCommitRanges
+		}
+	}
+
+	return newVersionRanges, unresolvedRanges, successfulRepos
+}
+
 // BuildVersionRange is a helper function that adds 'introduced', 'fixed', or 'last_affected'
 // events to an OSV version range. If 'intro' is empty, it defaults to "0".
 func BuildVersionRange(intro string, lastAff string, fixed string) *osvschema.Range {
@@ -193,3 +279,17 @@ func BuildVersionRange(intro string, lastAff string, fixed string) *osvschema.Ra
 
 	return &versionRange
 }
+
+// resolveVersionToCommit is a helper to convert a version string to a commit hash.
+// It logs the outcome of the conversion attempt and returns an empty string on failure.
+func resolveVersionToCommit(version string, normalizedTags map[string]git.NormalizedTag) string {
+	if version == "" {
+		return ""
+	}
+	commit, err := git.VersionToCommit(version, normalizedTags)
+	if err != nil {
+		return ""
+	}
+
+	return commit
+}

vulnfeeds/conversion/nvd/converter.go

@@ -50,7 +50,7 @@ func CVEToOSV(cve models.NVDCVE, repos []string, cache *git.RepoTagsCache, direc
 			return fmt.Errorf("no affected ranges for %q, and no repos to try and convert %+v to tags with", maybeProductName, versions.AffectedVersions)
 		}
 		metrics.AddNote("Trying to convert version tags to commits: %v with repos: %v", versions, repos)
-		versions = cves.GitVersionsToCommits(versions, repos, cache, metrics)
+		versions = cves.VersionInfoToCommits(versions, repos, cache, metrics)
 		hasAnyFixedCommits := false
 		for _, ac := range versions.AffectedCommits {
 			if ac.Fixed != "" {
@@ -140,7 +140,7 @@ func CVEToPackageInfo(cve models.NVDCVE, repos []string, cache *git.RepoTagsCach
 			return fmt.Errorf("no affected ranges for %q, and no repos to try and convert %+v to tags with", maybeProductName, versions.AffectedVersions)
 		}
 		logger.Info("Trying to convert version tags to commits", slog.String("cve", string(cve.ID)), slog.Any("versions", versions), slog.Any("repos", repos))
-		versions = cves.GitVersionsToCommits(versions, repos, cache, metrics)
+		versions = cves.VersionInfoToCommits(versions, repos, cache, metrics)
 	}
 
 	hasAnyFixedCommits := false

vulnfeeds/cvelist2osv/common.go

@@ -3,15 +3,11 @@ package cvelist2osv
 import (
 	"cmp"
 	"errors"
-	"log/slog"
 	"strconv"
 	"strings"
 
 	"github.com/google/osv/vulnfeeds/conversion"
-	"github.com/google/osv/vulnfeeds/git"
 	"github.com/google/osv/vulnfeeds/models"
-	"github.com/google/osv/vulnfeeds/utility"
-	"github.com/google/osv/vulnfeeds/utility/logger"
 	"github.com/google/osv/vulnfeeds/vulns"
 	"github.com/ossf/osv-schema/bindings/go/osvschema"
 )
@@ -55,117 +51,6 @@ func toVersionRangeType(s string) VersionRangeType {
 	}
 }
 
-// resolveVersionToCommit is a helper to convert a version string to a commit hash.
-// It logs the outcome of the conversion attempt and returns an empty string on failure.
-func resolveVersionToCommit(cveID models.CVEID, version, versionType, repo string, normalizedTags map[string]git.NormalizedTag) string {
-	if version == "" {
-		return ""
-	}
-	logger.Info("Attempting to resolve version to commit", slog.String("cve", string(cveID)), slog.String("version", version), slog.String("type", versionType), slog.String("repo", repo))
-	commit, err := git.VersionToCommit(version, normalizedTags)
-	if err != nil {
-		logger.Warn("Failed to get Git commit for version", slog.String("cve", string(cveID)), slog.String("version", version), slog.String("type", versionType), slog.String("repo", repo), slog.Any("err", err))
-		return ""
-	}
-	logger.Info("Successfully derived commit for version", slog.String("cve", string(cveID)), slog.String("commit", commit), slog.String("version", version), slog.String("type", versionType))
-
-	return commit
-}
-
-// Examines repos and tries to convert versions to commits by treating them as Git tags.
-// Takes a CVE ID string (for logging), VersionInfo with AffectedVersions and
-// typically no AffectedCommits and attempts to add AffectedCommits (including Fixed commits) where there aren't any.
-// Refuses to add the same commit to AffectedCommits more than once.
-func gitVersionsToCommits(cveID models.CVEID, versionRanges []*osvschema.Range, repos []string, metrics *models.ConversionMetrics, cache *git.RepoTagsCache) (*osvschema.Affected, error) {
-	var newAff osvschema.Affected
-	var newVersionRanges []*osvschema.Range
-	unresolvedRanges := versionRanges
-
-	for _, repo := range repos {
-		if len(unresolvedRanges) == 0 {
-			break // All ranges have been resolved.
-		}
-
-		normalizedTags, err := git.NormalizeRepoTags(repo, cache)
-		if err != nil {
-			metrics.AddNote("Failed to normalize tags - %s", repo)
-			continue
-		}
-
-		var stillUnresolvedRanges []*osvschema.Range
-		for _, vr := range unresolvedRanges {
-			var introduced, fixed, lastAffected string
-			for _, e := range vr.GetEvents() {
-				if e.GetIntroduced() != "" {
-					introduced = e.GetIntroduced()
-				}
-				if e.GetFixed() != "" {
-					fixed = e.GetFixed()
-				}
-				if e.GetLastAffected() != "" {
-					lastAffected = e.GetLastAffected()
-				}
-			}
-
-			var introducedCommit string
-			if introduced == "0" {
-				introducedCommit = "0"
-			} else {
-				introducedCommit = resolveVersionToCommit(cveID, introduced, "introduced", repo, normalizedTags)
-			}
-			fixedCommit := resolveVersionToCommit(cveID, fixed, "fixed", repo, normalizedTags)
-			lastAffectedCommit := resolveVersionToCommit(cveID, lastAffected, "last_affected", repo, normalizedTags)
-
-			if introducedCommit != "" && (fixedCommit != "" || lastAffectedCommit != "") {
-				var newVR *osvschema.Range
-
-				if fixedCommit != "" {
-					newVR = conversion.BuildVersionRange(introducedCommit, "", fixedCommit)
-				} else {
-					newVR = conversion.BuildVersionRange(introducedCommit, lastAffectedCommit, "")
-				}
-
-				newVR.Repo = repo
-				newVR.Type = osvschema.Range_GIT
-				if len(vr.GetEvents()) > 0 {
-					databaseSpecific, err := utility.NewStructpbFromMap(map[string]any{"versions": vr.GetEvents()})
-					if err != nil {
-						logger.Warn("failed to make database specific: %v", err)
-					} else {
-						newVR.DatabaseSpecific = databaseSpecific
-					}
-				}
-
-				newVersionRanges = append(newVersionRanges, newVR)
-			} else {
-				stillUnresolvedRanges = append(stillUnresolvedRanges, vr)
-			}
-		}
-		unresolvedRanges = stillUnresolvedRanges
-	}
-
-	var err error
-	if len(unresolvedRanges) > 0 {
-		databaseSpecific, err := utility.NewStructpbFromMap(map[string]any{"unresolved_ranges": unresolvedRanges})
-		if err != nil {
-			logger.Warn("failed to make database specific: %v", err)
-		} else {
-			newAff.DatabaseSpecific = databaseSpecific
-		}
-
-		metrics.UnresolvedRangesCount += len(unresolvedRanges)
-	}
-
-	if len(newVersionRanges) > 0 {
-		newAff.Ranges = newVersionRanges
-		metrics.ResolvedRangesCount += len(newVersionRanges)
-	} else if len(unresolvedRanges) > 0 { // Only error if there were ranges to resolve but none were.
-		err = errors.New("was not able to get git version ranges")
-	}
-
-	return &newAff, err
-}
-
 // findCPEVersionRanges extracts version ranges and CPE strings from the CNA's
 // CPE applicability statements in a CVE record.
 func findCPEVersionRanges(cve models.CVE5) (versionRanges []*osvschema.Range, cpes []string, err error) {

vulnfeeds/cvelist2osv/default_extractor.go

@@ -1,12 +1,11 @@
 package cvelist2osv
 
 import (
-	"log/slog"
-
 	"github.com/google/osv/vulnfeeds/conversion"
 	"github.com/google/osv/vulnfeeds/cves"
 	"github.com/google/osv/vulnfeeds/git"
 	"github.com/google/osv/vulnfeeds/models"
+	"github.com/google/osv/vulnfeeds/utility"
 	"github.com/google/osv/vulnfeeds/utility/logger"
 	"github.com/google/osv/vulnfeeds/vulns"
 	"github.com/ossf/osv-schema/bindings/go/osvschema"
@@ -39,40 +38,40 @@ func (d *DefaultVersionExtractor) ExtractVersions(cve models.CVE5, v *vulns.Vuln
 	ranges := d.handleAffected(cve.Containers.CNA.Affected, metrics)
 
 	if len(ranges) != 0 {
-		aff, err := gitVersionsToCommits(cve.Metadata.CVEID, ranges, repos, metrics, repoTagsCache)
-		if err != nil {
-			logger.Error("Failed to convert git versions to commits", slog.Any("err", err))
+		resolvedRanges, unresolvedRanges, _ := conversion.GitVersionsToCommits(ranges, repos, metrics, repoTagsCache)
+		if len(resolvedRanges) == 0 {
+			metrics.AddNote("Failed to convert git versions to commits")
 		} else {
 			gotVersions = true
 		}
-		conversion.AddAffected(v, aff, metrics)
+		addRangesToAffected(resolvedRanges, unresolvedRanges, v, metrics)
 	}
 
 	if !gotVersions {
 		metrics.AddNote("No versions in affected, attempting to extract from CPE")
 		versionRanges, _ := cpeVersionExtraction(cve, metrics)
 
 		if len(versionRanges) != 0 {
-			aff, err := gitVersionsToCommits(cve.Metadata.CVEID, versionRanges, repos, metrics, repoTagsCache)
-			if err != nil {
-				logger.Error("Failed to convert git versions to commits", slog.Any("err", err))
+			resolvedRanges, unresolvedRanges, _ := conversion.GitVersionsToCommits(versionRanges, repos, metrics, repoTagsCache)
+			if len(resolvedRanges) == 0 {
+				metrics.AddNote("Failed to convert git versions to commits")
 			} else {
 				gotVersions = true
 			}
-
-			conversion.AddAffected(v, aff, metrics)
+			addRangesToAffected(resolvedRanges, unresolvedRanges, v, metrics)
 		}
 	}
 
 	if !gotVersions {
 		metrics.AddNote("No versions in CPEs so attempting extraction from description")
 		versionRanges := textVersionExtraction(cve, metrics)
 		if len(versionRanges) != 0 {
-			aff, err := gitVersionsToCommits(cve.Metadata.CVEID, versionRanges, repos, metrics, repoTagsCache)
-			if err != nil {
-				logger.Error("Failed to convert git versions to commits", slog.Any("err", err))
+			resolvedRanges, unresolvedRanges, _ := conversion.GitVersionsToCommits(versionRanges, repos, metrics, repoTagsCache)
+			if len(resolvedRanges) == 0 {
+				metrics.AddNote("Failed to convert git versions to commits")
 			}
-			conversion.AddAffected(v, aff, metrics)
+
+			addRangesToAffected(resolvedRanges, unresolvedRanges, v, metrics)
 		}
 	}
 }
@@ -135,3 +134,20 @@ func (d *DefaultVersionExtractor) FindNormalAffectedRanges(affected models.Affec
 
 	return versionRanges, mostFrequentVersionType
 }
+
+func addRangesToAffected(resolvedRanges []*osvschema.Range, unresolvedRanges []*osvschema.Range, v *vulns.Vulnerability, metrics *models.ConversionMetrics) {
+	if len(resolvedRanges) > 0 {
+		aff := &osvschema.Affected{
+			Ranges: resolvedRanges,
+		}
+		if len(unresolvedRanges) > 0 {
+			databaseSpecific, err := utility.NewStructpbFromMap(map[string]any{"unresolved_ranges": unresolvedRanges})
+			if err != nil {
+				logger.Warn("failed to make database specific: %v", err)
+			} else {
+				aff.DatabaseSpecific = databaseSpecific
+			}
+		}
+		conversion.AddAffected(v, aff, metrics)
+	}
+}

vulnfeeds/cves/versions.go

@@ -1001,7 +1001,7 @@ func (c *VPRepoCache) Initialize(vpMap VendorProductToRepoMap) {
 // Takes a CVE ID string (for logging), VersionInfo with AffectedVersions and
 // typically no AffectedCommits and attempts to add AffectedCommits (including Fixed commits) where there aren't any.
 // Refuses to add the same commit to AffectedCommits more than once.
-func GitVersionsToCommits(versions models.VersionInfo, repos []string, cache *git.RepoTagsCache, metrics *models.ConversionMetrics) (v models.VersionInfo) {
+func VersionInfoToCommits(versions models.VersionInfo, repos []string, cache *git.RepoTagsCache, metrics *models.ConversionMetrics) (v models.VersionInfo) {
 	// versions is a VersionInfo with AffectedVersions and typically no AffectedCommits
 	// v is a VersionInfo with AffectedCommits (containing Fixed commits) included
 	v = versions