Skip to content
Permalink
Browse files
disallow EL evaluation of custom message templates without explicit p…
…ermission granted via configuration property
  • Loading branch information
mbenson committed Oct 16, 2018
1 parent f76bed1 commit 7d63a22f2921e7ce3c006fb0b6f6345b4ae9b406
Showing 6 changed files with 161 additions and 81 deletions.
@@ -181,4 +181,8 @@ public GroupsComputer getGroupsComputer() {
public ConstraintCached getConstraintsCache() {
return factory.getConstraintsCache();
}

public ApacheValidatorFactory getFactory() {
return factory;
}
}
@@ -0,0 +1,36 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.bval.jsr;

import javax.validation.MessageInterpolator;
import javax.validation.MessageInterpolator.Context;

/**
* Vendor-specific {@link MessageInterpolator.Context} interface extension to
* provide access to validator configuration properties.
*/
public interface ApacheMessageContext extends Context {

/**
* Get the configuration property value specified by {@code propertyKey}, if available.
* @param propertyKey
* @return {@link String} or {@code null}
*/
String getConfigurationProperty(String propertyKey);
}
@@ -49,5 +49,15 @@ interface Properties {
* Size to use for caching of constraint-related information. Default is {@code 50}.
*/
String CONSTRAINTS_CACHE_SIZE = "apache.bval.constraints-cache-size";

/**
* Specifies whether EL evaluation is permitted in non-default message
* templates. By default this feature is disabled; if you enable it you
* should ensure that no constraint validator builds violations using
* message templates containing unchecked text (e.g. the validated
* value). To do otherwise is to expose your system to potential
* injection attacks.
*/
String CUSTOM_TEMPLATE_EXPRESSION_EVALUATION = "apache.bval.custom-template-expression-evaluation";
}
}
@@ -21,6 +21,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.MissingResourceException;
import java.util.Objects;
import java.util.Optional;
import java.util.ResourceBundle;
import java.util.concurrent.ConcurrentHashMap;
@@ -101,36 +102,19 @@ public DefaultMessageInterpolator(ResourceBundle resourceBundle) {

/** {@inheritDoc} */
@Override
public String interpolate(String message, Context context) {
public String interpolate(final String message, final Context context) {
// probably no need for caching, but it could be done by parameters since the map
// is immutable and uniquely built per Validation definition, the comparison has to be based on == and not equals though
return interpolate(message, context, defaultLocale);
}

/** {@inheritDoc} */
@Override
public String interpolate(String message, Context context, Locale locale) {
return interpolateMessage(message, context.getConstraintDescriptor().getAttributes(), locale,
context.getValidatedValue());
}

/**
* Runs the message interpolation according to algorithm specified in JSR 303.
* <br/>
* Note:
* <br/>
* Lookups in user bundles are recursive whereas lookups in default bundle are not!
*
* @param message the message to interpolate
* @param annotationParameters the parameters of the annotation for which to interpolate this message
* @param locale the <code>Locale</code> to use for the resource bundle.
* @return the interpolated message.
*/
private String interpolateMessage(String message, Map<String, Object> annotationParameters, Locale locale,
Object validatedValue) {
ResourceBundle userResourceBundle = findUserResourceBundle(locale);
ResourceBundle defaultResourceBundle = findDefaultResourceBundle(locale);
public String interpolate(final String message, final Context context, final Locale locale) {
final ResourceBundle userResourceBundle = findUserResourceBundle(locale);
final ResourceBundle defaultResourceBundle = findDefaultResourceBundle(locale);

final Map<String, Object> annotationParameters = context.getConstraintDescriptor().getAttributes();
String userBundleResolvedMessage;
String resolvedMessage = message;
boolean evaluatedDefaultBundleOnce = false;
@@ -143,24 +127,41 @@ private String interpolateMessage(String message, Map<String, Object> annotation
if (evaluatedDefaultBundleOnce && !hasReplacementTakenPlace(userBundleResolvedMessage, resolvedMessage)) {
break;
}

// search the default bundle non recursive (step2)
resolvedMessage = replaceVariables(userBundleResolvedMessage, defaultResourceBundle, locale, false);

evaluatedDefaultBundleOnce = true;
} while (true);

// resolve annotation attributes (step 4)
resolvedMessage = replaceAnnotationAttributes(resolvedMessage, annotationParameters);

// EL handling
if (evaluator != null) {
resolvedMessage = evaluator.interpolate(resolvedMessage, annotationParameters, validatedValue);
if (evaluateExpressionLanguage(message, context)) {
resolvedMessage = evaluator.interpolate(resolvedMessage, annotationParameters, context.getValidatedValue());
}

return resolveEscapeSequences(resolvedMessage);
}

private boolean evaluateExpressionLanguage(String template, Context context) {
if (evaluator != null) {
if (Objects.equals(template, context.getConstraintDescriptor().getMessageTemplate())) {
return true;
}
final Optional<ApacheMessageContext> apacheMessageContext = Optional.of(context).map(ctx -> {
try {
return ctx.unwrap(ApacheMessageContext.class);
} catch (Exception e) {
return null;
}
});
return !apacheMessageContext.isPresent() || apacheMessageContext
.map(amc -> amc.getConfigurationProperty(
ApacheValidatorConfiguration.Properties.CUSTOM_TEMPLATE_EXPRESSION_EVALUATION))
.filter(Boolean::parseBoolean).isPresent();
}
return false;
}

private String resolveEscapeSequences(String s) {
int pos = s.indexOf('\\');
if (pos < 0) {
@@ -30,6 +30,8 @@
import javax.validation.metadata.ConstraintDescriptor;
import javax.validation.metadata.CrossParameterDescriptor;

import org.apache.bval.jsr.ApacheFactoryContext;
import org.apache.bval.jsr.ApacheMessageContext;
import org.apache.bval.jsr.descriptor.ComposedD;
import org.apache.bval.jsr.descriptor.ConstraintD;
import org.apache.bval.jsr.descriptor.CrossParameterD;
@@ -43,7 +45,7 @@
import org.apache.bval.util.Lazy;
import org.apache.bval.util.Validate;

public class ConstraintValidatorContextImpl<T> implements ConstraintValidatorContext, MessageInterpolator.Context {
public class ConstraintValidatorContextImpl<T> implements ConstraintValidatorContext, ApacheMessageContext {
public class ConstraintViolationBuilderImpl implements ConstraintValidatorContext.ConstraintViolationBuilder {
private final String template;
private final PathImpl path;
@@ -201,4 +203,9 @@ public Object getValidatedValue() {
private void addError(String messageTemplate, PathImpl propertyPath) {
violations.get().add(((ValidationJob) frame.getJob()).createViolation(messageTemplate, this, propertyPath));
}

@Override
public String getConfigurationProperty(String propertyKey) {
return frame.context.getValidatorContext().getFactory().getProperties().get(propertyKey);
}
}
@@ -23,6 +23,8 @@
import static org.junit.Assert.assertTrue;
import static org.junit.Assume.assumeThat;
import static org.junit.Assume.assumeTrue;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.when;

import java.lang.annotation.Annotation;
import java.net.URL;
@@ -42,6 +44,7 @@
import javax.validation.metadata.ConstraintDescriptor;

import org.apache.bval.constraints.NotEmpty;
import org.apache.bval.jsr.ApacheValidatorConfiguration;
import org.apache.bval.jsr.example.Author;
import org.apache.bval.jsr.example.PreferredGuest;
import org.junit.After;
@@ -51,6 +54,7 @@
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
import org.mockito.Mockito;

/**
* MessageResolverImpl Tester.
@@ -75,26 +79,6 @@ private static Predicate<ConstraintDescriptor<?>> forConstraintType(Class<? exte
return d -> Objects.equals(type, d.getAnnotation().annotationType());
}

private static MessageInterpolator.Context context(Object validatedValue, Supplier<ConstraintDescriptor<?>> descriptor){
return new MessageInterpolator.Context() {

@Override
public <T> T unwrap(Class<T> type) {
return null;
}

@Override
public Object getValidatedValue() {
return validatedValue;
}

@Override
public ConstraintDescriptor<?> getConstraintDescriptor() {
return descriptor.get();
}
};
}

private String elImpl;
private String elFactory;
private DefaultMessageInterpolator interpolator;
@@ -203,6 +187,23 @@ public void testRecursiveInterpolation() {
public void testNoELAvailable() {
assumeThat(elImpl, equalTo("invalid"));
assertFalse(elAvailable);

ApacheMessageContext context = context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")));

when(context
.getConfigurationProperty(ApacheValidatorConfiguration.Properties.CUSTOM_TEMPLATE_EXPRESSION_EVALUATION))
.thenAnswer(invocation -> Boolean.toString(true));

assertEquals("${regexp.charAt(4)}", interpolator.interpolate("${regexp.charAt(4)}",
context));
}

@Test
public void testDisallowCustomTemplateExpressionEvaluationByDefault() {
assumeTrue(elAvailable);

assertEquals("${regexp.charAt(4)}", interpolator.interpolate("${regexp.charAt(4)}",
context("12345678",
@@ -215,24 +216,26 @@ public void testNoELAvailable() {
public void testExpressionLanguageEvaluation() {
assumeTrue(elAvailable);

assertEquals("Expected value of length 8 to match pattern",
interpolator.interpolate("Expected value of length ${validatedValue.length()} to match pattern",
context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")))));
final MessageInterpolator.Context context = context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("anotherValue")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")));

assertEquals("Another value should match ....$",
interpolator.interpolate(context.getConstraintDescriptor().getMessageTemplate(), context));
}

@Test
public void testMixedEvaluation() {
assumeTrue(elAvailable);

assertEquals("Expected value of length 8 to match pattern ....$",
interpolator.interpolate("Expected value of length ${validatedValue.length()} to match pattern {regexp}",
context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")))));
final MessageInterpolator.Context context = context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("mixedMessageValue")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")));

assertEquals("Mixed message value of length 8 should match ....$",
interpolator.interpolate(context.getConstraintDescriptor().getMessageTemplate(), context));
}

@Test
@@ -245,44 +248,47 @@ public void testELEscapingTomcatJuel() {
// non-escaped $, but that would only expose us to inconsistency for composite expressions containing more
// than one component EL expression

ApacheMessageContext context = context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")));

when(context
.getConfigurationProperty(ApacheValidatorConfiguration.Properties.CUSTOM_TEMPLATE_EXPRESSION_EVALUATION))
.thenAnswer(invocation -> Boolean.toString(true));

assertEquals("${regexp.charAt(4)}", interpolator.interpolate("\\${regexp.charAt(4)}",
context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")))));
context));

assertEquals("${regexp.charAt(4)}", interpolator.interpolate("\\\\${regexp.charAt(4)}",
context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")))));
context));
}

@Test
public void testELEscapingRI() {
assumeTrue(elAvailable);
assumeThat(elImpl, equalTo("ri"));

ApacheMessageContext context = context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")));

when(context
.getConfigurationProperty(ApacheValidatorConfiguration.Properties.CUSTOM_TEMPLATE_EXPRESSION_EVALUATION))
.thenAnswer(invocation -> Boolean.toString(true));

assertEquals("returns literal", "${regexp.charAt(4)}",
interpolator.interpolate("\\${regexp.charAt(4)}",
context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")))));
context));

assertEquals("returns literal \\ followed by $, later interpreted as an escape sequence", "$",
interpolator.interpolate("\\\\${regexp.charAt(4)}",
context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")))));
context));

assertEquals("returns literal \\ followed by .", "\\.",
interpolator.interpolate("\\\\${regexp.charAt(3)}",
context("12345678",
() -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber")
.getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst()
.orElseThrow(() -> new AssertionError("expected constraint missing")))));
context));
}

@Test
@@ -309,12 +315,28 @@ public void testEscapedELPattern() {
.orElseThrow(() -> new AssertionError("expected constraint missing")))));
}

@SuppressWarnings("unchecked")
private ApacheMessageContext context(Object validatedValue, Supplier<ConstraintDescriptor<?>> descriptor) {
final ApacheMessageContext result = Mockito.mock(ApacheMessageContext.class);
when(result.unwrap(any(Class.class)))
.thenAnswer(invocation -> invocation.getArgumentAt(0, Class.class).cast(result));
when(result.getValidatedValue()).thenReturn(validatedValue);
when(result.getConstraintDescriptor()).thenAnswer(invocation -> descriptor.get());
return result;
}

public static class Person {

@Pattern(message = "Id number should match {regexp}", regexp = "....$")
public String idNumber;

@Pattern(message = "Other id should match {regexp}", regexp = ".\\n")
public String otherId;

@Pattern(message = "Another value should match ${regexp.intern()}", regexp = "....$")
public String anotherValue;

@Pattern(message = "Mixed message value of length ${validatedValue.length()} should match {regexp}", regexp = "....$")
public String mixedMessageValue;
}
}

0 comments on commit 7d63a22

Please sign in to comment.