-
Notifications
You must be signed in to change notification settings - Fork 518
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
Updated several (transitive) dependencies (OFBIZ-13123) #819
base: trunk
Are you sure you want to change the base?
Conversation
@JacquesLeRoux Feel free to review |
Thanks @dtrunk90, That's quite a help, much appreciated 👍 I'll review and test all that |
Hi Dan, You say
But there are currently no errors in trunk: https://nightlies.apache.org/ofbiz/trunk/checkstyle.html |
Hey, weird. For me there were some about simplifying expressions. And after upgrading the checkstyle toolVersion there were some more. |
Ah, maybe it's the reason... Will check that... Anyway so far all your fixes seems good to me ;) |
Sounds great. More to come. I'm planning on updating Tomcat to v10 and do all the javax to jakarta namespace changes so libraries depending on jakarta namespace can be updated too and we're able to use Spring 6 by then. Do you want me to put them in this PR or do another one after this got merged (because the changes will affect loads of files and could cause conflicts)? |
Please create a new PR, TIA |
Aight. I had to force push again because I forgot to add derbytools. Now I was able to boot up OFBiz and click around the backend without anything popping up in the error log. |
Please do not merge this PR or a new version of it until it is checked thorougly. I will have a look at it also after it is cleaned up and tested but not before mid/end of next week. Thanks! |
Also it would be better to separate in another PR the work done for fixing corrections based on Checkstyle errors. That would allow to easily revert one part or another if necessary. |
That's why it's in separate commits. I see no reason in doing another PR just for the checkstyle corrections. |
Hi @dtrunk90, I guess you only tested the framework. We have 2 failures with plugins:
When used with framework only, the last failure does not appear and OFBiz builds and runs. I guess the 2nd failure is only due to the Solr error. I ran the ZAP spider on it, which is not much reliable for finding errors, and got no relevant errors in error.log Disclaimer: I did not review this part yet. I mean I only reviewed the Checkstyle corrections. |
While reverting the patch, I saw that you renamed GroovyScriptTestCase to GroovyScriptAssert and GroovyTestCase to GroovyAssert. Not a big deal, just to say. |
Yes, I've only tested framework. I'll have a look at the plugins later that day. Didn't thought about them tbh. |
Yes, because Groovy deprecated the GroovyTestCase and refers to GroovyAssert as a replacement. I thought it would be better to make that change clear in the OFBiz class as well. |
I tried it this morning and indeed after 1h15 I got the result. It found 217 vulnerabilities! |
BTW @dtrunk90, the title speaks about transitive dependencies, how do you expect to fix them? If I understand well, they are embedded/used within OFBiz declared dependencies. Adding them in dependencies.gradle would not help, right? |
Actually you can update transitive dependencies: https://docs.gradle.org/current/userguide/dependency_constraints.html I've only updated to the smallest possible version (patch or minor) to not break anything. |
Great, I now understand better why you wanted to keep OWASP dependencies check. |
Hi Danny, For security reason, with https://issues.apache.org/jira/browse/OFBIZ-13124 I have already upgraded Tomcat to 9.0.91 Also if you did not spot it, you might be interested by this dev ML thread: |
No problem. I've dropped the commit. |
…Z-13121)" NVD REST API isn't stable but that shouldn't be the reason to abandon this feature. This reverts commit 0a9ee32.
It's used in the OFBiz codebase so this should be added as a dependency
Quality Gate passedIssues Measures |
Sorry @JacquesLeRoux and @dtrunk90 for the delay. To respond to the questions: there is a lot of dependency changes in this PR which is quite unusual and cannot be compared with the update of some dependency changes now and then like we did in the past. Those dependency changes might cause other transitive dependencies to change and this could lead to breaking changes. I would simply like to take my time to check all this, including to have a look at the changelogs of the libraries changed. They often give hints about deprecations or breaking changes. I am wondering why this PR does not raise more attention, it worries me a bit. I hope that I find some time to check this soon but cannot give a reliable point in time for it. |
Thanks @mbrohl for your clear answer. I agree that for major changes like this one, it's better to have a discussion on dev ML to indeed hopefully get more attention and opinions, the more brains and experiences the better. On the other hand, reviewing all dependencies change logs can take some time. Before we start a discussion on dev ML, @dtrunk90 would that be a problem for you? |
Up to you how you want to handle these changes. Take your time. I've merged my fork already into our projects. So we're good about the tons of CVEs reported by those outdated dependencies. |
Hi @dtrunk90, We have a small conflict here: https://github.com/apache/ofbiz-framework/pull/819/conflicts
is now pointless? TIA |
Improved:
Reverted:
Fixed:
I've updated several (transitive) dependencies. For the transitive dependencies see the
because
clause in their respective constraint.¹ Maven coordinates have changed for Groovy 4+ (see https://groovy-lang.org/releasenotes/groovy-4.0.html).
²
org.apache.derby.jdbc.EmbeddedDriver
is now inderbytools
.³ The new REST API from NVD isn't stable (currently) because it's under massive load and returning
HTTP 503 Service Unavailable
sometimes. On a clean/purged CVE DB I had to wait ~1h 30m fordependencyCheckAnalyze
to finish. But it worked and I think DependencyCheck is a good tool for finding at least some reasonable CVEs. This shouldn't be abandoned imho.