cmd/vulnreport: fix bug in duplicate-finding for triage

Fix a bug in which the "likely duplicate" label was applied
to all issues that have duplicates on the tracker. (For example,
if #1 and #2 both refer to GHSA-xxxx-yyyy-zzzz, only one of
these should be marked as a duplicate).

This also revealed some bugs in the fake in-memory implementation
of the GHSA API, which are now fixed.

Change-Id: Ifd98befdf3e23f1fc95df38533107de9c921b195
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/599456
Reviewed-by: Damien Neil <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

cmd/vulnreport/find_aliases.go

@@ -5,6 +5,7 @@
 package main
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 
@@ -172,19 +173,31 @@ type ghsaClient interface {
 
 type memGC struct {
 	ghsas map[string]ghsa.SecurityAdvisory
+	byCVE map[string][]*ghsa.SecurityAdvisory
 }
 
 func newMemGC(archive []byte) (*memGC, error) {
 	ar := txtar.Parse(archive)
 	m := &memGC{
 		ghsas: make(map[string]ghsa.SecurityAdvisory),
+		byCVE: make(map[string][]*ghsa.SecurityAdvisory),
 	}
 	for _, f := range ar.Files {
 		var g ghsa.SecurityAdvisory
-		if err := yaml.Unmarshal(f.Data, &g); err != nil {
+		d := yaml.NewDecoder(bytes.NewBuffer(f.Data))
+		d.KnownFields(true)
+		if err := d.Decode(&g); err != nil {
 			return nil, err
 		}
+		if f.Name != g.ID {
+			return nil, fmt.Errorf("filename (%s) != ID (%s)", f.Name, g.ID)
+		}
 		m.ghsas[f.Name] = g
+		for _, id := range g.Identifiers {
+			if id.Type == "CVE" {
+				m.byCVE[id.Value] = append(m.byCVE[id.Value], &g)
+			}
+		}
 	}
 	return m, nil
 }
@@ -197,12 +210,5 @@ func (m *memGC) FetchGHSA(_ context.Context, id string) (*ghsa.SecurityAdvisory,
 }
 
 func (m *memGC) ListForCVE(_ context.Context, cid string) (result []*ghsa.SecurityAdvisory, _ error) {
-	for _, sa := range m.ghsas {
-		for _, id := range sa.Identifiers {
-			if id.Type == "CVE" || id.Value == cid {
-				result = append(result, &sa)
-			}
-		}
-	}
-	return result, nil
+	return m.byCVE[cid], nil
 }

cmd/vulnreport/lint.go

@@ -55,10 +55,10 @@ func (l *linter) lint(r *yamlReport) error {
 }
 
 func (l *linter) canonicalModule(mp string) string {
-	if module, err := l.pxc.FindModule(mp); err == nil { // no error
+	if module, err := l.pxc.FindModule(mp); module != "" && err == nil { // no error
 		mp = module
 	}
-	if module, err := l.pxc.CanonicalAtLatest(mp); err == nil { // no error
+	if module, err := l.pxc.CanonicalAtLatest(mp); module != "" && err == nil { // no error
 		mp = module
 	}
 	return mp

cmd/vulnreport/run_test.go

@@ -12,11 +12,13 @@ import (
 	"fmt"
 	"io/fs"
 	"path/filepath"
+	"slices"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"golang.org/x/exp/maps"
 	"golang.org/x/tools/txtar"
 	"golang.org/x/vulndb/cmd/vulnreport/log"
 	"golang.org/x/vulndb/cmd/vulnreport/priority"
@@ -206,8 +208,10 @@ func writeGolden(t *testing.T, g *golden, comment string, written map[string][]b
 		{Name: "out", Data: g.out},
 		{Name: "logs", Data: g.logs},
 	}
-	for fname, b := range written {
-		files = append(files, txtar.File{Name: fname, Data: b})
+	sortedFilenames := maps.Keys(written)
+	slices.Sort(sortedFilenames)
+	for _, fname := range sortedFilenames {
+		files = append(files, txtar.File{Name: fname, Data: written[fname]})
 	}
 
 	return test.WriteTxtar(testFilename(t), files, comment)

cmd/vulnreport/testdata/TestFix/no_change.txtar

@@ -17,16 +17,6 @@ info: GO-9999-0001: skipping symbol checks for package golang.org/x/vulndb/cmd/v
 info: GO-9999-0001: checking for missing GHSAs and CVEs (use -skip-alias to skip this)
 info: GO-9999-0001: checking that all references are reachable
 info: fix: processed 1 report(s) (success=1; skip=0; error=0)
--- data/reports/GO-9999-0001.yaml --
-id: GO-9999-0001
-modules:
-    - module: golang.org/x/vulndb
-      vulnerable_at: 0.0.0-20240716161253-dd7900b89e20
-      packages:
-        - package: golang.org/x/vulndb/cmd/vulnreport
-summary: A problem with golang.org/x/vulndb
-description: A description of the issue
-review_status: REVIEWED
 -- data/osv/GO-9999-0001.json --
 {
   "schema_version": "1.3.1",
@@ -65,3 +55,13 @@ review_status: REVIEWED
     "review_status": "REVIEWED"
   }
 }
+-- data/reports/GO-9999-0001.yaml --
+id: GO-9999-0001
+modules:
+    - module: golang.org/x/vulndb
+      vulnerable_at: 0.0.0-20240716161253-dd7900b89e20
+      packages:
+        - package: golang.org/x/vulndb/cmd/vulnreport
+summary: A problem with golang.org/x/vulndb
+description: A description of the issue
+review_status: REVIEWED

cmd/vulnreport/testdata/TestTriage/all.txtar

@@ -12,17 +12,22 @@ issue test-issue-tracker/10 is high priority
   - golang.org/x/vuln has 101 importers (>= 100) and as many reviewed (0) as likely-binary reports (0)
 issue test-issue-tracker/11 is possibly not Go
   - more than 20 percent of reports (1 of 1) with this module are NOT_GO_CODE
-triaged 3 issues:
+issue test-issue-tracker/12 is likely duplicate
+  - #12 shares alias(es) CVE-1999-0002, GHSA-xxxx-yyyy-0002, GHSA-xxxx-yyyy-0003 with test-issue-tracker/13
+  - #12 shares alias(es) CVE-1999-0002, GHSA-xxxx-yyyy-0002, GHSA-xxxx-yyyy-0003 with test-issue-tracker/14
+issue test-issue-tracker/13 is likely duplicate
+  - #13 shares alias(es) CVE-1999-0002, GHSA-xxxx-yyyy-0002, GHSA-xxxx-yyyy-0003 with test-issue-tracker/14
+triaged 6 issues:
   - 1 high priority
-  - 2 low priority
+  - 5 low priority
   - 0 unknown priority
-  - 1 likely duplicate
+  - 3 likely duplicate
   - 1 possibly not Go
 helpful commands:
   $ vulnreport create 10
 -- logs --
 info: creating alias map for open issues
-info: triage: operating on 4 issue(s)
+info: triage: operating on 7 issue(s)
 info: triage: skipping issue #1 (already has report)
 info: triage 7
 info: issue test-issue-tracker/7 is low priority
@@ -31,4 +36,13 @@ info: triage 10
 info: triage 11
 info: issue test-issue-tracker/11 is low priority
   - collectd.org has 0 importers (< 100)
-info: triage: processed 4 issue(s) (success=3; skip=1; error=0)
+info: triage 12
+info: issue test-issue-tracker/12 is low priority
+  - golang.org/x/tools has 50 importers (< 100)
+info: triage 13
+info: issue test-issue-tracker/13 is low priority
+  - golang.org/x/tools has 50 importers (< 100)
+info: triage 14
+info: issue test-issue-tracker/14 is low priority
+  - golang.org/x/tools has 50 importers (< 100)
+info: triage: processed 7 issue(s) (success=6; skip=1; error=0)

cmd/vulnreport/testdata/issue_tracker.txtar

@@ -26,3 +26,18 @@ number: 11
 title: "x/vulndb: potential Go vuln in collectd.org: CVE-2021-0000"
 state: open
 
+-- 12 --
+number: 12
+title: "x/vulndb: potential Go vuln in golang.org/x/tools: GHSA-xxxx-yyyy-0002"
+state: open
+
+-- 13 --
+number: 13
+title: "x/vulndb: potential Go vuln in golang.org/x/tools: CVE-1999-0002"
+state: open
+
+-- 14 --
+number: 14
+title: "x/vulndb: potential Go vuln in golang.org/x/tools: GHSA-xxxx-yyyy-0003"
+state: open
+

cmd/vulnreport/testdata/legacy_ghsas.txtar

@@ -7,9 +7,20 @@
 -- GHSA-xxxx-yyyy-zzzz --
 id: GHSA-xxxx-yyyy-zzzz
 identifiers:
-    - identifier:
-        - type: CVE
-          id: CVE-1999-0005
+    - type: CVE
+      value: CVE-1999-0005
 
 -- GHSA-xxxx-yyyy-0001 --
 id: GHSA-xxxx-yyyy-0001
+
+-- GHSA-xxxx-yyyy-0002 --
+id: GHSA-xxxx-yyyy-0002
+identifiers:
+    - type: CVE
+      value: CVE-1999-0002
+
+-- GHSA-xxxx-yyyy-0003 --
+id: GHSA-xxxx-yyyy-0003
+identifiers:
+    - type: CVE
+      value: CVE-1999-0002

cmd/vulnreport/triage.go

@@ -27,6 +27,9 @@ type triage struct {
 	mu              sync.Mutex // protects aliasesToIssues and stats
 	aliasesToIssues map[string][]int
 	stats           []issuesList
+
+	// issues that have already been marked as duplicate
+	duplicates map[int]bool
 }
 
 func (*triage) name() string { return "triage" }
@@ -65,6 +68,7 @@ func (t *triage) setup(ctx context.Context, env environment) error {
 	}
 
 	log.Info("creating alias map for open issues")
+	t.duplicates = make(map[int]bool)
 	open, err := t.openIssues(ctx)
 	if err != nil {
 		return err
@@ -74,6 +78,9 @@ func (t *triage) setup(ctx context.Context, env environment) error {
 		for _, a := range aliases {
 			t.addAlias(a, iss.Number)
 		}
+		if iss.HasLabel(labelPossibleDuplicate) || iss.HasLabel(labelDuplicate) {
+			t.duplicates[iss.Number] = true
+		}
 	}
 	return nil
 }
@@ -127,6 +134,7 @@ func (t *triage) triage(ctx context.Context, iss *issues.Issue) {
 				strings.Join(aliases, ", "),
 				filepath.ToSlash(ref)))
 		}
+		slices.Sort(strs)
 		t.addStat(iss, statDuplicate, strings.Join(strs, listItem))
 		labels = append(labels, labelPossibleDuplicate)
 	}
@@ -191,8 +199,14 @@ func (t *triage) findDuplicates(ctx context.Context, iss *issues.Issue) map[stri
 			if iss.Number == dup {
 				continue
 			}
+			// If the other issue is already marked as a duplicate,
+			// we don't need to mark this one.
+			if t.duplicates[dup] {
+				continue
+			}
 			ref := t.ic.Reference(dup)
 			xrefs[ref] = append(xrefs[ref], a)
+			t.duplicates[iss.Number] = true
 		}
 	}