OWASP / java-html-sanitizer

Takes third-party HTML and produces HTML that is safe to embed in your web application. Fast and easy to configure.
Other
854 stars 214 forks source link

get rid of guava and use jdk8 as a minimum. #162

Closed carlmolemans closed 5 years ago

carlmolemans commented 5 years ago

From my perspective there is zero need for a Guava dependency. Streams, try-with-resources, StandardCharsets would most likely already eliminate 90% of the usages.

Later version of JDK is also fine by me.

`

org.apache.maven.plugins
      <artifactId>maven-compiler-plugin</artifactId>
      <version>3.3</version>
      <configuration>
        <source>6</source>
        <target>6</target>
      </configuration>
    </plugin>`

Targetting JDK6 implies you're ok with people still using JRE6 which is discontinued and full of security bugs.

solomax commented 5 years ago

mvn org.sonatype.ossindex.maven:ossindex-maven-plugin:audit -f pom.xml

Detected 1 vulnerable components:
[ERROR]   com.google.guava:guava:jar:19.0:compile; https://ossindex.sonatype.org/component/pkg:maven/com.google.guava/guava@19.0
[ERROR]     * [CVE-2018-10237]  Deserialization of Untrusted Data (5.9); https://ossindex.sonatype.org/vuln/24585a7f-eb6b-4d8d-a2a9-a6f16cc7c1d0

If it is absolutely impossible to remove guava, can it be updated?

mikesamuel commented 5 years ago

Does something about the way dependencies are mentioned in the POM prevent using a more modern Guava version?

solomax commented 5 years ago

I can update locally used guava version BUT according to guava devs: versions might be incompatible And I might get weird behavior in runtime as a result

carlmolemans commented 5 years ago

What about applying the OWASP best practices and keep the dependencies in this library up to date to begin with?

As @solomax already argued, due to lack of guarantees concerning backward compatibility, you cannot just tell everyone to upgrade the dependencies which you force upon them to begin with.

Another solution is to admit this library is hopelessly outdated and should not be used anymore; whichever works best I guess?

mikesamuel commented 5 years ago

@carlmolemans This library is up to date with the current state of the HTML standard.

I'm not telling, I'm asking.

I don't understand all the nuances to maven dependency versioning.

How might I better balance the needs of users of older versions of Java and make it easy to use the latest version of guava?

solomax commented 5 years ago

Hello @mikesamuel,

In case you need to target java6 the only option would be to have several major versions You have "date-alike" versions, so I would propose to have release-20181114.XXX for java6 and release 2019XXXX targeted java8

WDYT?

carlmolemans commented 5 years ago

Keep it simple: support only versions of Java for which Oracle provides (free) updates. Thus:

How to keep your maven dependencies up to date:

mikesamuel commented 5 years ago

@solomax I'm not sure I understand what is achieved by putting release- for Java8 but not Java6.

@carlmolemans Does part of that address the needs of existing users who might not want to upgrade in lockstep?

carlmolemans commented 5 years ago

I do not fully understand your question, but I can say this:

Users who do not want to update need to make their choice and live with the consequences.

solomax commented 5 years ago

Sorry @mikesamuel I seem to be not clear enough My point was: With different release numbering you might have version 6.x.x for java6 version 7.x.x for java7 version 8.x.x for java8 ................ right now your versions are dates: YYYYMMDD, so you don't have this option

mikesamuel commented 5 years ago

@solomax I'm not going to change the numbering scheme to something that looks like semver but isn't.

IIUC, per maven rules, <version>6.0.0</version> < <version>20160101.1</version> which might pose a problem.


I checked mvn org.sonatype.ossindex.maven:ossindex-maven-plugin:audit -f pom.xml per your note.

Per the CVE, this project does not use the problematic APIs, but pinning an older version in place would leave dependencies vulnerable as you note. guava:24.1.1-jre is the first non-vulnerable version.

Unbounded memory allocation in Google Guava 11.0 through 24.x before 24.1.1 allows remote attackers to conduct denial of service attacks

I'm going to bump <properties><guava.version> to 27.1-jre and document how older JDKs can require a different version.

solomax commented 5 years ago

@mikesamuel my intent was to show semver can help in releasing different branches in parallel It is definitely up to you how version of your library should look like :)

Thanks a lot for the change!

mikesamuel commented 5 years ago

Released a new version with this patched in and posted a compatibility note at https://groups.google.com/forum/#!topic/owasp-java-html-sanitizer-announce/ebauBXFIf58

claudioweiler commented 2 years ago

This issue is closed, but Guava unnecessary dependency was not discussed. We have a library with about 230 KB that depends on on an library that are almost 3 MB.