Update all non-major dependencies #4401
Conversation
IOhacker
force-pushed
the
develop
branch
3 times, most recently
from
f03ea13 to
908d5f0
Compare
|
@adamsaghy take a look please |
There was a problem hiding this comment.
The changes are upgrading multiple dependencies - with point upgrades. As the release manager, I want to make sure our release 1.11 is going to pass security scans.
The key thing to verify is whether the tests pass and then trusting that the tests are sufficient coverage for functionality. I think also trusting that deprecated functions in these libraries are still supported and will throw warnings in the log when used in code rather than break the application.
A sanity check is needed on this - not sure if our automated tools can detect problems but worth trying to see what code behaviors are problematic when we do a lot of such dependency upgrades.
|
@Ádám Sághy ***@***.***> @james Dailey ***@***.***>
I have been trying to find the locations of the errors:
Execution failed for task ':fineract-e2e-tests-core:test'.
Execution failed for task ':fineract-e2e-tests-runner:test'.
But I have been unable to detect where the issue is.
Regards
Victor
El mar, 4 mar 2025 a las 10:16, James D ***@***.***>)
escribió:
… ***@***.**** commented on this pull request.
The changes are upgrading multiple dependencies - with point upgrades. As
the release manager, I want to make sure our release 1.11 is going to pass
security scans.
The key thing to verify is whether the tests pass and then trusting that
the tests are sufficient coverage for functionality. I think also trusting
that deprecated functions in these libraries are still supported and will
throw warnings in the log when used in code rather than break the
application.
A sanity check is needed on this - not sure if our automated tools can
detect problems but worth trying to see what code behaviors are problematic
when we do a lot of such dependency upgrades.
—
Reply to this email directly, view it on GitHub
<#4401 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALD2ZAUG7JLUSFYY7GDS3X32SXGU7AVCNFSM6AAAAABYGAFPWGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNJYGE4DKNBSHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
First, reminder to immediately email any security findings to I understand Docker Scout / Snyk has identified issues and we need to fix these. @IOhacker, I was thinking these issues ware related to dependencies in upstream Docker images, but now I'm realizing these dependencies are Java libraries that need updating. I think we should still proceed with the v1.11.0 release and upgrade dependencies (and fix tests) on develop. I'm not certain this is the best path; we may end up having to ship a v1.11.1 hotfix / patch release depending on what we find. |
| dependency 'org.bouncycastle:bcpkix-jdk15to18:1.79' | ||
| dependency 'org.bouncycastle:bcprov-jdk15to18:1.79' | ||
| dependency 'org.bouncycastle:bcpkix-jdk15to18:1.80' | ||
| dependency 'org.bouncycastle:bcprov-jdk15to18:1.80' | ||
| dependency 'org.bouncycastle:bcprov-jdk15on:1.70' | ||
| dependency 'org.bouncycastle:bcpg-jdk15on:1.70' |
There was a problem hiding this comment.
why keep dependencies on jdk15on and jdk15to18 jars - don't they have the same classes (with some variations) and will therefore clash with each other?
I would recommend dropping all these and using bcpg-jdk18on, bcprov-jdk18on, bcpkix-jdk18on.
bcprov-jdk15on and bcpg-jdk15on 1.70 have security issues and no new jars are being released for jdk15on
Surely, you can accept Java 8 is a minimum these days.
|
I found a similar issue in Apache POI when upgrading to junit 5.12.0. I had to add a test runtime dependency on org.junit.platform:junit-platform-launcher:1.12.0. |
|
@IOhacker Please update the org.junit.jupiter:junit-jupiter-api and org.junit.jupiter:junit-jupiter dependencies as well to use the same: 5.11.3 - > 5.12.0 |
Thanks for highlighting this! To elaborate further, security findings should always be kept private until the release with the fix is out. Luckily, in this case there's no problem: because more often than not an advisory in a dependency does not actually impact the project (because the dependency is not used in a vulnerable way), we don't consider the output of scanning tools such as Docker Scout / Snyk as sensitive in themselves. You can read more about this at https://security.apache.org/report-dependency/ . Of course, upgrading those dependencies to remove the risk (and make the scanners happy) is still a good idea, also from a security perspective - so thanks for working on this!
(this makes sense to me but of course is up to you as a project) |
|
@jdailey @adamsaghy @pjfanning @meonkeys applying the fixes now |
IOhacker
force-pushed
the
develop
branch
5 times, most recently
from
b036896 to
03238dc
Compare
|
@adamsaghy @jdailey @meonkeys I will continue in few hours. Looking at the Quartz jobs failure. |
Description
Update all non-major dependencies
Ignore if these details are present on the associated Apache Fineract JIRA ticket.
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.