cmd/vulnreport: check if packages exist in vulnreport fix

Add a simple check for package existence in vulnreport fix, which pings pkg.go.dev to determine if a package exists. This is more likely to succeed (and faster) than the package/symbol check which downloads the whole package. We now skip this symbol-check when there are no symbols listed. There are still some cases in which this fails incorrectly (e.g. if pkgsite for some reason couldn't cache the given package/version), so the check can be bypassed. Change-Id: I922eae0dec9a376210f0f0fd1d70a67da934ffaa Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/599180 Reviewed-by: Damien Neil <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
golang · Jul 19, 2024 · ebcb244 · ebcb244
1 parent 0550a0e
commit ebcb244
Show file tree

Hide file tree

Showing 29 changed files with 331 additions and 68 deletions.
diff --git a/cmd/vulnreport/creator.go b/cmd/vulnreport/creator.go
@@ -161,7 +161,7 @@ func (c *creator) metaToSource(ctx context.Context, meta *reportMeta) report.Sou
 }
 
 func (c *creator) rawReport(ctx context.Context, meta *reportMeta) *report.Report {
-	return report.New(c.metaToSource(ctx, meta), c.pc,
+	return report.New(c.metaToSource(ctx, meta), c.pxc,
 		report.WithGoID(meta.id),
 		report.WithModulePath(meta.modulePath),
 		report.WithAliases(meta.aliases),
@@ -172,7 +172,7 @@ func (c *creator) rawReport(ctx context.Context, meta *reportMeta) *report.Repor
 
 func (c *creator) reportFromMeta(ctx context.Context, meta *reportMeta) (*yamlReport, error) {
 	// Find the underlying module if the "module" provided is actually a package path.
-	if module, err := c.pc.FindModule(meta.modulePath); err == nil { // no error
+	if module, err := c.pxc.FindModule(meta.modulePath); err == nil { // no error
 		meta.modulePath = module
 	}
 	meta.aliases = c.allAliases(ctx, meta.aliases)

diff --git a/cmd/vulnreport/environment.go b/cmd/vulnreport/environment.go
@@ -15,14 +15,16 @@ import (
 	"golang.org/x/vulndb/internal/ghsa"
 	"golang.org/x/vulndb/internal/gitrepo"
 	"golang.org/x/vulndb/internal/issues"
+	"golang.org/x/vulndb/internal/pkgsite"
 	"golang.org/x/vulndb/internal/proxy"
 )
 
 // environment stores fakes/mocks of external dependencies for testing.
 type environment struct {
 	reportRepo *git.Repository
 	reportFS   fs.FS
-	pc         *proxy.Client
+	pxc        *proxy.Client
+	pkc        *pkgsite.Client
 	wfs        wfs
 	ic         issueClient
 	gc         ghsaClient
@@ -50,13 +52,21 @@ func (e *environment) ReportFS() fs.FS {
 }
 
 func (e *environment) ProxyClient() *proxy.Client {
-	if v := e.pc; v != nil {
+	if v := e.pxc; v != nil {
 		return v
 	}
 
 	return proxy.NewDefaultClient()
 }
 
+func (e *environment) PkgsiteClient() *pkgsite.Client {
+	if v := e.pkc; v != nil {
+		return v
+	}
+
+	return pkgsite.Default()
+}
+
 func (e *environment) WFS() wfs {
 	if v := e.wfs; v != nil {
 		return v

diff --git a/cmd/vulnreport/fix.go b/cmd/vulnreport/fix.go
@@ -17,15 +17,18 @@ import (
 	"golang.org/x/exp/slices"
 	"golang.org/x/vulndb/cmd/vulnreport/log"
 	"golang.org/x/vulndb/internal/osvutils"
+	"golang.org/x/vulndb/internal/pkgsite"
 	"golang.org/x/vulndb/internal/report"
 	"golang.org/x/vulndb/internal/symbols"
 )
 
 var (
-	force       = flag.Bool("f", false, "for fix, force Fix to run even if there are no lint errors")
-	skipChecks  = flag.Bool("skip-checks", false, "for fix, skip all checks except lint")
-	skipAlias   = flag.Bool("skip-alias", false, "for fix, skip adding new GHSAs and CVEs")
-	skipSymbols = flag.Bool("skip-symbols", false, "for lint and fix, don't load package for symbols checks")
+	force        = flag.Bool("f", false, "for fix, force Fix to run even if there are no lint errors")
+	skipChecks   = flag.Bool("skip-checks", false, "for fix, skip all checks except lint")
+	skipAlias    = flag.Bool("skip-alias", false, "for fix, skip adding new GHSAs and CVEs")
+	skipSymbols  = flag.Bool("skip-symbols", false, "for fix, don't load package for symbols checks")
+	skipPackages = flag.Bool("skip-packages", false, "for fix, don't check if packages exist")
+	skipRefs     = flag.Bool("skip-refs", false, "for fix, don't check if references exist")
 )
 
 type fix struct {
@@ -58,9 +61,12 @@ type fixer struct {
 	*linter
 	*aliasFinder
 	*fileWriter
+
+	pkc *pkgsite.Client
 }
 
 func (f *fixer) setup(ctx context.Context, env environment) error {
+	f.pkc = env.PkgsiteClient()
 	f.linter = new(linter)
 	f.aliasFinder = new(aliasFinder)
 	f.fileWriter = new(fileWriter)
@@ -85,8 +91,8 @@ func (f *fixer) fixAndWriteAll(ctx context.Context, r *yamlReport, addNotes bool
 func (f *fixer) fix(ctx context.Context, r *yamlReport, addNotes bool) (fixed bool) {
 	fixed = true
 
-	if lints := r.Lint(f.pc); *force || len(lints) > 0 {
-		r.Fix(f.pc)
+	if lints := r.Lint(f.pxc); *force || len(lints) > 0 {
+		r.Fix(f.pxc)
 	}
 
 	if !*skipChecks {
@@ -97,12 +103,12 @@ func (f *fixer) fix(ctx context.Context, r *yamlReport, addNotes bool) (fixed bo
 
 	// Check for remaining lint errors.
 	if addNotes {
-		if r.LintAsNotes(f.pc) {
+		if r.LintAsNotes(f.pxc) {
 			log.Warnf("%s: still has lint errors after fix", r.ID)
 			fixed = false
 		}
 	} else {
-		if lints := r.Lint(f.pc); len(lints) > 0 {
+		if lints := r.Lint(f.pxc); len(lints) > 0 {
 			log.Warnf("%s: still has lint errors after fix:\n\t- %s", r.ID, strings.Join(lints, "\n\t- "))
 			fixed = false
 		}
@@ -124,10 +130,17 @@ func (f *fixer) allChecks(ctx context.Context, r *yamlReport, addNotes bool) (ok
 		ok = false
 	}
 
+	if !*skipPackages {
+		log.Infof("%s: checking that all packages exist", r.ID)
+		if err := r.CheckPackages(ctx, f.pkc); err != nil {
+			fixErr("package error: %s", err)
+		}
+	}
+
 	if !*skipSymbols {
-		log.Infof("%s: checking packages and symbols (use -skip-symbols to skip this)", r.ID)
+		log.Infof("%s: checking symbols (use -skip-symbols to skip this)", r.ID)
 		if err := r.checkSymbols(); err != nil {
-			fixErr("package or symbol error: %s", err)
+			fixErr("symbol error: %s", err)
 		}
 	}
 
@@ -138,9 +151,11 @@ func (f *fixer) allChecks(ctx context.Context, r *yamlReport, addNotes bool) (ok
 		}
 	}
 
-	// For now, this is a fix check instead of a lint.
-	log.Infof("%s: checking that all references are reachable", r.ID)
-	checkRefs(r.References, fixErr)
+	if !*skipRefs {
+		// For now, this is a fix check instead of a lint.
+		log.Infof("%s: checking that all references are reachable", r.ID)
+		checkRefs(r.References, fixErr)
+	}
 
 	return ok
 }
@@ -171,7 +186,16 @@ func (r *yamlReport) checkSymbols() error {
 		log.Infof("%s: excluded, skipping symbol checks", r.ID)
 		return nil
 	}
+	if len(r.Modules) == 0 {
+		log.Infof("%s: no modules, skipping symbol checks", r.ID)
+		return nil
+	}
 	for _, m := range r.Modules {
+		if len(m.Packages) == 0 {
+			log.Infof("%s: module %s has no packages, skipping symbol checks", r.ID, m.Module)
+			return nil
+		}
+
 		if m.IsFirstParty() {
 			gover := runtime.Version()
 			ver := semverForGoVersion(gover)
@@ -196,8 +220,16 @@ func (r *yamlReport) checkSymbols() error {
 		}
 
 		for _, p := range m.Packages {
-			if p.SkipFix != "" {
-				log.Infof("%s: skipping symbol checks for package %s (reason: %q)", r.ID, p.Package, p.SkipFix)
+			if len(p.AllSymbols()) == 0 && p.SkipFixSymbols != "" {
+				log.Warnf("%s: skip_fix not needed", r.Filename)
+				continue
+			}
+			if len(p.AllSymbols()) == 0 {
+				log.Infof("%s: skipping symbol checks for package %s (no symbols)", r.ID, p.Package)
+				continue
+			}
+			if p.SkipFixSymbols != "" {
+				log.Infof("%s: skipping symbol checks for package %s (reason: %q)", r.ID, p.Package, p.SkipFixSymbols)
 				continue
 			}
 			syms, err := symbols.Exported(m, p)

diff --git a/cmd/vulnreport/lint.go b/cmd/vulnreport/lint.go
@@ -39,26 +39,26 @@ func (l *lint) run(_ context.Context, input any) error {
 }
 
 type linter struct {
-	pc *proxy.Client
+	pxc *proxy.Client
 }
 
 func (l *linter) setup(_ context.Context, env environment) error {
-	l.pc = env.ProxyClient()
+	l.pxc = env.ProxyClient()
 	return nil
 }
 
 func (l *linter) lint(r *yamlReport) error {
-	if lints := r.Lint(l.pc); len(lints) > 0 {
+	if lints := r.Lint(l.pxc); len(lints) > 0 {
 		return fmt.Errorf("%v has %d lint warnings:%s%s", r.ID, len(lints), listItem, strings.Join(lints, listItem))
 	}
 	return nil
 }
 
 func (l *linter) canonicalModule(mp string) string {
-	if module, err := l.pc.FindModule(mp); err == nil { // no error
+	if module, err := l.pxc.FindModule(mp); err == nil { // no error
 		mp = module
 	}
-	if module, err := l.pc.CanonicalAtLatest(mp); err == nil { // no error
+	if module, err := l.pxc.CanonicalAtLatest(mp); err == nil { // no error
 		mp = module
 	}
 	return mp

diff --git a/cmd/vulnreport/run_test.go b/cmd/vulnreport/run_test.go
@@ -21,14 +21,16 @@ import (
 	"golang.org/x/vulndb/cmd/vulnreport/log"
 	"golang.org/x/vulndb/cmd/vulnreport/priority"
 	"golang.org/x/vulndb/internal/gitrepo"
+	"golang.org/x/vulndb/internal/pkgsite"
 	"golang.org/x/vulndb/internal/proxy"
 	"golang.org/x/vulndb/internal/test"
 )
 
-// go test ./cmd/vulnreport -update-test -proxy
+// go test ./cmd/vulnreport -update-test -proxy -pkgsite
 var (
 	testUpdate = flag.Bool("update-test", false, "(for test) whether to update test files")
 	realProxy  = flag.Bool("proxy", false, "(for test) whether to use real proxy")
+	usePkgsite = flag.Bool("pkgsite", false, "(for test) whether to use real pkgsite")
 )
 
 type testCase struct {
@@ -91,7 +93,12 @@ func newDefaultTestEnv(t *testing.T) (*environment, error) {
 		return nil, err
 	}
 
-	pc, err := proxy.NewTestClient(t, *realProxy)
+	pxc, err := proxy.NewTestClient(t, *realProxy)
+	if err != nil {
+		return nil, err
+	}
+
+	pkc, err := pkgsite.TestClient(t, *usePkgsite)
 	if err != nil {
 		return nil, err
 	}
@@ -113,7 +120,8 @@ func newDefaultTestEnv(t *testing.T) (*environment, error) {
 	return &environment{
 		reportRepo: repo,
 		reportFS:   fsys,
-		pc:         pc,
+		pxc:        pxc,
+		pkc:        pkc,
 		wfs:        newInMemoryWFS(),
 		ic:         ic,
 		gc:         gc,

diff --git a/cmd/vulnreport/testdata/TestFix/no_change.txtar b/cmd/vulnreport/testdata/TestFix/no_change.txtar
@@ -11,14 +11,19 @@ data/osv/GO-9999-0001.json
 -- logs --
 info: fix: operating on 1 report(s)
 info: fix data/reports/GO-9999-0001.yaml
-info: GO-9999-0001: checking packages and symbols (use -skip-symbols to skip this)
+info: GO-9999-0001: checking that all packages exist
+info: GO-9999-0001: checking symbols (use -skip-symbols to skip this)
+info: GO-9999-0001: skipping symbol checks for package golang.org/x/vulndb/cmd/vulnreport (no symbols)
 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
@@ -46,7 +51,13 @@ review_status: REVIEWED
           ]
         }
       ],
-      "ecosystem_specific": {}
+      "ecosystem_specific": {
+        "imports": [
+          {
+            "path": "golang.org/x/vulndb/cmd/vulnreport"
+          }
+        ]
+      }
     }
   ],
   "database_specific": {

diff --git a/cmd/vulnreport/testdata/TestOSV/ok.txtar b/cmd/vulnreport/testdata/TestOSV/ok.txtar
@@ -35,7 +35,13 @@ info: osv: processed 1 report(s) (success=1; skip=0; error=0)
           ]
         }
       ],
-      "ecosystem_specific": {}
+      "ecosystem_specific": {
+        "imports": [
+          {
+            "path": "golang.org/x/vulndb/cmd/vulnreport"
+          }
+        ]
+      }
     }
   ],
   "database_specific": {

diff --git a/cmd/vulnreport/testdata/pkgsite/TestCVE/err.json b/cmd/vulnreport/testdata/pkgsite/TestCVE/err.json
@@ -0,0 +1 @@
+{}
diff --git a/cmd/vulnreport/testdata/pkgsite/TestCVE/ok.json b/cmd/vulnreport/testdata/pkgsite/TestCVE/ok.json
@@ -0,0 +1 @@
+{}
diff --git a/cmd/vulnreport/testdata/pkgsite/TestFix/no_change.json b/cmd/vulnreport/testdata/pkgsite/TestFix/no_change.json
@@ -0,0 +1,3 @@
+{
+   "/mod/golang.org/x/vulndb/cmd/vulnreport": true
+}
diff --git a/cmd/vulnreport/testdata/pkgsite/TestLint/found_lints.json b/cmd/vulnreport/testdata/pkgsite/TestLint/found_lints.json
@@ -0,0 +1 @@
+{}
diff --git a/cmd/vulnreport/testdata/pkgsite/TestLint/no_lints.json b/cmd/vulnreport/testdata/pkgsite/TestLint/no_lints.json
@@ -0,0 +1 @@
+{}
diff --git a/cmd/vulnreport/testdata/pkgsite/TestOSV/err.json b/cmd/vulnreport/testdata/pkgsite/TestOSV/err.json
@@ -0,0 +1 @@
+{}
diff --git a/cmd/vulnreport/testdata/pkgsite/TestOSV/ok.json b/cmd/vulnreport/testdata/pkgsite/TestOSV/ok.json
@@ -0,0 +1 @@
+{}
diff --git a/cmd/vulnreport/testdata/pkgsite/TestTriage/all.json b/cmd/vulnreport/testdata/pkgsite/TestTriage/all.json
@@ -0,0 +1 @@
+{}
diff --git a/cmd/vulnreport/testdata/pkgsite/TestXref/found_xrefs.json b/cmd/vulnreport/testdata/pkgsite/TestXref/found_xrefs.json
@@ -0,0 +1 @@
+{}
diff --git a/cmd/vulnreport/testdata/pkgsite/TestXref/no_xrefs.json b/cmd/vulnreport/testdata/pkgsite/TestXref/no_xrefs.json
@@ -0,0 +1 @@
+{}
diff --git a/cmd/vulnreport/testdata/repo.txtar b/cmd/vulnreport/testdata/repo.txtar
@@ -6,6 +6,9 @@
 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

diff --git a/internal/cveutils/triage_test.go b/internal/cveutils/triage_test.go
@@ -24,11 +24,7 @@ var usePkgsite = flag.Bool("pkgsite", false, "use pkg.go.dev for tests")
 
 func TestTriageV4CVE(t *testing.T) {
 	ctx := context.Background()
-	cf, err := pkgsite.CacheFile(t)
-	if err != nil {
-		t.Fatal(err)
-	}
-	pc, err := pkgsite.TestClient(t, *usePkgsite, cf)
+	pc, err := pkgsite.TestClient(t, *usePkgsite)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -181,11 +177,7 @@ func TestTriageV4CVE(t *testing.T) {
 
 func TestTriageV5CVE(t *testing.T) {
 	ctx := context.Background()
-	cf, err := pkgsite.CacheFile(t)
-	if err != nil {
-		t.Fatal(err)
-	}
-	pc, err := pkgsite.TestClient(t, *usePkgsite, cf)
+	pc, err := pkgsite.TestClient(t, *usePkgsite)
 	if err != nil {
 		t.Fatal(err)
 	}