govulncheck: make output less verbose#614
Conversation
mckn
left a comment
mckn
left a comment
There was a problem hiding this comment.
Looks great! Added some small nits to consider.
| called[id].fixedVersion = msg.Finding.FixedVersion | ||
| } | ||
| if called[id].module == "" { | ||
| called[id].module = firstModule(msg.Finding.Trace) |
There was a problem hiding this comment.
There might be a bug in the "firstModule" function according to this:
According to the govulncheck JSON spec, Finding.Trace for source-mode is a call stack where trace[0] is the user's entry point and subsequent frames descend into the vulnerable dependency. So firstModule(msg.Finding.Trace) returns the user's own module, not the vulnerable dependency.
For example, for a plugin calling into golang.org/x/net, the trace is:
trace[0]: {module: "github.com/user/myplugin", function: "main"}
trace[1]: {module: "golang.org/x/net", function: "vulnerableFunc"}firstModule returns "github.com/user/myplugin", and the output becomes:
Update the following dependencies:
• github.com/user/myplugin v1.2.3 (GO-2024-AAAA) ← wrong
The fix should look at the last non-empty module in the trace (or specifically the frame that has Position == nil, which is the vulnerable symbol frame). The test data in sampleNDJSON happens to use the same module for all frames, so the unit test passes but doesn't catch this.
There was a problem hiding this comment.
good catch on the test, the fixture used the same module on every frame, so it didn't actually check which end firstModule reads from. updated in bee6cb5
the Finding.Trace doc in (govulncheck.go#L154-L156) states:
Frames are sorted starting from the imported vulnerable symbol until the entry point. The first frame in Frames should match Symbol.
it seems like trace[0] is the vulnerable dependency and the entry point is last. govulncheck's own renderer reads Trace[0].Module as the affected module (text.go#L279).
|
|
||
| type vulnInfo struct { | ||
| id string | ||
| summary string |
There was a problem hiding this comment.
Are we ever using this field? I can see that we set it but I can't find any usages of reading it except in tests.
| if sourceFindings[id] == nil { | ||
| sourceFindings[id] = info | ||
| } |
There was a problem hiding this comment.
Should we set this not only based on if it is nil but also set it when semver.Compare is greater than the previous set one so we always use the newest.
3 participants
Example output: