Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(vulnerable-code): Fix search for Go package vulnerabilities #9299

Conversation

wkl3nk
Copy link
Contributor

@wkl3nk wkl3nk commented Oct 17, 2024

For Go packages, both the namespace and name may contain path segments separated by a "/" character. The purl specification requires these "/" characters to be percent-encoded in the namespace and name components of a purl.
The VulnerableCode bulk-search API is unable to handle these percent-encoded "/" characters, resulting in no vulnerability records being returned.
This bugfix decodes any percent-encoded "/" characters just before making the VulnerableCode query to ensure proper functionality.

Fixes #9298

@sschuberth
Copy link
Member

sschuberth commented Oct 17, 2024

Fixes #9298

Nit: Missing period at the end of the sentence.

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.49%. Comparing base (ed4bccf) to head (6f5a942).
Report is 66 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9299      +/-   ##
============================================
+ Coverage     66.29%   67.49%   +1.19%     
+ Complexity     1201     1200       -1     
============================================
  Files           239      241       +2     
  Lines          8446     8493      +47     
  Branches        905      899       -6     
============================================
+ Hits           5599     5732     +133     
+ Misses         2478     2399      -79     
+ Partials        369      362       -7     
Flag Coverage Δ
funTest-docker 60.54% <ø> (+0.47%) ⬆️
funTest-non-docker 33.63% <ø> (-1.08%) ⬇️
test 37.47% <ø> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wkl3nk wkl3nk force-pushed the wkl3nk/fix-go-vulnerability-search branch 3 times, most recently from 2ce18f6 to c7861a8 Compare October 18, 2024 07:22
@sschuberth
Copy link
Member

@wkl3nk could you try if #9304 instead would also fix your issue?

@wkl3nk wkl3nk force-pushed the wkl3nk/fix-go-vulnerability-search branch from c7861a8 to e82d9aa Compare October 18, 2024 14:58
@wkl3nk wkl3nk force-pushed the wkl3nk/fix-go-vulnerability-search branch from e82d9aa to b4587a3 Compare October 18, 2024 15:05
@wkl3nk
Copy link
Contributor Author

wkl3nk commented Oct 18, 2024

Could you try if #9304 instead would also fix your issue?

This is about namespace segments?
It probably would, but from what I have seen in ort-result.zip

id: "Go::github.com/quic-go/quic-go:0.40.0"
purl: "pkg:golang/github.com%2Fquic-go%[email protected]"

I ask myself if there is not a general problem in detecting namespace and name for Go libraries.
Would not the following be more correct:

id: "Go:github.com/quic-go:quic-go:0.40.0"
purl: "pkg:golang/github.com/quic-go/[email protected]"

Because the github.com home of quic-go is https://github.com/quic-go and quic-go is just one of the subprojects there.

@sschuberth
Copy link
Member

Yes, that's exactly what my PR is about. But could be that there's still something wrong in my implementation.

@wkl3nk
Copy link
Contributor Author

wkl3nk commented Oct 21, 2024

Possibly I did not know enough when I wroted the posting on Friday. Because I am not a Go programmer, I don't even know if there exists a concept of namespaces. Nevertheless I proposed to use the namespace, based on some directory structure on a github.com account. I don't know if this is valid, or if it is better to say something like "There are no namespaces in Go". Don't know.

@sschuberth
Copy link
Member

I don't know if this is valid, or if it is better to say something like "There are no namespaces in Go".

You may want to refer to this comment.

@wkl3nk wkl3nk marked this pull request as ready for review October 22, 2024 06:47
@wkl3nk wkl3nk requested a review from a team as a code owner October 22, 2024 06:47
@sschuberth
Copy link
Member

I still believe we should solve this differently, in a more generic way. While Go apparently does not have namespaces, the purl standard treats them as if Go had namespaces. I'm currently preparing a fix for that.

@fviernau
Copy link
Member

fviernau commented Oct 22, 2024

I'm currently preparing a fix for that.

Related OSV code locations:

@sschuberth
Copy link
Member

In any case, @wkl3nk as mentioned here please add a test for this (in a new commit that could easily be cherry-picked) so we can verify the fix also for alternative solutions.

For Go packages, both the namespace and name may contain path
segments separated by a "/" character. The purl specification
requires these "/" characters to be percent-encoded in the
namespace and name components of a purl.

The VulnerableCode bulk-search API is unable to handle these
percent-encoded "/" characters, resulting in no vulnerability
records being returned.

This bugfix decodes any percent-encoded "/" characters just
before making the VulnerableCode query to ensure proper
functionality.

Fixes oss-review-toolkit#9298.

Signed-off-by: Wolfgang Klenk <[email protected]>
@wkl3nk wkl3nk force-pushed the wkl3nk/fix-go-vulnerability-search branch from b4587a3 to 6f5a942 Compare October 23, 2024 15:19
@wkl3nk
Copy link
Contributor Author

wkl3nk commented Oct 23, 2024

@sschuberth I added a test cases, hoping that this is what you had in mind. It will start to fail once the slashes in the purl name section are no longer percent-encoded. Once this test fails, this commit can be cherry-picked and removed.

@sschuberth
Copy link
Member

I added a test cases, hoping that this is what you had in mind.

Hmm, not really. I was hoping for a funTest that queries VC for a Go package which is known to have vulnerabilities. When run against main, the test would currently fail as the expected vulnerability is not found. But once there is a fix in place to produce the correct purl, the test would succeed.

@sschuberth
Copy link
Member

I added a test cases, hoping that this is what you had in mind.

Hmm, not really. I was hoping for a funTest that queries VC for a Go package which is known to have vulnerabilities.

Nevermind @wkl3nk, I'll now add such a test myself as I'm close to proposing a more generic fix.

@sschuberth
Copy link
Member

Closing in favor of the more generic solution in #9330.

@sschuberth sschuberth closed this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VulnerableCode returns no findings for Go packages
3 participants