Skip to content

Commit

Permalink
Prevent invalid empty Events from being added by AddPkgInfo() (#2298)
Browse files Browse the repository at this point in the history
While investigating some schema violations, I noticed that #2280 was
adding empty Event structs when there were multiple non-`introduced`
events for a repo.

Clean up some dead code that the static checker was complaining about.
  • Loading branch information
andrewpollock committed Jun 11, 2024
1 parent adae4f6 commit c50b373
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 27 deletions.
38 changes: 15 additions & 23 deletions vulnfeeds/vulns/vulns.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"cmp"
"encoding/json"
"errors"
"fmt"
"io"
"net/url"
"os"
Expand Down Expand Up @@ -224,41 +223,43 @@ func (v *Vulnerability) AddPkgInfo(pkgInfo PackageInfo) {
}
}

// Aggregate commits by their repo, and synthesize a zero introduced commit if necessary.
if len(pkgInfo.VersionInfo.AffectedCommits) > 0 {
gitCommitRangesByRepo := map[string]AffectedRange{}

hasAddedZeroIntroduced := make(map[string]bool)

for _, ac := range pkgInfo.VersionInfo.AffectedCommits {
entry, ok := gitCommitRangesByRepo[ac.Repo]
// Create the stub for the repo if necessary.
if !ok {
entry = AffectedRange{
Type: "GIT",
Events: []Event{},
Repo: ac.Repo,
}
}

if !pkgInfo.VersionInfo.HasIntroducedCommits(ac.Repo) && !hasAddedZeroIntroduced[ac.Repo] {
// There was no explicitly defined introduced commit, so create one at 0
entry.Events = append(entry.Events,
Event{
Introduced: "0",
},
)
hasAddedZeroIntroduced[ac.Repo] = true
if !pkgInfo.VersionInfo.HasIntroducedCommits(ac.Repo) && !hasAddedZeroIntroduced[ac.Repo] {
// There was no explicitly defined introduced commit, so create one at 0.
entry.Events = append(entry.Events,
Event{
Introduced: "0",
},
)
hasAddedZeroIntroduced[ac.Repo] = true
}
}

if pkgInfo.VersionInfo.HasIntroducedCommits(ac.Repo) {
if ac.Introduced != "" {
entry.Events = append(entry.Events, Event{Introduced: ac.Introduced})
}
if pkgInfo.VersionInfo.HasFixedCommits(ac.Repo) {
if ac.Fixed != "" {
entry.Events = append(entry.Events, Event{Fixed: ac.Fixed})
}
if pkgInfo.VersionInfo.HasLastAffectedCommits(ac.Repo) {
if ac.LastAffected != "" {
entry.Events = append(entry.Events, Event{LastAffected: ac.LastAffected})
}
if pkgInfo.VersionInfo.HasLimitCommits(ac.Repo) {
if ac.Limit != "" {
entry.Events = append(entry.Events, Event{Limit: ac.Limit})
}
gitCommitRangesByRepo[ac.Repo] = entry
Expand Down Expand Up @@ -599,18 +600,9 @@ func FromCVE(id cves.CVEID, cve cves.CVE) (*Vulnerability, []string) {
Details: cves.EnglishDescription(cve),
Aliases: extractAliases(id, cve),
}
var err error
var notes []string
v.Published = cve.Published.Format(time.RFC3339)
if err != nil {
notes = append(notes, fmt.Sprintf("Failed to parse published date: %v\n", err))
}

v.Modified = cve.LastModified.Format(time.RFC3339)
if err != nil {
notes = append(notes, fmt.Sprintf("Failed to parse modified date: %v\n", err))
}

v.References = ClassifyReferences(cve.References)
v.AddSeverity(cve.Metrics)
return &v, notes
Expand Down
18 changes: 14 additions & 4 deletions vulnfeeds/vulns/vulns_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,14 @@ func TestAddPkgInfo(t *testing.T) {
Fixed: "dsafwefwfe370a9e65d68d62ef37345597e4100b0e87021dfb",
Repo: "github.com/foo/bar",
},
{
Fixed: "658fe213",
Repo: "github.com/foo/bar",
},
{
LastAffected: "0xdeadf00d",
Repo: "github.com/foo/baz",
},
},
},
}
Expand All @@ -210,7 +218,7 @@ func TestAddPkgInfo(t *testing.T) {
AffectedVersions: []cves.AffectedVersion{
{
Introduced: "1.0.0-1",
Fixed: "1.2.3-4",
Fixed: "1.2.3-4",
},
},
},
Expand All @@ -220,7 +228,7 @@ func TestAddPkgInfo(t *testing.T) {
vuln.AddPkgInfo(testPkgInfoCommits) // This will end up in vuln.Affected[2]
vuln.AddPkgInfo(testPkgInfoHybrid) // This will end up in vuln.Affected[3]
vuln.AddPkgInfo(testPkgInfoCommitsMultiple) // This will end up in vuln.Affected[4]
vuln.AddPkgInfo(testPkgInfoEcoMultiple) // This will end up in vuln.Affected[5]
vuln.AddPkgInfo(testPkgInfoEcoMultiple) // This will end up in vuln.Affected[5]

t.Logf("Resulting vuln: %+v", vuln)

Expand Down Expand Up @@ -283,7 +291,7 @@ func TestAddPkgInfo(t *testing.T) {
// testPkgInfoCommits ^^^^^^^^^^^^^^^

// testPkgInfoCommitsMultiple vvvvvvvvvvvvv
if len(vuln.Affected[4].Ranges[0].Events) != 2 {
if len(vuln.Affected[4].Ranges[0].Events) != 3 {
t.Errorf("AddPkgInfo has not correctly added distinct range events from commits: %+v", vuln.Affected[4].Ranges)
}
// testPkgInfoCommitsMultiple ^^^^^^^^^^^^^
Expand All @@ -294,7 +302,6 @@ func TestAddPkgInfo(t *testing.T) {
}
// testPkgInfoEcoMultiple ^^^^^^^^^^^^^


for _, a := range vuln.Affected {
perRepoZeroIntroducedCommitHashCount := make(map[string]int)
for _, r := range a.Ranges {
Expand All @@ -307,6 +314,9 @@ func TestAddPkgInfo(t *testing.T) {
perRepoZeroIntroducedCommitHashCount[r.Repo]++
}
}
if e == (Event{}) {
t.Errorf("Empty event detected for the repo %s", r.Repo)
}
}
}
for repo, zeroIntroducedCommitHashCount := range perRepoZeroIntroducedCommitHashCount {
Expand Down

0 comments on commit c50b373

Please sign in to comment.