refactor(vulnfeeds): git commit resolution code for better reusability (#4052)

Part of splitting logic from #3951

Author: jess-lowe

vulnfeeds/cves/versions.go

@@ -515,44 +515,51 @@ func ValidateAndCanonicalizeLink(link string, httpClient *http.Client) (canonica
 }
 
 // For URLs referencing commits in supported Git repository hosts, return a cloneable AffectedCommit.
-func ExtractGitCommit(link string, commitType models.CommitType, httpClient *http.Client) (ac models.AffectedCommit, err error) {
-	r, err := Repo(link)
+func extractGitAffectedCommit(link string, commitType models.CommitType, httpClient *http.Client) (models.AffectedCommit, error) {
+	var ac models.AffectedCommit
+	c, r, err := ExtractGitCommit(link, httpClient, 0)
 	if err != nil {
 		return ac, err
 	}
 
+	ac.SetRepo(r)
+
+	models.SetCommitByType(&ac, commitType, c)
+
+	return ac, nil
+}
+
+func ExtractGitCommit(link string, httpClient *http.Client, depth int) (string, string, error) {
+	if depth > 10 {
+		return "", "", fmt.Errorf("max recursion depth exceeded for %s", link)
+	}
+	var commit string
+	r, err := Repo(link)
+	if err != nil {
+		return "", "", err
+	}
+
 	c, err := Commit(link)
 	if err != nil {
-		return ac, err
+		return "", "", err
 	}
 
+	commit = c
+
 	// If URL doesn't validate, treat it as linkrot.
 	possiblyDifferentLink, err := ValidateAndCanonicalizeLink(link, httpClient)
 	if err != nil {
-		return ac, err
+		return "", "", err
 	}
 
 	// restart the entire extraction process when the URL changes (i.e. handle a
 	// redirect to a completely different host, instead of a redirect within
 	// GitHub)
 	if possiblyDifferentLink != link {
-		return ExtractGitCommit(possiblyDifferentLink, commitType, httpClient)
+		return ExtractGitCommit(possiblyDifferentLink, httpClient, depth+1)
 	}
 
-	ac.SetRepo(r)
-
-	switch commitType {
-	case models.Introduced:
-		ac.SetIntroduced(c)
-	case models.LastAffected:
-		ac.SetLastAffected(c)
-	case models.Limit:
-		ac.SetLimit(c)
-	case models.Fixed:
-		ac.SetFixed(c)
-	}
-
-	return ac, nil
+	return commit, r, nil
 }
 
 func HasVersion(validVersions []string, version string) bool {
@@ -676,7 +683,7 @@ func deduplicateAffectedCommits(commits []models.AffectedCommit) []models.Affect
 func ExtractVersionInfo(cve CVE, validVersions []string, httpClient *http.Client) (v models.VersionInfo, notes []string) {
 	for _, reference := range cve.References {
 		// (Potentially faulty) Assumption: All viable Git commit reference links are fix commits.
-		if commit, err := ExtractGitCommit(reference.URL, models.Fixed, httpClient); err == nil {
+		if commit, err := extractGitAffectedCommit(reference.URL, models.Fixed, httpClient); err == nil {
 			v.AffectedCommits = append(v.AffectedCommits, commit)
 		}
 	}

vulnfeeds/cves/versions_test.go

@@ -686,20 +686,20 @@ func TestExtractGitCommit(t *testing.T) {
 				t.Skipf("test %q: running on Cloud Build", tc.description)
 			}
 			if time.Now().Before(tc.disableExpiryDate) {
-				t.Skipf("test %q: extractGitCommit for %q (%q) has been skipped due to known outage and will be reenabled on %s.", tc.description, tc.inputLink, tc.inputCommitType, tc.disableExpiryDate)
+				t.Skipf("test %q: extractGitAffectedCommit for %q (%q) has been skipped due to known outage and will be reenabled on %s.", tc.description, tc.inputLink, tc.inputCommitType, tc.disableExpiryDate)
 			}
 			if !tc.disableExpiryDate.IsZero() && time.Now().After(tc.disableExpiryDate) {
-				t.Logf("test %q: extractGitCommit(%q, %q) has been enabled on %s.", tc.description, tc.inputLink, tc.inputCommitType, tc.disableExpiryDate)
+				t.Logf("test %q: extractGitAffectedCommit(%q, %q) has been enabled on %s.", tc.description, tc.inputLink, tc.inputCommitType, tc.disableExpiryDate)
 			}
-			got, err := ExtractGitCommit(tc.inputLink, tc.inputCommitType, client)
+			got, err := extractGitAffectedCommit(tc.inputLink, tc.inputCommitType, client)
 			if err != nil && !tc.expectFailure {
-				t.Errorf("test %q: extractGitCommit for %q (%q) errored unexpectedly: %#v", tc.description, tc.inputLink, tc.inputCommitType, err)
+				t.Errorf("test %q: extractGitAffectedCommit for %q (%q) errored unexpectedly: %#v", tc.description, tc.inputLink, tc.inputCommitType, err)
 			}
 			if err == nil && tc.expectFailure {
-				t.Errorf("test %q: extractGitCommit for %q (%q) did not error as unexpected!", tc.description, tc.inputLink, tc.inputCommitType)
+				t.Errorf("test %q: extractGitAffectedCommit for %q (%q) did not error as unexpected!", tc.description, tc.inputLink, tc.inputCommitType)
 			}
 			if !reflect.DeepEqual(got, tc.expectedAffectedCommit) {
-				t.Errorf("test %q: extractGitCommit for %q was incorrect, got: %#v, expected: %#v", tc.description, tc.inputLink, got, tc.expectedAffectedCommit)
+				t.Errorf("test %q: extractGitAffectedCommit for %q was incorrect, got: %#v, expected: %#v", tc.description, tc.inputLink, got, tc.expectedAffectedCommit)
 			}
 		})
 	}

vulnfeeds/git/versions.go

@@ -25,20 +25,6 @@ import (
 
 var versionRangeRegex = regexp.MustCompile(`^(>=|<=|~|\^|>|<|=)\s*([0-9a-zA-Z\.\-]+)(?:,\s*(>=|<=|~|\^|>|<|=)\s*([0-9a-zA-Z\.\-]+))?$`) // Used to parse version strings from the GitHub CNA.
 
-// setCommitByType sets the appropriate commit field on an AffectedCommit based on the CommitType.
-func setCommitByType(ac *models.AffectedCommit, commitType models.CommitType, commitHash string) {
-	switch commitType {
-	case models.Introduced:
-		ac.SetIntroduced(commitHash)
-	case models.LastAffected:
-		ac.SetLastAffected(commitHash)
-	case models.Limit:
-		ac.SetLimit(commitHash)
-	case models.Fixed:
-		ac.SetFixed(commitHash)
-	}
-}
-
 // findFuzzyCommit takes an already normalized version and the mapping of repo tags to
 // normalized tags and commits, and performs fuzzy matching to find a commit hash.
 func findFuzzyCommit(normalizedVersion string, normalizedTags map[string]NormalizedTag) (string, bool) {
@@ -87,7 +73,7 @@ func VersionToAffectedCommit(version string, repo string, commitType models.Comm
 		return ac, err
 	}
 	ac.SetRepo(repo)
-	setCommitByType(&ac, commitType, commitHash)
+	models.SetCommitByType(&ac, commitType, commitHash)
 
 	return ac, nil
 }

vulnfeeds/models/types.go

@@ -15,6 +15,20 @@ type AffectedCommit struct {
 	LastAffected string `json:"last_affected,omitempty" yaml:"last_affected,omitempty"`
 }
 
+// SetCommitByType sets the appropriate commit field on an AffectedCommit based on the CommitType.
+func SetCommitByType(ac *AffectedCommit, commitType CommitType, commitHash string) {
+	switch commitType {
+	case Introduced:
+		ac.SetIntroduced(commitHash)
+	case LastAffected:
+		ac.SetLastAffected(commitHash)
+	case Limit:
+		ac.SetLimit(commitHash)
+	case Fixed:
+		ac.SetFixed(commitHash)
+	}
+}
+
 func (ac *AffectedCommit) SetRepo(repo string) {
 	// GitHub.com repos are demonstrably case-insensitive, and frequently
 	// expressed in URLs with varying cases, so normalize them to lowercase.