From 9c55f263e39b51734a1fb015ccfbfd506ce57d63 Mon Sep 17 00:00:00 2001 From: Tatiana Bradley Date: Fri, 23 Aug 2024 12:32:26 -0400 Subject: [PATCH] internal/report: use variables to represent excluded reasons Change-Id: Ic83a96592f865d4643a760a6bc10718cf5c075a0 Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/607817 Auto-Submit: Tatiana Bradley Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI --- cmd/inspect/main.go | 14 +++--- cmd/issue/main.go | 10 ++-- cmd/vulnreport/create_excluded.go | 2 +- cmd/vulnreport/creator.go | 4 +- cmd/vulnreport/unexclude.go | 3 +- internal/report/client_test.go | 2 +- internal/report/lint.go | 8 ++-- internal/report/lint_test.go | 2 +- internal/report/new.go | 4 +- internal/report/report.go | 57 ++++++++++++++++------- internal/report/report_test.go | 5 +- internal/triage/cve5_test.go | 2 +- internal/triage/priority/priority.go | 9 ++-- internal/triage/priority/priority_test.go | 16 +++---- 14 files changed, 83 insertions(+), 55 deletions(-) diff --git a/cmd/inspect/main.go b/cmd/inspect/main.go index eecadb6a..2de232ac 100644 --- a/cmd/inspect/main.go +++ b/cmd/inspect/main.go @@ -105,8 +105,8 @@ func display(overall *summary, byYear map[int]*summary) { } data("Withdrawn reports", 1, func(s *summary) int { return s.withdrawn }) data("Excluded reports", 1, func(s *summary) int { return s.excluded }) - for _, er := range report.ExcludedReasons { - data(string(er), 2, func(s *summary) int { return s.excludedByReason[er] }) + for _, er := range report.ExcludedTypes { + data(string(er), 2, func(s *summary) int { return s.excludedByType[er] }) } data("Reports with no GHSA (+)", 1, func(s *summary) int { return s.noGHSA }) data("Stdlib, toolchain and x/ reports", 1, func(s *summary) int { return s.firstParty }) @@ -138,14 +138,14 @@ type summary struct { reports, regular, withdrawn, excluded, noGHSA, firstParty int ghsas int ghsasNotInVDB []string - excludedByReason map[report.ExcludedReason]int + excludedByType map[report.ExcludedType]int regularByReview map[report.ReviewStatus]int } func newSummary() *summary { return &summary{ - excludedByReason: make(map[report.ExcludedReason]int), - regularByReview: make(map[report.ReviewStatus]int), + excludedByType: make(map[report.ExcludedType]int), + regularByReview: make(map[report.ReviewStatus]int), } } @@ -174,10 +174,10 @@ func summarize(ghsas []*genericosv.Entry, reports []*report.Report) (*summary, m if r.IsExcluded() { overall.excluded++ - overall.excludedByReason[r.Excluded]++ + overall.excludedByType[r.Excluded]++ yearSummary.excluded++ - yearSummary.excludedByReason[r.Excluded]++ + yearSummary.excludedByType[r.Excluded]++ } else if r.Withdrawn != nil { overall.withdrawn++ yearSummary.withdrawn++ diff --git a/cmd/issue/main.go b/cmd/issue/main.go index 6f18a9fc..40f07d7b 100644 --- a/cmd/issue/main.go +++ b/cmd/issue/main.go @@ -92,7 +92,7 @@ func createExcluded(ctx context.Context, c *issues.Client, ghsaClient *ghsa.Clie return err } for _, r := range records { - if err := constructIssue(ctx, c, ghsaClient, pc, r.identifier, []string{r.category.ToLabel()}); err != nil { + if err := constructIssue(ctx, c, ghsaClient, pc, r.identifier, []string{r.label}); err != nil { return err } } @@ -182,7 +182,7 @@ func publishIssue(ctx context.Context, c *issues.Client, packages, aliases, bodi type record struct { identifier string - category report.ExcludedReason + label string } func parseAliases(filename string) (aliases []string, err error) { @@ -204,8 +204,12 @@ func parseExcluded(filename string) (records []*record, err error) { if len(parts) != 2 { return nil, fmt.Errorf("wrong number of fields on line %d: %q", i, line) } + er, ok := report.ToExcludedType(parts[0]) + if !ok { + return nil, fmt.Errorf("%s is not a valid excluded reason", parts[0]) + } r := &record{ - category: report.ExcludedReason(parts[0]), + label: er.ToLabel(), identifier: parts[1], } records = append(records, r) diff --git a/cmd/vulnreport/create_excluded.go b/cmd/vulnreport/create_excluded.go index b8c788e7..17f853f6 100644 --- a/cmd/vulnreport/create_excluded.go +++ b/cmd/vulnreport/create_excluded.go @@ -64,7 +64,7 @@ func (c *createExcluded) skip(input any) string { } func isExcluded(iss *issues.Issue) bool { - for _, er := range report.ExcludedReasons { + for _, er := range report.ExcludedTypes { if iss.HasLabel(er.ToLabel()) { return true } diff --git a/cmd/vulnreport/creator.go b/cmd/vulnreport/creator.go index 65d4c4ca..f9f50120 100644 --- a/cmd/vulnreport/creator.go +++ b/cmd/vulnreport/creator.go @@ -288,7 +288,7 @@ const ( labelOutOfScope = "excluded: OUT_OF_SCOPE" ) -func excludedReason(iss *issues.Issue) report.ExcludedReason { +func excludedReason(iss *issues.Issue) report.ExcludedType { for _, label := range iss.Labels { if reason, ok := report.FromLabel(label); ok { return reason @@ -325,7 +325,7 @@ type reportMeta struct { id string modulePath string aliases []string - excluded, unexcluded report.ExcludedReason + excluded, unexcluded report.ExcludedType reviewStatus report.ReviewStatus originalCVE string } diff --git a/cmd/vulnreport/unexclude.go b/cmd/vulnreport/unexclude.go index 98e5d2f7..42d286ff 100644 --- a/cmd/vulnreport/unexclude.go +++ b/cmd/vulnreport/unexclude.go @@ -43,7 +43,8 @@ func (u *unexclude) skip(input any) string { } // Usually, we only unexclude reports that are effectively private or not importable. - if ex := r.Excluded; ex != "EFFECTIVELY_PRIVATE" && ex != "NOT_IMPORTABLE" { + if ex := r.Excluded; ex != report.ExcludedEffectivelyPrivate && + ex != report.ExcludedNotImportable { if *force { log.Warnf("%s: excluded for reason %q, but -f was specified, continuing", r.ID, ex) return "" diff --git a/internal/report/client_test.go b/internal/report/client_test.go index 30b4442a..27285241 100644 --- a/internal/report/client_test.go +++ b/internal/report/client_test.go @@ -34,7 +34,7 @@ var ( CVEMetadata: &CVEMeta{ ID: "CVE-9999-0002", }, - Excluded: "EFFECTIVELY_PRIVATE", + Excluded: ExcludedEffectivelyPrivate, } fname4 = "data/reports/GO-9999-0004.yaml" r4 = Report{ diff --git a/internal/report/lint.go b/internal/report/lint.go index dfe9973f..72f58a39 100644 --- a/internal/report/lint.go +++ b/internal/report/lint.go @@ -614,7 +614,7 @@ func (p *Package) lint(l *linter, m *Module, r *Report) { } func (r *Report) lintModules(l *linter, pc *proxy.Client) { - if r.Excluded != "NOT_GO_CODE" && len(r.Modules) == 0 { + if r.Excluded != ExcludedNotGoCode && len(r.Modules) == 0 { l.Group("modules").Error(missing) } @@ -636,12 +636,12 @@ func (m *Module) IsFirstParty() bool { return stdlib.IsStdModule(m.Module) || stdlib.IsCmdModule(m.Module) } -func (e *ExcludedReason) lint(l *linter) { +func (e *ExcludedType) lint(l *linter) { if e == nil || *e == "" { return } - if !slices.Contains(ExcludedReasons, *e) { - l.Errorf("excluded reason (%q) is not a valid excluded reason (accepted: %v)", *e, ExcludedReasons) + if !e.IsValid() { + l.Errorf("excluded reason (%q) is not a valid excluded reason (accepted: %v)", *e, ExcludedTypes) } } diff --git a/internal/report/lint_test.go b/internal/report/lint_test.go index 7f361cc3..2dc5cdf2 100644 --- a/internal/report/lint_test.go +++ b/internal/report/lint_test.go @@ -82,7 +82,7 @@ func validStdReport(f func(r *Report)) Report { func validExcludedReport(f func(r *Report)) Report { r := Report{ ID: "GO-0000-0000", - Excluded: "NOT_GO_CODE", + Excluded: ExcludedNotGoCode, CVEs: []string{"CVE-2022-1234545"}, } f(&r) diff --git a/internal/report/new.go b/internal/report/new.go index d928b195..704c083e 100644 --- a/internal/report/new.go +++ b/internal/report/new.go @@ -103,7 +103,7 @@ func WithReviewStatus(status ReviewStatus) NewOption { } } -func WithUnexcluded(reason ExcludedReason) NewOption { +func WithUnexcluded(reason ExcludedType) NewOption { return func(h *cfg) { h.Unexcluded = reason } @@ -115,7 +115,7 @@ type cfg struct { Created time.Time GoID string ReviewStatus ReviewStatus - Unexcluded ExcludedReason + Unexcluded ExcludedType } const PendingID = "GO-ID-PENDING" diff --git a/internal/report/report.go b/internal/report/report.go index 8de36b43..c79739d0 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -198,35 +198,56 @@ type CVEMeta struct { References []string `yaml:",omitempty"` } -// ExcludedReason is the reason a report is excluded from the database. +// ExcludedType is the reason a report is excluded from the database. // -// It must be one of the values in ExcludedReasons. -type ExcludedReason string +// It must be one of the values in ExcludedTypes. +type ExcludedType string -// ExcludedReasons are the set of reasons a report may be excluded from the database. +const ( + ExcludedNotImportable ExcludedType = "NOT_IMPORTABLE" + ExcludedNotGoCode ExcludedType = "NOT_GO_CODE" + ExcludedNotAVulnerability ExcludedType = "NOT_A_VULNERABILITY" + ExcludedEffectivelyPrivate ExcludedType = "EFFECTIVELY_PRIVATE" + ExcludedDependentVulnerabilty ExcludedType = "DEPENDENT_VULNERABILITY" + ExcludedLegacyFalsePositive ExcludedType = "LEGACY_FALSE_POSITIVE" +) + +// ExcludedTypes are the set of reasons a report may be excluded from the database. // These are described in detail at // https://go.googlesource.com/vulndb/+/refs/heads/master/doc/format.md. -var ExcludedReasons = []ExcludedReason{ - "NOT_IMPORTABLE", - "NOT_GO_CODE", - "NOT_A_VULNERABILITY", - "EFFECTIVELY_PRIVATE", - "DEPENDENT_VULNERABILITY", - "LEGACY_FALSE_POSITIVE", +var ExcludedTypes = []ExcludedType{ + ExcludedNotImportable, + ExcludedNotGoCode, + ExcludedNotAVulnerability, + ExcludedEffectivelyPrivate, + ExcludedDependentVulnerabilty, + ExcludedLegacyFalsePositive, +} + +func (e *ExcludedType) IsValid() bool { + return slices.Contains(ExcludedTypes, *e) +} + +func ToExcludedType(s string) (ExcludedType, bool) { + e := ExcludedType(s) + if !e.IsValid() { + return "", false + } + return e, true } const excludedLabelPrefix = "excluded: " -func (er ExcludedReason) ToLabel() string { - return fmt.Sprintf("%s%s", excludedLabelPrefix, string(er)) +func (e ExcludedType) ToLabel() string { + return fmt.Sprintf("%s%s", excludedLabelPrefix, string(e)) } -func FromLabel(label string) (ExcludedReason, bool) { +func FromLabel(label string) (ExcludedType, bool) { pre, er, ok := strings.Cut(label, excludedLabelPrefix) - if pre != "" { + if pre != "" || !ok { return "", false } - return ExcludedReason(er), ok + return ToExcludedType(er) } // A Reference is a link to some external resource. @@ -302,7 +323,7 @@ type Report struct { ID string `yaml:",omitempty"` // Excluded indicates an excluded report. - Excluded ExcludedReason `yaml:",omitempty"` + Excluded ExcludedType `yaml:",omitempty"` Modules []*Module `yaml:",omitempty"` @@ -352,7 +373,7 @@ type Report struct { // (For unexcluded reports) The reason this report was previously // excluded. Not published to OSV. - Unexcluded ExcludedReason `yaml:"unexcluded,omitempty"` + Unexcluded ExcludedType `yaml:"unexcluded,omitempty"` } // This wrapper is needed so we can define YAML functions on this type. diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 692f1556..44f606af 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -60,7 +60,7 @@ func TestYAMLFilename(t *testing.T) { }, { name: "excluded", - r: &Report{ID: "GO-1999-0002", Excluded: "NOT_IMPORTABLE"}, + r: &Report{ID: "GO-1999-0002", Excluded: ExcludedNotImportable}, want: "data/excluded/GO-1999-0002.yaml", }, } @@ -76,9 +76,8 @@ func TestYAMLFilename(t *testing.T) { } func TestToFromLabel(t *testing.T) { - str := "EFFECTIVELY_PRIVATE" label := "excluded: EFFECTIVELY_PRIVATE" - er := ExcludedReason(str) + er := ExcludedEffectivelyPrivate if got, want := er.ToLabel(), label; got != want { t.Errorf("(%s).ToLabel = %s, want %s", er, got, want) } diff --git a/internal/triage/cve5_test.go b/internal/triage/cve5_test.go index 6f92f0d5..3c1b84c7 100644 --- a/internal/triage/cve5_test.go +++ b/internal/triage/cve5_test.go @@ -152,7 +152,7 @@ func pickRandomCVEs(ctx context.Context, n int) ([]string, []string, error) { Categorize: for _, f := range files { for _, r := range rc.ReportsByAlias(f.ID()) { - if r.Excluded != "NOT_GO_CODE" { + if r.Excluded != report.ExcludedNotGoCode { goCVEs = append(goCVEs, f.ID()) continue Categorize } diff --git a/internal/triage/priority/priority.go b/internal/triage/priority/priority.go index 491debcb..cc0270fe 100644 --- a/internal/triage/priority/priority.go +++ b/internal/triage/priority/priority.go @@ -151,11 +151,14 @@ const ( func state(r *report.Report) reportState { if r.IsExcluded() { switch e := r.Excluded; e { - case "NOT_GO_CODE": + case report.ExcludedNotGoCode: return excludedNotGo - case "EFFECTIVELY_PRIVATE", "NOT_IMPORTABLE", "LEGACY_FALSE_POSITIVE": + case report.ExcludedEffectivelyPrivate, + report.ExcludedNotImportable, + report.ExcludedLegacyFalsePositive: return excludedBinary - case "NOT_A_VULNERABILITY", "DEPENDENT_VULNERABILITY": + case report.ExcludedNotAVulnerability, + report.ExcludedDependentVulnerabilty: return excludedOther default: return unknownReportState diff --git a/internal/triage/priority/priority_test.go b/internal/triage/priority/priority_test.go index d5a9c4fd..42315e61 100644 --- a/internal/triage/priority/priority_test.go +++ b/internal/triage/priority/priority_test.go @@ -13,7 +13,7 @@ import ( var ( notGo1 = &report.Report{ - Excluded: "NOT_GO_CODE", + Excluded: report.ExcludedNotGoCode, } reviewed1 = &report.Report{ ReviewStatus: report.Reviewed, @@ -23,29 +23,29 @@ var ( } reviewedBinary = &report.Report{ ReviewStatus: report.Reviewed, - Unexcluded: "NOT_IMPORTABLE", + Unexcluded: report.ExcludedNotImportable, } unreviewed1 = &report.Report{ ReviewStatus: report.Unreviewed, } binary1 = &report.Report{ - Excluded: "NOT_IMPORTABLE", + Excluded: report.ExcludedNotImportable, } binary2 = &report.Report{ - Excluded: "EFFECTIVELY_PRIVATE", + Excluded: report.ExcludedEffectivelyPrivate, } binary3 = &report.Report{ - Excluded: "LEGACY_FALSE_POSITIVE", + Excluded: report.ExcludedLegacyFalsePositive, } unreviewedBinary = &report.Report{ ReviewStatus: report.Unreviewed, - Unexcluded: "NOT_IMPORTABLE", + Unexcluded: report.ExcludedNotImportable, } notAVuln1 = &report.Report{ - Excluded: "NOT_A_VULNERABILITY", + Excluded: report.ExcludedNotAVulnerability, } dependent1 = &report.Report{ - Excluded: "DEPENDENT_VULNERABILITY", + Excluded: report.ExcludedDependentVulnerabilty, } )