HV-1816 Disable Expression Language by default for custom constraint violations #1138

gsmet · 2020-11-25T16:29:21Z

https://hibernate.atlassian.net/browse/HV-1816

documentation/src/main/asciidoc/ch06.asciidoc

documentation/src/main/asciidoc/ch12.asciidoc

.../test/java/org/hibernate/validator/referenceguide/chapter06/elinjection/UnsafeValidator.java

...n/java/org/hibernate/validator/constraintvalidation/HibernateConstraintViolationBuilder.java

...src/main/java/org/hibernate/validator/messageinterpolation/ParameterMessageInterpolator.java

...ain/java/org/hibernate/validator/messageinterpolation/ResourceBundleMessageInterpolator.java

...c/main/java/org/hibernate/validator/messageinterpolation/ExpressionLanguageFeatureLevel.java

...n/java/org/hibernate/validator/constraintvalidation/HibernateConstraintViolationBuilder.java

yrodiere · 2020-12-03T11:42:34Z

engine/src/main/java/org/hibernate/validator/BaseHibernateValidatorConfiguration.java

@@ -427,4 +428,15 @@ default S locales(Locale... locales) {

 	@Incubating
 	S beanMetaDataClassNormalizer(BeanMetaDataClassNormalizer beanMetaDataClassNormalizer);
+
+	/**
+	 * Allows setting the Expression Language feature level.


So... is this the default for all constraints? Or only for default (built-in) constraints? Is it used as the default for custom constraint violations?

I find this a bit confusing...

I clarified it in the Javadoc.

I know it's a bit confusing but the defaults are different and I don't want the global default to be the one used by custom violations as they are more sensitive to security issues.

documentation/src/main/asciidoc/ch12.asciidoc

engine/src/main/java/org/hibernate/validator/BaseHibernateValidatorConfiguration.java

yrodiere · 2020-12-04T08:38:29Z

engine/src/main/java/org/hibernate/validator/BaseHibernateValidatorConfiguration.java

+
+	/**
+	 * Property for configuring the Expression Language feature level for custom violations, allowing to define which
+	 * Expression Language features are available for message interpolation.


Maybe add a link to the method that allows customizing this setting for custom violations?

engine/src/main/java/org/hibernate/validator/BaseHibernateValidatorConfiguration.java

yrodiere · 2020-12-04T09:05:22Z

...hibernate/validator/internal/engine/constraintvalidation/ConstraintValidatorContextImpl.java

@@ -181,7 +189,7 @@ private ConstraintViolationCreationContext getDefaultConstraintViolationCreation
 	private abstract class NodeBuilderBase {

 		protected final String messageTemplate;
-		protected boolean expressionLanguageEnabled;
+		protected ExpressionLanguageFeatureLevel expressionLanguageFeatureLevel = defaultCustomViolationExpressionLanguageFeatureLevel;


Since EL may now be enabled by default depending on configuration, shouldn't enableExpressionLanguage be renamed to expressionLanguageFeatureLevel?
On second thought, it also feels weird that we can call enableExpressionLanguage(NONE).

I think I prefer it the way it is.

...java/org/hibernate/validator/internal/engine/messageinterpolation/el/VariablesELContext.java

yrodiere · 2020-12-04T09:14:20Z

...c/main/java/org/hibernate/validator/messageinterpolation/ExpressionLanguageFeatureLevel.java

+	 * <p>
+	 * For custom violations, the default is {@link ExpressionLanguageFeatureLevel#NONE} and, if Expression Language is
+	 * enabled for a given custom violation via the API, the default becomes
+	 * {@link ExpressionLanguageFeatureLevel#VARIABLES} in the context of this given custom violation.


It's not quite the full story, though.

From what I can see:

If not enabled explicitly, the default is whatever the user configured through properties, falling back to NONE.

If enabled explicitly, the default is VARIABLES, and whatever the user configured through properties is ignored.

Which seems... complex?

...or/test/internal/engine/messageinterpolation/ExpressionLanguageMessageInterpolationTest.java

…violations

gsmet · 2020-12-04T10:38:03Z

/cc @pwntester this might be of interest to you. I plan to include this in a 6.2 and a 7.

I gave up on hoping that users will read the documentation and also the most problematic way is that there is no spec-compliant way to be safe.

The way I did it passes the Bean Validation TCK while reducing the EL Features even for standard constraints.

For custom violations, it's completely disabled by default and you can enable EL on a case per case basis. We also have a global settings for compatibility reasons but the idea is that people should enable EL for specific custom violations and thus be careful when doing so.

pwntester · 2020-12-04T10:52:53Z

Secure by default, way to go!

gsmet · 2020-12-04T10:58:02Z

Well, secure by default is easy.

The hard part is to still offer the flexibility of EL without compromising the whole thing (i.e. avoid pushing people to use a global switch which IMHO is equally as bad) and still passing the TCK.

I think we need to push addExpressionVariable() to the spec.

….0.Final (master) (#3732) > ### Expression Language overhaul > > Expression Language disabled by default for custom violations. > > Ability to define the Expression Language features enabled for more safety. - https://hibernate.org/validator/releases/6.2/#whats-new - https://hibernate.atlassian.net/browse/HV-1816 - hibernate/hibernate-validator#1138 - https://docs.jboss.org/hibernate/validator/6.2/reference/en-US/html_single/#el-features Co-authored-by: Renovate Bot <[email protected]> Co-authored-by: Jochen Schalanda <[email protected]>

gsmet force-pushed the HV-1816 branch 3 times, most recently from 2c0fff3 to d60e495 Compare November 25, 2020 19:22

yrodiere reviewed Dec 3, 2020

View reviewed changes

gsmet force-pushed the HV-1816 branch 2 times, most recently from 354df4a to c211b53 Compare December 3, 2020 19:51

yrodiere reviewed Dec 4, 2020

View reviewed changes

gsmet added 2 commits December 4, 2020 11:32

HV-1816 Disable Expression Language by default for custom constraint …

a186069

…violations

HV-1816 Limit the EL features exposed by default

6c67a0a

gsmet force-pushed the HV-1816 branch from 36a5b2b to 6c67a0a Compare December 4, 2020 10:33

gsmet changed the base branch from 6.1 to 6.2 December 4, 2020 10:33

gsmet merged this pull request into hibernate:6.2 Dec 4, 2020

joschi mentioned this pull request Apr 24, 2021

Update dependency org.hibernate.validator:hibernate-validator to v6.2.0.Final (master) dropwizard/dropwizard#3732

Merged

1 task

HV-1816 Disable Expression Language by default for custom constraint violations #1138

HV-1816 Disable Expression Language by default for custom constraint violations #1138

Conversation

gsmet commented Nov 25, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yrodiere Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

gsmet Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yrodiere Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yrodiere Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

gsmet Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yrodiere Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gsmet commented Dec 4, 2020

Uh oh!

pwntester commented Dec 4, 2020

Uh oh!

gsmet commented Dec 4, 2020

Uh oh!

Uh oh!