internal/{report,worker}: update display of xrefs

Unify the display of xrefs in the worker and in vulnreport xref.

Call out duplicate aliases more prominently, as they indicate a problem,
whereas module xrefs are informational.

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

Author: tatianab

cmd/vulnreport/testdata/TestXref/found_xrefs.txtar

@@ -6,9 +6,11 @@ Expected output of test TestXref/found_xrefs
 command: "vulnreport xref 4"
 
 -- out --
-data/reports/GO-9999-0004.yaml: found xrefs:
-Module golang.org/x/tools appears in data/reports/GO-9999-0005.yaml
-GO-9999-0004 priority is low
+GO-9999-0004: found module xrefs
+- golang.org/x/tools appears in 1 other report(s):
+  - data/reports/GO-9999-0005.yaml    (https://github.com/golang/vulndb#5)
+
+GO-9999-0004: priority is low
  - golang.org/x/tools has 50 importers (< 100)
 -- logs --
 info: xref: operating on 1 report(s)

cmd/vulnreport/testdata/TestXref/no_xrefs.txtar

@@ -6,7 +6,7 @@ Expected output of test TestXref/no_xrefs
 command: "vulnreport xref 1"
 
 -- out --
-GO-9999-0001 priority is unknown
+GO-9999-0001: priority is unknown
  - module golang.org/x/vulndb not found
 -- logs --
 info: xref: operating on 1 report(s)

cmd/vulnreport/xref.go

@@ -7,10 +7,8 @@ package main
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	"golang.org/x/exp/constraints"
-	"golang.org/x/exp/maps"
 	"golang.org/x/exp/slices"
 	"golang.org/x/vulndb/cmd/vulnreport/log"
 	"golang.org/x/vulndb/cmd/vulnreport/priority"
@@ -44,13 +42,13 @@ func (x *xref) run(ctx context.Context, input any) (err error) {
 	r := input.(*yamlReport)
 
 	if xrefs := x.xref(r); len(xrefs) > 0 {
-		log.Outf("%s: found xrefs:%s", r.Filename, xrefs)
+		log.Out(xrefs)
 	} else {
 		log.Infof("%s: no xrefs found", r.Filename)
 	}
 
 	pr, notGo := x.reportPriority(r.Report)
-	log.Outf("%s priority is %s\n - %s", r.ID, pr.Priority, pr.Reason)
+	log.Outf("%s: priority is %s\n - %s", r.ID, pr.Priority, pr.Reason)
 	if notGo != nil {
 		log.Outf("%s is likely not Go\n - %s", r.ID, notGo.Reason)
 	}
@@ -84,21 +82,9 @@ type xrefer struct {
 }
 
 func (x *xrefer) xref(r *yamlReport) string {
-	out := &strings.Builder{}
-	matches := x.rc.XRef(r.Report)
-	delete(matches, r.Filename)
-	// This sorts as CVEs, GHSAs, and then modules.
-	for _, fname := range sorted(maps.Keys(matches)) {
-		for _, id := range sorted(matches[fname]) {
-			fmt.Fprintf(out, "\n%v appears in %v", id, fname)
-			if r, ok := x.rc.Report(fname); ok {
-				if r.IsExcluded() {
-					fmt.Fprintf(out, "  %v", r.Excluded)
-				}
-			}
-		}
-	}
-	return out.String()
+	aliasTitle := fmt.Sprintf("%s: found possible duplicates", r.ID)
+	moduleTitle := fmt.Sprintf("%s: found module xrefs", r.ID)
+	return x.rc.XRef(r.Report).ToString(aliasTitle, moduleTitle, "")
 }
 
 func (x *xrefer) modulePriority(modulePath string) (*priority.Result, *priority.NotGoResult) {

internal/report/client.go

@@ -6,7 +6,10 @@ package report
 
 import (
 	"context"
+	"fmt"
+	"io"
 	"path/filepath"
+	"strings"
 
 	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing/object"
@@ -36,8 +39,8 @@ const (
 type Client struct {
 	byFile   map[string]*Report
 	byIssue  map[int]*Report
-	byAlias  map[string][]*Report
-	byModule map[string][]*Report
+	byAlias  map[string][]*File
+	byModule map[string][]*File
 }
 
 // NewClient returns a Client for accessing the reports in
@@ -82,33 +85,91 @@ func (c *Client) List() []*Report {
 	return maps.Values(c.byFile)
 }
 
-// XRef returns cross-references for a report.
-// The output, matches, is a map from filenames to aliases (CVE & GHSA IDs)
-// and modules (excluding std and cmd).
-func (c *Client) XRef(r *Report) (matches map[string][]string) {
-	mods := make(map[string]bool)
-	for _, m := range r.Modules {
-		if mod := m.Module; mod != "" && mod != "std" && mod != "cmd" {
-			mods[m.Module] = true
+type Xrefs struct {
+	// map from aliases to files
+	Aliases map[string][]*File
+	// map from modules to files
+	Modules map[string][]*File
+}
+
+func fprintMap(out io.Writer, m map[string][]*File) {
+	sortedKeys := func(m map[string][]*File) []string {
+		s := slices.Clone(maps.Keys(m))
+		slices.Sort(s)
+		return s
+	}
+
+	for _, k := range sortedKeys(m) {
+		fs := m[k]
+		fmt.Fprintf(out, "- %s appears in %d other report(s):\n", k, len(fs))
+		for _, f := range fs {
+			fmt.Fprintf(out, "  - %s    (https://github.com/golang/vulndb#%d)", f.Filename, f.IssNum)
+			if f.Report.IsExcluded() {
+				fmt.Fprintf(out, "    %v", f.Report.Excluded)
+			}
+			fmt.Fprintf(out, "\n")
 		}
 	}
+}
 
-	// matches is a map from filename -> alias/module
-	matches = make(map[string][]string)
-	for fname, rr := range c.byFile {
-		for _, alias := range rr.Aliases() {
-			if slices.Contains(r.Aliases(), alias) {
-				matches[fname] = append(matches[fname], alias)
+func (xs *Xrefs) ToString(aliasTitle, moduleTitle, noneMessage string) string {
+	if len(xs.Modules) == 0 && len(xs.Aliases) == 0 {
+		return noneMessage
+	}
+
+	out := &strings.Builder{}
+
+	if len(xs.Aliases) != 0 {
+		fmt.Fprint(out, aliasTitle+"\n")
+		fprintMap(out, xs.Aliases)
+		if len(xs.Modules) != 0 {
+			fmt.Fprintf(out, "\n")
+		}
+	}
+
+	if len(xs.Modules) != 0 {
+		fmt.Fprint(out, moduleTitle+"\n")
+		fprintMap(out, xs.Modules)
+	}
+
+	return out.String()
+}
+
+type File struct {
+	Filename string
+	IssNum   int
+	*Report
+}
+
+// XRef returns cross-references for a report.
+func (c *Client) XRef(r *Report) *Xrefs {
+	x := &Xrefs{
+		Aliases: make(map[string][]*File),
+		Modules: make(map[string][]*File),
+	}
+
+	for _, alias := range r.Aliases() {
+		for _, f := range c.byAlias[alias] {
+			if r.ID == f.Report.ID {
+				continue
 			}
+			x.Aliases[alias] = append(x.Aliases[alias], f)
+		}
+	}
+
+	for _, m := range r.Modules {
+		if m.IsFirstParty() {
+			continue
 		}
-		for _, m := range rr.Modules {
-			if mods[m.Module] {
-				k := "Module " + m.Module
-				matches[fname] = append(matches[fname], k)
+		for _, f := range c.byModule[m.Module] {
+			if r.ID == f.Report.ID {
+				continue
 			}
+			x.Modules[m.Module] = append(x.Modules[m.Module], f)
 		}
 	}
-	return matches
+
+	return x
 }
 
 // Report returns the report with the given filename in vulndb, or
@@ -128,13 +189,21 @@ func (c *Client) HasReport(githubID int) (found bool) {
 // ReportsByAlias returns a list of reports in vulndb with the given
 // alias.
 func (c *Client) ReportsByAlias(alias string) []*Report {
-	return c.byAlias[alias]
+	var rs []*Report
+	for _, f := range c.byAlias[alias] {
+		rs = append(rs, f.Report)
+	}
+	return rs
 }
 
 // ReportsByModule returns a list of reports in vulndb with the given
 // module.
 func (c *Client) ReportsByModule(module string) []*Report {
-	return c.byModule[module]
+	var rs []*Report
+	for _, f := range c.byModule[module] {
+		rs = append(rs, f.Report)
+	}
+	return rs
 }
 
 // AliasHasReport returns whether the given alias exists in vulndb.
@@ -147,8 +216,8 @@ func newClient() *Client {
 	return &Client{
 		byIssue:  make(map[int]*Report),
 		byFile:   make(map[string]*Report),
-		byAlias:  make(map[string][]*Report),
-		byModule: make(map[string][]*Report),
+		byAlias:  make(map[string][]*File),
+		byModule: make(map[string][]*File),
 	}
 }
 
@@ -187,13 +256,19 @@ func (c *Client) addReport(filename string, r *Report) error {
 		return err
 	}
 
+	f := &File{
+		Filename: filename,
+		IssNum:   iss,
+		Report:   r,
+	}
+
 	c.byFile[filename] = r
 	c.byIssue[iss] = r
 	for _, alias := range r.Aliases() {
-		c.byAlias[alias] = append(c.byAlias[alias], r)
+		c.byAlias[alias] = append(c.byAlias[alias], f)
 	}
 	for _, m := range r.Modules {
-		c.byModule[m.Module] = append(c.byModule[m.Module], r)
+		c.byModule[m.Module] = append(c.byModule[m.Module], f)
 	}
 
 	return nil

internal/report/client_test.go

@@ -42,7 +42,6 @@ var (
 		Modules: []*Module{
 			{Module: "example.com/another/module"},
 		},
-
 		GHSAs: []string{
 			"GHSA-9999-abcd-efgh",
 		},
@@ -55,6 +54,14 @@ var (
 		},
 		CVEs: []string{"CVE-9999-0005"},
 	}
+	fname6 = "data/reports/GO-9999-0006.yaml"
+	r6     = Report{
+		ID: "GO-9999-0006",
+		Modules: []*Module{
+			{Module: "example.com/another/module"},
+		},
+		GHSAs: []string{"GHSA-9999-abcd-efgh"},
+	}
 
 	txtarFile = filepath.Join("testdata", "repo.txtar")
 )
@@ -71,7 +78,7 @@ func TestList(t *testing.T) {
 	}
 
 	got := rc.List()
-	want := []*Report{&r1, &r2, &r4, &r5}
+	want := []*Report{&r1, &r2, &r4, &r5, &r6}
 	byID := func(a, b *Report) bool { return a.ID < b.ID }
 	if diff := cmp.Diff(got, want, cmpopts.SortSlices(byID)); diff != "" {
 		t.Errorf("mismatch (-got, +want): %s", diff)
@@ -91,6 +98,7 @@ func TestXRef(t *testing.T) {
 	tests := []struct {
 		name        string
 		r           *Report
+		wantXrefs   *Xrefs
 		wantMatches map[string][]string
 	}{
 		{
@@ -103,7 +111,10 @@ func TestXRef(t *testing.T) {
 					ID: "CVE-9999-0003",
 				},
 			},
-			wantMatches: map[string][]string{},
+			wantXrefs: &Xrefs{
+				Aliases: map[string][]*File{},
+				Modules: map[string][]*File{},
+			},
 		},
 		{
 			name: "Ignores std lib modules",
@@ -113,7 +124,10 @@ func TestXRef(t *testing.T) {
 				},
 				CVEs: []string{"CVE-9999-0003"},
 			},
-			wantMatches: map[string][]string{},
+			wantXrefs: &Xrefs{
+				Aliases: map[string][]*File{},
+				Modules: map[string][]*File{},
+			},
 		},
 		{
 			name: "Match on CVE (ignores std module)",
@@ -123,26 +137,37 @@ func TestXRef(t *testing.T) {
 				},
 				CVEs: []string{"CVE-9999-0001"},
 			},
-			wantMatches: map[string][]string{
-				fname1: {"CVE-9999-0001"},
+			wantXrefs: &Xrefs{
+				Aliases: map[string][]*File{
+					"CVE-9999-0001": {
+						{Filename: fname1, IssNum: 1, Report: &r1},
+					},
+				},
+				Modules: map[string][]*File{},
 			},
 		},
 		{
 			name: "Match on GHSA & module",
 			r:    &r4,
-			wantMatches: map[string][]string{
-				fname4: {
-					"GHSA-9999-abcd-efgh",
-					"Module example.com/another/module",
+			wantXrefs: &Xrefs{
+				Aliases: map[string][]*File{
+					"GHSA-9999-abcd-efgh": {
+						{Filename: fname6, IssNum: 6, Report: &r6},
+					},
+				},
+				Modules: map[string][]*File{
+					"example.com/another/module": {
+						{Filename: fname6, IssNum: 6, Report: &r6},
+					},
 				},
 			},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotMatches := rc.XRef(tt.r)
-			if diff := cmp.Diff(gotMatches, tt.wantMatches); diff != "" {
-				t.Errorf("XRef(): matches mismatch (-got, +want): %s", diff)
+			got := rc.XRef(tt.r)
+			if diff := cmp.Diff(got, tt.wantXrefs); diff != "" {
+				t.Errorf("XRef(): mismatch (-got, +want): %s", diff)
 			}
 		})
 	}
@@ -198,14 +223,19 @@ func TestNewClient(t *testing.T) {
 	}
 
 	files := map[string]*Report{
-		fname1: &r1, fname2: &r2, fname4: &r4, fname5: &r5,
+		fname1: &r1, fname2: &r2, fname4: &r4, fname5: &r5, fname6: &r6,
 	}
 	tc, err := NewTestClient(files)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	if diff := cmp.Diff(c, tc, cmp.AllowUnexported(Client{})); diff != "" {
+	less := func(f1, f2 *File) bool {
+		return f1.ID < f2.ID
+	}
+
+	if diff := cmp.Diff(c, tc, cmp.AllowUnexported(Client{}),
+		cmpopts.SortSlices(less)); diff != "" {
 		t.Errorf("NewClient() / NewTestClient() mismatch (-New, +NewTest): %s", diff)
 	}
 }