Skip to content
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

Fix ValidationErrorMessageBodyWriter #4136

Merged
merged 1 commit into from
May 28, 2019
Merged

Conversation

senivam
Copy link
Contributor

@senivam senivam commented May 24, 2019

Fixes #4134

Signed-off-by: Maxim Nesen [email protected]

@senivam senivam requested a review from jansupol May 24, 2019 06:27
@senivam senivam force-pushed the i4134 branch 5 times, most recently from 85e0afc to 0ace9e3 Compare May 28, 2019 05:49
Copy link
Member

@Tomas-Kraus Tomas-Kraus left a comment

Choose a reason for hiding this comment

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

LGTM

@jansupol jansupol merged commit 4787e76 into eclipse-ee4j:master May 28, 2019
@davewichers
Copy link

I just reviewed this 'fix' and implementing your own HTML encoder is TERRIBLE. There are MANY ways this custom encoder can be bypassed. I strongly suggest you replace use of this custom HTML encoder with a strong encoder provided by a standard security library. Like the OWASP Java Encoder or something from Apache, or whatever.

@senivam senivam deleted the i4134 branch April 28, 2022 10:00
@senivam
Copy link
Contributor Author

senivam commented Apr 28, 2022

Hi @davewichers, thanks for the comment. Regarding the custom HTML encoder - for now, I do not recall exactly why this solution was taken, but most probably just because of avoiding another dependency in the project (due to CQ bureaucracy). The method probably was just copied from an open library. However you are right, it's always preferable to use some maintained solution instead of some custom hacks. So, I'm opening another PR for this: #5053, and filling a CQ about the usage of Apache's commons-text (using your original suggestion from bugs.eclipse.org).

@jansupol
Copy link
Contributor

@davewichers In general, we understand your concern, which is twofold:

First, it is the message which can be displayed wrongly because of the HTML tags. That is what we tried to fix with the custom code.

Second, it is the security that can be at stake here. But the error message comes from a ConstraintViolationException which is a user code, provided by a class that is part of the application. It is not a custom message that an attacker can pass in. For that, we see no urge to utilize a 3rd party library that a user would need to deploy along with Jersey to an application server/microservices runtime.

You accuse us of implementing our own HTML encoder which you claim to be TERRIBLE without proof or an example of how this can cause a bypass as you suggest. Can you provide us with such an example?

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.

Fix ValidationErrorMessageBodyWriter
4 participants