search

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
833 stars 210 forks source link

Guava removal breaks compatibility (with JDK9) #301

Closed csware closed 3 months ago

csware commented 5 months ago

The minimum required Java version was explicitly raised to 9 with the recent removal of Guava (cf. commit 3b6cc1b7e6d7992c5b15f87f91dc163de7d35c05).

I appreciate that Guava was dropped recently, however, there is a problem: Map.copyOf (also List.copyOf) was first introduced with Java 10.

The method Files.readString used in the tests, was introduced with Java 11 (here, Commons-IO could be used, or just plain new String(Files.readAllBytes(path), Charsets.toCharset(charset));).

I suppose reasons are:

  1. The compiler plugin is too old and does not support the --release parameter (cf. #304)
  2. No automatic checks are automatically executed (cf. #305)
csware commented 5 months ago

PR #302 reduces the usage of *.copyOf, still there are some places where it is used.

Should we introduce our own helper method? Or build a multi-release jar in which the helper just delegates to the JRE classes?

In general, JDK8 compatibility can be achieved without too much effort if needed.

csware commented 5 months ago

Btw. for me the following tests fails (itroduced by commit 91c5fdc146a01aab1e8b0db38be449a960fe88c1):

org.owasp.html.HtmlPolicyBuilderTest
testRelLinksWhenRelisPartOfData(org.owasp.html.HtmlPolicyBuilderTest)
junit.framework.AssertionFailedError: Failure in testRelLinksWhenRelisPartOfData
melloware commented 5 months ago

I am not sure Java9 should be supported as its a Non LTS version: https://www.oracle.com/java/technologies/java-se-support-roadmap.html

I think Java 11 LTS is the lowest version that should be supported

csware commented 5 months ago

Many Apache Common projects still target 1.8 that's why I'm asking. Otherwise, we should target Java 11 (and update all respective references).

csware commented 5 months ago

Here you can find a proof of concept for supporting Java 1.8: https://github.com/csware/java-html-sanitizer/tree/target-jdk8, diff: https://github.com/csware/java-html-sanitizer/compare/main..target-jdk8

melloware commented 5 months ago

I like the idea of supporting JDK8

csware commented 5 months ago

I'd like to have the other PRs mergred first, then I can provide a new MR that will bring JDK9 and (if wanted) JDK8 compatibility. Please merge the other PRs first as there are dependencies.

I have two questions:

melloware commented 5 months ago

Ok I think we should shoot for JDK8

csware commented 5 months ago

@mikesamuel I would be happy to discuss this with you...

mikesamuel commented 5 months ago

My concern with defensive copies without {Map,List,Set}.copyOf, as noted at https://github.com/OWASP/java-html-sanitizer/pull/321#issuecomment-1924646239 is that defensive copies after the first are not cheap.

iirc, the decision to go with JDK 10 (we thought they were 9 which is on me) was that the .of and .copyOf factories recognize each others' outputs and so one can pass a collection through multiple layers that defensively copy without incurring the cost of those copies multiple times.

which is referring to these implementation notes:

static <K,V> Map<K,V> copyOf​(Map<? extends K,? extends V> map)

Implementation Note: If the given Map is an unmodifiable Map, calling copyOf will generally not create a copy.

How might we keep redundant copies cheap?

csware commented 5 months ago

I'm using a wrapper in my JDK8 branch and building a multi-release jar (cf. https://github.com/csware/java-html-sanitizer/compare/main..target-jdk8b).

Also, are there many places where additional copies are made (apart from the ones mentioned in https://github.com/OWASP/java-html-sanitizer/issues/301#issuecomment-1919825639)?

wcruppel commented 3 months ago

Removing the dependency on Guava is GOOD. Adding a dependency on Java 9 is BAD. I mean, really?! This isn't even LTS.

This is a surefire way to force people (including my group) to find an alternative. We have a hard requirement on Java 8 and will for the foreseeable future.

So, please reconsider.

As @csware notes, JDK8 compatibility can be achieved without too much effort

rombert commented 3 months ago

Should this issue be closed? https://github.com/OWASP/java-html-sanitizer/releases/tag/release-20240325.1 is listed as having Java 8 compatibility via https://github.com/OWASP/java-html-sanitizer/pull/328 .