Skip to content

Conversation

@gaol
Copy link

@gaol gaol commented Aug 12, 2024

Copy link
Member

@marko-bekhta marko-bekhta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks for the patch. Maybe you could take a look at

and try adding a test there, to confirm that this update actually resolved the problem?

Also, 6.0 is an EOL version... would you need a release from us with this patch included ?

-->
<classmate.version>1.3.4</classmate.version>
<jsoup.version>1.8.3</jsoup.version>
<jsoup.version>1.15.4</jsoup.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that upgrading from JSoup 1.8 to 1.15 means some JSoup features will change incompatibly.

Applications that happen to use JSoup directly, or through other dependencies than Hibernate Validator, and rely on those features, will break.

Doing this in a micro release is generally frowned upon.

If we absolutely need this to fix a CVE, so be it, but +1 on Marko's request to add a test to make sure this actually fixes the CVE.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reviews. @marko-bekhta, @yrodiere

We are using a combination of jsoup:1.14.2 and hibernate-validator:6.0.23.Final.

PST scanned the jsoup as one of the shipment to mark our project as CVE affected, the jsoup:1.15.4 version is so far the clean version, which is suited for us. But the upgrade will fail because hibernate-validator:6.0.23.Final uses the Whitelist which got removed and replaced by Safelist in jsoup:1.15.1+, that is the reason to drive this upgrade of jsoup in hibernate-validator 6.0 branch, and replace the Whitelist with Safelist.

Which means we need to address the security flaw which does not need to go through hibernate-validator. An example is CVE-2022-36033, sets SafeList.preserveRelativeLinks to false can be a workaround, but it is always false from hibernate-validator point of view.

So yes, it would be great if we can do the upgrade in hibernate-validator:6.0 branch and a new release for it. :)

For the tests, I don't see much we can add to SafehtmlValidatorTest, because so far I think it is pure jsoup issue and fix. 🤝

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, mm ok, if it's purely a JSoup problem and nothing to test in HV, then we should be ok. 👍🏻

I'll merge this one and tag it for you. As per Yoann's comment on the Jira it usually was enough to just have a tag. So I won't be looking into doing a release for 6.0 at the moment, let's see if the tag will be enough. It would be better if we don't introduce braking change in a micro. I'll monitor that ticket for any further developments.
And thanks for the patch once more! 🎉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we've noticed that there were a few commits since the 6.0.23 release in the 6.0 branch. We will branch out from the release point and add this upgrade, so only this change is included.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much on the tag, SP tag works for me 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok great! Thanks for letting us know.🙂

@marko-bekhta marko-bekhta merged commit 354c248 into hibernate:6.0 Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants