vulncheck: remove isLocal check from fetchVulnerabilities

isLocal check was added to improve efficiency by avoiding
fetch of data that's not going to be used. Version info is
inaccurate or unavailable for modules that are in writable
local directories so vuln check for those modules are skipped
anyway.

With the check, fetchVulnerabilities excludes vulnerabilities
for modules if their source files are outside the module cache.
The location of the module cache was determined by querying
GOMODCACHE and GOPATH environment variables of the govulncheck
process. That worked well for govulncheck when it is used
for source scanning.

The logic was copied to vulncheck API internal. However,
relying on process's GOMODCACHE/GOPATH environment variables
limit the API's utility. For example, Gopls may use different
GOMODCACHE/GOPATH for each workspace it's processing and they
can be different from the Gopls's own GOMODCACHE/GOPATH env vars.
Test data can be loaded with a fake GOMODCACHE that's different
from the GOMODCACHE env var of the test process.

There was an escape flag to skip this check to work with
the test environment where the module cache and GOPATH
are different from the test process's. But that is unexported;
external packages cannot utilize it and that prevents
writing tests from external packages.

This CL proposes to remove the isLocal check. There is already
a cache that reduces volume of data fetch over network, and
vulncheck can potentially address the efficiency issue in different
ways. Users and applications that need to exclude
vulnerabilities of local modules, may utilize
golang.org/x/vuln/client.Client
and implement filtering from GetByModule. Or, if this problem
is common, we may consider an explicit setting in the
vulncheck.Config.

Change-Id: Iced93351b91a00fdc623a6d1c3076da86fbe2c70
Reviewed-on: https://go-review.googlesource.com/c/exp/+/391914
Trust: Hyang-Ah Hana Kim <[email protected]>
Run-TryBot: Hyang-Ah Hana Kim <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Zvonimir Pavlinovic <[email protected]>

Author: hyangah

vulncheck/binary_test.go

@@ -81,9 +81,6 @@ func TestBinary(t *testing.T) {
 	})
 	defer e.Cleanup()
 
-	// Make sure local vulns can be loaded.
-	fetchingInTesting = true
-
 	cmd := exec.Command("go", "build")
 	cmd.Dir = e.Config.Dir
 	cmd.Env = e.Config.Env

vulncheck/fetch.go

@@ -7,10 +7,6 @@ package vulncheck
 import (
 	"context"
 	"fmt"
-	"go/build"
-	"os"
-	"path/filepath"
-	"strings"
 
 	"golang.org/x/vuln/client"
 )
@@ -66,12 +62,6 @@ func fetchVulnerabilities(ctx context.Context, client client.Client, modules []*
 			modPath = mod.Replace.Path
 		}
 
-		// skip loading vulns for local imports
-		if isLocal(mod) {
-			// TODO: what if client has its own db
-			// with local vulns?
-			continue
-		}
 		vulns, err := client.GetByModule(ctx, modPath)
 		if err != nil {
 			return nil, err
@@ -86,29 +76,3 @@ func fetchVulnerabilities(ctx context.Context, client client.Client, modules []*
 	}
 	return mv, nil
 }
-
-// fetchingInTesting is a flag used to avoid skipping
-// loading local vulnerabilities in testing.
-var fetchingInTesting bool = false
-
-func isLocal(mod *Module) bool {
-	if fetchingInTesting {
-		return false
-	}
-	modDir := mod.Dir
-	if mod.Replace != nil {
-		modDir = mod.Replace.Dir
-	}
-	return modDir != "" && !strings.HasPrefix(modDir, modCacheDirectory())
-}
-func modCacheDirectory() string {
-	var modCacheDir string
-	// TODO: define modCacheDir using something similar to cmd/go/internal/cfg.GOMODCACHE?
-	if modCacheDir = os.Getenv("GOMODCACHE"); modCacheDir == "" {
-		if modCacheDir = os.Getenv("GOPATH"); modCacheDir == "" {
-			modCacheDir = build.Default.GOPATH
-		}
-		modCacheDir = filepath.Join(modCacheDir, "pkg", "mod")
-	}
-	return modCacheDir
-}

vulncheck/fetch_test.go

@@ -23,30 +23,30 @@ func TestFetchVulnerabilities(t *testing.T) {
 	}
 
 	mv, err := fetchVulnerabilities(context.Background(), mc, []*Module{
-		{Path: "example.mod/a", Dir: modCacheDirectory(), Version: "v1.0.0"},
-		{Path: "example.mod/b", Dir: modCacheDirectory(), Version: "v1.0.4"},
-		{Path: "example.mod/c", Replace: &Module{Path: "example.mod/d", Dir: modCacheDirectory(), Version: "v1.0.0"}, Version: "v2.0.0"},
-		{Path: "example.mod/e", Replace: &Module{Path: "../local/example.mod/d", Dir: modCacheDirectory(), Version: "v1.0.1"}, Version: "v2.1.0"},
+		{Path: "example.mod/a", Version: "v1.0.0"},
+		{Path: "example.mod/b", Version: "v1.0.4"},
+		{Path: "example.mod/c", Replace: &Module{Path: "example.mod/d", Version: "v1.0.0"}, Version: "v2.0.0"},
+		{Path: "example.mod/e", Replace: &Module{Path: "../local/example.mod/d", Version: "v1.0.1"}, Version: "v2.1.0"},
 	})
 	if err != nil {
 		t.Fatalf("FetchVulnerabilities failed: %s", err)
 	}
 
 	expected := moduleVulnerabilities{
 		{
-			mod: &Module{Path: "example.mod/a", Dir: modCacheDirectory(), Version: "v1.0.0"},
+			mod: &Module{Path: "example.mod/a", Version: "v1.0.0"},
 			vulns: []*osv.Entry{
 				{ID: "a", Affected: []osv.Affected{{Package: osv.Package{Name: "example.mod/a"}, Ranges: osv.Affects{{Type: osv.TypeSemver, Events: []osv.RangeEvent{{Fixed: "2.0.0"}}}}}}},
 			},
 		},
 		{
-			mod: &Module{Path: "example.mod/b", Dir: modCacheDirectory(), Version: "v1.0.4"},
+			mod: &Module{Path: "example.mod/b", Version: "v1.0.4"},
 			vulns: []*osv.Entry{
 				{ID: "b", Affected: []osv.Affected{{Package: osv.Package{Name: "example.mod/b"}, Ranges: osv.Affects{{Type: osv.TypeSemver, Events: []osv.RangeEvent{{Fixed: "1.1.1"}}}}}}},
 			},
 		},
 		{
-			mod: &Module{Path: "example.mod/c", Replace: &Module{Path: "example.mod/d", Dir: modCacheDirectory(), Version: "v1.0.0"}, Version: "v2.0.0"},
+			mod: &Module{Path: "example.mod/c", Replace: &Module{Path: "example.mod/d", Version: "v1.0.0"}, Version: "v2.0.0"},
 			vulns: []*osv.Entry{
 				{ID: "c", Affected: []osv.Affected{{Package: osv.Package{Name: "example.mod/d"}, Ranges: osv.Affects{{Type: osv.TypeSemver, Events: []osv.RangeEvent{{Fixed: "2.0.0"}}}}}}},
 			},

vulncheck/source_test.go

@@ -96,8 +96,6 @@ func TestImportsOnly(t *testing.T) {
 	})
 	defer e.Cleanup()
 
-	// Make sure local vulns can be loaded.
-	fetchingInTesting = true
 	// Load x and y as entry packages.
 	pkgs, err := loadPackages(e, path.Join(e.Temp(), "entry/x"), path.Join(e.Temp(), "entry/y"))
 	if err != nil {
@@ -341,8 +339,6 @@ func TestCallGraph(t *testing.T) {
 	})
 	defer e.Cleanup()
 
-	// Make sure local vulns can be loaded.
-	fetchingInTesting = true
 	// Load x and y as entry packages.
 	pkgs, err := loadPackages(e, path.Join(e.Temp(), "entry/x"), path.Join(e.Temp(), "entry/y"))
 	if err != nil {
@@ -440,8 +436,6 @@ func TestFiltering(t *testing.T) {
 		},
 	}
 
-	// Make sure local vulns can be loaded.
-	fetchingInTesting = true
 	// Load x as entry package.
 	pkgs, err := loadPackages(e, path.Join(e.Temp(), "entry/x"))
 	if err != nil {
@@ -527,8 +521,6 @@ func TestAllSymbolsVulnerable(t *testing.T) {
 		},
 	}
 
-	// Make sure local vulns can be loaded.
-	fetchingInTesting = true
 	// Load x as entry package.
 	pkgs, err := loadPackages(e, path.Join(e.Temp(), "entry/x"))
 	if err != nil {