This is an automated email from the ASF dual-hosted git repository. henrib pushed a commit to branch JEXL-458 in repository https://gitbox.apache.org/repos/asf/commons-jexl.git
commit 5fdd3a0a2b90cc78508373a6d052065f971ec542 Author: Henrib <[email protected]> AuthorDate: Thu Mar 12 12:51:38 2026 +0100 JEXL-458: added +/- syntax for permission package parsing; - refined ability to specify and correctly apply class permissions; - added new JexlPermissions methods (allow(class, method), allow(class, field); --- .../jexl3/internal/introspection/ClassMap.java | 22 +- .../jexl3/internal/introspection/Permissions.java | 384 ++++++++++++++------- .../internal/introspection/PermissionsParser.java | 49 ++- .../jexl3/introspection/JexlPermissions.java | 139 +++++--- .../org/apache/commons/jexl3/ArithmeticTest.java | 34 +- .../org/apache/commons/jexl3/Issues400Test.java | 17 +- .../java/org/apache/commons/jexl3/JXLTTest.java | 11 +- src/test/java/org/apache/commons/jexl3/Jexl.java | 2 +- .../org/apache/commons/jexl3/JexlTestCase.java | 8 +- .../jexl3/internal/introspection/NoJexlTest.java | 4 +- .../internal/introspection/PermissionsTest.java | 2 + .../jexl3/scripting/JexlScriptEngineTest.java | 4 +- 12 files changed, 460 insertions(+), 216 deletions(-) diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/ClassMap.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/ClassMap.java index a0303c7b..220ee92c 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/introspection/ClassMap.java +++ b/src/main/java/org/apache/commons/jexl3/internal/introspection/ClassMap.java @@ -88,13 +88,13 @@ final class ClassMap { // // We also ignore all SecurityExceptions that might happen due to SecurityManager restrictions. // - for (Class<?> classToReflect = clazz; classToReflect != null; classToReflect = classToReflect.getSuperclass()) { - if (Modifier.isPublic(classToReflect.getModifiers()) && ClassTool.isExported(classToReflect)) { - populateWithClass(cache, permissions, classToReflect, log); + for (Class<?> superz = clazz; superz != null; superz = superz.getSuperclass()) { + if (Modifier.isPublic(superz.getModifiers()) && ClassTool.isExported(superz)) { + populateWithClass(clazz, cache, permissions, superz, log); } - final Class<?>[] interfaces = classToReflect.getInterfaces(); + final Class<?>[] interfaces = superz.getInterfaces(); for (final Class<?> anInterface : interfaces) { - populateWithInterface(cache, permissions, anInterface, log); + populateWithInterface(superz, cache, permissions, anInterface, log); } } // now that we've got all methods keyed in, lets organize them by name @@ -136,7 +136,7 @@ final class ClassMap { * @param clazz the class to populate the cache from * @param log the Log */ - private static void populateWithClass(final ClassMap cache, + private static void populateWithClass(final Class<?> source, final ClassMap cache, final JexlPermissions permissions, final Class<?> clazz, final Log log) { @@ -149,7 +149,7 @@ final class ClassMap { } // add method to byKey cache; do not override final MethodKey key = new MethodKey(mi); - final Method pmi = cache.byKey.putIfAbsent(key, permissions.allow(mi) ? mi : CACHE_MISS); + final Method pmi = cache.byKey.putIfAbsent(key, permissions.allow(source, mi) ? mi : CACHE_MISS); if (pmi != null && pmi != CACHE_MISS && log.isDebugEnabled() && !key.equals(new MethodKey(pmi))) { // foo(int) and foo(Integer) have the same signature for JEXL log.debug("Method " + pmi + " is already registered, key: " + key.debugString()); @@ -171,15 +171,15 @@ final class ClassMap { * @param iface the interface to populate the cache from * @param log the Log */ - private static void populateWithInterface(final ClassMap cache, + private static void populateWithInterface(final Class<?> source, final ClassMap cache, final JexlPermissions permissions, final Class<?> iface, final Log log) { if (Modifier.isPublic(iface.getModifiers())) { - populateWithClass(cache, permissions, iface, log); + populateWithClass(source, cache, permissions, iface, log); final Class<?>[] supers = iface.getInterfaces(); for (final Class<?> aSuper : supers) { - populateWithInterface(cache, permissions, aSuper, log); + populateWithInterface(source, cache, permissions, aSuper, log); } } } @@ -249,7 +249,7 @@ final class ClassMap { if (fields.length > 0) { final Map<String, Field> cache = new HashMap<>(); for (final Field field : fields) { - if (permissions.allow(field)) { + if (permissions.allow(aClass, field)) { cache.put(field.getName(), field); } } diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java index 6c2bdfa6..e25b7816 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java +++ b/src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java @@ -20,58 +20,73 @@ package org.apache.commons.jexl3.internal.introspection; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import java.lang.reflect.Proxy; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedHashSet; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.function.BiPredicate; import org.apache.commons.jexl3.annotations.NoJexl; import org.apache.commons.jexl3.introspection.JexlPermissions; /** - * Checks whether an element (ctor, field or method) is visible by JEXL introspection. - * <p>Default implementation does this by checking if element has been annotated with NoJexl.</p> + * Checks whether an element (ctor, field, or method) is visible by JEXL introspection. + * <p>The default implementation does this by checking if an element has been annotated with NoJexl.</p> * - * <p>The NoJexl annotation allows a fine grain permissions on executable objects (methods, fields, constructors). + * <p>The NoJexl annotation allows a fine grain permission on executable objects (methods, fields, constructors). * </p> * <ul> - * <li>NoJexl of a package implies all classes (including derived classes) and all interfaces + * <li>NoJexl of a package implies all classes (including derived classes), and all interfaces * of that package are invisible to JEXL.</li> - * <li>NoJexl on a class implies this class and all its derived classes are invisible to JEXL.</li> + * <li>NoJexl on a class implies this class, and all its derived classes are invisible to JEXL.</li> * <li>NoJexl on a (public) field makes it not visible as a property to JEXL.</li> * <li>NoJexl on a constructor prevents that constructor to be used to instantiate through 'new'.</li> * <li>NoJexl on a method prevents that method and any of its overrides to be visible to JEXL.</li> * <li>NoJexl on an interface prevents all methods of that interface and their overrides to be visible to JEXL.</li> * </ul> - * <p> It is possible to further refine permissions on classes used through libraries where source code form can - * not be altered using an instance of permissions using {@link JexlPermissions#parse(String...)}.</p> + * <p>It is possible to define permissions on external library classes used for which the source code + * cannot be altered using an instance of permissions using {@link JexlPermissions#parse(String...)}.</p> */ public class Permissions implements JexlPermissions { + /** + * Represents the ability to create a copy of an object. + * Any class implementing this interface must provide a concrete + * implementation for the {@code copy()} method, which returns + * a new instance of the object that is a logical copy of the original. + * + * @param <T> the type of object that can be copied + */ + interface Copyable<T> { + T copy() ; + } /** - * A positive NoJexl construct that defines what is denied by absence in the set. - * <p>Field or method that are named are the only one allowed access.</p> + * Creates a copy of a map containing copyable values. + * @param map the map to copy + * @return the copy of the map + * @param <T> the type of Copyable values */ - static class JexlClass extends NoJexlClass { - @Override boolean deny(final Constructor<?> method) { return !super.deny(method); } - @Override boolean deny(final Field field) { return !super.deny(field); } - @Override boolean deny(final Method method) { return !super.deny(method); } + static <T extends Copyable<T>> Map<String, T> copyMap(Map<String, T> map) { + Map<String, T> njc = new HashMap<>(map.size()); + for(Map.Entry<String, T> entry : map.entrySet()) { + njc.put(entry.getKey(), entry.getValue().copy()); + } + return njc; } /** - * Equivalent of @NoJexl on a ctor, a method or a field in a class. + * Equivalent of @NoJexl on a ctor, a method, or a field in a class. * <p>Field or method that are named are denied access.</p> */ - static class NoJexlClass { + static class NoJexlClass implements Copyable<NoJexlClass> { // the NoJexl method names (including ctor, name of class) - protected final Set<String> methodNames; + final Set<String> methodNames; // the NoJexl field names - protected final Set<String> fieldNames; + final Set<String> fieldNames; NoJexlClass() { this(new HashSet<>(), new HashSet<>()); @@ -81,28 +96,51 @@ public class Permissions implements JexlPermissions { methodNames = methods; fieldNames = fields; } + + @Override public NoJexlClass copy() { + return new NoJexlClass(new HashSet<>(methodNames), new HashSet<>(fieldNames)); + } boolean deny(final Constructor<?> method) { return methodNames.contains(method.getDeclaringClass().getSimpleName()); } boolean deny(final Field field) { - return fieldNames.contains(field.getName()); + return isEmpty() || fieldNames.contains(field.getName()); } boolean deny(final Method method) { - return methodNames.contains(method.getName()); + return isEmpty() || methodNames.contains(method.getName()); } boolean isEmpty() { return methodNames.isEmpty() && fieldNames.isEmpty(); } } + /** + * A positive NoJexl construct that defines what is denied by absence in the set. + * <p>Field or method that are named are the only one allowed access.</p> + */ + static class JexlClass extends NoJexlClass { + JexlClass(Set<String> methods, Set<String> fields) { + super(methods, fields); + } + JexlClass() { + super(); + } + @Override public JexlClass copy() { + return new JexlClass(new HashSet<>(methodNames), new HashSet<>(fieldNames)); + } + @Override boolean deny(final Constructor<?> method) { return !super.deny(method); } + @Override boolean deny(final Field field) { return !super.deny(field); } + @Override boolean deny(final Method method) { return !super.deny(method); } + } + /** * Equivalent of @NoJexl on a class in a package. */ - static class NoJexlPackage { + protected static class NoJexlPackage implements Copyable<NoJexlPackage> { // the NoJexl class names - protected final Map<String, NoJexlClass> nojexl; + final Map<String, NoJexlClass> nojexl; /** * Default ctor. @@ -117,7 +155,7 @@ public class Permissions implements JexlPermissions { * @param map the map of NoJexl classes */ NoJexlPackage(final Map<String, NoJexlClass> map) { - this.nojexl = new ConcurrentHashMap<>(map == null ? Collections.emptyMap() : map); + this.nojexl = map == null || map.isEmpty() ? new HashMap<>() : map; } void addNoJexl(final String key, final NoJexlClass njc) { @@ -133,6 +171,29 @@ public class Permissions implements JexlPermissions { } boolean isEmpty() { return nojexl.isEmpty(); } + + @Override public NoJexlPackage copy() { + return new NoJexlPackage(copyMap(nojexl)); + } + } + + /** + * A package where classes are allowed by default. + */ + static class JexlPackage extends NoJexlPackage { + JexlPackage(Map<String, NoJexlClass> map) { + super(map); + } + + @Override + NoJexlClass getNoJexl(final Class<?> clazz) { + NoJexlClass njc = nojexl.get(classKey(clazz)); + return njc != null ? njc : JEXL_CLASS; + } + + @Override public JexlPackage copy() { + return new JexlPackage(copyMap(nojexl)); + } } /** Marker for whole NoJexl class. */ @@ -151,7 +212,7 @@ public class Permissions implements JexlPermissions { }; /** Marker for allowed class. */ - static final NoJexlClass JEXL_CLASS = new NoJexlClass(Collections.emptySet(), Collections.emptySet()) { + static final NoJexlClass JEXL_CLASS = new JexlClass(Collections.emptySet(), Collections.emptySet()) { @Override boolean deny(final Constructor<?> method) { return false; } @@ -184,6 +245,42 @@ public class Permissions implements JexlPermissions { */ static final Permissions UNRESTRICTED = new Permissions(); + /** + * The @NoJexl execution-time map. + */ + private final Map<String, NoJexlPackage> packages; + /** + * The closed world package patterns. + */ + private final Set<String> allowed; + + /** Allow inheritance. */ + protected Permissions() { + this(Collections.emptySet(), Collections.emptyMap()); + } + + /** + * Default ctor. + * + * @param perimeter the allowed wildcard set of packages + * @param nojexl the NoJexl external map + */ + protected Permissions(final Set<String> perimeter, final Map<String, NoJexlPackage> nojexl) { + this.allowed = perimeter; + this.packages = nojexl; + } + + /** + * Creates a new set of permissions by composing these permissions with a new set of rules. + * + * @param src the rules + * @return the new permissions + */ + @Override + public Permissions compose(final String... src) { + return new PermissionsParser().parse(new HashSet<>(allowed), copyMap(packages), src); + } + /** * Creates a class key joining enclosing ascendants with '$'. * <p>As in {@code outer$inner} for <code>class outer { class inner...</code>.</p> @@ -221,7 +318,7 @@ public class Permissions implements JexlPermissions { } /** - * Whether the wilcard set of packages allows a given package to be introspected. + * Whether the wildcard set of packages allows a given package to be introspected. * * @param allowed the allowed set (not null, may be empty) * @param name the package name (not null) @@ -241,34 +338,71 @@ public class Permissions implements JexlPermissions { } /** - * The @NoJexl execution-time map. + * Gets the package constraints. + * + * @param packageName the package name + * @return the package constraints instance, not-null. */ - private final Map<String, NoJexlPackage> packages; + private NoJexlPackage getNoJexlPackage(final String packageName) { + return packages.getOrDefault(packageName, JEXL_PACKAGE); + } /** - * The closed world package patterns. + * @return the packages */ - private final Set<String> allowed; + Map<String, NoJexlPackage> getPackages() { + return packages == null ? Collections.emptyMap() : Collections.unmodifiableMap(packages); + } - /** Allow inheritance. */ - protected Permissions() { - this(Collections.emptySet(), Collections.emptyMap()); + /** + * @return the wildcards + */ + Set<String> getWildcards() { + return allowed == null ? Collections.emptySet() : Collections.unmodifiableSet(allowed); } /** - * Default ctor. + * Whether the wildcard set of packages allows a given class to be introspected. * - * @param perimeter the allowed wildcard set of packages - * @param nojexl the NoJexl external map + * @param clazz the package name (not null) + * @return true if allowed, false otherwise */ - protected Permissions(final Set<String> perimeter, final Map<String, NoJexlPackage> nojexl) { - this.allowed = perimeter; - this.packages = nojexl; + private boolean wildcardAllow(final Class<?> clazz) { + return wildcardAllow(allowed, ClassTool.getPackageName(clazz)); + } + + /** + * Determines whether a specified permission check is allowed for a given class. + * The check involves verifying if a class or its corresponding package explicitly permits + * a name (e.g., method) based on a given condition. + * + * @param <T> the type of the name to check (e.g., method, constructor) + * @param clazz the class to evaluate (not null) + * @param name the name to verify (not null) + * @param check the condition to test whether the specified name is allowed (not null) + * @return true if the specified name is allowed based on the condition, false otherwise + */ + private <T> boolean specifiedAllow(final Class<?> clazz, T name, BiPredicate<NoJexlClass, T> check) { + final String packageName = ClassTool.getPackageName(clazz); + if (wildcardAllow(allowed, packageName)) { + return true; + } + final NoJexlPackage njp = packages.get(packageName); + if (njp != null && check != null) { + // there is a package permission, check if there is a class permission + final NoJexlClass njc = njp.getNoJexl(clazz); + // if there is a class permission, perform the check + if (njc != null) { + return check.test(njc, name); + } + } + // nothing explicit + return false; } /** * Checks whether a class or one of its super-classes or implemented interfaces - * explicitly disallows JEXL introspection. + * explicitly allows JEXL introspection. * * @param clazz the class to check * @return true if JEXL is allowed to introspect, false otherwise @@ -288,7 +422,7 @@ public class Permissions implements JexlPermissions { return false; } // no super class can be denied and at least one must be allowed - boolean explicit = wildcardAllow(clazz); + boolean explicit = specifiedAllow(clazz, clazz, (njc, c) -> njc instanceof JexlClass); Class<?> walk = clazz.getSuperclass(); while (walk != null) { if (deny(walk)) { @@ -313,15 +447,17 @@ public class Permissions implements JexlPermissions { */ private boolean allow(final Class<?> clazz, final Method method, final boolean[] explicit) { try { - // check if method in that class is declared ie overrides + // check if the method in that class is declared thus overrides final Method override = clazz.getDeclaredMethod(method.getName(), method.getParameterTypes()); - // should not be possible... - if (denyMethod(override)) { - return false; - } - // explicit |= ... - if (!explicit[0]) { - explicit[0] = wildcardAllow(clazz) || specifiedAllow(clazz, override, (njc, m) -> !njc.deny(m)); + if (override != method) { + // should not be possible... + if (denyMethod(override)) { + return false; + } + // explicit |= ... + if (!explicit[0]) { + explicit[0] = specifiedAllow(clazz, override, (njc, m) -> !njc.deny(m)); + } } return true; } catch (final NoSuchMethodException ex) { @@ -334,7 +470,7 @@ public class Permissions implements JexlPermissions { } /** - * Checks whether a constructor explicitly disallows JEXL introspection. + * Checks whether a constructor explicitly allows JEXL introspection. * * @param ctor the constructor to check * @return true if JEXL is allowed to introspect, false otherwise @@ -355,36 +491,12 @@ public class Permissions implements JexlPermissions { return false; } // check wildcards - return wildcardAllow(clazz); + return specifiedAllow(clazz, clazz, (njc, c) -> !njc.deny(ctor)); } - /** - * Determines whether a specified permission check is allowed for a given class. - * The check involves verifying if a class or its corresponding package explicitly permits - * a name (e.g., method) based on a given condition. - * - * @param <T> the type of the name to check (e.g., method, constructor) - * @param clazz the class to evaluate (not null) - * @param name the name to verify (not null) - * @param check the condition to test whether the specified name is allowed (not null) - * @return true if the specified name is allowed based on the condition, false otherwise - */ - private <T> boolean specifiedAllow(final Class<?> clazz, T name, BiPredicate<NoJexlClass, T> check) { - final NoJexlPackage njp = packages.get(ClassTool.getPackageName(clazz)); - if (njp != null && check != null) { - // there is a package permission, check if there is a class permission - final NoJexlClass njc = njp.getNoJexl(clazz); - // if there is a class permission, perform the check - if (njc != null) { - return check.test(njc, name); - } - } - // nothing explicit - return false; - } /** - * Checks whether a field explicitly disallows JEXL introspection. + * Checks whether a field explicitly allows JEXL introspection. * * @param field the field to check * @return true if JEXL is allowed to introspect, false otherwise @@ -405,13 +517,63 @@ public class Permissions implements JexlPermissions { return false; } // check wildcards - return wildcardAllow(clazz) || specifiedAllow(clazz, field, (njc, m) -> !njc.deny(m)); + return specifiedAllow(clazz, field, (njc, m) -> !njc.deny(m)); + } + + @Override + public boolean allow(final Class<?> clazz, final Method method) { + if (!validate(clazz) || !validate(method)) { + return false; + } + if ((method.getModifiers() & Modifier.STATIC) == 0) { + Class<?> declaring = method.getDeclaringClass(); + if (clazz != declaring) { + if (deny(clazz)) { + return false; + } + // just check this is an override of a method in clazz, if not, it is not allowed (obviously) + if (!declaring.isAssignableFrom(clazz)) { + return false; + } + // if there is an explicit permission, allow if not denied + NoJexlClass njc = getNoJexl(clazz, null); + if (njc != null) { + return !njc.deny(method); + } + } + } + return allow(method); + } + + @Override + public boolean allow(final Class<?> clazz, final Field field) { + if (!validate(clazz) || !validate(field)) { + return false; + } + if ((field.getModifiers() & Modifier.STATIC) == 0) { + Class<?> declaring = field.getDeclaringClass(); + if (clazz != declaring) { + if (deny(clazz)) { + return false; + } + // just check this clazz extends/inherits from declaring, if not, it is not allowed (obviously) + if (!declaring.isAssignableFrom(clazz)) { + return false; + } + // if there is an explicit permission, allow if not denied + NoJexlClass njc = getNoJexl(clazz, null); + if (njc != null) { + return !njc.deny(field); + } + } + } + return allow(field); } /** - * Checks whether a method explicitly disallows JEXL introspection. + * Checks whether a method explicitly allows JEXL introspection. * <p>Since methods can be overridden, this also checks that no superclass or interface - * explicitly disallows this methods.</p> + * explicitly disallows this method.</p> * * @param method the method to check * @return true if JEXL is allowed to introspect, false otherwise @@ -428,7 +590,7 @@ public class Permissions implements JexlPermissions { } Class<?> clazz = method.getDeclaringClass(); // gather if the packages explicitly allow any implementation of the method - final boolean[] explicit = { wildcardAllow(clazz) || specifiedAllow(clazz, method, (njc, m) -> !njc.deny(m)) }; + final boolean[] explicit = { specifiedAllow(clazz, method, (njc, m) -> !njc.deny(m)) }; // let's walk all interfaces for (final Class<?> inter : clazz.getInterfaces()) { if (!allow(inter, method, explicit)) { @@ -454,19 +616,20 @@ public class Permissions implements JexlPermissions { */ @Override public boolean allow(final Package pack) { - return validate(pack) && !deny(pack); + // field must be public + if (!validate(pack)) { + return false; + } + // check declared restrictions + if (deny(pack)) { + return false; + } + // must have specified a wildcard or some non-empty permission for this package + final String name = pack.getName(); + final NoJexlPackage njp = packages.get(name); + return njp == null ? wildcardAllow(allowed, name) : !njp.isEmpty(); } - /** - * Creates a new set of permissions by composing these permissions with a new set of rules. - * - * @param src the rules - * @return the new permissions - */ - @Override - public Permissions compose(final String... src) { - return new PermissionsParser().parse(new LinkedHashSet<>(allowed),new ConcurrentHashMap<>(packages), src); - } /** * Tests whether a whole class is denied Jexl visibility. @@ -568,6 +731,9 @@ public class Permissions implements JexlPermissions { * @return the class constraints instance, not-null. */ private NoJexlClass getNoJexl(final Class<?> clazz) { + return getNoJexl(clazz, JEXL_CLASS); + } + private NoJexlClass getNoJexl(final Class<?> clazz, NoJexlClass ifNone) { final String pkgName = ClassTool.getPackageName(clazz); final NoJexlPackage njp = getNoJexlPackage(pkgName); if (njp != null) { @@ -576,40 +742,6 @@ public class Permissions implements JexlPermissions { return njc; } } - return JEXL_CLASS; - } - - /** - * Gets the package constraints. - * - * @param packageName the package name - * @return the package constraints instance, not-null. - */ - private NoJexlPackage getNoJexlPackage(final String packageName) { - return packages.getOrDefault(packageName, JEXL_PACKAGE); - } - - /** - * @return the packages - */ - Map<String, NoJexlPackage> getPackages() { - return packages == null ? Collections.emptyMap() : Collections.unmodifiableMap(packages); - } - - /** - * @return the wilcards - */ - Set<String> getWildcards() { - return allowed == null ? Collections.emptySet() : Collections.unmodifiableSet(allowed); - } - - /** - * Whether the wildcard set of packages allows a given class to be introspected. - * - * @param clazz the package name (not null) - * @return true if allowed, false otherwise - */ - private boolean wildcardAllow(final Class<?> clazz) { - return wildcardAllow(allowed, ClassTool.getPackageName(clazz)); + return ifNone; } } diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/PermissionsParser.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/PermissionsParser.java index 63f286ba..1f9ba13d 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/introspection/PermissionsParser.java +++ b/src/main/java/org/apache/commons/jexl3/internal/introspection/PermissionsParser.java @@ -216,11 +216,15 @@ public class PermissionsParser { i += 1; } // empty class means allow or deny all - if (njname != null && njclass.isEmpty()) { - njpackage.addNoJexl(njname, njclass instanceof Permissions.JexlClass - ? Permissions.JEXL_CLASS - : Permissions.NOJEXL_CLASS); - + if (njname != null) { + if (njclass.isEmpty()) { + njpackage.addNoJexl(njname, + njclass instanceof Permissions.JexlClass + ? Permissions.JEXL_CLASS + : Permissions.NOJEXL_CLASS); + } else { + njpackage.addNoJexl(njname, njclass); + } } return i; } @@ -306,6 +310,7 @@ public class PermissionsParser { int i = 0; int j = -1; String pname = null; + Boolean negative = null; while (i < size) { final char c = src.charAt(i); // if no parsing progress can be made, we are in error @@ -340,16 +345,44 @@ public class PermissionsParser { } // package mode if (njpackage == null) { + // negative or positive package + if (negative == null) { + if (c == '-') { + negative = true; + i += 1; + continue; + } + if (c == '+') { + negative = false; + i += 1; + continue; + } + // default is deny specified classes + //negative = true; + } if (c == '{') { + Boolean specified = negative; njpackage = packages.compute(pname, - (n, p) -> new Permissions.NoJexlPackage(p == null? null : p.nojexl) + (n, p) -> { + // if we haven't specified allow/deny, + // keep the existing specification if any, otherwise default to deny + boolean deny = specified == null ? !(p instanceof Permissions.JexlPackage) : specified; + Map<String, Permissions.NoJexlClass> pkgMap = p == null ? null : p.nojexl; + return deny + ? new Permissions.NoJexlPackage(pkgMap) + : new Permissions.JexlPackage(pkgMap); + } ); i += 1; } } else if (c == '}') { - // empty means whole package + // empty means the whole package if (njpackage.isEmpty()) { - packages.put(pname, Permissions.NOJEXL_PACKAGE); + packages.put(pname, negative == null || negative + ? Permissions.NOJEXL_PACKAGE + : Permissions.JEXL_PACKAGE); + } else { + packages.put(pname, njpackage); } njpackage = null; // can restart anew pname = null; diff --git a/src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java b/src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java index 304ab139..8a944cee 100644 --- a/src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java +++ b/src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java @@ -69,7 +69,7 @@ public interface JexlPermissions { /** * A permission delegation that augments the RESTRICTED permission with an explicit * set of classes. - * <p>Typical use case is to deny access to a package - and thus all its classes - but allow + * <p>A typical use case is to deny access to a package - and thus all its classes - but allow * a few specific classes.</p> * <p>Note that the newer positive restriction syntax is preferable as in: * <code>RESTRICTED.compose("java.lang { +Class {} }")</code>.</p> @@ -104,17 +104,27 @@ public interface JexlPermissions { @Override public boolean allow(final Class<?> clazz) { - return validate(clazz) && isClassAllowed(clazz) || super.allow(clazz); + return validate(clazz) && (isClassAllowed(clazz) || super.allow(clazz)); } @Override public boolean allow(final Constructor<?> constructor) { - return validate(constructor) && isClassAllowed(constructor.getDeclaringClass()) || super.allow(constructor); + return validate(constructor) && + (allowedClasses.contains(constructor.getDeclaringClass().getCanonicalName()) || super.allow(constructor)); } @Override - public boolean allow(final Method method) { - return validate(method) && isClassAllowed(method.getDeclaringClass()) || super.allow(method); + public boolean allow(final Class<?> clazz, final Method method) { + if (!validate(method)) { + return false; + } + if (!method.getDeclaringClass().isAssignableFrom(clazz)) { + return false; + } + if (super.allow(clazz, method)) { + return true; + } + return isClassAllowed(clazz); } @Override @@ -122,9 +132,23 @@ public interface JexlPermissions { return new ClassPermissions(base.compose(src), allowedClasses); } - private boolean isClassAllowed(final Class<?> clazz) { - return allowedClasses.contains(clazz.getCanonicalName()); + private boolean isClassAllowed(final Class<?> aClass) { + Class<?> clazz = aClass; + // let's walk all interfaces + for (final Class<?> inter : clazz.getInterfaces()) { + if (allowedClasses.contains(inter.getCanonicalName())) { + return true; + } + } + // let's walk all super classes + while (clazz != null) { + if (allowedClasses.contains(clazz.getCanonicalName())) { + return true; + } + clazz = clazz.getSuperclass(); } + return false; + } } /** @@ -210,52 +234,35 @@ public interface JexlPermissions { * <li>org.apache.commons.jexl3.internal { Engine {} Engine32 {} TemplateEngine {} }</li> * <li>org.apache.commons.jexl3.internal.introspection { Uberspect {} Introspector {} }</li> * <li>java.lang { Runtime {} System {} ProcessBuilder {} Process {} RuntimePermission {} SecurityManager {} Thread {} ThreadGroup {} Class {} }</li> - * <li>java.lang.annotation {}</li> - * <li>java.lang.classfile {}</li> - * <li>java.lang.constant {}</li> - * <li>java.lang.foreign {}</li> - * <li>java.lang.instrument {}</li> - * <li>java.lang.invoke {}</li> - * <li>java.lang.management {}</li> - * <li>java.lang.module {}</li> - * <li>java.lang.ref {}</li> - * <li>java.lang.reflect {}</li> - * <li>java.net {}</li> * <li>java.io { +PrintWriter {} +Writer {} +StringWriter {} +Reader {} +InputStream {} +OutputStream {} }</li> - * <li>java.nio { Path {} Paths {} Files {} }</li> + * <li>java.nio +{}</li> * <li>java.rmi {}</li> * </ul> */ - JexlPermissions RESTRICTED = JexlPermissions.parse( - "# Restricted Uberspect Permissions", - "java.nio.*", - "java.lang.*", - "java.math.*", - "java.text.*", - "java.util.*", - "org.w3c.dom.*", - "org.apache.commons.jexl3.*", - "org.apache.commons.jexl3 { JexlBuilder{} }", - "org.apache.commons.jexl3.introspection { JexlPermissions{} JexlPermissions$ClassPermissions{} }", - "org.apache.commons.jexl3.internal { Engine{} Engine32{} TemplateEngine{} }", - "org.apache.commons.jexl3.internal.introspection { Uberspect{} Introspector{} }", - "java.lang { Runtime{} System{} ProcessBuilder{} Process{}" + - " RuntimePermission{} SecurityManager{}" + - " Thread{} ThreadGroup{} Class{} }", - "java.lang.annotation {}", - "java.lang.classfile {}", - "java.lang.constant {}", - "java.lang.foreign {}", - "java.lang.instrument {}", - "java.lang.invoke {}", - "java.lang.management {}", - "java.lang.module {}", - "java.lang.ref {}", - "java.lang.reflect {}", - "java.net {}", - "java.io { +PrintWriter{} +Writer {} +StringWriter {} +Reader {} +InputStream {} +OutputStream {} }", - "java.nio { Path {} Paths {} Files {} }", - "java.rmi" + + JexlPermissions DEFAULT = JexlPermissions.parse( + "# Default Uberspect Permissions", + "java.math.*", + "java.text.*", + "java.time.*", + "java.util.*", + "org.w3c.dom.*", + "java.lang +{" + + "-Runtime{} -System{} -ProcessBuilder{} -Process{}" + + "-RuntimePermission{} -SecurityManager{}" + + "-Thread{} -ThreadGroup{} -Class{} -ClassLoader{}" + + "}", + "java.io -{ +PrintWriter{} +Writer{} +StringWriter{} +Reader{} +InputStream{} +OutputStream{} }", + "java.nio +{}", + "java.nio.charset +{}" + ); + JexlPermissions RESTRICTED = DEFAULT.compose( + // the following ones are mostly for tests, these should be reassessed (jxlt) + "org.apache.commons.jexl3.*", + "org.apache.commons.jexl3 +{ -JexlBuilder{} }", + "org.apache.commons.jexl3.introspection -{ JexlPermissions{} JexlPermissions$ClassPermissions{} }", + "org.apache.commons.jexl3.internal -{ Engine{} Engine32{} TemplateEngine{} }", + "org.apache.commons.jexl3.internal.introspection -{ Uberspect{} Introspector{} }" ); /** @@ -361,8 +368,8 @@ public interface JexlPermissions { boolean allow(Constructor<?> ctor); /** - * Checks whether a field explicitly disallows JEXL introspection. - * <p>If a field is not allowed, it cannot resolved and accessed in scripts or expressions.</p> + * Checks whether a field explicitly allows JEXL introspection. + * <p>If a field is not allowed, it cannot be resolved and accessed in scripts or expressions.</p> * * @param field the field to check * @return true if JEXL is allowed to introspect, false otherwise @@ -370,11 +377,23 @@ public interface JexlPermissions { */ boolean allow(Field field); + /** + * Checks whether a field explicitly allows JEXL introspection. + * <p>If a field is not allowed, it cannot be resolved and accessed in scripts or expressions.</p> + * @param clazz the class from which the field is accessed, used to check that the field is allowed for this class + * @param field the field to check + * @return true if JEXL is allowed to introspect, false otherwise + * @since 3.7 + */ + default boolean allow(Class<?> clazz, Field field) { + return allow(field); + } + /** * Checks whether a method allows JEXL introspection. - * <p>If a method is not allowed, it cannot resolved and called in scripts or expressions.</p> + * <p>If a method is not allowed, it cannot be resolved and called in scripts or expressions.</p> * <p>Since methods can be overridden and overloaded, this also checks that no superclass or interface - * explicitly disallows this methods.</p> + * explicitly disallows this method.</p> * * @param method the method to check * @return true if JEXL is allowed to introspect, false otherwise @@ -382,6 +401,20 @@ public interface JexlPermissions { */ boolean allow(Method method); + /** + * Checks whether a method allows JEXL introspection. + * <p>If a method is not allowed, it cannot be resolved and called in scripts or expressions.</p> + * <p>Since methods can be overridden and overloaded, this checks that this class explicitly allows + * this method - superseding any superclass or interface specified permissions.</p> + * + * @param method the method to check + * @return true if JEXL is allowed to introspect, false otherwise + * @since 3.7 + */ + default boolean allow(Class<?> clazz, Method method) { + return allow(method); + } + /** * Checks whether a package allows JEXL introspection. * <p>If the package disallows JEXL introspection, none of its classes or interfaces are visible diff --git a/src/test/java/org/apache/commons/jexl3/ArithmeticTest.java b/src/test/java/org/apache/commons/jexl3/ArithmeticTest.java index ac4ad183..21b903cc 100644 --- a/src/test/java/org/apache/commons/jexl3/ArithmeticTest.java +++ b/src/test/java/org/apache/commons/jexl3/ArithmeticTest.java @@ -440,6 +440,9 @@ class ArithmeticTest extends JexlTestCase { } } + /** + * An arithmetic that has some DOM node knowledge. + */ public static class XmlArithmetic extends JexlArithmetic { public XmlArithmetic(final boolean astrict) { super(astrict); @@ -456,6 +459,27 @@ class ArithmeticTest extends JexlTestCase { public int size(final org.w3c.dom.Element elt) { return elt.getChildNodes().getLength(); } + + public Object propertyGet(final NamedNodeMap attrs, final String name) { + Node node = attrs.getNamedItem(name); + return node == null ? JexlEngine.TRY_FAILED : node.getNodeValue(); + } + + public Object propertySet(final NamedNodeMap attrs, final String name, final Object value) { + Node node = attrs.getNamedItem(name); + if (node == null) { + return JexlEngine.TRY_FAILED; + } + node.setNodeValue(value.toString()); + return value; + } + + public Object propertyGet(final Node elt, final String name) { + if ("attributes".equals(name)) { + return elt.getAttributes(); + } + return JexlEngine.TRY_FAILED; + } } /** A small delta to compare doubles. */ @@ -1022,8 +1046,8 @@ class ArithmeticTest extends JexlTestCase { } /** - * test some simple mathematical calculations - */ + * test some simple mathematical calculations + */ @Test void testCalculations() throws Exception { asserter.setStrict(true, false); @@ -2242,18 +2266,18 @@ class ArithmeticTest extends JexlTestCase { jc.set("x", xmlx); final String y = "456"; jc.set("y", y); - final JexlScript s = jexl.createScript("x.attribute.info = y"); + final JexlScript s = jexl.createScript("x.attributes.info = y"); Object r; try { - r = s.execute(jc); nnm = xml.getLastChild().getAttributes(); info = (Attr) nnm.getNamedItem("info"); + r = s.execute(jc); assertEquals(y, r); assertEquals(y, info.getValue()); } catch (final JexlException.Property xprop) { // test fails in java > 11 because modules, etc; need investigation assertTrue(xprop.getMessage().contains("attribute")); - assertTrue(getJavaVersion() > 11); + assertTrue(getJavaVersion() > 11, getJavaVersion() + " >? 11"); } } diff --git a/src/test/java/org/apache/commons/jexl3/Issues400Test.java b/src/test/java/org/apache/commons/jexl3/Issues400Test.java index 280fd6d6..119385d4 100644 --- a/src/test/java/org/apache/commons/jexl3/Issues400Test.java +++ b/src/test/java/org/apache/commons/jexl3/Issues400Test.java @@ -864,21 +864,27 @@ public class Issues400Test { } @Test - void test450b() { + void test450b() throws NoSuchMethodException { // cannot load System with RESTRICTED assertThrows(JexlException.Method.class, () -> run450b(JexlPermissions.RESTRICTED), "should not be able to load System with RESTRICTED"); - // can load System with UNRESTRICTED + // can load System.exit with UNRESTRICTED + Method exitMethod = java.lang.System.class.getMethod("exit", int.class); + assertTrue(UNRESTRICTED.allow(exitMethod)); + assertFalse(RESTRICTED.allow(exitMethod)); assertEquals(java.lang.System.class, run450b(UNRESTRICTED)); // need to explicitly allow Uberspect and the current class loader to load System + Class<? extends ClassLoader> cloader = getClass().getClassLoader().getClass(); JexlPermissions perm = new JexlPermissions.ClassPermissions( - getClass().getClassLoader().getClass(), + ClassLoader.class, org.apache.commons.jexl3.internal.introspection.Uberspect.class); + //assertTrue(perm.allow(cloader), "should be able to access ClassLoader"); + assertTrue(perm.allow(cloader, cloader.getMethod("loadClass", String.class)), "should be able to access ClassLoader::loadClass"); assertEquals(java.lang.System.class, run450b(perm)); } private static Object run450b(JexlPermissions perm) { - JexlEngine jexl = new JexlBuilder().silent(false).permissions(perm).create(); + JexlEngine jexl = new JexlBuilder().safe(false).strict(true).silent(false).permissions(perm).create(); String uscript = "new('org.apache.commons.jexl3.internal.introspection.Uberspect', " + "null, null, perm).getClassLoader().loadClass('java.lang.System')"; JexlScript u0 = jexl.createScript(uscript, "perm"); @@ -962,10 +968,11 @@ public class Issues400Test { } @Test - void test451() { + void test451() throws NoSuchMethodException { JexlEngine jexl = new JexlBuilder().create(); assertEquals("42", jexl.createScript("o.toString()", "o").execute(null, "42")); JexlPermissions perms = RESTRICTED.compose("java.lang { +Class { getSimpleName(); } }"); + assertTrue(perms.allow(String.class.getMethod("toString"))); JexlSandbox sandbox = new JexlSandbox(false, true); sandbox.permissions(Object.class.getName(), true, true, false, false); sandbox.allow(String.class.getName()).execute("toString"); diff --git a/src/test/java/org/apache/commons/jexl3/JXLTTest.java b/src/test/java/org/apache/commons/jexl3/JXLTTest.java index fe654a29..fe9515f1 100644 --- a/src/test/java/org/apache/commons/jexl3/JXLTTest.java +++ b/src/test/java/org/apache/commons/jexl3/JXLTTest.java @@ -44,6 +44,7 @@ import org.apache.commons.jexl3.internal.TemplateDebugger; import org.apache.commons.jexl3.internal.TemplateInterpreter; import org.apache.commons.jexl3.internal.introspection.Permissions; import org.apache.commons.jexl3.internal.introspection.Uberspect; +import org.apache.commons.jexl3.introspection.JexlPermissions; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.junit.jupiter.api.AfterEach; @@ -57,6 +58,9 @@ import org.junit.jupiter.params.provider.MethodSource; */ @SuppressWarnings({"UnnecessaryBoxing", "AssertEqualsBetweenInconvertibleTypes"}) class JXLTTest extends JexlTestCase { + static JexlPermissions P0 = JexlPermissions.DEFAULT; + static JexlPermissions P1 = JexlPermissions.RESTRICTED; + public static class Context311 extends MapContext implements JexlContext.OptionsHandle, JexlContext.ThreadLocal { private JexlOptions options; @@ -146,10 +150,11 @@ class JXLTTest extends JexlTestCase { public static List<JexlBuilder> engines() { final JexlFeatures f = new JexlFeatures(); f.lexical(true).lexicalShade(true); + JexlPermissions permissions = JexlPermissions.DEFAULT.compose("org.apache.commons.jexl3 +{ JXLTTest{} }"); return Arrays.asList( - new JexlBuilder().silent(false).lexical(true).lexicalShade(true).cache(128).strict(true), - new JexlBuilder().features(f).silent(false).cache(128).strict(true), - new JexlBuilder().silent(false).cache(128).strict(true)); + new JexlBuilder().permissions(permissions).silent(false).lexical(true).lexicalShade(true).cache(128).strict(true), + new JexlBuilder().permissions(permissions).features(f).silent(false).cache(128).strict(true), + new JexlBuilder().permissions(permissions).silent(false).cache(128).strict(true)); } private static String refactor(final TemplateDebugger td, final JxltEngine.Template ts) { diff --git a/src/test/java/org/apache/commons/jexl3/Jexl.java b/src/test/java/org/apache/commons/jexl3/Jexl.java index 91f0f9e1..9f895950 100644 --- a/src/test/java/org/apache/commons/jexl3/Jexl.java +++ b/src/test/java/org/apache/commons/jexl3/Jexl.java @@ -29,7 +29,7 @@ import org.apache.commons.jexl3.internal.Engine; public class Jexl { public static void main(final String[] args) { - final JexlEngine JEXL = new Engine(); + final JexlEngine JEXL = new JexlBuilder().create(); final Map<Object, Object> m = System.getProperties(); // dummy context to get variables final JexlContext context = new MapContext(); diff --git a/src/test/java/org/apache/commons/jexl3/JexlTestCase.java b/src/test/java/org/apache/commons/jexl3/JexlTestCase.java index c8022d55..246be186 100644 --- a/src/test/java/org/apache/commons/jexl3/JexlTestCase.java +++ b/src/test/java/org/apache/commons/jexl3/JexlTestCase.java @@ -79,7 +79,6 @@ public class JexlTestCase { // define mode pro50 static final JexlOptions MODE_PRO50 = new JexlOptions(); - static { MODE_PRO50.setFlags("+strict +cancellable +lexical +lexicalShade -safe".split(" ")); } @@ -89,6 +88,13 @@ public class JexlTestCase { */ public static final JexlPermissions SECURE = JexlPermissions.RESTRICTED; + public static final JexlPermissions TESTPERMS = JexlPermissions.RESTRICTED.compose( + "org.apache.commons.jexl3.*", + "org.apache.commons.jexl3 +{ -JexlBuilder{} }", + "org.apache.commons.jexl3.introspection -{ JexlPermissions{} JexlPermissions$ClassPermissions{} }", + "org.apache.commons.jexl3.internal -{ Engine{} Engine32{} TemplateEngine{} }", + "org.apache.commons.jexl3.internal.introspection -{ Uberspect{} Introspector{} }"); + static JexlEngine createEngine() { return new JexlBuilder().create(); } diff --git a/src/test/java/org/apache/commons/jexl3/internal/introspection/NoJexlTest.java b/src/test/java/org/apache/commons/jexl3/internal/introspection/NoJexlTest.java index 6519a42f..c1f6b405 100644 --- a/src/test/java/org/apache/commons/jexl3/internal/introspection/NoJexlTest.java +++ b/src/test/java/org/apache/commons/jexl3/internal/introspection/NoJexlTest.java @@ -25,6 +25,7 @@ import java.lang.reflect.Field; import java.lang.reflect.Method; import org.apache.commons.jexl3.annotations.NoJexl; +import org.apache.commons.jexl3.introspection.JexlPermissions; import org.junit.jupiter.api.Test; /** @@ -85,6 +86,7 @@ class NoJexlTest { @Test void testNoJexlPermissions() throws Exception { + final JexlPermissions r = Permissions.RESTRICTED; final Permissions p = Permissions.UNRESTRICTED; assertFalse(p.allow((Field) null)); assertFalse(p.allow((Package) null)); @@ -115,7 +117,7 @@ class NoJexlTest { final Field fA = A.class.getField("i"); assertNotNull(fA); - assertTrue(p.allow(fA)); + assertTrue(p.allow(A.class, fA)); final Field fA0 = A0.class.getField("i0"); assertNotNull(fA0); diff --git a/src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsTest.java b/src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsTest.java index 2b60e9f6..9d970470 100644 --- a/src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsTest.java +++ b/src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsTest.java @@ -201,6 +201,7 @@ class PermissionsTest { final Field fA0 = A0.class.getField("i0"); assertNotNull(fA0); assertFalse(p.allow(fA0)); + assertFalse(p.allow(A0.class, fA0)); final Field fA1 = A1.class.getDeclaredField("i1"); assertNotNull(fA1); assertFalse(p.allow(fA0)); @@ -482,6 +483,7 @@ class PermissionsTest { java.lang.ref.SoftReference.class, java.lang.reflect.Method.class); for(final Class<?> nac : nacs) { + assertFalse(JexlTestCase.SECURE.allow(nac), nac::getName); final Package p = nac.getPackage(); assertNotNull(p, nac::getName); assertFalse(JexlTestCase.SECURE.allow(p), nac::getName); diff --git a/src/test/java/org/apache/commons/jexl3/scripting/JexlScriptEngineTest.java b/src/test/java/org/apache/commons/jexl3/scripting/JexlScriptEngineTest.java index 3f4966cb..bd1b622e 100644 --- a/src/test/java/org/apache/commons/jexl3/scripting/JexlScriptEngineTest.java +++ b/src/test/java/org/apache/commons/jexl3/scripting/JexlScriptEngineTest.java @@ -213,8 +213,8 @@ class JexlScriptEngineTest { assertEquals(initialValue, engine.eval("0;123")); // multiple statements final ScriptException xscript = assertThrows(ScriptException.class, () -> engine.eval("sys=context.class.forName(\"java.lang.System\");now=sys.currentTimeMillis();")); - final JexlException.Method xjexl = (JexlException.Method) xscript.getCause(); - assertEquals("forName", xjexl.getMethod()); + final JexlException.Property xjexl = (JexlException.Property) xscript.getCause(); + assertEquals("class", xjexl.getProperty()); engine.put("value", initialValue); assertEquals(initialValue, engine.get("value")); final Integer newValue = 124;
