replace allowUnsafeMethod with a Method Access validator interface. P…

…rovide a NoOp method access and a default one (#493)
PebbleTemplates · May 16, 2020 · 4753ce4 · 4753ce4
1 parent ba51c35
commit 4753ce4
Show file tree

Hide file tree

Showing 18 changed files with 454 additions and 108 deletions.
diff --git a/docs/src/orchid/resources/wiki/guide/installation.md b/docs/src/orchid/resources/wiki/guide/installation.md
@@ -84,7 +84,7 @@ All the settings are set during the construction of the `PebbleEngine` object.
 | `executorService` | An `ExecutorService` that allows the usage of some advanced multithreading features, such as the `parallel` tag. | `null` |
 | `loader` | An implementation of the `Loader` interface which is used to find templates. | An implementation of the `DelegatingLoader` which uses a `ClasspathLoader` and a `FileLoader` behind the scenes. |
 | `strictVariables` | If set to true, Pebble will throw an exception if you try to access a variable or attribute that does not exist (or an attribute of a null variable). If set to false, your template will treat non-existing variables/attributes as null without ever skipping a beat. | `false` |
-| `allowUnsafeMethods` | If set to false, Pebble will throw an exception if you try to access unsafe methods. Unsafe methods are defined [here](https://github.com/PebbleTemplates/pebble/tree/master/pebble/src/main/resources/unsafeMethods.properties) | `true` in 2.x, 'false' in v3.x |
+| `methodAccessValidator` | Pebble provides two implementations. NoOpMethodAccessValidator which do nothing and BlacklistMethodAccessValidator which checks that the method being called is not blacklisted. | `BlacklistMethodAccessValidator` 
 | `literalDecimalTreatedAsInteger` | option for toggling to enable/disable literal decimal treated as integer | `false` |
 | `literalNumbersAsBigDecimals` | option for toggling to enable/disable literal numbers treated as BigDecimals | `false` |
 | `greedyMatchMethod` | option for toggling to enable/disable greedy matching mode for finding java method. Reduce the limit of the parameter type, try to find other method which has compatible parameter types. | `false` |
diff --git a/pebble/src/main/java/com/mitchellbosecke/pebble/PebbleEngine.java b/pebble/src/main/java/com/mitchellbosecke/pebble/PebbleEngine.java
@@ -9,6 +9,8 @@
 package com.mitchellbosecke.pebble;
 
 
+import com.mitchellbosecke.pebble.attributes.methodaccess.BlacklistMethodAccessValidator;
+import com.mitchellbosecke.pebble.attributes.methodaccess.MethodAccessValidator;
 import com.mitchellbosecke.pebble.cache.CacheKey;
 import com.mitchellbosecke.pebble.cache.PebbleCache;
 import com.mitchellbosecke.pebble.cache.tag.ConcurrentMapTagCache;
@@ -39,14 +41,13 @@
 import com.mitchellbosecke.pebble.template.EvaluationOptions;
 import com.mitchellbosecke.pebble.template.PebbleTemplate;
 import com.mitchellbosecke.pebble.template.PebbleTemplateImpl;
-
 import java.io.IOException;
 import java.io.Reader;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.concurrent.ExecutorService;
-
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -155,12 +156,12 @@ private PebbleTemplate getPebbleTemplate(String templateName, Loader loader, Obj
     Reader templateReader = loader.getReader(cacheKey);
 
     try {
-      logger.debug("Tokenizing template named {}", templateName);
+      this.logger.debug("Tokenizing template named {}", templateName);
       LexerImpl lexer = new LexerImpl(this.syntax,
           this.extensionRegistry.getUnaryOperators().values(),
           this.extensionRegistry.getBinaryOperators().values());
       TokenStream tokenStream = lexer.tokenize(templateReader, templateName);
-      logger.trace("TokenStream: {}", tokenStream);
+      this.logger.trace("TokenStream: {}", tokenStream);
 
       Parser parser = new ParserImpl(this.extensionRegistry.getUnaryOperators(),
           this.extensionRegistry.getBinaryOperators(), this.extensionRegistry.getTokenParsers(),
@@ -263,7 +264,7 @@ public static class Builder {
 
     private Loader<?> loader;
 
-    private List<Extension> userProvidedExtensions = new ArrayList<>();
+    private final List<Extension> userProvidedExtensions = new ArrayList<>();
 
     private Syntax syntax;
 
@@ -281,9 +282,7 @@ public static class Builder {
 
     private PebbleCache<CacheKey, Object> tagCache;
 
-    private EscaperExtension escaperExtension = new EscaperExtension();
-
-    private boolean allowUnsafeMethods;
+    private final EscaperExtension escaperExtension = new EscaperExtension();
 
     private boolean literalDecimalTreatedAsInteger = false;
 
@@ -293,6 +292,8 @@ public static class Builder {
 
     private boolean literalNumbersAsBigDecimals = false;
 
+    private MethodAccessValidator methodAccessValidator = new BlacklistMethodAccessValidator();
+
     /**
      * Creates the builder.
      */
@@ -318,9 +319,7 @@ public Builder loader(Loader<?> loader) {
      * @return This builder object
      */
     public Builder extension(Extension... extensions) {
-      for (Extension extension : extensions) {
-        this.userProvidedExtensions.add(extension);
-      }
+      Collections.addAll(this.userProvidedExtensions, extensions);
       return this;
     }
 
@@ -479,13 +478,13 @@ public Builder cacheActive(boolean cacheActive) {
     }
 
     /**
-     * Enable/disable unsafe methods access for attributes
+     * Validator that can be used to validate object/method access
      *
-     * @param allowUnsafeMethods toggle to enable/disable unsafe methods access
+     * @param methodAccessValidator Validator that can be used to validate object/method access
      * @return This builder object
      */
-    public Builder allowUnsafeMethods(boolean allowUnsafeMethods) {
-      this.allowUnsafeMethods = allowUnsafeMethods;
+    public Builder methodAccessValidator(MethodAccessValidator methodAccessValidator) {
+      this.methodAccessValidator = methodAccessValidator;
       return this;
     }
 
@@ -585,7 +584,7 @@ public PebbleEngine build() {
       parserOptions.setLiteralNumbersAsBigDecimals(this.literalNumbersAsBigDecimals);
 
       EvaluationOptions evaluationOptions = new EvaluationOptions();
-      evaluationOptions.setAllowUnsafeMethods(this.allowUnsafeMethods);
+      evaluationOptions.setMethodAccessValidator(this.methodAccessValidator);
       evaluationOptions.setGreedyMatchMethod(this.greedyMatchMethod);
 
       return new PebbleEngine(this.loader, this.syntax, this.strictVariables, this.defaultLocale,

diff --git a/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/DefaultAttributeResolver.java b/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/DefaultAttributeResolver.java
@@ -1,7 +1,9 @@
 package com.mitchellbosecke.pebble.attributes;
 
+import com.mitchellbosecke.pebble.error.ClassAccessException;
 import com.mitchellbosecke.pebble.node.ArgumentsNode;
 import com.mitchellbosecke.pebble.template.EvaluationContextImpl;
+import com.mitchellbosecke.pebble.template.EvaluationOptions;
 import com.mitchellbosecke.pebble.template.MacroAttributeProvider;
 import com.mitchellbosecke.pebble.utils.TypeUtils;
 import java.lang.reflect.Field;
@@ -59,12 +61,12 @@ public ResolvedAttribute resolve(Object instance,
                   lineNumber);
         }
 
-        member = this.memberCacheUtils
-            .cacheMember(instance, attributeName, argumentTypes, context, filename, lineNumber);
+        member = this.memberCacheUtils.cacheMember(instance, attributeName, argumentTypes, context);
       }
 
       if (member != null) {
-        return new ResolvedAttribute(this.invokeMember(instance, member, argumentValues));
+        return new ResolvedAttribute(this.invokeMember(instance, member, argumentValues,
+            filename, lineNumber, context.getEvaluationOptions()));
       }
     }
     return null;
@@ -91,13 +93,19 @@ private Class<?>[] getArgumentTypes(Object[] argumentValues) {
   /**
    * Invoke the "Member" that was found via reflection.
    */
-  private Object invokeMember(Object object, Member member, Object[] argumentValues) {
+  private Object invokeMember(Object object, Member member, Object[] argumentValues,
+      String filename, int lineNumber, EvaluationOptions evaluationOptions) {
     Object result = null;
     try {
       if (member instanceof Method) {
-        argumentValues = TypeUtils
-            .compatibleCast(argumentValues, ((Method) member).getParameterTypes());
-        result = ((Method) member).invoke(object, argumentValues);
+        Method method = (Method) member;
+        boolean methodAccessAllowed = evaluationOptions.getMethodAccessValidator()
+            .isMethodAccessAllowed(object, method);
+        if (!methodAccessAllowed) {
+          throw new ClassAccessException(method, filename, lineNumber);
+        }
+        argumentValues = TypeUtils.compatibleCast(argumentValues, method.getParameterTypes());
+        result = method.invoke(object, argumentValues);
       } else if (member instanceof Field) {
         result = ((Field) member).get(object);
       }

diff --git a/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/MemberCacheUtils.java b/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/MemberCacheUtils.java
@@ -1,9 +1,7 @@
 package com.mitchellbosecke.pebble.attributes;
 
-import com.mitchellbosecke.pebble.error.ClassAccessException;
 import com.mitchellbosecke.pebble.template.EvaluationContextImpl;
 import com.mitchellbosecke.pebble.template.EvaluationOptions;
-
 import java.lang.reflect.AccessibleObject;
 import java.lang.reflect.Member;
 import java.lang.reflect.Method;
@@ -16,7 +14,6 @@
 import java.util.concurrent.ConcurrentHashMap;
 
 class MemberCacheUtils {
-  private final UnsafeMethods unsafeMethods = new UnsafeMethods();
   private final ConcurrentHashMap<MemberCacheKey, Member> memberCache = new ConcurrentHashMap<>(100,
       0.9f, 1);
 
@@ -27,13 +24,12 @@ Member getMember(Object instance, String attributeName, Class<?>[] argumentTypes
   Member cacheMember(Object instance,
       String attributeName,
       Class<?>[] argumentTypes,
-      EvaluationContextImpl context,
-      String filename,
-      int lineNumber) {
-    Member member = this.reflect(instance, attributeName, argumentTypes, filename, lineNumber,
+      EvaluationContextImpl context) {
+    Member member = this.reflect(instance, attributeName, argumentTypes,
         context.getEvaluationOptions());
     if (member != null) {
-      this.memberCache.put(new MemberCacheKey(instance.getClass(), attributeName, argumentTypes), member);
+      this.memberCache
+          .put(new MemberCacheKey(instance.getClass(), attributeName, argumentTypes), member);
     }
     return member;
   }
@@ -42,14 +38,14 @@ Member cacheMember(Object instance,
    * Performs the actual reflection to obtain a "Member" from a class.
    */
   private Member reflect(Object object, String attributeName, Class<?>[] parameterTypes,
-      String filename, int lineNumber, EvaluationOptions evaluationOptions) {
+      EvaluationOptions evaluationOptions) {
 
     Class<?> clazz = object.getClass();
 
     // capitalize first letter of attribute for the following attempts
     String attributeCapitalized =
         Character.toUpperCase(attributeName.charAt(0)) + attributeName.substring(1);
-    
+
     // search well known super classes first to avoid illegal reflective access
     List<Class<?>> agenda = Arrays.asList(
         List.class,
@@ -67,27 +63,23 @@ private Member reflect(Object object, String attributeName, Class<?>[] parameter
 
       // check get method
       Member result = this
-          .findMethod(type, "get" + attributeCapitalized, parameterTypes, filename, lineNumber,
-              evaluationOptions);
+          .findMethod(type, "get" + attributeCapitalized, parameterTypes, evaluationOptions);
 
       // check is method
       if (result == null) {
         result = this
-            .findMethod(type, "is" + attributeCapitalized, parameterTypes, filename, lineNumber,
-                evaluationOptions);
+            .findMethod(type, "is" + attributeCapitalized, parameterTypes, evaluationOptions);
       }
 
       // check has method
       if (result == null) {
-        result = this
-            .findMethod(type, "has" + attributeCapitalized, parameterTypes, filename, lineNumber,
-                evaluationOptions);
+        result = this.findMethod(type, "has" + attributeCapitalized, parameterTypes,
+            evaluationOptions);
       }
 
       // check if attribute is a public method
       if (result == null) {
-        result = this.findMethod(type, attributeName, parameterTypes, filename, lineNumber,
-            evaluationOptions);
+        result = this.findMethod(type, attributeName, parameterTypes, evaluationOptions);
       }
 
       // public field
@@ -110,8 +102,8 @@ private Member reflect(Object object, String attributeName, Class<?>[] parameter
    * Finds an appropriate method by comparing if parameter types are compatible. This is more
    * relaxed than class.getMethod.
    */
-  private Method findMethod(Class<?> clazz, String name, Class<?>[] requiredTypes, String filename,
-      int lineNumber, EvaluationOptions evaluationOptions) {
+  private Method findMethod(Class<?> clazz, String name, Class<?>[] requiredTypes,
+      EvaluationOptions evaluationOptions) {
     List<Method> candidates = this.getCandidates(clazz, name, requiredTypes);
 
     // perfect match
@@ -146,7 +138,6 @@ private Method findMethod(Class<?> clazz, String name, Class<?>[] requiredTypes,
       }
     }
     if(bestMatch != null) {
-      this.verifyUnsafeMethod(filename, lineNumber, evaluationOptions, bestMatch);
       return bestMatch;
     }
 
@@ -163,7 +154,6 @@ private Method findMethod(Class<?> clazz, String name, Class<?>[] requiredTypes,
         }
 
         if (compatibleTypes) {
-          this.verifyUnsafeMethod(filename, lineNumber, evaluationOptions, candidate);
           return candidate;
         }
       }
@@ -172,12 +162,6 @@ private Method findMethod(Class<?> clazz, String name, Class<?>[] requiredTypes,
     return null;
   }
 
-  private void verifyUnsafeMethod(String filename, int lineNumber, EvaluationOptions evaluationOptions, Method method) {
-    if (!evaluationOptions.isAllowUnsafeMethods() && this.unsafeMethods.isUnsafeMethod(method)) {
-      throw new ClassAccessException(lineNumber, filename);
-    }
-  }
-
   /**
    * Performs a widening conversion (primitive to boxed type)
    */

diff --git a/...va/com/mitchellbosecke/pebble/attributes/methodaccess/BlacklistMethodAccessValidator.java b/...va/com/mitchellbosecke/pebble/attributes/methodaccess/BlacklistMethodAccessValidator.java
@@ -0,0 +1,41 @@
+package com.mitchellbosecke.pebble.attributes.methodaccess;
+
+import java.lang.reflect.AccessibleObject;
+import java.lang.reflect.Method;
+
+public class BlacklistMethodAccessValidator implements MethodAccessValidator {
+
+  private static final String[] FORBIDDEN_METHODS = {"getClass",
+      "wait",
+      "notify",
+      "notifyAll"};
+
+  @Override
+  public boolean isMethodAccessAllowed(Object object, Method method) {
+    boolean methodForbidden = object instanceof Class
+        || object instanceof Runtime
+        || object instanceof Thread
+        || object instanceof ThreadGroup
+        || object instanceof System
+        || object instanceof AccessibleObject
+        || this.isUnsafeMethod(method);
+    return !methodForbidden;
+  }
+
+  private boolean isUnsafeMethod(Method member) {
+    return this.isAnyOfMethods(member, FORBIDDEN_METHODS);
+  }
+
+  private boolean isAnyOfMethods(Method member, String... methods) {
+    for (String method : methods) {
+      if (this.isMethodWithName(member, method)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  private boolean isMethodWithName(Method member, String method) {
+    return member.getName().equals(method);
+  }
+}
diff --git a/...c/main/java/com/mitchellbosecke/pebble/attributes/methodaccess/MethodAccessValidator.java b/...c/main/java/com/mitchellbosecke/pebble/attributes/methodaccess/MethodAccessValidator.java
@@ -0,0 +1,8 @@
+package com.mitchellbosecke.pebble.attributes.methodaccess;
+
+import java.lang.reflect.Method;
+
+public interface MethodAccessValidator {
+
+  boolean isMethodAccessAllowed(Object object, Method method);
+}
diff --git a/...in/java/com/mitchellbosecke/pebble/attributes/methodaccess/NoOpMethodAccessValidator.java b/...in/java/com/mitchellbosecke/pebble/attributes/methodaccess/NoOpMethodAccessValidator.java
@@ -0,0 +1,11 @@
+package com.mitchellbosecke.pebble.attributes.methodaccess;
+
+import java.lang.reflect.Method;
+
+public class NoOpMethodAccessValidator implements MethodAccessValidator {
+
+  @Override
+  public boolean isMethodAccessAllowed(Object object, Method method) {
+    return true;
+  }
+}
diff --git a/pebble/src/main/java/com/mitchellbosecke/pebble/error/ClassAccessException.java b/pebble/src/main/java/com/mitchellbosecke/pebble/error/ClassAccessException.java
@@ -8,12 +8,15 @@
  */
 package com.mitchellbosecke.pebble.error;
 
+import java.lang.reflect.Method;
+
 public class ClassAccessException extends PebbleException {
 
   private static final long serialVersionUID = 5109892021088141417L;
 
-  public ClassAccessException(Integer lineNumber, String filename) {
-    super(null, "For security reasons access to class/getClass attribute is denied.", lineNumber,
+  public ClassAccessException(Method method, String filename, Integer lineNumber) {
+    super(null, String.format("For security reasons access to %s method is denied.", method),
+        lineNumber,
         filename);
   }
 }