From 4ce4f93a92fba791168c16f327d01e9abb86c4c6 Mon Sep 17 00:00:00 2001 From: "Hana (Hyang-Ah) Kim" Date: Thu, 17 Nov 2022 10:07:16 -0500 Subject: [PATCH] gopls/internal/lsp: add gopls.fetch_vulncheck_result This CL changes the signature of View's Vulnerabilities and SetVulnerabilities, to use govulncheck.Result instead of []*govulncheck.Vuln. That allows us to hold extra information about the analysis in addition to the list of vulnerabilities. Also, instead of aliasing x/vuln/exp/govulncheck.Result define our own gopls/internal/govulncheck.Result type so the gopls documentation is less confusing. Change-Id: I7a18da9bf047b9ebed6fc0264b5e0f66c04ba3f3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/451696 Run-TryBot: Hyang-Ah Hana Kim Reviewed-by: Suzy Mueller Reviewed-by: Robert Findley TryBot-Result: Gopher Robot gopls-CI: kokoro --- gopls/doc/commands.md | 20 +++++++++++ gopls/internal/govulncheck/types.go | 12 +++++++ gopls/internal/govulncheck/types_118.go | 7 ++-- gopls/internal/govulncheck/types_not118.go | 7 ---- gopls/internal/lsp/cache/session.go | 2 +- gopls/internal/lsp/cache/view.go | 20 +++++++---- gopls/internal/lsp/command.go | 17 ++++++++-- gopls/internal/lsp/command/command_gen.go | 20 +++++++++++ gopls/internal/lsp/command/interface.go | 6 ++++ gopls/internal/lsp/mod/diagnostics.go | 9 +++-- gopls/internal/lsp/mod/hover.go | 9 +++-- gopls/internal/lsp/source/api_json.go | 7 ++++ gopls/internal/lsp/source/view.go | 4 +-- gopls/internal/regtest/misc/vuln_test.go | 39 ++++++++++++++++++++++ 14 files changed, 150 insertions(+), 29 deletions(-) create mode 100644 gopls/internal/govulncheck/types.go diff --git a/gopls/doc/commands.md b/gopls/doc/commands.md index c942be48fd..b6eee96f88 100644 --- a/gopls/doc/commands.md +++ b/gopls/doc/commands.md @@ -100,6 +100,26 @@ Args: } ``` +### **Get known vulncheck result** +Identifier: `gopls.fetch_vulncheck_result` + +Fetch the result of latest vulnerability check (`govulncheck`). + +Args: + +``` +{ + // The file URI. + "URI": string, +} +``` + +Result: + +``` +map[golang.org/x/tools/gopls/internal/lsp/protocol.DocumentURI]*golang.org/x/tools/gopls/internal/govulncheck.Result +``` + ### **Toggle gc_details** Identifier: `gopls.gc_details` diff --git a/gopls/internal/govulncheck/types.go b/gopls/internal/govulncheck/types.go new file mode 100644 index 0000000000..6cf0415a24 --- /dev/null +++ b/gopls/internal/govulncheck/types.go @@ -0,0 +1,12 @@ +// Copyright 2022 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 govulncheck + +// Result is the result of vulnerability scanning. +type Result struct { + // Vulns contains all vulnerabilities that are called or imported by + // the analyzed module. + Vulns []*Vuln +} diff --git a/gopls/internal/govulncheck/types_118.go b/gopls/internal/govulncheck/types_118.go index 7410963195..7b354d622a 100644 --- a/gopls/internal/govulncheck/types_118.go +++ b/gopls/internal/govulncheck/types_118.go @@ -8,7 +8,9 @@ // Package govulncheck provides an experimental govulncheck API. package govulncheck -import "golang.org/x/vuln/exp/govulncheck" +import ( + "golang.org/x/vuln/exp/govulncheck" +) var ( // Source reports vulnerabilities that affect the analyzed packages. @@ -22,9 +24,6 @@ type ( // Config is the configuration for Main. Config = govulncheck.Config - // Result is the result of executing Source. - Result = govulncheck.Result - // Vuln represents a single OSV entry. Vuln = govulncheck.Vuln diff --git a/gopls/internal/govulncheck/types_not118.go b/gopls/internal/govulncheck/types_not118.go index ae92caa5ea..faf5a7055b 100644 --- a/gopls/internal/govulncheck/types_not118.go +++ b/gopls/internal/govulncheck/types_not118.go @@ -13,13 +13,6 @@ import ( "golang.org/x/vuln/osv" ) -// Result is the result of executing Source or Binary. -type Result struct { - // Vulns contains all vulnerabilities that are called or imported by - // the analyzed module. - Vulns []*Vuln -} - // Vuln represents a single OSV entry. type Vuln struct { // OSV contains all data from the OSV entry for this vulnerability. diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go index 9b4355331a..4916f4235f 100644 --- a/gopls/internal/lsp/cache/session.go +++ b/gopls/internal/lsp/cache/session.go @@ -239,7 +239,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, name: name, folder: folder, moduleUpgrades: map[span.URI]map[string]string{}, - vulns: map[span.URI][]*govulncheck.Vuln{}, + vulns: map[span.URI]*govulncheck.Result{}, filesByURI: map[span.URI]*fileBase{}, filesByBase: map[string][]*fileBase{}, rootURI: root, diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go index f373c00508..8198353eca 100644 --- a/gopls/internal/lsp/cache/view.go +++ b/gopls/internal/lsp/cache/view.go @@ -61,7 +61,7 @@ type View struct { // Each modfile has a map of module name to upgrade version. moduleUpgrades map[span.URI]map[string]string - vulns map[span.URI][]*govulncheck.Vuln + vulns map[span.URI]*govulncheck.Result // keep track of files by uri and by basename, a single file may be mapped // to multiple uris, and the same basename may map to multiple files @@ -1044,16 +1044,24 @@ func (v *View) ClearModuleUpgrades(modfile span.URI) { delete(v.moduleUpgrades, modfile) } -func (v *View) Vulnerabilities(modfile span.URI) []*govulncheck.Vuln { +func (v *View) Vulnerabilities(modfile ...span.URI) map[span.URI]*govulncheck.Result { + m := make(map[span.URI]*govulncheck.Result) v.mu.Lock() defer v.mu.Unlock() - vulns := make([]*govulncheck.Vuln, len(v.vulns[modfile])) - copy(vulns, v.vulns[modfile]) - return vulns + if len(modfile) == 0 { + for k, v := range v.vulns { + m[k] = v + } + return m + } + for _, f := range modfile { + m[f] = v.vulns[f] + } + return m } -func (v *View) SetVulnerabilities(modfile span.URI, vulns []*govulncheck.Vuln) { +func (v *View) SetVulnerabilities(modfile span.URI, vulns *govulncheck.Result) { v.mu.Lock() defer v.mu.Unlock() diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go index 28cf3471a7..90b8408dca 100644 --- a/gopls/internal/lsp/command.go +++ b/gopls/internal/lsp/command.go @@ -833,6 +833,17 @@ type pkgLoadConfig struct { Tests bool } +func (c *commandHandler) FetchVulncheckResult(ctx context.Context, arg command.URIArg) (map[protocol.DocumentURI]*govulncheck.Result, error) { + ret := map[protocol.DocumentURI]*govulncheck.Result{} + err := c.run(ctx, commandConfig{forURI: arg.URI}, func(ctx context.Context, deps commandDeps) error { + for modfile, result := range deps.snapshot.View().Vulnerabilities() { + ret[protocol.URIFromSpanURI(modfile)] = result + } + return nil + }) + return ret, err +} + func (c *commandHandler) RunVulncheckExp(ctx context.Context, args command.VulncheckArgs) error { if args.URI == "" { return errors.New("VulncheckArgs is missing URI field") @@ -887,10 +898,10 @@ func (c *commandHandler) RunVulncheckExp(ctx context.Context, args command.Vulnc // TODO: for easy debugging, log the failed stdout somewhere? return fmt.Errorf("failed to parse govulncheck output: %v", err) } - vulns := result.Vulns - deps.snapshot.View().SetVulnerabilities(args.URI.SpanURI(), vulns) - c.s.diagnoseSnapshot(deps.snapshot, nil, false) + deps.snapshot.View().SetVulnerabilities(args.URI.SpanURI(), &result) + c.s.diagnoseSnapshot(deps.snapshot, nil, false) + vulns := result.Vulns affecting := make([]string, 0, len(vulns)) for _, v := range vulns { if v.IsCalled() { diff --git a/gopls/internal/lsp/command/command_gen.go b/gopls/internal/lsp/command/command_gen.go index 615f9509c8..5111e673ee 100644 --- a/gopls/internal/lsp/command/command_gen.go +++ b/gopls/internal/lsp/command/command_gen.go @@ -24,6 +24,7 @@ const ( ApplyFix Command = "apply_fix" CheckUpgrades Command = "check_upgrades" EditGoDirective Command = "edit_go_directive" + FetchVulncheckResult Command = "fetch_vulncheck_result" GCDetails Command = "gc_details" Generate Command = "generate" GenerateGoplsMod Command = "generate_gopls_mod" @@ -50,6 +51,7 @@ var Commands = []Command{ ApplyFix, CheckUpgrades, EditGoDirective, + FetchVulncheckResult, GCDetails, Generate, GenerateGoplsMod, @@ -102,6 +104,12 @@ func Dispatch(ctx context.Context, params *protocol.ExecuteCommandParams, s Inte return nil, err } return nil, s.EditGoDirective(ctx, a0) + case "gopls.fetch_vulncheck_result": + var a0 URIArg + if err := UnmarshalArgs(params.Arguments, &a0); err != nil { + return nil, err + } + return s.FetchVulncheckResult(ctx, a0) case "gopls.gc_details": var a0 protocol.DocumentURI if err := UnmarshalArgs(params.Arguments, &a0); err != nil { @@ -276,6 +284,18 @@ func NewEditGoDirectiveCommand(title string, a0 EditGoDirectiveArgs) (protocol.C }, nil } +func NewFetchVulncheckResultCommand(title string, a0 URIArg) (protocol.Command, error) { + args, err := MarshalArgs(a0) + if err != nil { + return protocol.Command{}, err + } + return protocol.Command{ + Title: title, + Command: "gopls.fetch_vulncheck_result", + Arguments: args, + }, nil +} + func NewGCDetailsCommand(title string, a0 protocol.DocumentURI) (protocol.Command, error) { args, err := MarshalArgs(a0) if err != nil { diff --git a/gopls/internal/lsp/command/interface.go b/gopls/internal/lsp/command/interface.go index 23b9f65504..e6ab13da47 100644 --- a/gopls/internal/lsp/command/interface.go +++ b/gopls/internal/lsp/command/interface.go @@ -17,6 +17,7 @@ package command import ( "context" + "golang.org/x/tools/gopls/internal/govulncheck" "golang.org/x/tools/gopls/internal/lsp/protocol" ) @@ -153,6 +154,11 @@ type Interface interface { // // Run vulnerability check (`govulncheck`). RunVulncheckExp(context.Context, VulncheckArgs) error + + // FetchVulncheckResult: Get known vulncheck result + // + // Fetch the result of latest vulnerability check (`govulncheck`). + FetchVulncheckResult(context.Context, URIArg) (map[protocol.DocumentURI]*govulncheck.Result, error) } type RunTestsArgs struct { diff --git a/gopls/internal/lsp/mod/diagnostics.go b/gopls/internal/lsp/mod/diagnostics.go index 3b74bd8835..ec0993fc9e 100644 --- a/gopls/internal/lsp/mod/diagnostics.go +++ b/gopls/internal/lsp/mod/diagnostics.go @@ -180,14 +180,17 @@ func ModVulnerabilityDiagnostics(ctx context.Context, snapshot source.Snapshot, return nil, err } - vs := snapshot.View().Vulnerabilities(fh.URI()) - // TODO(suzmue): should we just store the vulnerabilities like this? + vs := snapshot.View().Vulnerabilities(fh.URI())[fh.URI()] + if vs == nil || len(vs.Vulns) == 0 { + return nil, nil + } + type modVuln struct { mod *govulncheck.Module vuln *govulncheck.Vuln } vulnsByModule := make(map[string][]modVuln) - for _, vuln := range vs { + for _, vuln := range vs.Vulns { for _, mod := range vuln.Modules { vulnsByModule[mod.Path] = append(vulnsByModule[mod.Path], modVuln{mod, vuln}) } diff --git a/gopls/internal/lsp/mod/hover.go b/gopls/internal/lsp/mod/hover.go index 520a029924..7706262fbf 100644 --- a/gopls/internal/lsp/mod/hover.go +++ b/gopls/internal/lsp/mod/hover.go @@ -71,7 +71,7 @@ func Hover(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, } // Get the vulnerability info. - affecting, nonaffecting := lookupVulns(snapshot.View().Vulnerabilities(fh.URI()), req.Mod.Path) + affecting, nonaffecting := lookupVulns(snapshot.View().Vulnerabilities(fh.URI())[fh.URI()], req.Mod.Path) // Get the `go mod why` results for the given file. why, err := snapshot.ModWhy(ctx, fh) @@ -117,8 +117,11 @@ func formatHeader(modpath string, options *source.Options) string { return b.String() } -func lookupVulns(vulns []*govulncheck.Vuln, modpath string) (affecting, nonaffecting []*govulncheck.Vuln) { - for _, vuln := range vulns { +func lookupVulns(vulns *govulncheck.Result, modpath string) (affecting, nonaffecting []*govulncheck.Vuln) { + if vulns == nil { + return nil, nil + } + for _, vuln := range vulns.Vulns { for _, mod := range vuln.Modules { if mod.Path != modpath { continue diff --git a/gopls/internal/lsp/source/api_json.go b/gopls/internal/lsp/source/api_json.go index e283c3b35c..fdc46486b1 100755 --- a/gopls/internal/lsp/source/api_json.go +++ b/gopls/internal/lsp/source/api_json.go @@ -689,6 +689,13 @@ var GeneratedAPIJSON = &APIJSON{ Doc: "Runs `go mod edit -go=version` for a module.", ArgDoc: "{\n\t// Any document URI within the relevant module.\n\t\"URI\": string,\n\t// The version to pass to `go mod edit -go`.\n\t\"Version\": string,\n}", }, + { + Command: "gopls.fetch_vulncheck_result", + Title: "Get known vulncheck result", + Doc: "Fetch the result of latest vulnerability check (`govulncheck`).", + ArgDoc: "{\n\t// The file URI.\n\t\"URI\": string,\n}", + ResultDoc: "map[golang.org/x/tools/gopls/internal/lsp/protocol.DocumentURI]*golang.org/x/tools/gopls/internal/govulncheck.Result", + }, { Command: "gopls.gc_details", Title: "Toggle gc_details", diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index 7466484952..aed3f54938 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -314,11 +314,11 @@ type View interface { // Vulnerabilites returns known vulnerabilities for the given modfile. // TODO(suzmue): replace command.Vuln with a different type, maybe // https://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck/govulnchecklib#Summary? - Vulnerabilities(modfile span.URI) []*govulncheck.Vuln + Vulnerabilities(modfile ...span.URI) map[span.URI]*govulncheck.Result // SetVulnerabilities resets the list of vulnerabilites that exists for the given modules // required by modfile. - SetVulnerabilities(modfile span.URI, vulnerabilities []*govulncheck.Vuln) + SetVulnerabilities(modfile span.URI, vulncheckResult *govulncheck.Result) // FileKind returns the type of a file FileKind(FileHandle) FileKind diff --git a/gopls/internal/regtest/misc/vuln_test.go b/gopls/internal/regtest/misc/vuln_test.go index b511eb7e43..a9587c2f44 100644 --- a/gopls/internal/regtest/misc/vuln_test.go +++ b/gopls/internal/regtest/misc/vuln_test.go @@ -15,6 +15,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "golang.org/x/tools/gopls/internal/govulncheck" "golang.org/x/tools/gopls/internal/lsp/command" "golang.org/x/tools/gopls/internal/lsp/protocol" . "golang.org/x/tools/gopls/internal/lsp/regtest" @@ -180,9 +181,43 @@ func main() { // TODO(hyangah): once the diagnostics are published, wait for diagnostics. ShownMessage("Found GOSTDLIB"), ) + testFetchVulncheckResult(t, env, map[string][]string{"go.mod": {"GOSTDLIB"}}) }) } +func testFetchVulncheckResult(t *testing.T, env *Env, want map[string][]string) { + t.Helper() + + var result map[protocol.DocumentURI]*govulncheck.Result + fetchCmd, err := command.NewFetchVulncheckResultCommand("fetch", command.URIArg{ + URI: env.Sandbox.Workdir.URI("go.mod"), + }) + if err != nil { + t.Fatal(err) + } + env.ExecuteCommand(&protocol.ExecuteCommandParams{ + Command: fetchCmd.Command, + Arguments: fetchCmd.Arguments, + }, &result) + + for _, v := range want { + sort.Strings(v) + } + got := map[string][]string{} + for k, r := range result { + var osv []string + for _, v := range r.Vulns { + osv = append(osv, v.OSV.ID) + } + sort.Strings(osv) + modfile := env.Sandbox.Workdir.RelPath(k.SpanURI().Filename()) + got[modfile] = osv + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("fetch vulnchheck result = got %v, want %v: diff %v", got, want, diff) + } +} + const workspace1 = ` -- go.mod -- module golang.org/entry @@ -332,6 +367,9 @@ func TestRunVulncheckWarning(t *testing.T) { ReadDiagnostics("go.mod", gotDiagnostics), ), ) + testFetchVulncheckResult(t, env, map[string][]string{ + "go.mod": {"GO-2022-01", "GO-2022-02", "GO-2022-03"}, + }) env.OpenFile("x/x.go") lineX := env.RegexpSearch("x/x.go", `c\.C1\(\)\.Vuln1\(\)`) env.OpenFile("y/y.go") @@ -470,6 +508,7 @@ func TestRunVulncheckInfo(t *testing.T) { ReadDiagnostics("go.mod", gotDiagnostics)), ShownMessage("No vulnerabilities found")) // only count affecting vulnerabilities. + testFetchVulncheckResult(t, env, map[string][]string{"go.mod": {"GO-2022-02"}}) // wantDiagnostics maps a module path in the require // section of a go.mod to diagnostics that will be returned // when running vulncheck.