Don't allow jinjava constructs outside of the AllowlistGroup classes

jasmith-hs · jasmith-hs · commit 2a8ef10172fc · 2026-02-20T18:17:23.000-05:00
from being allowlisted
diff --git a/pom.xml b/pom.xml
@@ -189,6 +189,7 @@
     <dependency>
       <groupId>com.google.errorprone</groupId>
       <artifactId>error_prone_annotations</artifactId>
+      <scope>runtime</scope>
     </dependency>
 
     <dependency>
diff --git a/src/main/java/com/hubspot/jinjava/JinjavaConfig.java b/src/main/java/com/hubspot/jinjava/JinjavaConfig.java
@@ -48,7 +48,6 @@
 import java.time.ZoneOffset;
 import java.util.Locale;
 import java.util.function.BiConsumer;
-import javax.annotation.Nullable;
 import javax.el.ELResolver;
 import org.immutables.value.Value;
 
@@ -204,17 +203,11 @@ default boolean isEnableFilterChainOptimization() {
     return false;
   }
 
-  @Nullable
-  ObjectMapper getObjectMapperOrNull();
-
-  @Value.Derived
+  @Value.Default
   default ObjectMapper getObjectMapper() {
-    ObjectMapper objectMapper = getObjectMapperOrNull();
-    if (objectMapper == null) {
-      objectMapper = new ObjectMapper().registerModule(new Jdk8Module());
-      if (getLegacyOverrides().isUseSnakeCasePropertyNaming()) {
-        objectMapper.setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE);
-      }
+    ObjectMapper objectMapper = new ObjectMapper().registerModule(new Jdk8Module());
+    if (getLegacyOverrides().isUseSnakeCasePropertyNaming()) {
+      objectMapper.setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE);
     }
     return objectMapper;
   }
diff --git a/src/main/java/com/hubspot/jinjava/JinjavaImmutableStyle.java b/src/main/java/com/hubspot/jinjava/JinjavaImmutableStyle.java
@@ -1,16 +1,25 @@
 package com.hubspot.jinjava;
 
+import com.hubspot.immutable.collection.encoding.ImmutableListEncodingEnabled;
+import com.hubspot.immutable.collection.encoding.ImmutableMapEncodingEnabled;
+import com.hubspot.immutable.collection.encoding.ImmutableSetEncodingEnabled;
 import org.immutables.value.Value;
 
 @Value.Style(
   init = "set*",
   get = { "is*", "get*" } // Detect 'get' and 'is' prefixes in accessor methods
 )
+@ImmutableSetEncodingEnabled
+@ImmutableListEncodingEnabled
+@ImmutableMapEncodingEnabled
 public @interface JinjavaImmutableStyle {
   @Value.Style(
     init = "with*",
     get = { "is*", "get*" } // Detect 'get' and 'is' prefixes in accessor methods
   )
+  @ImmutableSetEncodingEnabled
+  @ImmutableListEncodingEnabled
+  @ImmutableMapEncodingEnabled
   @interface WithStyle {
   }
 }
diff --git a/src/main/java/com/hubspot/jinjava/LegacyOverrides.java b/src/main/java/com/hubspot/jinjava/LegacyOverrides.java
@@ -1,15 +1,9 @@
 package com.hubspot.jinjava;
 
-import com.hubspot.immutable.collection.encoding.ImmutableListEncodingEnabled;
-import com.hubspot.immutable.collection.encoding.ImmutableMapEncodingEnabled;
-import com.hubspot.immutable.collection.encoding.ImmutableSetEncodingEnabled;
 import org.immutables.value.Value;
 
 @Value.Immutable(singleton = true)
-@Value.Style(init = "with*", get = { "is*", "get*" })
-@ImmutableSetEncodingEnabled
-@ImmutableListEncodingEnabled
-@ImmutableMapEncodingEnabled
+@JinjavaImmutableStyle.WithStyle
 public interface LegacyOverrides extends WithLegacyOverrides {
   LegacyOverrides NONE = new Builder().build();
   LegacyOverrides THREE_POINT_0 = new Builder()
diff --git a/src/main/java/com/hubspot/jinjava/el/ext/AllowlistGroup.java b/src/main/java/com/hubspot/jinjava/el/ext/AllowlistGroup.java
@@ -24,6 +24,7 @@
 import com.hubspot.jinjava.util.ForLoop;
 import java.lang.reflect.Method;
 import java.math.BigDecimal;
+import java.time.ZonedDateTime;
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
 import java.util.Map;
@@ -150,7 +151,19 @@ String[] allowedReturnTypeCanonicalClassPrefixes() {
       return ARRAY;
     }
   },
-  JinjavaFunctions,
+  JinjavaFunctions {
+    private static final String[] ARRAY = { ZonedDateTime.class.getCanonicalName() };
+
+    @Override
+    String[] allowedDeclaredMethodsFromClasses() {
+      return ARRAY;
+    }
+
+    @Override
+    String[] allowedReturnTypeClasses() {
+      return ARRAY;
+    }
+  },
   JinjavaExpTests {
     private static final String[] ARRAY = { ExpTest.class.getPackageName() };
 
diff --git a/src/main/java/com/hubspot/jinjava/el/ext/AllowlistMethodValidator.java b/src/main/java/com/hubspot/jinjava/el/ext/AllowlistMethodValidator.java
@@ -4,6 +4,7 @@
 import com.google.common.collect.ImmutableSet;
 import java.lang.reflect.Method;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Consumer;
 
 public final class AllowlistMethodValidator {
 
@@ -14,6 +15,7 @@ public final class AllowlistMethodValidator {
   private final ImmutableSet<Method> allowedMethods;
   private final ImmutableSet<String> allowedDeclaredMethodsFromCanonicalClassPrefixes;
   private final ImmutableSet<String> allowedDeclaredMethodsFromCanonicalClassNames;
+  private final Consumer<Method> onRejectedMethod;
   private final ImmutableList<MethodValidator> additionalValidators;
 
   public static AllowlistMethodValidator create(
@@ -35,6 +37,7 @@ private AllowlistMethodValidator(
       methodValidatorConfig.allowedDeclaredMethodsFromCanonicalClassPrefixes();
     this.allowedDeclaredMethodsFromCanonicalClassNames =
       methodValidatorConfig.allowedDeclaredMethodsFromCanonicalClassNames();
+    this.onRejectedMethod = methodValidatorConfig.onRejectedMethod();
     this.additionalValidators = additionalValidators;
     this.allowedMethodsCache = new ConcurrentHashMap<>();
   }
@@ -58,11 +61,12 @@ public Method validateMethod(Method m) {
       }
     );
     if (!isAllowedMethod) {
+      onRejectedMethod.accept(m);
       return null;
     }
     for (MethodValidator v : additionalValidators) {
-      m = v.validateMethod(m);
-      if (m == null) {
+      if (v.validateMethod(m) == null) {
+        onRejectedMethod.accept(m);
         return null;
       }
     }
diff --git a/src/main/java/com/hubspot/jinjava/el/ext/AllowlistReturnTypeValidator.java b/src/main/java/com/hubspot/jinjava/el/ext/AllowlistReturnTypeValidator.java
@@ -3,6 +3,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Consumer;
 
 public final class AllowlistReturnTypeValidator {
 
@@ -15,6 +16,7 @@ public final class AllowlistReturnTypeValidator {
   private final ImmutableSet<String> allowedCanonicalClassPrefixes;
   private final ImmutableSet<String> allowedCanonicalClassNames;
   private final boolean allowArrays;
+  private final Consumer<Class<?>> onRejectedClass;
   private final ImmutableList<ReturnTypeValidator> additionalValidators;
 
   public static AllowlistReturnTypeValidator create(
@@ -36,6 +38,7 @@ private AllowlistReturnTypeValidator(
     this.allowedCanonicalClassNames =
       returnTypeValidatorConfig.allowedCanonicalClassNames();
     this.allowArrays = returnTypeValidatorConfig.allowArrays();
+    this.onRejectedClass = returnTypeValidatorConfig.onRejectedClass();
     this.additionalValidators = additionalValidators;
     this.allowedReturnTypesCache = new ConcurrentHashMap<>();
   }
@@ -56,11 +59,12 @@ public Object validateReturnType(Object o) {
         allowedCanonicalClassPrefixes.stream().anyMatch(canonicalClassName::startsWith)
     );
     if (!isAllowedClassName) {
+      onRejectedClass.accept(clazz);
       return null;
     }
     for (ReturnTypeValidator v : additionalValidators) {
-      o = v.validateReturnType(o);
-      if (o == null) {
+      if (v.validateReturnType(o) == null) {
+        onRejectedClass.accept(clazz);
         return null;
       }
     }
@@ -79,10 +83,12 @@ public boolean allowReturnTypeClass(Class<?> clazz) {
         allowedCanonicalClassPrefixes.stream().anyMatch(canonicalClassName::startsWith)
     );
     if (!isAllowedReturnType) {
+      onRejectedClass.accept(clazz);
       return false;
     }
     for (ReturnTypeValidator v : additionalValidators) {
       if (!v.allowReturnTypeClass(clazz)) {
+        onRejectedClass.accept(clazz);
         return false;
       }
     }
diff --git a/src/main/java/com/hubspot/jinjava/el/ext/BannedAllowlistOptions.java b/src/main/java/com/hubspot/jinjava/el/ext/BannedAllowlistOptions.java
@@ -0,0 +1,66 @@
+package com.hubspot.jinjava.el.ext;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableSet;
+import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Stream;
+import org.immutables.value.Value;
+
+public class BannedAllowlistOptions {
+
+  // These aren't required, but they prevent someone from misconfiguring Jinjava to allow sandbox bypass unintentionally
+  private static final String JAVA_LANG_REFLECT_PACKAGE =
+    Method.class.getPackage().getName(); // java.lang.reflect
+  private static final String JACKSON_DATABIND_PACKAGE =
+    ObjectMapper.class.getPackage().getName(); // com.fasterxml.jackson.databind
+
+  private static final String[] BANNED_PREFIXES = {
+    Class.class.getCanonicalName(),
+    Object.class.getCanonicalName(),
+    JAVA_LANG_REFLECT_PACKAGE,
+    JACKSON_DATABIND_PACKAGE,
+  };
+
+  private static final Set<String> ALLOWED_JINJAVA_PREFIXES = Stream
+    .concat(
+      Stream.of("com.hubspot.jinjava.testobjects"),
+      Arrays
+        .stream(AllowlistGroup.values())
+        .flatMap(g ->
+          Stream
+            .of(
+              g.allowedDeclaredMethodsFromCanonicalClassPrefixes(),
+              g.allowedReturnTypeCanonicalClassPrefixes(),
+              g.allowedDeclaredMethodsFromClasses(),
+              g.allowedReturnTypeClasses()
+            )
+            .flatMap(Arrays::stream)
+        )
+    )
+    .collect(ImmutableSet.toImmutableSet());
+
+  @Value.Check
+  public static List<String> findBannedPrefixes(Stream<String> prefixes) {
+    return prefixes
+      .filter(prefixOrName ->
+        Arrays
+          .stream(BANNED_PREFIXES)
+          .anyMatch(banned ->
+            banned.startsWith(prefixOrName) || prefixOrName.startsWith(banned)
+          ) ||
+        isIllegalJinjavaClass(prefixOrName)
+      )
+      .toList();
+  }
+
+  private static boolean isIllegalJinjavaClass(String prefixOrName) {
+    if (!prefixOrName.startsWith("com.hubspot.jinjava")) {
+      return false;
+    }
+    // e.g. com.hubspot.jinjava.lib.exptest is allowed, but com.hubspot.jinjava.Jinjava will not be
+    return ALLOWED_JINJAVA_PREFIXES.stream().noneMatch(prefixOrName::startsWith);
+  }
+}
diff --git a/src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java b/src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java
@@ -129,7 +129,7 @@ public AstNode createAstNode(AstNode... children) {
   }
 
   protected AstNode interpreter() {
-    return identifier(INTERPRETER);
+    return new AstNull();
   }
 
   @Override
diff --git a/src/main/java/com/hubspot/jinjava/el/ext/MethodValidator.java b/src/main/java/com/hubspot/jinjava/el/ext/MethodValidator.java
@@ -1,7 +1,9 @@
 package com.hubspot.jinjava.el.ext;
 
 import java.lang.reflect.Method;
+import javax.annotation.Nullable;
 
 public interface MethodValidator {
+  @Nullable
   Method validateMethod(Method m);
 }
diff --git a/src/main/java/com/hubspot/jinjava/el/ext/MethodValidatorConfig.java b/src/main/java/com/hubspot/jinjava/el/ext/MethodValidatorConfig.java
@@ -1,10 +1,10 @@
 package com.hubspot.jinjava.el.ext;
 
-import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableSet;
 import com.hubspot.jinjava.JinjavaImmutableStyle;
 import java.lang.reflect.Method;
-import java.util.Arrays;
+import java.util.List;
+import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.stream.Stream;
 import org.immutables.value.Value;
@@ -13,60 +13,34 @@
 @JinjavaImmutableStyle
 public abstract class MethodValidatorConfig {
 
-  // These aren't required, but they prevent someone from misconfiguring Jinjava to allow sandbox bypass unintentionally
-  private static final String JAVA_LANG_REFLECT_PACKAGE =
-    Method.class.getPackage().getName(); // java.lang.reflect
-  private static final String JACKSON_DATABIND_PACKAGE =
-    ObjectMapper.class.getPackage().getName(); // com.fasterxml.jackson.databind
-
-  private static final String[] BANNED_PREFIXES = {
-    Class.class.getCanonicalName(),
-    Object.class.getCanonicalName(),
-    JAVA_LANG_REFLECT_PACKAGE,
-    JACKSON_DATABIND_PACKAGE,
-  };
-
   public abstract ImmutableSet<Method> allowedMethods();
 
   public abstract ImmutableSet<String> allowedDeclaredMethodsFromCanonicalClassPrefixes();
 
   public abstract ImmutableSet<String> allowedDeclaredMethodsFromCanonicalClassNames();
 
+  @Value.Default
+  public Consumer<Method> onRejectedMethod() {
+    return m -> {};
+  }
+
   @Value.Check
   void banClassesAndMethods() {
-    if (
-      allowedMethods()
-        .stream()
-        .map(method -> method.getDeclaringClass().getCanonicalName())
-        .anyMatch(canonicalName ->
-          Arrays
-            .stream(BANNED_PREFIXES)
-            .anyMatch(banned ->
-              Arrays.stream(BANNED_PREFIXES).anyMatch(canonicalName::startsWith)
-            )
-        )
-    ) {
-      throw new IllegalStateException(
-        "Methods from banned classes or packages (Object.class, Class.class, java.lang.reflect, com.fasterxml.jackson.databind) are not allowed"
-      );
-    }
-    if (
+    List<String> list = BannedAllowlistOptions.findBannedPrefixes(
       Stream
         .of(
+          allowedMethods()
+            .stream()
+            .map(method -> method.getDeclaringClass().getCanonicalName()),
           allowedDeclaredMethodsFromCanonicalClassPrefixes().stream(),
           allowedDeclaredMethodsFromCanonicalClassNames().stream()
         )
         .flatMap(Function.identity())
-        .anyMatch(prefixOrName ->
-          Arrays
-            .stream(BANNED_PREFIXES)
-            .anyMatch(banned ->
-              banned.startsWith(prefixOrName) || prefixOrName.startsWith(banned)
-            )
-        )
-    ) {
+    );
+    if (!list.isEmpty()) {
       throw new IllegalStateException(
-        "Banned classes or prefixes (Object.class, Class.class, java.lang.reflect, com.fasterxml.jackson.databind) are not allowed"
+        "Banned classes or prefixes (Object.class, Class.class, java.lang.reflect, com.fasterxml.jackson.databind) are not allowed: " +
+        list
       );
     }
   }
@@ -81,7 +55,7 @@ public static Builder builder() {
 
   public static class Builder extends ImmutableMethodValidatorConfig.Builder {
 
-    Builder() {}/**/
+    Builder() {}
 
     public Builder addDefaultAllowlistGroups() {
       return addAllowlistGroups(AllowlistGroup.values());
diff --git a/src/main/java/com/hubspot/jinjava/el/ext/ReturnTypeValidator.java b/src/main/java/com/hubspot/jinjava/el/ext/ReturnTypeValidator.java
@@ -1,6 +1,10 @@
 package com.hubspot.jinjava.el.ext;
 
+import javax.annotation.Nullable;
+
 public interface ReturnTypeValidator {
+  @Nullable
   Object validateReturnType(Object o);
+
   boolean allowReturnTypeClass(Class<?> c);
 }
diff --git a/src/main/java/com/hubspot/jinjava/el/ext/ReturnTypeValidatorConfig.java b/src/main/java/com/hubspot/jinjava/el/ext/ReturnTypeValidatorConfig.java
diff --git a/src/test/java/com/hubspot/jinjava/el/ext/ExtendedParserTest.java b/src/test/java/com/hubspot/jinjava/el/ext/ExtendedParserTest.java
diff --git a/src/test/java/com/hubspot/jinjava/tree/ExpressionNodeTest.java b/src/test/java/com/hubspot/jinjava/tree/ExpressionNodeTest.java

Original file line number	Diff line number	Diff line change
`@@ -1,16 +1,25 @@`
`1`	`1`	`package com.hubspot.jinjava;`
`2`	`2`
	`3`	`+import com.hubspot.immutable.collection.encoding.ImmutableListEncodingEnabled;`
	`4`	`+import com.hubspot.immutable.collection.encoding.ImmutableMapEncodingEnabled;`
	`5`	`+import com.hubspot.immutable.collection.encoding.ImmutableSetEncodingEnabled;`
`3`	`6`	`import org.immutables.value.Value;`
`4`	`7`
`5`	`8`	`@Value.Style(`
`6`	`9`	`init = "set*",`
`7`	`10`	`get = { "is", "get" } // Detect 'get' and 'is' prefixes in accessor methods`
`8`	`11`	`)`
	`12`	`+@ImmutableSetEncodingEnabled`
	`13`	`+@ImmutableListEncodingEnabled`
	`14`	`+@ImmutableMapEncodingEnabled`
`9`	`15`	`public @interface JinjavaImmutableStyle {`
`10`	`16`	`@Value.Style(`
`11`	`17`	`init = "with*",`
`12`	`18`	`get = { "is", "get" } // Detect 'get' and 'is' prefixes in accessor methods`
`13`	`19`	`)`
	`20`	`+ @ImmutableSetEncodingEnabled`
	`21`	`+ @ImmutableListEncodingEnabled`
	`22`	`+ @ImmutableMapEncodingEnabled`
`14`	`23`	`@interface WithStyle {`
`15`	`24`	`}`
`16`	`25`	`}`
Original file line number	Diff line number	Diff line change
`@@ -129,7 +129,7 @@ public AstNode createAstNode(AstNode... children) {`
`129`	`129`	`}`
`130`	`130`
`131`	`131`	`protected AstNode interpreter() {`
`132`		`- return identifier(INTERPRETER);`
	`132`	`+ return new AstNull();`
`133`	`133`	`}`
`134`	`134`
`135`	`135`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,9 @@`
`1`	`1`	`package com.hubspot.jinjava.el.ext;`
`2`	`2`
`3`	`3`	`import java.lang.reflect.Method;`
	`4`	`+import javax.annotation.Nullable;`
`4`	`5`
`5`	`6`	`public interface MethodValidator {`
	`7`	`+ @Nullable`
`6`	`8`	`Method validateMethod(Method m);`
`7`	`9`	`}`