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

Author: gsmet

documentation/src/main/asciidoc/ch06.asciidoc

@@ -173,35 +173,12 @@ It is important to add each configured constraint violation by calling `addConst
 Only after that the new constraint violation will be created.
 ====
 
-[[el-injection-caution]]
-[CAUTION]
-====
-**Be aware that the custom message template is passed directly to the Expression Language engine.**
-
-Thus, you should be very careful when integrating user input in a custom message template as it will be interpreted
-by the Expression Language engine, which is usually not the behavior you want and **could allow malicious users to leak
-sensitive data or even execute arbitrary code**.
-
-If you need to integrate user input in your message, you must <<section-hibernateconstraintvalidatorcontext,pass it as an expression variable>>
-by unwrapping the context to `HibernateConstraintValidatorContext`.
-
-The following validator is very unsafe as it includes user input in the violation message.
-If the validated `value` contains EL expressions, they will be executed by the EL engine.
-
-[source, JAVA, indent=0]
-----
-include::{sourcedir}/org/hibernate/validator/referenceguide/chapter06/elinjection/UnsafeValidator.java[tags=include]
-----
-
-The following pattern must be used instead:
+By default, Expression Language is not enabled for custom violations created in the `ConstraintValidatorContext`.
 
-[source]
-----
-include::{sourcedir}/org/hibernate/validator/referenceguide/chapter06/elinjection/SafeValidator.java[tags=include]
-----
+However, for some advanced requirements, using Expression Language might be necessary.
 
-By using expression variables, Hibernate Validator properly handles escaping and EL expressions won't be executed.
-====
+In this case, you need to unwrap the `HibernateConstraintValidatorContext` and enable Expression Language explicitly.
+See <<section-hibernateconstraintvalidatorcontext>> for more information.
 
 Refer to <<section-custom-property-paths>> to learn how to use the `ConstraintValidatorContext` API to
 control the property path of constraint violations for class-level constraints.

documentation/src/main/asciidoc/ch12.asciidoc

@@ -510,6 +510,7 @@ custom extensions for both of these interfaces.
 [[section-custom-constraint-validator-context]]
 `HibernateConstraintValidatorContext` is a subtype of `ConstraintValidatorContext` which allows you to:
 
+* enable Expression Language interpolation for a particular custom violation - see below
 * set arbitrary parameters for interpolation via the Expression Language message interpolation
 facility using `HibernateConstraintValidatorContext#addExpressionVariable(String, Object)`
 or `HibernateConstraintValidatorContext#addMessageParameter(String, Object)`.
@@ -535,8 +536,8 @@ include::{sourcedir}/org/hibernate/validator/referenceguide/chapter12/context/My
 [NOTE]
 ====
 Apart from the syntax, the main difference between message parameters and expression variables is that message parameters
-are simply interpolated whereas expression variables are interpreted using the expression language engine.
-In practice, it should not change anything.
+are simply interpolated whereas expression variables are interpreted using the Expression Language engine.
+In practice, use message parameters if you do not need the advanced features of an Expression Language.
 ====
 +
 [NOTE]
@@ -550,6 +551,38 @@ You can, however, update the parameters between invocations of
 ====
 * set an arbitrary dynamic payload - see <<section-dynamic-payload>>
 
+By default, Expression Language interpolation is **disabled** for custom violations,
+this to avoid arbitrary code execution or sensitive data leak if user input is not properly escaped.
+
+It is possible to enable Expression Language for a given custom violation by using `enableExpressionLanguage()` as shown in the example below:
+
+[source]
+----
+include::{sourcedir}/org/hibernate/validator/referenceguide/chapter06/elinjection/SafeValidator.java[tags=include]
+----
+
+In this case, the message template will be interpolated by the Expression Language engine.
+
+[CAUTION]
+====
+Using `addExpressionVariable()` is the only safe way to inject a variable into an expression.
+
+If you inject user input by simply concatenating the user input in the message,
+you will allow potential arbitrary code execution and sensitive data leak:
+if the user input contains valid expressions, they will be executed by the Expression Language engine.
+
+Here is an example of something you should **ABSOLUTELY NOT** do:
+
+[source, JAVA, indent=0]
+----
+include::{sourcedir}/org/hibernate/validator/referenceguide/chapter06/elinjection/UnsafeValidator.java[tags=include]
+----
+
+In the example above, if `value`, which might be user input, contains a valid expression,
+it will be interpolated by the Expression Language engine,
+potentially leading to unsafe behaviors.
+====
+
 ==== `HibernateMessageInterpolatorContext`
 
 Hibernate Validator also offers a custom extension of `MessageInterpolatorContext`, namely

documentation/src/test/java/org/hibernate/validator/referenceguide/chapter06/elinjection/SafeValidator.java

@@ -23,6 +23,7 @@ public boolean isValid(String value, ConstraintValidatorContext context) {
 			hibernateContext
 					.addExpressionVariable( "validatedValue", value )
 					.buildConstraintViolationWithTemplate( "${validatedValue} is not a valid ZIP code" )
+					.enableExpressionLanguage()
 					.addConstraintViolation();
 
 			return false;

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

@@ -1,5 +1,6 @@
 package org.hibernate.validator.referenceguide.chapter06.elinjection;
 
+import org.hibernate.validator.constraintvalidation.HibernateConstraintValidatorContext;
 import org.hibernate.validator.referenceguide.chapter06.constraintvalidatorpayload.ZipCode;
 
 import javax.validation.ConstraintValidator;
@@ -16,9 +17,15 @@ public boolean isValid(String value, ConstraintValidatorContext context) {
 
 		context.disableDefaultConstraintViolation();
 
+		HibernateConstraintValidatorContext hibernateContext = context.unwrap(
+				HibernateConstraintValidatorContext.class );
+		hibernateContext.disableDefaultConstraintViolation();
+
 		if ( isInvalid( value ) ) {
-			context
+			hibernateContext
+					// THIS IS UNSAFE, DO NOT COPY THIS EXAMPLE
 					.buildConstraintViolationWithTemplate( value + " is not a valid ZIP code" )
+					.enableExpressionLanguage()
 					.addConstraintViolation();
 
 			return false;

engine/src/main/java/org/hibernate/validator/constraintvalidation/HibernateConstraintValidatorContext.java

@@ -21,6 +21,9 @@
  */
 public interface HibernateConstraintValidatorContext extends ConstraintValidatorContext {
 
+	@Override
+	HibernateConstraintViolationBuilder buildConstraintViolationWithTemplate(String messageTemplate);
+
 	/**
 	 * Allows to set an additional named parameter which can be interpolated in the constraint violation message. The
 	 * variable will be available for interpolation for all constraint violations generated for this constraint.

engine/src/main/java/org/hibernate/validator/constraintvalidation/HibernateConstraintViolationBuilder.java

@@ -0,0 +1,27 @@
+/*
+ * Hibernate Validator, declare and validate application constraints
+ *
+ * License: Apache License, Version 2.0
+ * See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.
+ */
+
+package org.hibernate.validator.constraintvalidation;
+
+import javax.validation.ConstraintValidatorContext.ConstraintViolationBuilder;
+
+import org.hibernate.validator.Incubating;
+
+public interface HibernateConstraintViolationBuilder extends ConstraintViolationBuilder {
+
+	/**
+	 * Enable Expression Language for the constraint violation created by this builder if the chosen
+	 * {@code MessageInterpolator} supports it.
+	 * <p>
+	 * If enabling this, you need to make sure your message template does not contain any unescaped user input (such as
+	 * the validated value): use {@code addExpressionVariable()} to inject properly escaped variables into the template.
+	 *
+	 * @since 6.2
+	 */
+	@Incubating
+	HibernateConstraintViolationBuilder enableExpressionLanguage();
+}

engine/src/main/java/org/hibernate/validator/internal/engine/MessageInterpolatorContext.java

@@ -39,19 +39,22 @@ public class MessageInterpolatorContext implements HibernateMessageInterpolatorC
 	private final Map<String, Object> messageParameters;
 	@Immutable
 	private final Map<String, Object> expressionVariables;
+	private final boolean expressionLanguageEnabled;
 
 	public MessageInterpolatorContext(ConstraintDescriptor<?> constraintDescriptor,
 					Object validatedValue,
 					Class<?> rootBeanType,
 					Path propertyPath,
 					Map<String, Object> messageParameters,
-					Map<String, Object> expressionVariables) {
+					Map<String, Object> expressionVariables,
+					boolean expressionLanguageEnabled) {
 		this.constraintDescriptor = constraintDescriptor;
 		this.validatedValue = validatedValue;
 		this.rootBeanType = rootBeanType;
 		this.propertyPath = propertyPath;
 		this.messageParameters = toImmutableMap( messageParameters );
 		this.expressionVariables = toImmutableMap( expressionVariables );
+		this.expressionLanguageEnabled = expressionLanguageEnabled;
 	}
 
 	@Override
@@ -74,6 +77,11 @@ public Map<String, Object> getMessageParameters() {
 		return messageParameters;
 	}
 
+	@Override
+	public boolean isExpressionLanguageEnabled() {
+		return expressionLanguageEnabled;
+	}
+
 	@Override
 	public Map<String, Object> getExpressionVariables() {
 		return expressionVariables;
@@ -135,6 +143,7 @@ public String toString() {
 		sb.append( ", propertyPath=" ).append( propertyPath );
 		sb.append( ", messageParameters=" ).append( messageParameters );
 		sb.append( ", expressionVariables=" ).append( expressionVariables );
+		sb.append( ", expressionLanguageEnabled=" ).append( expressionLanguageEnabled );
 		sb.append( '}' );
 		return sb.toString();
 	}

engine/src/main/java/org/hibernate/validator/internal/engine/constraintvalidation/ConstraintValidatorContextImpl.java

@@ -6,6 +6,7 @@
  */
 package org.hibernate.validator.internal.engine.constraintvalidation;
 
+import java.lang.annotation.Annotation;
 import java.lang.invoke.MethodHandles;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -28,6 +29,7 @@
 import javax.validation.metadata.ConstraintDescriptor;
 
 import org.hibernate.validator.constraintvalidation.HibernateConstraintValidatorContext;
+import org.hibernate.validator.constraintvalidation.HibernateConstraintViolationBuilder;
 import org.hibernate.validator.internal.engine.path.PathImpl;
 import org.hibernate.validator.internal.util.CollectionHelper;
 import org.hibernate.validator.internal.util.Contracts;
@@ -75,7 +77,7 @@ public final String getDefaultConstraintMessageTemplate() {
 	}
 
 	@Override
-	public ConstraintViolationBuilder buildConstraintViolationWithTemplate(String messageTemplate) {
+	public HibernateConstraintViolationBuilder buildConstraintViolationWithTemplate(String messageTemplate) {
 		return new ConstraintViolationBuilderImpl(
 				messageTemplate,
 				getCopyOfBasePath()
@@ -168,6 +170,7 @@ protected final PathImpl getCopyOfBasePath() {
 	private ConstraintViolationCreationContext getDefaultConstraintViolationCreationContext() {
 		return new ConstraintViolationCreationContext(
 				getDefaultConstraintMessageTemplate(),
+				true, // EL is enabled for the default constraint violation
 				basePath,
 				messageParameters != null ? new HashMap<>( messageParameters ) : Collections.emptyMap(),
 				expressionVariables != null ? new HashMap<>( expressionVariables ) : Collections.emptyMap(),
@@ -178,6 +181,7 @@ private ConstraintViolationCreationContext getDefaultConstraintViolationCreation
 	private abstract class NodeBuilderBase {
 
 		protected final String messageTemplate;
+		protected boolean expressionLanguageEnabled;
 		protected PathImpl propertyPath;
 
 		protected NodeBuilderBase(String template, PathImpl path) {
@@ -189,9 +193,14 @@ public ConstraintValidatorContext addConstraintViolation() {
 			if ( constraintViolationCreationContexts == null ) {
 				constraintViolationCreationContexts = CollectionHelper.newArrayList( 3 );
 			}
+			if ( !(expressionVariables == null || expressionVariables.isEmpty()) && !expressionLanguageEnabled ) {
+				LOG.expressionVariablesDefinedWithExpressionLanguageNotEnabled(
+						constraintDescriptor.getAnnotation() != null ? constraintDescriptor.getAnnotation().annotationType() : Annotation.class );
+			}
 			constraintViolationCreationContexts.add(
 					new ConstraintViolationCreationContext(
 							messageTemplate,
+							expressionLanguageEnabled,
 							propertyPath,
 							messageParameters != null ? new HashMap<>( messageParameters ) : Collections.emptyMap(),
 							expressionVariables != null ? new HashMap<>( expressionVariables ) : Collections.emptyMap(),
@@ -202,12 +211,18 @@ public ConstraintValidatorContext addConstraintViolation() {
 		}
 	}
 
-	protected class ConstraintViolationBuilderImpl extends NodeBuilderBase implements ConstraintViolationBuilder {
+	protected class ConstraintViolationBuilderImpl extends NodeBuilderBase implements HibernateConstraintViolationBuilder {
 
 		protected ConstraintViolationBuilderImpl(String template, PathImpl path) {
 			super( template, path );
 		}
 
+		@Override
+		public HibernateConstraintViolationBuilder enableExpressionLanguage() {
+			expressionLanguageEnabled = true;
+			return this;
+		}
+
 		@Override
 		@Deprecated
 		public NodeBuilderDefinedContext addNode(String name) {
@@ -221,12 +236,12 @@ public NodeBuilderDefinedContext addNode(String name) {
 		public NodeBuilderCustomizableContext addPropertyNode(String name) {
 			dropLeafNodeIfRequired();
 
-			return new DeferredNodeBuilder( messageTemplate, propertyPath, name, ElementKind.PROPERTY );
+			return new DeferredNodeBuilder( messageTemplate, expressionLanguageEnabled, propertyPath, name, ElementKind.PROPERTY );
 		}
 
 		@Override
 		public LeafNodeBuilderCustomizableContext addBeanNode() {
-			return new DeferredNodeBuilder( messageTemplate, propertyPath, null, ElementKind.BEAN );
+			return new DeferredNodeBuilder( messageTemplate, expressionLanguageEnabled, propertyPath, null, ElementKind.BEAN );
 		}
 
 		@Override
@@ -238,7 +253,7 @@ public NodeBuilderDefinedContext addParameterNode(int index) {
 		public ContainerElementNodeBuilderCustomizableContext addContainerElementNode(String name, Class<?> containerType, Integer typeArgumentIndex) {
 			dropLeafNodeIfRequired();
 
-			return new DeferredNodeBuilder( messageTemplate, propertyPath, name, containerType, typeArgumentIndex );
+			return new DeferredNodeBuilder( messageTemplate, expressionLanguageEnabled, propertyPath, name, containerType, typeArgumentIndex );
 		}
 
 		/**
@@ -267,17 +282,17 @@ public ConstraintViolationBuilder.NodeBuilderCustomizableContext addNode(String
 
 		@Override
 		public NodeBuilderCustomizableContext addPropertyNode(String name) {
-			return new DeferredNodeBuilder( messageTemplate, propertyPath, name, ElementKind.PROPERTY );
+			return new DeferredNodeBuilder( messageTemplate, expressionLanguageEnabled, propertyPath, name, ElementKind.PROPERTY );
 		}
 
 		@Override
 		public LeafNodeBuilderCustomizableContext addBeanNode() {
-			return new DeferredNodeBuilder( messageTemplate, propertyPath, null, ElementKind.BEAN );
+			return new DeferredNodeBuilder( messageTemplate, expressionLanguageEnabled, propertyPath, null, ElementKind.BEAN );
 		}
 
 		@Override
 		public ContainerElementNodeBuilderCustomizableContext addContainerElementNode(String name, Class<?> containerType, Integer typeArgumentIndex) {
-			return new DeferredNodeBuilder( messageTemplate, propertyPath, name, containerType, typeArgumentIndex );
+			return new DeferredNodeBuilder( messageTemplate, expressionLanguageEnabled, propertyPath, name, containerType, typeArgumentIndex );
 		}
 	}
 
@@ -293,16 +308,23 @@ private class DeferredNodeBuilder extends NodeBuilderBase
 
 		private final Integer leafNodeTypeArgumentIndex;
 
-		private DeferredNodeBuilder(String template, PathImpl path, String nodeName, ElementKind leafNodeKind) {
+		private DeferredNodeBuilder(String template, boolean expressionLanguageEnabled, PathImpl path, String nodeName, ElementKind leafNodeKind) {
 			super( template, path );
+			this.expressionLanguageEnabled = expressionLanguageEnabled;
 			this.leafNodeName = nodeName;
 			this.leafNodeKind = leafNodeKind;
 			this.leafNodeContainerType = null;
 			this.leafNodeTypeArgumentIndex = null;
 		}
 
-		private DeferredNodeBuilder(String template, PathImpl path, String nodeName, Class<?> leafNodeContainerType, Integer leafNodeTypeArgumentIndex) {
+		private DeferredNodeBuilder(String template,
+				boolean expressionLanguageEnabled,
+				PathImpl path,
+				String nodeName,
+				Class<?> leafNodeContainerType,
+				Integer leafNodeTypeArgumentIndex) {
 			super( template, path );
+			this.expressionLanguageEnabled = expressionLanguageEnabled;
 			this.leafNodeName = nodeName;
 			this.leafNodeKind = ElementKind.CONTAINER_ELEMENT;
 			this.leafNodeContainerType = leafNodeContainerType;
@@ -344,19 +366,19 @@ public NodeBuilderCustomizableContext addNode(String name) {
 		@Override
 		public NodeBuilderCustomizableContext addPropertyNode(String name) {
 			addLeafNode();
-			return new DeferredNodeBuilder( messageTemplate, propertyPath, name, ElementKind.PROPERTY );
+			return new DeferredNodeBuilder( messageTemplate, expressionLanguageEnabled, propertyPath, name, ElementKind.PROPERTY );
 		}
 
 		@Override
 		public ContainerElementNodeBuilderCustomizableContext addContainerElementNode(String name, Class<?> containerType, Integer typeArgumentIndex) {
 			addLeafNode();
-			return new DeferredNodeBuilder( messageTemplate, propertyPath, name, containerType, typeArgumentIndex );
+			return new DeferredNodeBuilder( messageTemplate, expressionLanguageEnabled, propertyPath, name, containerType, typeArgumentIndex );
 		}
 
 		@Override
 		public LeafNodeBuilderCustomizableContext addBeanNode() {
 			addLeafNode();
-			return new DeferredNodeBuilder( messageTemplate, propertyPath, null, ElementKind.BEAN );
+			return new DeferredNodeBuilder( messageTemplate, expressionLanguageEnabled, propertyPath, null, ElementKind.BEAN );
 		}
 
 		@Override