Skip to content

Commit

Permalink
Disable message interpolation in ConstraintViolations by default (#3208)
Browse files Browse the repository at this point in the history
Disable message interpolation in ConstraintViolations by default but allow enabling it explicitly with `SelfValidating#escapeExpressions()`.

Additionally, `ConstraintViolations` now provides a set of methods which take a map of message parameters for interpolation.
The message parameters will be escaped by default.

Refs #3153
Refs #3157
  • Loading branch information
joschi committed Mar 26, 2020
1 parent 6bd2cba commit d5a512f
Show file tree
Hide file tree
Showing 5 changed files with 222 additions and 31 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* 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 io.dropwizard.validation;

import javax.annotation.Nullable;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
* Utilities used for message interpolation.
*
* @author Guillaume Smet
* @since 2.0.3
*/
public final class InterpolationHelper {

public static final char BEGIN_TERM = '{';
public static final char END_TERM = '}';
public static final char EL_DESIGNATOR = '$';
public static final char ESCAPE_CHARACTER = '\\';

private static final Pattern ESCAPE_MESSAGE_PARAMETER_PATTERN = Pattern.compile("([\\" + ESCAPE_CHARACTER + BEGIN_TERM + END_TERM + EL_DESIGNATOR + "])");

private InterpolationHelper() {
}

@Nullable
public static String escapeMessageParameter(@Nullable String messageParameter) {
if (messageParameter == null) {
return null;
}
return ESCAPE_MESSAGE_PARAMETER_PATTERN.matcher(messageParameter).replaceAll(Matcher.quoteReplacement(String.valueOf(ESCAPE_CHARACTER)) + "$1");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.util.Map;

/**
* The annotated element has methods annotated by
Expand All @@ -24,4 +25,16 @@
Class<?>[] groups() default {};

Class<? extends Payload>[] payload() default {};

/**
* Escape EL expressions to avoid template injection attacks.
* <p>
* This has serious security implications and you will
* have to escape the violation messages added to {@link ViolationCollector} appropriately.
*
* @see ViolationCollector#addViolation(String, Map)
* @see ViolationCollector#addViolation(String, String, Map)
* @see ViolationCollector#addViolation(String, Integer, String, Map)
*/
boolean escapeExpressions() default true;
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,17 @@ public class SelfValidatingValidator implements ConstraintValidator<SelfValidati
private final AnnotationConfiguration annotationConfiguration = new AnnotationConfiguration.StdConfiguration(AnnotationInclusion.INCLUDE_AND_INHERIT_IF_INHERITED);
private final TypeResolver typeResolver = new TypeResolver();
private final MemberResolver memberResolver = new MemberResolver(typeResolver);
private boolean escapeExpressions = true;

@Override
public void initialize(SelfValidating constraintAnnotation) {
escapeExpressions = constraintAnnotation.escapeExpressions();
}

@SuppressWarnings({"unchecked", "rawtypes"})
@Override
public boolean isValid(Object value, ConstraintValidatorContext context) {
final ViolationCollector collector = new ViolationCollector(context);
final ViolationCollector collector = new ViolationCollector(context, escapeExpressions);
context.disableDefaultConstraintViolation();
for (ValidationCaller caller : methodMap.computeIfAbsent(value.getClass(), this::findMethods)) {
caller.setValidationObject(value);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,64 +1,116 @@
package io.dropwizard.validation.selfvalidating;

import org.hibernate.validator.constraintvalidation.HibernateConstraintValidatorContext;

import javax.annotation.Nullable;
import javax.validation.ConstraintValidatorContext;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.Collections;
import java.util.Map;

import static io.dropwizard.validation.InterpolationHelper.escapeMessageParameter;

/**
* This class is a simple wrapper around the ConstraintValidatorContext of hibernate validation.
* It collects all the violations of the SelfValidation methods of an object.
*/
public class ViolationCollector {
private static final Pattern ESCAPE_PATTERN = Pattern.compile("\\$\\{");
private final ConstraintValidatorContext constraintValidatorContext;
private final boolean escapeExpressions;

private boolean violationOccurred = false;
private ConstraintValidatorContext context;

public ViolationCollector(ConstraintValidatorContext constraintValidatorContext) {
this(constraintValidatorContext, true);
}

public ViolationCollector(ConstraintValidatorContext context) {
this.context = context;
public ViolationCollector(ConstraintValidatorContext constraintValidatorContext, boolean escapeExpressions) {
this.constraintValidatorContext = constraintValidatorContext;
this.escapeExpressions = escapeExpressions;
}

/**
* Adds a new violation to this collector. This also sets {@code violationOccurred} to {@code true}.
* <p>
* Prefer the method with explicit message parameters if you want to interpolate the message.
*
* @param message the message of the violation (any EL expression will be escaped and not parsed)
* @param message the message of the violation
* @see #addViolation(String, Map)
*/
public void addViolation(String message) {
addViolation(message, Collections.emptyMap());
}

/**
* Adds a new violation to this collector. This also sets {@code violationOccurred} to {@code true}.
*
* @param message the message of the violation
* @param messageParameters a map of message parameters which can be interpolated in the violation message
* @since 2.0.3
*/
public void addViolation(String message, Map<String, Object> messageParameters) {
violationOccurred = true;
String messageTemplate = escapeEl(message);
context.buildConstraintViolationWithTemplate(messageTemplate)
getContextWithMessageParameters(messageParameters)
.buildConstraintViolationWithTemplate(sanitizeTemplate(message))
.addConstraintViolation();
}

/**
* Adds a new violation to this collector. This also sets {@code violationOccurred} to {@code true}.
* <p>
* Prefer the method with explicit message parameters if you want to interpolate the message.
*
* @param propertyName the name of the property
* @param message the message of the violation (any EL expression will be escaped and not parsed)
* @param message the message of the violation
* @see #addViolation(String, String, Map)
* @since 2.0.2
*/
public void addViolation(String propertyName, String message) {
addViolation(propertyName, message, Collections.emptyMap());
}

/**
* Adds a new violation to this collector. This also sets {@code violationOccurred} to {@code true}.
*
* @param propertyName the name of the property
* @param message the message of the violation
* @param messageParameters a map of message parameters which can be interpolated in the violation message
* @since 2.0.3
*/
public void addViolation(String propertyName, String message, Map<String, Object> messageParameters) {
violationOccurred = true;
String messageTemplate = escapeEl(message);
context.buildConstraintViolationWithTemplate(messageTemplate)
getContextWithMessageParameters(messageParameters)
.buildConstraintViolationWithTemplate(sanitizeTemplate(message))
.addPropertyNode(propertyName)
.addConstraintViolation();
}

/**
* Adds a new violation to this collector. This also sets {@code violationOccurred} to {@code true}.
* Prefer the method with explicit message parameters if you want to interpolate the message.
*
* @param propertyName the name of the property with the violation
* @param index the index of the element with the violation
* @param message the message of the violation (any EL expression will be escaped and not parsed)
* @see ViolationCollector#addViolation(String, Integer, String, Map)
* @since 2.0.2
*/
public void addViolation(String propertyName, Integer index, String message) {
addViolation(propertyName, index, message, Collections.emptyMap());
}

/**
* Adds a new violation to this collector. This also sets {@code violationOccurred} to {@code true}.
*
* @param propertyName the name of the property with the violation
* @param index the index of the element with the violation
* @param message the message of the violation
* @param messageParameters a map of message parameters which can be interpolated in the violation message
* @since 2.0.3
*/
public void addViolation(String propertyName, Integer index, String message, Map<String, Object> messageParameters) {
violationOccurred = true;
String messageTemplate = escapeEl(message);
context.buildConstraintViolationWithTemplate(messageTemplate)
getContextWithMessageParameters(messageParameters)
.buildConstraintViolationWithTemplate(sanitizeTemplate(message))
.addPropertyNode(propertyName)
.addBeanNode().inIterable().atIndex(index)
.addConstraintViolation();
Expand All @@ -69,32 +121,46 @@ public void addViolation(String propertyName, Integer index, String message) {
*
* @param propertyName the name of the property with the violation
* @param key the key of the element with the violation
* @param message the message of the violation (any EL expression will be escaped and not parsed)
* @param message the message of the violation
* @since 2.0.2
*/
public void addViolation(String propertyName, String key, String message) {
addViolation(propertyName, key, message, Collections.emptyMap());
}

/**
* Adds a new violation to this collector. This also sets {@code violationOccurred} to {@code true}.
*
* @param propertyName the name of the property with the violation
* @param key the key of the element with the violation
* @param message the message of the violation
* @param messageParameters a map of message parameters which can be interpolated in the violation message
* @since 2.0.3
*/
public void addViolation(String propertyName, String key, String message, Map<String, Object> messageParameters) {
violationOccurred = true;
String messageTemplate = escapeEl(message);
final String messageTemplate = sanitizeTemplate(message);
final HibernateConstraintValidatorContext context = getContextWithMessageParameters(messageParameters);
context.buildConstraintViolationWithTemplate(messageTemplate)
.addPropertyNode(propertyName)
.addBeanNode().inIterable().atKey(key)
.addConstraintViolation();
}

@Nullable
private String escapeEl(@Nullable String s) {
if (s == null || s.isEmpty()) {
return s;
}

final Matcher m = ESCAPE_PATTERN.matcher(s);
final StringBuffer sb = new StringBuffer(s.length() + 16);
while (m.find()) {
m.appendReplacement(sb, "\\\\\\${");
private HibernateConstraintValidatorContext getContextWithMessageParameters(Map<String, Object> messageParameters) {
final HibernateConstraintValidatorContext context =
constraintValidatorContext.unwrap(HibernateConstraintValidatorContext.class);
for (Map.Entry<String, Object> messageParameter : messageParameters.entrySet()) {
final Object value = messageParameter.getValue();
final String escapedValue = value == null ? null : escapeMessageParameter(value.toString());
context.addMessageParameter(messageParameter.getKey(), escapedValue);
}
m.appendTail(sb);
return context;
}

return sb.toString();
@Nullable
private String sanitizeTemplate(@Nullable String message) {
return escapeExpressions ? escapeMessageParameter(message) : message;
}

/**
Expand All @@ -104,7 +170,7 @@ private String escapeEl(@Nullable String s) {
* @return the wrapped Hibernate ConstraintValidatorContext
*/
public ConstraintValidatorContext getContext() {
return context;
return constraintValidatorContext;
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.dropwizard.validation;

import io.dropwizard.util.Maps;
import io.dropwizard.validation.selfvalidating.SelfValidating;
import io.dropwizard.validation.selfvalidating.SelfValidation;
import io.dropwizard.validation.selfvalidating.ViolationCollector;
Expand All @@ -12,6 +13,7 @@

import javax.annotation.concurrent.NotThreadSafe;
import javax.validation.Validator;
import java.util.Collections;

import static org.assertj.core.api.Assertions.assertThat;

Expand Down Expand Up @@ -146,12 +148,48 @@ public static class InjectionExample {
@SelfValidation
public void validateFail(ViolationCollector col) {
col.addViolation("${'value'}");
col.addViolation("$\\A{1+1}");
col.addViolation("{value}", Collections.singletonMap("value", "TEST"));
col.addViolation("${'property'}", "${'value'}");
col.addViolation("${'property'}", 1, "${'value'}");
col.addViolation("${'property'}", "${'key'}", "${'value'}");
}
}

@SelfValidating(escapeExpressions = false)
public static class EscapingDisabledExample {
@SuppressWarnings("unused")
@SelfValidation
public void validateFail(ViolationCollector col) {
col.addViolation("${'value'}");
col.addViolation("$\\A{1+1}");
col.addViolation("{value}", Collections.singletonMap("value", "TEST"));
col.addViolation("${'property'}", "${'value'}");
col.addViolation("${'property'}", 1, "${'value'}");
col.addViolation("${'property'}", "${'key'}", "${'value'}");
}
}

@SelfValidating(escapeExpressions = false)
public static class MessageParametersExample {
@SuppressWarnings("unused")
@SelfValidation
public void validateFail(ViolationCollector col) {
col.addViolation("{1+1}");
col.addViolation("{value}", Collections.singletonMap("value", "VALUE"));
col.addViolation("No parameter", Collections.singletonMap("value", "VALUE"));
col.addViolation("{value} {unsetParameter}", Collections.singletonMap("value", "VALUE"));
col.addViolation("{value", Collections.singletonMap("value", "VALUE"));
col.addViolation("value}", Collections.singletonMap("value", "VALUE"));
col.addViolation("{ value }", Collections.singletonMap("value", "VALUE"));
col.addViolation("Mixed ${'value'} {value}", Collections.singletonMap("value", "VALUE"));
col.addViolation("Nested {value}", Collections.singletonMap("value", "${'nested'}"));
col.addViolation("{property}", "{value}", Maps.of("property", "PROPERTY", "value", "VALUE"));
col.addViolation("{property}", 1, "{value}", Maps.of("property", "PROPERTY", "value", "VALUE"));
col.addViolation("{property}", "{key}", "{value}", Maps.of("property", "PROPERTY", "key", "KEY", "value", "VALUE"));
}
}

private final Validator validator = BaseValidator.newValidator();

@Test
Expand Down Expand Up @@ -271,13 +309,47 @@ public void giveWarningIfNoValidationMethods() {
}

@Test
public void violationMessagesAreEscaped() {
public void violationMessagesAreEscapedByDefault() {
assertThat(ConstraintViolations.format(validator.validate(new InjectionExample()))).containsExactly(
" $\\A{1+1}",
" ${'value'}",
" {value}",
"${'property'} ${'value'}",
"${'property'}[${'key'}] ${'value'}",
"${'property'}[1] ${'value'}"
);
assertThat(TestLoggerFactory.getAllLoggingEvents()).isEmpty();
}

@Test
public void violationMessagesAreInterpolatedIfEscapingDisabled() {
assertThat(ConstraintViolations.format(validator.validate(new EscapingDisabledExample()))).containsExactly(
" A2",
" TEST",
" value",
"${'property'} value",
"${'property'}[${'key'}] value",
"${'property'}[1] value"
);
assertThat(TestLoggerFactory.getAllLoggingEvents()).isEmpty();
}

@Test
public void messageParametersExample() {
assertThat(ConstraintViolations.format(validator.validate(new MessageParametersExample()))).containsExactly(
" Mixed value VALUE",
" Nested ${'nested'}",
" No parameter",
" VALUE",
" VALUE {unsetParameter}",
" value}",
" { value }",
" {1+1}",
" {value",
"{property} VALUE",
"{property}[1] VALUE",
"{property}[{key}] VALUE"
);
assertThat(TestLoggerFactory.getAllLoggingEvents()).isEmpty();
}
}

0 comments on commit d5a512f

Please sign in to comment.