From 76c7a5b6fdde8a296fd039003c3ebf8413a46a6d Mon Sep 17 00:00:00 2001 From: Tatiana Bradley Date: Wed, 17 Jul 2024 13:21:42 -0400 Subject: [PATCH] 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 Reviewed-by: Damien Neil --- .../testdata/TestXref/found_xrefs.txtar | 8 +- .../testdata/TestXref/no_xrefs.txtar | 2 +- cmd/vulnreport/xref.go | 24 +--- internal/report/client.go | 133 ++++++++++++++---- internal/report/client_test.go | 60 ++++++-- internal/report/testdata/repo.txtar | 9 +- internal/worker/worker.go | 44 +----- internal/worker/worker_test.go | 20 ++- 8 files changed, 187 insertions(+), 113 deletions(-) diff --git a/cmd/vulnreport/testdata/TestXref/found_xrefs.txtar b/cmd/vulnreport/testdata/TestXref/found_xrefs.txtar index 32453652..779398b1 100644 --- a/cmd/vulnreport/testdata/TestXref/found_xrefs.txtar +++ b/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) diff --git a/cmd/vulnreport/testdata/TestXref/no_xrefs.txtar b/cmd/vulnreport/testdata/TestXref/no_xrefs.txtar index 96c68c31..1925cfa8 100644 --- a/cmd/vulnreport/testdata/TestXref/no_xrefs.txtar +++ b/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) diff --git a/cmd/vulnreport/xref.go b/cmd/vulnreport/xref.go index 4151781e..c68eeea0 100644 --- a/cmd/vulnreport/xref.go +++ b/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) { diff --git a/internal/report/client.go b/internal/report/client.go index 21c1a97a..3528e2a8 100644 --- a/internal/report/client.go +++ b/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") + } + } +} + +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") } } - // 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) - } - } - for _, m := range rr.Modules { - if mods[m.Module] { - k := "Module " + m.Module - matches[fname] = append(matches[fname], k) + 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) } } - return matches + + for _, m := range r.Modules { + if m.IsFirstParty() { + continue + } + 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 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 diff --git a/internal/report/client_test.go b/internal/report/client_test.go index c18059c6..30b4442a 100644 --- a/internal/report/client_test.go +++ b/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) } } diff --git a/internal/report/testdata/repo.txtar b/internal/report/testdata/repo.txtar index 3a25da6c..1b3630b1 100644 --- a/internal/report/testdata/repo.txtar +++ b/internal/report/testdata/repo.txtar @@ -31,4 +31,11 @@ id: GO-9999-0005 modules: - module: example.com/adiff/module cves: - - CVE-9999-0005 \ No newline at end of file + - CVE-9999-0005 + +-- data/reports/GO-9999-0006.yaml -- +id: GO-9999-0006 +modules: + - module: example.com/another/module +ghsas: + - GHSA-9999-abcd-efgh \ No newline at end of file diff --git a/internal/worker/worker.go b/internal/worker/worker.go index 410990a9..92245179 100644 --- a/internal/worker/worker.go +++ b/internal/worker/worker.go @@ -18,8 +18,6 @@ import ( "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/object" - "golang.org/x/exp/maps" - "golang.org/x/exp/slices" "golang.org/x/time/rate" "golang.org/x/vulndb/internal/cve4" "golang.org/x/vulndb/internal/cveutils" @@ -181,38 +179,9 @@ func CreateIssues(ctx context.Context, st store.Store, client *issues.Client, pc // xref returns cross-references for a report: Information about other reports // for the same CVE, GHSA, or module. func xref(r *report.Report, rc *report.Client) string { - out := &strings.Builder{} - sorted := func(s []string) []string { - s = slices.Clone(s) - slices.Sort(s) - return s - } - - matches := rc.XRef(r) - for _, fname := range sorted(maps.Keys(matches)) { - for _, match := range sorted(matches[fname]) { - // Getting issue number from file name - var appearsIn string - _, _, issueNum, err := report.ParseFilepath(fname) - if err != nil { - appearsIn = fmt.Sprintf("%s (unable to convert file name to issue number, %v)", fname, err) - } else { - appearsIn = strconv.Itoa(issueNum) - } - - fmt.Fprintf(out, "- %v appears in issue #%v", match, appearsIn) - if r, ok := rc.Report(fname); ok { - if r.IsExcluded() { - fmt.Fprintf(out, " %v", r.Excluded) - } - } - fmt.Fprintf(out, "\n") - } - } - if len(matches) == 0 { - fmt.Fprint(out, "No existing reports found with this module or alias.") - } - return out.String() + aliasTitle, moduleTitle, noneMessage := "!! Possible duplicate report !!", + "Cross references:", "No existing reports found with this module or alias." + return rc.XRef(r).ToString(aliasTitle, moduleTitle, noneMessage) } func createCVEIssues(ctx context.Context, st store.Store, client *issues.Client, pc *proxy.Client, rc *report.Client, limit int) (err error) { @@ -323,8 +292,8 @@ func createGHSAIssues(ctx context.Context, st store.Store, client *issues.Client func isDuplicate(sa *ghsa.SecurityAdvisory, pc *proxy.Client, rc *report.Client) bool { r := report.New(sa, pc) - for _, aliases := range rc.XRef(r) { - if slices.Contains(aliases, sa.ID) { + for alias := range rc.XRef(r).Aliases { + if sa.ID == alias { return true } } @@ -451,9 +420,8 @@ Description: References:{{range .References}} - {{.Type}}: {{.URL}}{{end}} -Cross references: {{.Xrefs}} -See [doc/triage.md](https://github.com/golang/vulndb/blob/master/doc/triage.md) for instructions on how to triage this report. +See [doc/quickstart.md](https://github.com/golang/vulndb/blob/master/doc/quickstart.md) for instructions on how to triage this report. {{if (and .Pre .ReportStr) -}} {{.Pre}} diff --git a/internal/worker/worker_test.go b/internal/worker/worker_test.go index 4c94ddeb..97a454e9 100644 --- a/internal/worker/worker_test.go +++ b/internal/worker/worker_test.go @@ -302,11 +302,15 @@ a description References: - ADVISORY: https://nvd.nist.gov/vuln/detail/CVE-2000-0001 -Cross references: -- CVE-2000-0001 appears in issue #2 -- Module golang.org/x/vulndb appears in issue #2 +!! Possible duplicate report !! +- CVE-2000-0001 appears in 1 other report(s): + - data/reports/GO-9999-0002.yaml (https://github.com/golang/vulndb#2) -See [doc/triage.md](https://github.com/golang/vulndb/blob/master/doc/triage.md) for instructions on how to triage this report. +Cross references: +- golang.org/x/vulndb appears in 1 other report(s): + - data/reports/GO-9999-0002.yaml (https://github.com/golang/vulndb#2) + +See [doc/quickstart.md](https://github.com/golang/vulndb/blob/master/doc/quickstart.md) for instructions on how to triage this report. ` + "```" + ` id: GO-ID-PENDING @@ -326,6 +330,7 @@ review_status: UNREVIEWED ` + "```" if diff := cmp.Diff(unindent(want), got); diff != "" { t.Errorf("mismatch (-want, +got):\n%s", diff) + t.Errorf("got: %s", got) } } @@ -380,10 +385,11 @@ References: - ADVISORY: https://github.com/advisories/GHSA-xxxx-yyyy-zzzz - FIX: https://example.com/commit/12345 -Cross references: -- GHSA-xxxx-yyyy-zzzz appears in issue #1 EXCLUDED +!! Possible duplicate report !! +- GHSA-xxxx-yyyy-zzzz appears in 1 other report(s): + - data/excluded/GO-9999-0001.yaml (https://github.com/golang/vulndb#1) EXCLUDED -See [doc/triage.md](https://github.com/golang/vulndb/blob/master/doc/triage.md) for instructions on how to triage this report. +See [doc/quickstart.md](https://github.com/golang/vulndb/blob/master/doc/quickstart.md) for instructions on how to triage this report. ` + "```" + ` id: GO-ID-PENDING