Validate ReturnTypes at the top-level so that accessing values of

arrays, lists, maps have their return values validated

Author: jasmith-hs

src/main/java/com/hubspot/jinjava/el/ExpressionResolver.java

@@ -41,7 +41,7 @@ public class ExpressionResolver {
 
   private final JinjavaInterpreter interpreter;
   private final ExpressionFactory expressionFactory;
-  private final JinjavaInterpreterResolver resolver;
+  private final ReturnTypeValidatingJinjavaInterpreterResolver resolver;
   private final JinjavaELContext elContext;
   private final ObjectUnwrapper objectUnwrapper;
 
@@ -55,7 +55,11 @@ public ExpressionResolver(JinjavaInterpreter interpreter, Jinjava jinjava) {
         ? jinjava.getEagerExpressionFactory()
         : jinjava.getExpressionFactory();
 
-    this.resolver = new JinjavaInterpreterResolver(interpreter);
+    this.resolver =
+      new ReturnTypeValidatingJinjavaInterpreterResolver(
+        interpreter.getConfig().getReturnTypeValidator(),
+        new JinjavaInterpreterResolver(interpreter)
+      );
     this.elContext = new JinjavaELContext(interpreter, resolver);
     for (ELFunctionDefinition fn : jinjava.getGlobalContext().getAllFunctions()) {
       this.elContext.setFunction(fn.getNamespace(), fn.getLocalName(), fn.getMethod());

src/main/java/com/hubspot/jinjava/el/ReturnTypeValidatingJinjavaInterpreterResolver.java

@@ -0,0 +1,69 @@
+package com.hubspot.jinjava.el;
+
+import com.hubspot.jinjava.el.ext.AllowlistReturnTypeValidator;
+import java.beans.FeatureDescriptor;
+import java.util.Iterator;
+import javax.el.ELContext;
+import javax.el.ELResolver;
+
+class ReturnTypeValidatingJinjavaInterpreterResolver extends ELResolver {
+
+  private final AllowlistReturnTypeValidator returnTypeValidator;
+  private final JinjavaInterpreterResolver delegate;
+
+  ReturnTypeValidatingJinjavaInterpreterResolver(
+    AllowlistReturnTypeValidator returnTypeValidator,
+    JinjavaInterpreterResolver delegate
+  ) {
+    this.returnTypeValidator = returnTypeValidator;
+    this.delegate = delegate;
+  }
+
+  @Override
+  public Class<?> getCommonPropertyType(ELContext context, Object base) {
+    return delegate.getCommonPropertyType(context, base);
+  }
+
+  @Override
+  public Iterator<FeatureDescriptor> getFeatureDescriptors(
+    ELContext context,
+    Object base
+  ) {
+    return delegate.getFeatureDescriptors(context, base);
+  }
+
+  @Override
+  public Class<?> getType(ELContext context, Object base, Object property) {
+    return delegate.getType(context, base, property);
+  }
+
+  @Override
+  public Object getValue(ELContext context, Object base, Object property) {
+    return returnTypeValidator.validateReturnType(
+      delegate.getValue(context, base, property)
+    );
+  }
+
+  @Override
+  public boolean isReadOnly(ELContext context, Object base, Object property) {
+    return delegate.isReadOnly(context, base, property);
+  }
+
+  @Override
+  public void setValue(ELContext context, Object base, Object property, Object value) {
+    delegate.setValue(context, base, property, value);
+  }
+
+  @Override
+  public Object invoke(
+    ELContext context,
+    Object base,
+    Object method,
+    Class<?>[] paramTypes,
+    Object[] params
+  ) {
+    return returnTypeValidator.validateReturnType(
+      delegate.invoke(context, base, method, paramTypes, params)
+    );
+  }
+}

src/main/java/com/hubspot/jinjava/el/ext/JinjavaBeanELResolver.java

@@ -70,9 +70,7 @@ public Class<?> getType(ELContext context, Object base, Object property) {
 
   @Override
   public Object getValue(ELContext context, Object base, Object property) {
-    return getJinjavaConfig()
-      .getReturnTypeValidator()
-      .validateReturnType(super.getValue(context, base, transformPropertyName(property)));
+    return super.getValue(context, base, transformPropertyName(property));
   }
 
   @Override
@@ -111,9 +109,7 @@ public Object invoke(
       );
     }
 
-    return getJinjavaConfig()
-      .getReturnTypeValidator()
-      .validateReturnType(super.invoke(context, base, method, paramTypes, params));
+    return super.invoke(context, base, method, paramTypes, params);
   }
 
   @Override
@@ -181,7 +177,7 @@ private static JinjavaConfig getJinjavaConfig() {
     return Objects
       .requireNonNull(
         JinjavaInterpreter.getCurrent(),
-        "JinjavaInterpreter.closeablePushCurrent must be used if using a JinjavaInterpreter directly"
+        "JinjavaInterpreter.closeablePushCurrent must be used if using a JinjavaBeanELResolver directly"
       )
       .getConfig();
   }

src/test/java/com/hubspot/jinjava/el/ext/JinjavaBeanELResolverTest.java

@@ -1,53 +1,25 @@
 package com.hubspot.jinjava.el.ext;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
 
-import com.hubspot.jinjava.JinjavaConfig;
+import com.hubspot.jinjava.Jinjava;
 import com.hubspot.jinjava.el.JinjavaELContext;
-import com.hubspot.jinjava.interpret.AutoCloseableSupplier;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
 import javax.el.ELContext;
-import javax.el.MethodNotFoundException;
-import javax.el.PropertyNotFoundException;
 import org.junit.Before;
 import org.junit.Test;
 
 public class JinjavaBeanELResolverTest {
 
   private JinjavaBeanELResolver jinjavaBeanELResolver;
   private ELContext elContext;
-
-  JinjavaInterpreter interpreter = mock(JinjavaInterpreter.class);
-  JinjavaConfig config = mock(JinjavaConfig.class);
+  private Jinjava jinjava;
 
   @Before
   public void setUp() throws Exception {
-    jinjavaBeanELResolver =
-      new JinjavaBeanELResolver(
-        AllowlistMethodValidator.create(
-          MethodValidatorConfig
-            .builder()
-            .addDefaultAllowlistGroups()
-            .addAllowedDeclaredMethodsFromCanonicalClassPrefixes(
-              JinjavaBeanELResolverTest.class.getCanonicalName()
-            )
-            .build()
-        ),
-        AllowlistReturnTypeValidator.create(
-          ReturnTypeValidatorConfig
-            .builder()
-            .addDefaultAllowlistGroups()
-            .addAllowedCanonicalClassPrefixes(
-              JinjavaBeanELResolverTest.class.getCanonicalName()
-            )
-            .build()
-        )
-      );
+    jinjavaBeanELResolver = new JinjavaBeanELResolver();
     elContext = new JinjavaELContext();
-    when(interpreter.getConfig()).thenReturn(config);
+    jinjava = new Jinjava();
   }
 
   @Test
@@ -139,40 +111,10 @@ public void itPrefersPrimitives() {
       .isEqualTo("int Integer"); // should be "Number int", but we can't figure that out
   }
 
-  @Test
-  public void itThrowsExceptionWhenMethodIsRestrictedFromConfig() {
-    JinjavaInterpreter.pushCurrent(interpreter);
-    //    when(config.getRestrictedMethods()).thenReturn(ImmutableSet.of("foo"));
-    assertThatThrownBy(() ->
-        jinjavaBeanELResolver.invoke(elContext, "abcd", "foo", null, new Object[] { 1 })
-      )
-      .isInstanceOf(MethodNotFoundException.class)
-      .hasMessageStartingWith("Cannot find method 'foo'");
-    JinjavaInterpreter.popCurrent();
-  }
-
-  @Test
-  public void itThrowsExceptionWhenPropertyIsRestrictedFromConfig() {
-    JinjavaInterpreter.pushCurrent(interpreter);
-    //    when(config.getRestrictedProperties()).thenReturn(ImmutableSet.of("property1"));
-    assertThatThrownBy(() ->
-        jinjavaBeanELResolver.getValue(elContext, "abcd", "property1")
-      )
-      .isInstanceOf(PropertyNotFoundException.class)
-      .hasMessageStartingWith("Could not find property");
-    JinjavaInterpreter.popCurrent();
-  }
-
   @Test
   public void itDoesNotAllowAccessingPropertiesOfInterpreter() {
-    try (
-      AutoCloseableSupplier.AutoCloseableImpl<JinjavaInterpreter> c = JinjavaInterpreter
-        .closeablePushCurrent(interpreter)
-        .get()
-    ) {
-      assertThat(jinjavaBeanELResolver.getValue(elContext, interpreter, "config"))
-        .isNull();
-    }
+    JinjavaInterpreter interpreter = jinjava.newInterpreter();
+    assertThat(jinjavaBeanELResolver.getValue(elContext, interpreter, "config")).isNull();
   }
 
   private static class TempItInvokesBestMethodWithSingleParam {

src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstDotTest.java

@@ -56,8 +56,11 @@ public void setup() {
 
   @Test
   public void itDefersWhenDotThrowsDeferredValueException() {
-    interpreter.getContext().put("foo", new Foo());
-    assertThat(interpreter.render("{{ foo.deferred }}")).isEqualTo("{{ foo.deferred }}");
+    try (var a = JinjavaInterpreter.closeablePushCurrent(interpreter).get()) {
+      interpreter.getContext().put("foo", new Foo());
+      assertThat(interpreter.render("{{ foo.deferred }}"))
+        .isEqualTo("{{ foo.deferred }}");
+    }
   }
 
   @Test

src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java

@@ -273,6 +273,7 @@ public void itSplitsAndIndexesOnNonWords() {
 
   @Test
   public void itSupportsOrderOfOperations() {
+    eagerResolveExpression("['a','b'][1]");
     EagerExpressionResult eagerExpressionResult = eagerResolveExpression(
       "[0,1]|reverse + deferred"
     );