internal/report: add new status NEEDS_REVIEW

Add a new YAML report status, NEEDS_REVIEW, which indicates that
a report has been automatically generated but needs to be reviewed
by a human later.

The goal of this new status is to allow us to quickly publish initial
versions of *most* reports that will require review.

A report with status NEEDS_REVIEW has slightly stricter requirements
than UNREVIEWED reports:
    - NEEDS_REVIEW reports must have a fixed version for each affected module
    - NEEDS_REVIEW reports must not have any "unsupported_versions"

These stricter requirements prevent us from publishing low-information reports
that could affect many users. Auto-generated reports that do not meet these
requirements need to be manually reviewed by a human.

When a new NEEDS_REVIEW report is committed, the automatically generated
commit message includes "Updates #NNN" for the corresponding issue instead
of "Fixes #NNN", because additional action is still needed.

NEEDS_REVIEW is an internal status only - it is converted to UNREVIEWED
when published to OSV.

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

all_test.go

@@ -140,10 +140,10 @@ func TestLintReports(t *testing.T) {
 			// This can happen because the initial quick triage algorithm
 			// doesn't know about all affected modules - just the one
 			// listed in the Github issue.
-			if r.IsUnreviewed() && !r.UnreviewedOK {
+			if r.IsUnreviewed() && !r.IsExcluded() && !r.UnreviewedOK {
 				pr, _ := priority.AnalyzeReport(r, rc, modulesToImports)
 				if pr.Priority == priority.High {
-					t.Errorf("UNREVIEWED report %s is high priority (should be REVIEWED) - reason: %s", filename, pr.Reason)
+					t.Errorf("UNREVIEWED report %s is high priority (should be NEEDS_REVIEW or REVIEWED) - reason: %s", filename, pr.Reason)
 				}
 			}
 			// Check that a correct OSV file was generated for each YAML report.

cmd/vulnreport/commit.go

@@ -217,6 +217,10 @@ func actionPhrases(status git.Status, r *yamlReport) (reportAction, issueAction
 			// Update instead of fixing the issue because we still need
 			// to manually publish the CVE record after submitting the CL.
 			return addReportAction, updateIssueAction, nil
+		case r.NeedsReview():
+			// Update instead of fixing the issue because we still need
+			// to review the report later.
+			return addReportAction, updateIssueAction, nil
 		default:
 			return addReportAction, fixIssueAction, nil
 		}

cmd/vulnreport/creator.go

@@ -127,20 +127,23 @@ func reviewStatusOf(iss *issues.Issue, reviewStatus report.ReviewStatus) report.
 	// If a valid review status is provided, it overrides the priority label.
 	if reviewStatus != 0 {
 		if d != reviewStatus {
-			log.Warnf("issue #%d: should be %s based on label(s) but this was overridden with the -status=%s flag", iss.Number, d, reviewStatus)
+			log.Warnf("issue #%d: would be %s based on label(s) but this was overridden with the -status=%s flag", iss.Number, d, reviewStatus)
 		}
 		return reviewStatus
 	}
 	return d
 }
 
 func defaultReviewStatus(iss *issues.Issue) report.ReviewStatus {
-	if iss.HasLabel(labelHighPriority) ||
-		iss.HasLabel(labelDirect) ||
+	if iss.HasLabel(labelDirect) ||
 		iss.HasLabel(labelFirstParty) {
 		return report.Reviewed
 	}
 
+	if iss.HasLabel(labelHighPriority) {
+		return report.NeedsReview
+	}
+
 	return report.Unreviewed
 }
 
@@ -161,6 +164,7 @@ func (c *creator) metaToSource(ctx context.Context, meta *reportMeta) report.Sou
 }
 
 func (c *creator) rawReport(ctx context.Context, meta *reportMeta) *report.Report {
+	log.Infof("%s: creating new %s report", meta.id, meta.reviewStatus)
 	return report.New(c.metaToSource(ctx, meta), c.pxc,
 		report.WithGoID(meta.id),
 		report.WithModulePath(meta.modulePath),
@@ -195,12 +199,11 @@ func (c *creator) reportFromMeta(ctx context.Context, meta *reportMeta) (*yamlRe
 	// The initial quick triage algorithm doesn't know about all
 	// affected modules, so double check the priority after the
 	// report is created.
-	if raw.IsUnreviewed() {
+	if raw.IsUnreviewed() && !raw.IsExcluded() {
 		pr, _ := c.reportPriority(raw)
 		if pr.Priority == priority.High {
-			log.Warnf("%s: re-generating; vuln is high priority and should be REVIEWED; reason: %s", raw.ID, pr.Reason)
-			meta.reviewStatus = report.Reviewed
-			raw = c.rawReport(ctx, meta)
+			log.Warnf("%s: vuln is high priority and should be NEEDS_REVIEW or REVIEWED; reason: %s", raw.ID, pr.Reason)
+			raw.ReviewStatus = report.NeedsReview
 		}
 	}
 
@@ -239,7 +242,7 @@ func (c *creator) reportFromMeta(ctx context.Context, meta *reportMeta) (*yamlRe
 	switch {
 	case raw.IsExcluded():
 		// nothing
-	case raw.IsUnreviewed():
+	case !raw.IsReviewed():
 		r.removeUnreachableRefs()
 	default:
 		// Regular, full-length reports.

doc/format.md

@@ -424,12 +424,19 @@ type `string`
 
 **required**
 
-The status of this report, either UNREVIEWED or REVIEWED.
+The status of this report, either UNREVIEWED, NEEDS_REVIEW, or REVIEWED.
 
 Unreviewed reports are generally auto-generated or nearly so. Their
 details have not been verified and no attempt has been made to determine
 packages or symbols. These reports must have an advisory link.
 
+Needs review reports are like unreviewed reports in that they are auto-generated,
+but we plan to review them in the near future. They have stricter requirements
+for version data than unreviewed reports. The NEEDS_REVIEW status allows us to
+quickly publish initial versions of most reports that will require review.
+NEEDS_REVIEW is an internal status only: it is converted to UNREVIEWED when
+published to OSV.
+
 Reviewed reports are reports for which we have made a good faith effort
 to determine correct affected versions, packages, and symbols. (In some
 cases it is not possible to determine all of these, which is OK). Descriptions

internal/database/new.go

@@ -8,7 +8,7 @@ import (
 	"fmt"
 
 	"golang.org/x/vulndb/internal/osv"
-	"golang.org/x/vulndb/internal/version"
+	"golang.org/x/vulndb/internal/osvutils"
 )
 
 // New creates a new database from the given entries.
@@ -63,7 +63,7 @@ func (m *ModulesIndex) add(entry osv.Entry) {
 		module.Vulns = append(module.Vulns, ModuleVuln{
 			ID:       entry.ID,
 			Modified: entry.Modified,
-			Fixed:    latestFixedVersion(affected.Ranges),
+			Fixed:    osvutils.LatestFixed(affected.Ranges),
 		})
 	}
 }
@@ -79,25 +79,3 @@ func (v *VulnsIndex) add(entry osv.Entry) error {
 	}
 	return nil
 }
-
-func latestFixedVersion(ranges []osv.Range) string {
-	var latestFixed string
-	for _, r := range ranges {
-		if r.Type == osv.RangeTypeSemver {
-			for _, e := range r.Events {
-				if fixed := e.Fixed; fixed != "" && version.Before(latestFixed, fixed) {
-					latestFixed = fixed
-				}
-			}
-			// If the vulnerability was re-introduced after the latest fix
-			// we found, there is no latest fix for this range.
-			for _, e := range r.Events {
-				if introduced := e.Introduced; introduced != "" && introduced != "0" && version.Before(latestFixed, introduced) {
-					latestFixed = ""
-					break
-				}
-			}
-		}
-	}
-	return string(latestFixed)
-}

internal/database/new_test.go

@@ -126,132 +126,3 @@ func TestNew(t *testing.T) {
 		t.Errorf("New: unexpected diff (-want, +got):\n%v", diff)
 	}
 }
-
-func TestLatestFixedVersion(t *testing.T) {
-	tests := []struct {
-		name   string
-		ranges []osv.Range
-		want   string
-	}{
-		{
-			name:   "empty",
-			ranges: []osv.Range{},
-			want:   "",
-		},
-		{
-			name: "no fix",
-			ranges: []osv.Range{{
-				Type: osv.RangeTypeSemver,
-				Events: []osv.RangeEvent{
-					{
-						Introduced: "0",
-					},
-				},
-			}},
-			want: "",
-		},
-		{
-			name: "no latest fix",
-			ranges: []osv.Range{{
-				Type: osv.RangeTypeSemver,
-				Events: []osv.RangeEvent{
-					{Introduced: "0"},
-					{Fixed: "1.0.4"},
-					{Introduced: "1.1.2"},
-				},
-			}},
-			want: "",
-		},
-		{
-			name: "unsorted no latest fix",
-			ranges: []osv.Range{{
-				Type: osv.RangeTypeSemver,
-				Events: []osv.RangeEvent{
-					{Fixed: "1.0.4"},
-					{Introduced: "0"},
-					{Introduced: "1.1.2"},
-					{Introduced: "1.5.0"},
-					{Fixed: "1.1.4"},
-				},
-			}},
-			want: "",
-		},
-		{
-			name: "unsorted with fix",
-			ranges: []osv.Range{{
-				Type: osv.RangeTypeSemver,
-				Events: []osv.RangeEvent{
-					{
-						Fixed: "1.0.0",
-					},
-					{
-						Introduced: "0",
-					},
-					{
-						Fixed: "0.1.0",
-					},
-					{
-						Introduced: "0.5.0",
-					},
-				},
-			}},
-			want: "1.0.0",
-		},
-		{
-			name: "multiple ranges",
-			ranges: []osv.Range{{
-				Type: osv.RangeTypeSemver,
-				Events: []osv.RangeEvent{
-					{
-						Introduced: "0",
-					},
-					{
-						Fixed: "0.1.0",
-					},
-				},
-			},
-				{
-					Type: osv.RangeTypeSemver,
-					Events: []osv.RangeEvent{
-						{
-							Introduced: "0",
-						},
-						{
-							Fixed: "0.2.0",
-						},
-					},
-				}},
-			want: "0.2.0",
-		},
-		{
-			name: "pseudoversion",
-			ranges: []osv.Range{{
-				Type: osv.RangeTypeSemver,
-				Events: []osv.RangeEvent{
-					{
-						Introduced: "0",
-					},
-					{
-						Fixed: "0.0.0-20220824120805-abc",
-					},
-					{
-						Introduced: "0.0.0-20230824120805-efg",
-					},
-					{
-						Fixed: "0.0.0-20240824120805-hij",
-					},
-				},
-			}},
-			want: "0.0.0-20240824120805-hij",
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			got := latestFixedVersion(test.ranges)
-			if got != test.want {
-				t.Errorf("latestFixedVersion = %q, want %q", got, test.want)
-			}
-		})
-	}
-}

internal/osv/review_status.go

@@ -30,10 +30,6 @@ func (r ReviewStatus) String() string {
 	return statusStrs[r]
 }
 
-func ReviewStatusValues() []string {
-	return statusStrs[1:]
-}
-
 func (r ReviewStatus) IsValid() bool {
 	return int(r) >= 0 && int(r) < len(statusStrs)
 }

internal/osvutils/fixed.go

@@ -0,0 +1,32 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package osvutils
+
+import (
+	"golang.org/x/vulndb/internal/osv"
+	"golang.org/x/vulndb/internal/version"
+)
+
+func LatestFixed(ranges []osv.Range) string {
+	var latestFixed string
+	for _, r := range ranges {
+		if r.Type == osv.RangeTypeSemver {
+			for _, e := range r.Events {
+				if fixed := e.Fixed; fixed != "" && version.Before(latestFixed, fixed) {
+					latestFixed = fixed
+				}
+			}
+			// If the vulnerability was re-introduced after the latest fix
+			// we found, there is no latest fix for this range.
+			for _, e := range r.Events {
+				if introduced := e.Introduced; introduced != "" && introduced != "0" && version.Before(latestFixed, introduced) {
+					latestFixed = ""
+					break
+				}
+			}
+		}
+	}
+	return string(latestFixed)
+}