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

Invoke MavenResolverExtrator when scanning pom.xml #1028

Merged
merged 11 commits into from
Jun 19, 2024
Merged

Conversation

cuixq
Copy link
Contributor

@cuixq cuixq commented Jun 5, 2024

#35

In this PR, MavenResolverExtrator is invoked when scanning pom.xml to report vulnerabilities in transitive dependencies.
However, the default Maven extractor is still being used with offline mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to avoid import cycle :(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense also. the OSV-Scanner ErrAPIFailed seems like it should be kept just to OSV-specific API calls.

@cuixq cuixq marked this pull request as draft June 5, 2024 10:26
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 11 lines in your changes missing coverage. Please review.

Project coverage is 65.30%. Comparing base (ace9154) to head (5e603d7).

Files Patch % Lines
pkg/osvscanner/osvscanner.go 70.96% 6 Missing and 3 partials ⚠️
internal/resolution/datasource/maven_registry.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1028      +/-   ##
==========================================
+ Coverage   65.28%   65.30%   +0.01%     
==========================================
  Files         150      150              
  Lines       12498    12525      +27     
==========================================
+ Hits         8159     8179      +20     
- Misses       3879     3882       +3     
- Partials      460      464       +4     

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

@cuixq cuixq marked this pull request as ready for review June 6, 2024 05:23
Copy link
Collaborator

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWESOME!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense also. the OSV-Scanner ErrAPIFailed seems like it should be kept just to OSV-specific API calls.

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 7, 2024

sorry, after posting my review and seeing all my comments together it's very clear they're all just a single comment 😅:

comparing locally is different from comparing offline - only the latter requires the former; we should continue checking if we're offline, not if we're comparing locally

@cuixq
Copy link
Contributor Author

cuixq commented Jun 14, 2024

@another-rex @G-Rath PTAL- thanks!

Copy link
Collaborator

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but are your concerns addressed @G-Rath ?

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though the test packages seem to have a lot of vulnerabilities though, can we pick one with only a few vulns?

@cuixq
Copy link
Contributor Author

cuixq commented Jun 19, 2024

I updated the fixtures with the example in the blog post.

@cuixq cuixq merged commit 27db6bf into google:main Jun 19, 2024
13 checks passed
@cuixq cuixq deleted the extractor branch June 19, 2024 01:29
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.

5 participants