ctubbsii commented on code in PR #2294:
URL: https://github.com/apache/zookeeper/pull/2294#discussion_r2280037589
##########
pom.xml:
##########
@@ -562,15 +564,10 @@
<artifactId>slf4j-api</artifactId>
<version>${slf4j.version}</version>
</dependency>
- <dependency>
- <groupId>ch.qos.logback</groupId>
- <artifactId>logback-core</artifactId>
- <version>${logback-version}</version>
- </dependency>
Review Comment:
I'm not sure if this was ever really needed. It's a transitive dependency of
logback-classic, and would have been picked up transitively whenever it was
needed. It's possible some of the test code used code from this directly. If
that was the case, then it's useful to keep it to make the
maven-dependency-plugin:analyze goal happy. However, this project does not
appear to use that plugin to analyze dependencies, so it's fine to remove it.
##########
owaspSuppressions.xml:
##########
@@ -18,6 +18,23 @@
-->
<suppressions
xmlns="https://jeremylong.github.io/DependencyCheck/dependency-suppression.1.2.xsd";>
+ <suppress>
+ <!--
+ We have updated jetty[1] to 9.4.57.v20241219[2] which includes a
fix[3] for CVE-2024-6763[4].
+ But it is not listed as fixed version since 9.x is EOL[5]. So we
still have to suppress this
+ to pass vulnerabilities check. Besides above, ZooKeeper does not use
HttpURI[6] thus should
+ not be affected by this CVE anyway.
+
+ Refs:
+ [1]: https://github.com/apache/zookeeper/pull/2220
+ [2]:
https://github.com/jetty/jetty.project/releases/tag/jetty-9.4.57.v20241219
+ [3]: https://github.com/jetty/jetty.project/pull/12532
+ [4]: https://github.com/advisories/GHSA-qh8g-58pp-2wxh
+ [5]:
https://gitlab.eclipse.org/security/cve-assignement/-/issues/25#note_2968611
+ [6]: https://issues.apache.org/jira/browse/ZOOKEEPER-4876
+ -->
+ <cve>CVE-2024-6763</cve>
+ </suppress>
Review Comment:
This is unrelated to logging, but appears to have been picked up from the
same change on branch-3.8.5. This PR is against branch 3.8, so it looks like a
new change, but it isn't.
##########
zookeeper-contrib/zookeeper-contrib-loggraph/pom.xml:
##########
@@ -54,13 +54,7 @@
</dependency>
<dependency>
<groupId>ch.qos.logback</groupId>
- <artifactId>logback-core</artifactId>
- <exclusions>
- <exclusion>
- <groupId>*</groupId>
- <artifactId>*</artifactId>
- </exclusion>
- </exclusions>
+ <artifactId>logback-classic</artifactId>
Review Comment:
```suggestion
<artifactId>logback-classic</artifactId>
<scope>test</scope>
```
##########
zookeeper-contrib/zookeeper-contrib-zooinspector/pom.xml:
##########
@@ -91,13 +91,9 @@
</dependency>
<dependency>
<groupId>ch.qos.logback</groupId>
- <artifactId>logback-core</artifactId>
- <exclusions>
- <exclusion>
- <groupId>*</groupId>
- <artifactId>*</artifactId>
- </exclusion>
- </exclusions>
+ <artifactId>logback-classic</artifactId>
+ <scope>runtime</scope>
+ <optional>true</optional>
Review Comment:
To avoid propagation, it was decided on #2155 that optional runtime
dependencies should be added as a test dependency when they are needed for
tests, to prevent them being propagated when used as a library, rather than
using optional:
```suggestion
<scope>test</scope>
```
##########
zookeeper-contrib/pom.xml:
##########
@@ -56,6 +56,21 @@
</profile>
</profiles>
+ <dependencyManagement>
Review Comment:
I am not sure what's going on in the zookeeper-contrib directory, which
looks like a blend of both Ant/Ivy and Maven build configurations. However, I
don't think you need to add anything to this pom.xml at all. For the contrib
directories that use slf4j/logback for tests, you can add logback-classic as a
`<scope>test</scope>` dependency. For others, you can omit it.
So, you only have to add the test dependency to:
* zookeeper-contrib-loggraph/pom.xml
* zookeeper-contrib-rest/pom.xml
* zookeeper-contrib-zooinspector/pom.xml
##########
zookeeper-contrib/zookeeper-contrib-fatjar/pom.xml:
##########
@@ -62,6 +62,10 @@
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ </dependency>
Review Comment:
The fatjar is used as a library. You shouldn't let this runtime dependency
propagate for the library use case, so this should be omitted here.
```suggestion
```
##########
zookeeper-contrib/zookeeper-contrib-rest/pom.xml:
##########
@@ -70,13 +70,7 @@
</dependency>
<dependency>
<groupId>ch.qos.logback</groupId>
- <artifactId>logback-core</artifactId>
- <exclusions>
- <exclusion>
- <groupId>*</groupId>
- <artifactId>*</artifactId>
- </exclusion>
- </exclusions>
+ <artifactId>logback-classic</artifactId>
Review Comment:
```suggestion
<artifactId>logback-classic</artifactId>
<scope>test</scope>
```
##########
pom.xml:
##########
@@ -458,8 +458,10 @@
<redirectTestOutputToFile>true</redirectTestOutputToFile>
<!-- dependency versions -->
- <slf4j.version>1.7.30</slf4j.version>
- <logback-version>1.2.13</logback-version>
+ <slf4j.version>1.7.36</slf4j.version>
+ <slf4j.application.version>2.0.13</slf4j.application.version>
+ <logback.test.version>1.2.13</logback.test.version>
+ <logback.application.version>1.3.15</logback.application.version>
Review Comment:
If you're using `<scope>test</scope>`, as per #2155 backported, then you
don't need a separate library version, since #2155 prevents the library version
from being propagated. So it doesn't matter what version you use. It can be the
same as the application version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
