-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Upgrade owasp-java-html-sanitizer from 20220608.1 to 20240325.1 #28386
Conversation
@casewalker Thanks for the PR! Are you please able to add the signature to the commit message as other commits in Keycloak repository? See https://github.com/keycloak/keycloak/blob/main/CONTRIBUTING.md#developers-certificate-of-origin for the details. @pskopek Do you think it is ok to update this dependency? |
@mposolda Added the signature now. |
@casewalker Thanks. It seems there are compilation failures in the |
It seems there were some sporadic Guava dependencies still throughout the repo, only one was directly related to For the class With the Guava issues addressed, the services were still failing to compile with:
So I switched out the This PR is now bigger than it was, please let me know your thoughts! |
7de0ae0
to
fa26e07
Compare
Test failing with:
I wasn't paying quite enough attention on the OWASP dependency upgrade, it seems it now also needs this |
I made some branch mistakes trying to update from main and add the dependencies, but it should be all settled now. |
That is used in annotations in FilesPlainTextVaultProvider --> | ||
<groupId>com.googlecode.owasp-java-html-sanitizer</groupId> | ||
<artifactId>owasp-java-html-sanitizer</artifactId> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this since I also replaced the no-longer-existent @NonNull
with the one from Jakarta.
<artifactId>*</artifactId> | ||
</exclusion> | ||
</exclusions> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if exclusions are necessary, they were done above so I just did them here too. Things seemed to work out ok with this.
It's been a while since I really worked on Java and Maven. I forgot to pull the dependency in on the This time, I ran the code in the IDELauncher just to see that it can startup successfully. I ran it before changing the poms and saw the server error, then updated the poms and got through without errors. |
I am not sure what happened in Keycloak Operator CI / Test local, it looks like maybe it just timed out?
Does anyone know what might have happened here? Should we retry that test to see if it was just taking longer than usual? Or was this an issue with my code change? |
Hey @mposolda, please let me know if there is anything I could/should do to try to get this PR landed. |
@casewalker Thanks, we need to verify internally if we can update dependencies as every dependency update should be carefully in our internal productization. So added also @pskopek to review this PR, who can provider more details. One quick question: Keycloak server needs to support just java 17 and newer. So considering this, is it really needed to have both |
@casewalker Why did you request a review from the UI team? I don't see any UI code that is affected. |
@ssilvert I guess it might be accident with "branch mistakes" as @casewalker mentioned above when he probably added some unrelated commits to this PR, which did some changes in the UI? I am removing the |
@mposolda Thanks for keeping this PR in order. Yes, the UI team was pulled in when I screwed up merging updates from As for the shim dependencies, they are new jars released as sub-modules of the same java-html-sanitizer repo, they seem to allow that code to work in Java10+ as well as the lower versions down to 8. I am not sure exactly why they were added in this way, and I agree that it doesn't seem like Keycloak should need the Java8 Shim, but the Java10 Shim lists it as a dependency, so I was thinking dependencies might break if you only include the Java10 Shim. I also was following the suggestion made here for reference. Deeper questions on the topic may be out of my depth, but please let me know if there are more questions or concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We have productized new dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pskopek Thanks!
@casewalker Could you please just fix the conflicts? I hope we can have this merged afterwards. Class CustomFuseContainer
was removed from the codebase in the meantime, so you can probably remove all changes of this class from your PR.
Signed-off-by: Case Walker <case.b.walker@gmail.com>
It looks like more Guava was used since I opened this PR. It was used just for creating immutable data structures, so I replaced all of them with unmodifiable data structures from the java Collections framework. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@casewalker @pskopek Thanks to both for all the work on this!
Closes #28385
There may be a direct vulnerability from this dependency (CVE-2011-4457), but removing a transitive dependency on Guava is at least a step in the right direction.
As well, removing Guava (and also apparently some FindBugs dependency) removed the availability of:
com.google.common.base.Strings#isNullOrEmpty
com.google.common.base.Charsets#UTF_8
com.google.common.base.Predicate
org.checkerframework.checker.nullness.qual.NonNull
com.google.common.collect.(ImmutableMap,ImmutableList,ImmutableSet)
These were all replaced with:
org.keycloak.utils.StringUtil#isNullOrEmpty
java.nio.charset.StandardCharsets#UTF_8
java.util.function.Predicate
jakarta.annotation.Nonnull
java.util.Collections
Respectively in a variety of places throughout the code.
Beyond that, this upgrade required new dependencies from owasp-java-html-sanitizer, some new "Java Shim" dependencies added to that project, so those dependencies were added here too wherever necessary.