diff --git a/docs/src/orchid/resources/wiki/guide/installation.md b/docs/src/orchid/resources/wiki/guide/installation.md index cee1b8133..f46f6c1ea 100644 --- 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 index 55b15c1f1..c6ecf0d1f 100644 --- 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 userProvidedExtensions = new ArrayList<>(); + private final List userProvidedExtensions = new ArrayList<>(); private Syntax syntax; @@ -281,9 +282,7 @@ public static class Builder { private PebbleCache 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 index f6559cc7f..d6317a089 100644 --- 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 index 0068eee0b..cd2d3abd2 100644 --- 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 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> 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 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/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/methodaccess/BlacklistMethodAccessValidator.java b/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/methodaccess/BlacklistMethodAccessValidator.java new file mode 100644 index 000000000..c5083da68 --- /dev/null +++ b/pebble/src/main/java/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/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/methodaccess/MethodAccessValidator.java b/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/methodaccess/MethodAccessValidator.java new file mode 100644 index 000000000..c5187f5d1 --- /dev/null +++ b/pebble/src/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/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/methodaccess/NoOpMethodAccessValidator.java b/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/methodaccess/NoOpMethodAccessValidator.java new file mode 100644 index 000000000..7aa43ce9d --- /dev/null +++ b/pebble/src/main/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 index d01faaa52..4f336ab97 100644 --- 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); } } diff --git a/pebble/src/main/java/com/mitchellbosecke/pebble/template/EvaluationOptions.java b/pebble/src/main/java/com/mitchellbosecke/pebble/template/EvaluationOptions.java index 412ce052f..bc04d5671 100644 --- a/pebble/src/main/java/com/mitchellbosecke/pebble/template/EvaluationOptions.java +++ b/pebble/src/main/java/com/mitchellbosecke/pebble/template/EvaluationOptions.java @@ -1,37 +1,37 @@ package com.mitchellbosecke.pebble.template; +import com.mitchellbosecke.pebble.attributes.methodaccess.MethodAccessValidator; + /** * Evaluation options. * * @author yanxiyue */ public class EvaluationOptions { - /** - * toggle to enable/disable unsafe methods access + * toggle to enable/disable greedy matching mode for finding java method */ - private boolean allowUnsafeMethods; + private boolean greedyMatchMethod; /** - * toggle to enable/disable greedy matching mode for finding java method + * Validator that can be used to validate object/method access */ - private boolean greedyMatchMethod; + private MethodAccessValidator methodAccessValidator; - public boolean isAllowUnsafeMethods() { - return this.allowUnsafeMethods; + public boolean isGreedyMatchMethod() { + return this.greedyMatchMethod; } - public EvaluationOptions setAllowUnsafeMethods(boolean allowUnsafeMethods) { - this.allowUnsafeMethods = allowUnsafeMethods; - return this; + public void setGreedyMatchMethod(boolean greedyMatchMethod) { + this.greedyMatchMethod = greedyMatchMethod; } - public boolean isGreedyMatchMethod() { - return this.greedyMatchMethod; + public MethodAccessValidator getMethodAccessValidator() { + return this.methodAccessValidator; } - public EvaluationOptions setGreedyMatchMethod(boolean greedyMatchMethod) { - this.greedyMatchMethod = greedyMatchMethod; - return this; + public void setMethodAccessValidator( + MethodAccessValidator methodAccessValidator) { + this.methodAccessValidator = methodAccessValidator; } } diff --git a/pebble/src/test/java/com/mitchellbosecke/pebble/GetAttributeTest.java b/pebble/src/test/java/com/mitchellbosecke/pebble/GetAttributeTest.java index 61c4fab0e..dd8f66561 100644 --- a/pebble/src/test/java/com/mitchellbosecke/pebble/GetAttributeTest.java +++ b/pebble/src/test/java/com/mitchellbosecke/pebble/GetAttributeTest.java @@ -8,15 +8,17 @@ */ package com.mitchellbosecke.pebble; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.fail; + +import com.mitchellbosecke.pebble.attributes.methodaccess.NoOpMethodAccessValidator; import com.mitchellbosecke.pebble.error.AttributeNotFoundException; import com.mitchellbosecke.pebble.error.ClassAccessException; import com.mitchellbosecke.pebble.error.PebbleException; import com.mitchellbosecke.pebble.error.RootAttributeNotFoundException; import com.mitchellbosecke.pebble.loader.StringLoader; import com.mitchellbosecke.pebble.template.PebbleTemplate; - -import org.junit.jupiter.api.Test; - import java.io.IOException; import java.io.StringWriter; import java.io.Writer; @@ -25,10 +27,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.fail; +import org.junit.jupiter.api.Test; class GetAttributeTest { @@ -229,7 +228,7 @@ void testMethodAttribute() throws PebbleException, IOException { void testAccessingClass_AllowUnsafeMethodsOn_StrictVariableOff_Property() throws PebbleException, IOException { PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()) - .allowUnsafeMethods(true) + .methodAccessValidator(new NoOpMethodAccessValidator()) .strictVariables(false) .build(); @@ -246,7 +245,7 @@ void testAccessingClass_AllowUnsafeMethodsOn_StrictVariableOff_Property() void testAccessingClass_AllowUnsafeMethodsOn_StrictVariableOff_Method() throws PebbleException, IOException { PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()) - .allowUnsafeMethods(true) + .methodAccessValidator(new NoOpMethodAccessValidator()) .strictVariables(false) .build(); @@ -263,7 +262,7 @@ void testAccessingClass_AllowUnsafeMethodsOn_StrictVariableOff_Method() void testAccessingClass_AllowUnsafeMethodsOn_StrictVariableOn_Property() throws PebbleException, IOException { PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()) - .allowUnsafeMethods(true) + .methodAccessValidator(new NoOpMethodAccessValidator()) .strictVariables(true) .build(); @@ -280,7 +279,7 @@ void testAccessingClass_AllowUnsafeMethodsOn_StrictVariableOn_Property() void testAccessingClass_AllowUnsafeMethodsOn_StrictVariableOn_Method() throws PebbleException, IOException { PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()) - .allowUnsafeMethods(true) + .methodAccessValidator(new NoOpMethodAccessValidator()) .strictVariables(true) .build(); @@ -298,7 +297,6 @@ void testAccessingClass_AllowUnsafeMethodsOff_StrictVariableOff_Property() throws PebbleException, IOException { assertThrows(ClassAccessException.class, () -> { PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()) - .allowUnsafeMethods(false) .strictVariables(false) .build(); @@ -316,7 +314,6 @@ void testAccessingClass_AllowUnsafeMethodsOff_StrictVariableOff_Method() throws PebbleException, IOException { assertThrows(ClassAccessException.class, () -> { PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()) - .allowUnsafeMethods(false) .strictVariables(false) .build(); @@ -334,7 +331,6 @@ void testAccessingClass_AllowUnsafeMethodsOff_StrictVariableOn_Property() throws PebbleException, IOException { assertThrows(ClassAccessException.class, () -> { PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()) - .allowUnsafeMethods(false) .strictVariables(true) .build(); @@ -352,7 +348,6 @@ void testAccessingClass_AllowUnsafeMethodsOff_StrictVariableOn_Method() throws PebbleException, IOException { assertThrows(ClassAccessException.class, () -> { PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()) - .allowUnsafeMethods(false) .strictVariables(true) .build(); @@ -370,7 +365,7 @@ void testAccessingClass_AllowUnsafeMethodsOnIsCaseInsensitive_Property() throws PebbleException, IOException { PebbleEngine pebble = new PebbleEngine.Builder() .loader(new StringLoader()) - .allowUnsafeMethods(true) + .methodAccessValidator(new NoOpMethodAccessValidator()) .build(); PebbleTemplate template = pebble.getTemplate("hello [{{ object.ClAsS }}]"); @@ -388,7 +383,6 @@ void testAccessingClass_AllowUnsafeMethodsOffIsCaseInsensitive_Property() assertThrows(ClassAccessException.class, () -> { PebbleEngine pebble = new PebbleEngine.Builder() .loader(new StringLoader()) - .allowUnsafeMethods(false) .build(); PebbleTemplate template = pebble.getTemplate("hello [{{ object.ClAsS }}]"); @@ -405,7 +399,7 @@ void testAccessingClass_AllowUnsafeMethodsOnIsCaseInsensitive_Method() throws PebbleException, IOException { PebbleEngine pebble = new PebbleEngine.Builder() .loader(new StringLoader()) - .allowUnsafeMethods(true) + .methodAccessValidator(new NoOpMethodAccessValidator()) .build(); PebbleTemplate template = pebble.getTemplate("hello [{{ object.GeTcLAsS() }}]"); @@ -423,7 +417,6 @@ void testAccessingClass_AllowUnsafeMethodsOffIsCaseInsensitive_Method() assertThrows(ClassAccessException.class, () -> { PebbleEngine pebble = new PebbleEngine.Builder() .loader(new StringLoader()) - .allowUnsafeMethods(false) .build(); PebbleTemplate template = pebble.getTemplate("hello [{{ object.GeTcLAsS() }}]"); @@ -441,7 +434,6 @@ void testAccessingClass_AllowUnsafeMethodsOffForMethodNotify_thenThrowException( assertThrows(ClassAccessException.class, () -> { PebbleEngine pebble = new PebbleEngine.Builder() .loader(new StringLoader()) - .allowUnsafeMethods(false) .build(); PebbleTemplate template = pebble.getTemplate("hello [{{ object.notify() }}]"); diff --git a/pebble/src/test/java/com/mitchellbosecke/pebble/MethodAccessTemplateTest.java b/pebble/src/test/java/com/mitchellbosecke/pebble/MethodAccessTemplateTest.java new file mode 100644 index 000000000..0a5bfe99b --- /dev/null +++ b/pebble/src/test/java/com/mitchellbosecke/pebble/MethodAccessTemplateTest.java @@ -0,0 +1,180 @@ +package com.mitchellbosecke.pebble; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.mitchellbosecke.pebble.attributes.methodaccess.NoOpMethodAccessValidator; +import com.mitchellbosecke.pebble.error.ClassAccessException; +import com.mitchellbosecke.pebble.loader.StringLoader; +import com.mitchellbosecke.pebble.template.PebbleTemplate; +import java.io.IOException; +import java.io.StringWriter; +import java.io.Writer; +import java.lang.reflect.Constructor; +import java.lang.reflect.Method; +import java.util.HashMap; +import java.util.Map; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; + +class MethodAccessTemplateTest { + + @Nested + class ClassTest { + + @Test + void testIfAccessIsForbiddenWhenAllowUnsafeMethodsIsFalse() { + PebbleEngine pebble = MethodAccessTemplateTest.this.pebbleEngine(); + assertThrows(ClassAccessException.class, this.templateEvaluation(pebble)); + } + + @Test + void testIfAccessIsAllowedWhenAllowUnsafeMethodsIsTrue() throws Throwable { + PebbleEngine pebble = MethodAccessTemplateTest.this.unsafePebbleEngine(); + this.templateEvaluation(pebble).execute(); + } + + private Executable templateEvaluation(PebbleEngine pebble) { + return () -> { + String source = "{{clazz.getPackage()}}"; + PebbleTemplate template = pebble.getTemplate(source); + Map context = new HashMap<>(); + context.put("clazz", Object.class); + MethodAccessTemplateTest.this.evaluateTemplate(template, context); + }; + } + } + + @Nested + class RuntimeTest { + + @Test + void testIfAccessIsForbiddenWhenAllowUnsafeMethodsIsFalse() { + PebbleEngine pebble = MethodAccessTemplateTest.this.pebbleEngine(); + assertThrows(ClassAccessException.class, this.templateEvaluation(pebble)); + } + + @Test + void testIfAccessIsAllowedWhenAllowUnsafeMethodsIsTrue() throws Throwable { + PebbleEngine pebble = MethodAccessTemplateTest.this.unsafePebbleEngine(); + this.templateEvaluation(pebble).execute(); + } + + private Executable templateEvaluation(PebbleEngine pebble) { + return () -> { + String source = "{{runtime.availableProcessors()}}"; + PebbleTemplate template = pebble.getTemplate(source); + Map context = new HashMap<>(); + context.put("runtime", Runtime.getRuntime()); + MethodAccessTemplateTest.this.evaluateTemplate(template, context); + }; + } + } + + @Nested + class ThreadTest { + + @Test + void testIfAccessIsForbiddenWhenAllowUnsafeMethodsIsFalse() { + PebbleEngine pebble = MethodAccessTemplateTest.this.pebbleEngine(); + assertThrows(ClassAccessException.class, this.templateEvaluation(pebble)); + } + + @Test + void testIfAccessIsAllowedWhenAllowUnsafeMethodsIsTrue() throws Throwable { + PebbleEngine pebble = MethodAccessTemplateTest.this.unsafePebbleEngine(); + this.templateEvaluation(pebble).execute(); + } + + private Executable templateEvaluation(PebbleEngine pebble) { + return () -> { + String source = "{{thread.sleep(500)}}"; + PebbleTemplate template = pebble.getTemplate(source); + Map context = new HashMap<>(); + context.put("thread", new Thread()); + MethodAccessTemplateTest.this.evaluateTemplate(template, context); + }; + } + } + + @Nested + class SystemTest { + + @Test + void testIfAccessIsForbiddenWhenAllowUnsafeMethodsIsFalse() { + PebbleEngine pebble = MethodAccessTemplateTest.this.pebbleEngine(); + assertThrows(ClassAccessException.class, this.templateEvaluation(pebble)); + } + + @Test + void testIfAccessIsAllowedWhenAllowUnsafeMethodsIsTrue() throws Throwable { + PebbleEngine pebble = MethodAccessTemplateTest.this.unsafePebbleEngine(); + this.templateEvaluation(pebble).execute(); + } + + private Executable templateEvaluation(PebbleEngine pebble) { + return () -> { + Class systemClass = Class.forName("java.lang.System"); + systemClass.getMethod("gc").setAccessible(true); + + Constructor constructor = systemClass.getDeclaredConstructor(); + constructor.setAccessible(true); + System system = (System) constructor.newInstance(); + + String source = "{{system.gc()}}"; + PebbleTemplate template = pebble.getTemplate(source); + Map context = new HashMap<>(); + context.put("system", system); + MethodAccessTemplateTest.this.evaluateTemplate(template, context); + }; + } + } + + @Nested + class MethodTest { + + @Test + void testIfAccessIsForbiddenWhenAllowUnsafeMethodsIsFalse() { + PebbleEngine pebble = MethodAccessTemplateTest.this.pebbleEngine(); + assertThrows(ClassAccessException.class, this.templateEvaluation(pebble)); + } + + @Test + void testIfAccessIsAllowedWhenAllowUnsafeMethodsIsTrue() throws Throwable { + PebbleEngine pebble = MethodAccessTemplateTest.this.unsafePebbleEngine(); + this.templateEvaluation(pebble).execute(); + } + + private Executable templateEvaluation(PebbleEngine pebble) { + return () -> { + Class systemClass = Class.forName("java.lang.System"); + Method gcMethod = systemClass.getMethod("gc"); + gcMethod.setAccessible(true); + gcMethod.invoke(null); + + String source = "{{gc.invoke(null, null)}}"; + PebbleTemplate template = pebble.getTemplate(source); + Map context = new HashMap<>(); + context.put("gc", gcMethod); + + MethodAccessTemplateTest.this.evaluateTemplate(template, context); + }; + } + } + + private PebbleEngine unsafePebbleEngine() { + return new PebbleEngine.Builder().loader(new StringLoader()) + .methodAccessValidator(new NoOpMethodAccessValidator()) + .build(); + } + + private PebbleEngine pebbleEngine() { + return new PebbleEngine.Builder().loader(new StringLoader()).build(); + } + + private void evaluateTemplate(PebbleTemplate template, Map context) + throws IOException { + Writer writer = new StringWriter(); + template.evaluate(writer, context); + } +} \ No newline at end of file diff --git a/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/BlacklistMethodAccessValidatorTest.java b/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/BlacklistMethodAccessValidatorTest.java new file mode 100644 index 000000000..877d2661f --- /dev/null +++ b/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/BlacklistMethodAccessValidatorTest.java @@ -0,0 +1,46 @@ +package com.mitchellbosecke.pebble.attributes.methodaccess; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.lang.reflect.Method; +import java.util.stream.Stream; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + + +class BlacklistMethodAccessValidatorTest { + + private final InstanceProvider instanceProvider = new InstanceProvider(); + private final MethodAccessValidator underTest = new BlacklistMethodAccessValidator(); + + @ParameterizedTest + @MethodSource("provideUnsafeMethod") + void checkIfAccessIsForbidden(Method unsafeMethod) + throws NoSuchFieldException, NoSuchMethodException { + Class declaringClass = unsafeMethod.getDeclaringClass(); + Object instance = this.instanceProvider.createObject(declaringClass); + + boolean methodAccessAllowed = this.underTest.isMethodAccessAllowed(instance, unsafeMethod); + + assertThat(methodAccessAllowed).isFalse(); + } + + @ParameterizedTest + @MethodSource("provideAllowedMethod") + void checkIfAccessIsAllowed(Method allowedMethod) throws Throwable { + Class declaringClass = allowedMethod.getDeclaringClass(); + Object instance = this.instanceProvider.createObject(declaringClass); + + boolean methodAccessAllowed = this.underTest.isMethodAccessAllowed(instance, allowedMethod); + + assertThat(methodAccessAllowed).isTrue(); + } + + private static Stream provideUnsafeMethod() { + return MethodsProvider.UNSAFE_METHODS.stream(); + } + + private static Stream provideAllowedMethod() { + return MethodsProvider.ALLOWED_METHODS.stream(); + } +} \ No newline at end of file diff --git a/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/Foo.java b/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/Foo.java new file mode 100644 index 000000000..ea236f60a --- /dev/null +++ b/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/Foo.java @@ -0,0 +1,16 @@ +package com.mitchellbosecke.pebble.attributes.methodaccess; + +public class Foo { + + private String x; + + public void getX() { + } + + private Foo() { + } + + public void setX(String x) { + this.x = x; + } +} \ No newline at end of file diff --git a/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/InstanceProvider.java b/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/InstanceProvider.java new file mode 100644 index 000000000..b45d8db59 --- /dev/null +++ b/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/InstanceProvider.java @@ -0,0 +1,30 @@ +package com.mitchellbosecke.pebble.attributes.methodaccess; + +import java.lang.reflect.Constructor; + +public class InstanceProvider { + + Object createObject(Class declaringClass) throws NoSuchFieldException, NoSuchMethodException { + try { + Constructor constructor = declaringClass.getDeclaredConstructor(); + constructor.setAccessible(true); + return constructor.newInstance(); + } catch (Exception e) { + switch (declaringClass.getName()) { + case "java.lang.reflect.Field": + return Foo.class.getDeclaredField("x"); + case "java.lang.reflect.Method": + return Foo.class.getDeclaredMethod("getX", (Class[]) null); + case "java.lang.Class": + return Foo.class; + case "java.lang.reflect.Constructor": + return Foo.class.getDeclaredConstructor(); + case "java.lang.Integer": + return Integer.valueOf(1); + default: + throw new RuntimeException( + String.format("No object instance defined for class %s", declaringClass.getName())); + } + } + } +} \ No newline at end of file diff --git a/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/UnsafeMethods.java b/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/MethodsProvider.java similarity index 67% rename from pebble/src/main/java/com/mitchellbosecke/pebble/attributes/UnsafeMethods.java rename to pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/MethodsProvider.java index 3bafa54d3..4189f99fd 100644 --- a/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/UnsafeMethods.java +++ b/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/MethodsProvider.java @@ -1,4 +1,4 @@ -package com.mitchellbosecke.pebble.attributes; +package com.mitchellbosecke.pebble.attributes.methodaccess; import java.io.IOException; @@ -11,20 +11,28 @@ import java.util.Set; import java.util.StringTokenizer; -class UnsafeMethods { +class MethodsProvider { - private static final String UNSAFE_METHODS_PROPERTIES = "/unsafeMethods.properties"; - private static final Set UNSAFE_METHODS = createUnsafeMethodsSet(); + private static final String UNSAFE_METHODS_PROPERTIES = "/security/unsafeMethods.properties"; + private static final String ALLOWED_METHODS_PROPERTIES = "/security/allowedMethods.properties"; - UnsafeMethods() { } + static final Set UNSAFE_METHODS = createUnsafeMethodsSet(); + static final Set ALLOWED_METHODS = createAllowedMethodsSet(); - boolean isUnsafeMethod(Method method) { - return UNSAFE_METHODS.contains(method); + MethodsProvider() { } - private static final Set createUnsafeMethodsSet() { + private static Set createAllowedMethodsSet() { + return createMethodsSet(ALLOWED_METHODS_PROPERTIES); + } + + private static Set createUnsafeMethodsSet() { + return createMethodsSet(UNSAFE_METHODS_PROPERTIES); + } + + private static Set createMethodsSet(String filename) { try { - Properties props = loadProperties(UNSAFE_METHODS_PROPERTIES); + Properties props = loadProperties(filename); Set set = new HashSet<>(props.size() * 4 / 3, 1f); Map primClasses = createPrimitiveClassesMap(); for (Object key : props.keySet()) { @@ -32,7 +40,8 @@ private static final Set createUnsafeMethodsSet() { } return set; } catch (Exception e) { - throw new RuntimeException("Could not load unsafe method set", e); + throw new RuntimeException(String.format("Could not load method set from file %s", filename), + e); } } @@ -72,14 +81,8 @@ private static Map createPrimitiveClassesMap() { private static Properties loadProperties(String resource) throws IOException { Properties props = new Properties(); - InputStream is = null; - try { - is = UnsafeMethods.class.getResourceAsStream(resource); + try (InputStream is = MethodsProvider.class.getResourceAsStream(resource)) { props.load(is); - } finally { - if (is != null) { - is.close(); - } } return props; } diff --git a/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/NoOpMethodAccessValidatorTest.java b/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/NoOpMethodAccessValidatorTest.java new file mode 100644 index 000000000..7a8bf0f44 --- /dev/null +++ b/pebble/src/test/java/com/mitchellbosecke/pebble/attributes/methodaccess/NoOpMethodAccessValidatorTest.java @@ -0,0 +1,17 @@ +package com.mitchellbosecke.pebble.attributes.methodaccess; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; + +public class NoOpMethodAccessValidatorTest { + + private final MethodAccessValidator underTest = new NoOpMethodAccessValidator(); + + @Test + void whenIsMethodAccessAllowed_thenReturnTrue() { + boolean methodAccessAllowed = this.underTest.isMethodAccessAllowed(null, null); + + assertThat(methodAccessAllowed).isTrue(); + } +} diff --git a/pebble/src/test/resources/security/allowedMethods.properties b/pebble/src/test/resources/security/allowedMethods.properties new file mode 100644 index 000000000..f59193424 --- /dev/null +++ b/pebble/src/test/resources/security/allowedMethods.properties @@ -0,0 +1,9 @@ +java.lang.Object.toString() +java.lang.Object.hashCode() +java.lang.Object.equals(java.lang.Object) + +com.mitchellbosecke.pebble.attributes.methodaccess.Foo.getX() +com.mitchellbosecke.pebble.attributes.methodaccess.Foo.setX(java.lang.String) +com.mitchellbosecke.pebble.attributes.methodaccess.Foo.toString() +com.mitchellbosecke.pebble.attributes.methodaccess.Foo.hashCode() +com.mitchellbosecke.pebble.attributes.methodaccess.Foo.equals(java.lang.Object) \ No newline at end of file diff --git a/pebble/src/main/resources/unsafeMethods.properties b/pebble/src/test/resources/security/unsafeMethods.properties similarity index 99% rename from pebble/src/main/resources/unsafeMethods.properties rename to pebble/src/test/resources/security/unsafeMethods.properties index 080bd60d4..e86ec4508 100644 --- a/pebble/src/main/resources/unsafeMethods.properties +++ b/pebble/src/test/resources/security/unsafeMethods.properties @@ -68,7 +68,6 @@ java.lang.ThreadGroup.resume() java.lang.ThreadGroup.setDaemon(boolean) java.lang.ThreadGroup.setMaxPriority(int) java.lang.ThreadGroup.stop() -java.lang.Thread.suspend() java.lang.Runtime.addShutdownHook(java.lang.Thread) java.lang.Runtime.exec(java.lang.String)