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 <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
diff --git a/vulncheck/binary_test.go b/vulncheck/binary_test.go
index 3ff589f..63c090f 100644
--- a/vulncheck/binary_test.go
+++ b/vulncheck/binary_test.go
@@ -81,9 +81,6 @@
})
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
diff --git a/vulncheck/fetch.go b/vulncheck/fetch.go
index fe9781e..bc8cce2 100644
--- a/vulncheck/fetch.go
+++ b/vulncheck/fetch.go
@@ -7,10 +7,6 @@
import (
"context"
"fmt"
- "go/build"
- "os"
- "path/filepath"
- "strings"
"golang.org/x/vuln/client"
)
@@ -66,12 +62,6 @@
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 @@
}
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
-}
diff --git a/vulncheck/fetch_test.go b/vulncheck/fetch_test.go
index c52b5be..da7fd88 100644
--- a/vulncheck/fetch_test.go
+++ b/vulncheck/fetch_test.go
@@ -23,10 +23,10 @@
}
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)
@@ -34,19 +34,19 @@
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"}}}}}}},
},
diff --git a/vulncheck/source_test.go b/vulncheck/source_test.go
index ed25385..6d65706 100644
--- a/vulncheck/source_test.go
+++ b/vulncheck/source_test.go
@@ -96,8 +96,6 @@
})
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 @@
})
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 @@
},
}
- // 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 @@
},
}
- // 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 {
