ofbiz-framework icon indicating copy to clipboard operation
ofbiz-framework copied to clipboard

Updated several (transitive) dependencies (OFBIZ-13123)

Open dtrunk90 opened this issue 1 year ago • 37 comments

Improved:

  • Update Apache PDFBox to 2.0.32
  • Update Apache CXF Runtime JAX-RS Frontend to 3.6.4
  • Update Asciidoctor Gradle Plugin to 4.0.2
  • Update transitive dependency testng to 7.7.0
  • Update Groovy to 4.0.22 ¹
  • Update Apache MINA sshd to 2.13.1
  • Update poi to 5.3.0
  • Update ez-vcard to 0.12.1
  • Update jdom to 2.0.6.1
  • Update Apache CXF Runtime JAX-RS Frontend to 3.6.3
  • Update transitive dependency bcprov-jdk18on to 1.78
  • Update tika parsers to 2.9.2
  • Update fop to 2.9
  • Update transitive dependency mime4j to 0.8.10
  • Update clojure to 1.11.3
  • Update derby to 10.16.1.1 ²
  • Update jackson-databind to 2.17.1
  • Update esapi to 2.5.4.0
  • Add guava as dependency
  • Set checkstyle.toolVersion
  • Update org.owasp.dependencycheck to 10.0.2
  • Upgrade to gradle 8.8

Reverted:

  • Improved: Abandon the Gradle Owasp dependencycheck task (OFBIZ-13121) 0a9ee32 ³

Fixed:

  • Corrections based on Checkstyle errors

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 in derbytools.

³ 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 for dependencyCheckAnalyze 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.

dtrunk90 avatar Jul 03 '24 19:07 dtrunk90

@JacquesLeRoux Feel free to review

dtrunk90 avatar Jul 03 '24 19:07 dtrunk90

Thanks @dtrunk90,

That's quite a help, much appreciated :+1: I'll review and test all that

JacquesLeRoux avatar Jul 04 '24 06:07 JacquesLeRoux

Hi Dan,

You say

Fixed:

  • Corrections based on Checkstyle errors

But there are currently no errors in trunk: https://nightlies.apache.org/ofbiz/trunk/checkstyle.html

JacquesLeRoux avatar Jul 04 '24 15:07 JacquesLeRoux

Hey,

weird. For me there were some about simplifying expressions. And after upgrading the checkstyle toolVersion there were some more.

dtrunk90 avatar Jul 04 '24 15:07 dtrunk90

Ah, maybe it's the reason... Will check that... Anyway so far all your fixes seems good to me ;)

JacquesLeRoux avatar Jul 04 '24 16:07 JacquesLeRoux

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)?

dtrunk90 avatar Jul 04 '24 16:07 dtrunk90

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 Also I have created https://issues.apache.org/jira/browse/OFBIZ-13123 for this PR For the new one https://issues.apache.org/jira/browse/OFBIZ-12989 exists already This is also interesting: https://lists.apache.org/[email protected]:gte=1d:%22tomcat%2010%22 Most of the time, for a such task we discuss on dev ML before...

JacquesLeRoux avatar Jul 04 '24 17:07 JacquesLeRoux

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.

dtrunk90 avatar Jul 04 '24 17:07 dtrunk90

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!

mbrohl avatar Jul 04 '24 17:07 mbrohl

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.

JacquesLeRoux avatar Jul 05 '24 07:07 JacquesLeRoux

That's why it's in separate commits. I see no reason in doing another PR just for the checkstyle corrections.

dtrunk90 avatar Jul 05 '24 07:07 dtrunk90

OK, so it's only about (in order of appearance): https://github.com/apache/ofbiz-framework/pull/819/commits/b6ed6a626ef94f6a93a87aedbca251fce074fc50 https://github.com/apache/ofbiz-framework/pull/819/commits/e5464886765d7c25ff340a54ee29d32752ee62d4 right ? With the new checkstyle toolVersion of course ie https://github.com/apache/ofbiz-framework/pull/819/commits/759c8752542a89b7229e232634bb5ec731cbc0d3

JacquesLeRoux avatar Jul 05 '24 07:07 JacquesLeRoux

After a review on GH UI, the checkstyle corrections work perfectly locally. 34 issues before b6ed6a6, 14 remaining before e546488, then none, +1 for this part.

JacquesLeRoux avatar Jul 05 '24 17:07 JacquesLeRoux

Hi @dtrunk90,

I guess you only tested the framework. We have 2 failures with plugins:

1: Task failed with an exception.

  • Where: Build file 'C:\projectsASF\Git\ofbiz-framework\plugins\solr\build.gradle' line: 30

  • What went wrong: A problem occurred evaluating project ':plugins:solr'.

No signature of method: org.gradle.api.internal.artifacts.ivyservice.dependencysubstitution.DefaultDependencySubstitutions$1.with() is applicable for argument types: (org.gradle.internal.component.external.model.DefaultModuleComponentS elector) values: [com.google.guava:guava:28.0-jre] Possible solutions: with(groovy.lang.Closure), with(boolean, groovy.lang.Closure), wait(), wait(long), find(), wait(long, int)

2: Task failed with an exception.

  • What went wrong: A problem occurred configuring root project 'ofbiz'.

No hooks found

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.

JacquesLeRoux avatar Jul 06 '24 07:07 JacquesLeRoux

While reverting the patch, I saw that you renamed GroovyScriptTestCase to GroovyScriptAssert and GroovyTestCase to GroovyAssert. Not a big deal, just to say.

JacquesLeRoux avatar Jul 06 '24 07:07 JacquesLeRoux

Yes, I've only tested framework. I'll have a look at the plugins later that day. Didn't thought about them tbh.

dtrunk90 avatar Jul 06 '24 07:07 dtrunk90

While reverting the patch, I saw that you renamed GroovyScriptTestCase to GroovyScriptAssert and GroovyTestCase to GroovyAssert. Not a big deal, just to say.

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.

dtrunk90 avatar Jul 06 '24 07:07 dtrunk90

I tried it this morning and indeed after 1h15 I got the result. It found 217 vulnerabilities! I guess (did not check all!) most of them, if not all, are transitive dependencies that are currently not in dependencies.gradle. Anyway I agree that it can still be usefull. So I reopened rOFBIZ-13121 to revert with this PR.

JacquesLeRoux avatar Jul 07 '24 08:07 JacquesLeRoux

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?

JacquesLeRoux avatar Jul 07 '24 10:07 JacquesLeRoux

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.

dtrunk90 avatar Jul 07 '24 10:07 dtrunk90

Great, I now understand better why you wanted to keep OWASP dependencies check.

JacquesLeRoux avatar Jul 07 '24 11:07 JacquesLeRoux

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: https://lists.apache.org/thread/xoqcjo20moz3gt4v40k51h1ngoh44lor

JacquesLeRoux avatar Jul 11 '24 07:07 JacquesLeRoux

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: https://lists.apache.org/thread/xoqcjo20moz3gt4v40k51h1ngoh44lor

No problem. I've dropped the commit.

dtrunk90 avatar Jul 11 '24 07:07 dtrunk90

@JacquesLeRoux @mbrohl Were you able to review/test the rest yet?

dtrunk90 avatar Jul 13 '24 12:07 dtrunk90

Hi Danny,

I did not review nor tested since your commits 2 days ago. Before all was OK with me as much as I was able to test. Do you know what are the differences in these last commits?

JacquesLeRoux avatar Jul 13 '24 13:07 JacquesLeRoux

Hi Danny,

I did not review nor tested since your commits 2 days ago. Before all was OK with me as much as I was able to test. Do you know what are the differences in these last commits?

I just dropped the tomcat upgrade and resolved the conflicts.

dtrunk90 avatar Jul 13 '24 13:07 dtrunk90

Then it's OK with me

JacquesLeRoux avatar Jul 13 '24 14:07 JacquesLeRoux

Any news when this will be merged?

dtrunk90 avatar Jul 22 '24 06:07 dtrunk90

Hi @dtrunk90,

Please update Apache PDFBox to Version 2.0.32. No CVEs but several bug fixes: https://lists.apache.org/thread/p1vg0j8pg23lptlwvmjybm1l7lbbdzjn TIA

JacquesLeRoux avatar Jul 25 '24 10:07 JacquesLeRoux

Hi @dtrunk90,

Please update Apache PDFBox to Version 2.0.32. No CVEs but several bug fixes: https://lists.apache.org/thread/p1vg0j8pg23lptlwvmjybm1l7lbbdzjn TIA

Done. Would be nice to get this merged today so I can finally merge it into our project. We need this tomorrow. Otherwise I have to merge my fork which I actually wanted to avoid.

dtrunk90 avatar Jul 25 '24 11:07 dtrunk90