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: Add dependencyManagement exclusions to the child exclusions #6969

Merged
merged 2 commits into from
Jul 9, 2024

Conversation

coheigea
Copy link
Contributor

@coheigea coheigea commented Jun 19, 2024

I noticed that if I have a child pom with a dependency with some exclusions, and a parent pom with a dependencyManagement section with the same dependency with different exclusions, then Trivy only uses the child exclusions.

This is not the behaviour that the maven command line uses, the mvn dependency:tree combines the set of exclusions in this case.

To reproduce unzip "trivy.zip" attached and run:

 mvn dependency:tree | grep jettison

This will return nothing as jettison is excluded in the parent pom. Now run:

 trivy fs . | grep jettison

and see it returns findings in jettison even though it's not on the classpath

@CLAassistant
Copy link

CLAassistant commented Jun 19, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coheigea coheigea force-pushed the coheigea/exclusions branch from 75b02ab to 2bac371 Compare June 19, 2024 14:18
@coheigea
Copy link
Contributor Author

trivy.zip

@coheigea coheigea changed the title Add dependencyManagement exclusions to the child exclusions fix: Add dependencyManagement exclusions to the child exclusions Jun 19, 2024
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

hello @coheigea
Thanks for your work!

Can you create new test for these changes?

Regards, Dmitriy

@coheigea coheigea force-pushed the coheigea/exclusions branch 2 times, most recently from 81492e4 to af516ff Compare June 20, 2024 08:24
@coheigea
Copy link
Contributor Author

@DmitriyLewen How do I run the tests...even on main, I'm getting failures with:

go test ./pkg/dependency/parser/java/pom

@DmitriyLewen
Copy link
Contributor

@coheigea coheigea force-pushed the coheigea/exclusions branch from af516ff to d6179cb Compare June 20, 2024 12:49
@coheigea
Copy link
Contributor Author

@DmitriyLewen I added a test, but I'm having trouble running the tests locally even with mage, can you enable the workflow please?

@coheigea coheigea force-pushed the coheigea/exclusions branch from d6179cb to 8c1aa26 Compare June 20, 2024 12:55
@DmitriyLewen
Copy link
Contributor

it should work

cd ./pkg/dependency/parser/java/pom 
➜  go test -run "TestPom_Parse"   
PASS
ok      github.com/aquasecurity/trivy/pkg/dependency/parser/java/pom    0.039s

@coheigea coheigea force-pushed the coheigea/exclusions branch from 8c1aa26 to 7092c74 Compare June 21, 2024 06:42
@coheigea
Copy link
Contributor Author

Please re-run the tests

@coheigea
Copy link
Contributor Author

All checks passed 👍

@DmitriyLewen
Copy link
Contributor

DmitriyLewen commented Jun 24, 2024

@coheigea I changed your test to show that mvn merges exceptions from base pom and parent (your test showed that mvn takes exceptions from parent, but didn't show that exceptions from the base pom are also used).
Can you take a look?

@coheigea
Copy link
Contributor Author

coheigea commented Jul 8, 2024

Yes it's fine thanks! @DmitriyLewen

Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

@coheigea Thanks for your work!

LGTM

@knqyf263 take a look, when you have time, please

@knqyf263 knqyf263 added this pull request to the merge queue Jul 9, 2024
Merged via the queue into aquasecurity:main with commit dc68a66 Jul 9, 2024
12 checks passed
@aqua-bot aqua-bot mentioned this pull request Jul 9, 2024
@coheigea coheigea deleted the coheigea/exclusions branch July 9, 2024 09:07
skahn007gl pushed a commit to skahn007gl/trivy that referenced this pull request Jul 23, 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.

4 participants