Copilot commented on code in PR #400:
URL: https://github.com/apache/commons-jexl/pull/400#discussion_r3120053386
##########
src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java:
##########
@@ -454,19 +616,20 @@ public boolean allow(final Method method) {
*/
@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();
Review Comment:
`Permissions.allow(Package)` returns `false` for explicit "allow package"
rules represented by the `JEXL_PACKAGE` marker (since its internal map is empty
and `!njp.isEmpty()` is false). With the new `+{}` syntax (e.g., `java.nio
+{}`), this makes `allow(package)` inconsistent with the configured
permissions. Treat the `JEXL_PACKAGE` marker as allowed (and only deny for
`NOJEXL_PACKAGE`).
```suggestion
// an explicit package entry is allowed unless it is the deny marker
final String name = pack.getName();
final NoJexlPackage njp = packages.get(name);
return njp == null ? wildcardAllow(allowed, name) :
!Objects.equals(NOJEXL_PACKAGE, njp);
```
##########
src/main/java/org/apache/commons/jexl3/internal/introspection/ClassMap.java:
##########
@@ -88,13 +88,13 @@ private static void create(final ClassMap cache, final
JexlPermissions permissio
//
// 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);
Review Comment:
`ClassMap.create()` passes `superz` as the `source` when populating
interface methods (`populateWithInterface(superz, ...)`). That means interface
methods inherited via a superclass are checked against the superclass'
permissions, not the original class being introspected, which can bypass
subclass-specific method restrictions implemented via `allow(Class, Method)`.
Use the original `clazz` as the `source` for interface population as well.
```suggestion
populateWithInterface(clazz, cache, permissions,
anInterface, log);
```
##########
src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java:
##########
@@ -69,71 +89,112 @@ 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>
*/
- final class ClassPermissions extends JexlPermissions.Delegate {
-
- /** The set of explicitly allowed classes, overriding the delegate
permissions. */
- private final Set<String> allowedClasses;
-
- /**
- * Creates permissions based on the RESTRICTED set but allowing an
explicit set.
- *
- * @param allow the set of allowed classes
- */
- public ClassPermissions(final Class<?>... allow) {
- this(JexlPermissions.RESTRICTED,
- Arrays.stream(Objects.requireNonNull(allow))
- .map(Class::getCanonicalName)
- .collect(Collectors.toList()));
+ final class ClassPermissions extends JexlPermissions.Delegate {
+ /**
+ * The set of explicitly allowed classes, overriding the delegate
permissions.
+ */
+ private final Set<String> allowedClasses;
+
+ /**
+ * Creates permissions based on the RESTRICTED set but allowing an
explicit set.
+ *
+ * @param allow the set of allowed classes
+ */
+ public ClassPermissions(final Class<?>... allow) {
+ this(JexlPermissions.RESTRICTED, allow);
+ }
+
+ public ClassPermissions(final JexlPermissions permissions, final
Class<?>... allow) {
+ this(permissions,
Arrays.stream(Objects.requireNonNull(allow)).map(Class::getCanonicalName).collect(Collectors.toList()));
+ }
+
+ /**
+ * Required for compose().
+ *
+ * @param delegate the base to delegate to
+ * @param allow the list of class canonical names
+ */
+ public ClassPermissions(final JexlPermissions delegate, final
Collection<String> allow) {
+ super(Objects.requireNonNull(delegate));
+ allowedClasses = new HashSet<>(Objects.requireNonNull(allow));
+ }
+
+ @Override
+ public boolean allow(final Constructor<?> constructor) {
+ return validate(constructor) &&
+
(allowedClasses.contains(constructor.getDeclaringClass().getCanonicalName()) ||
super.allow(constructor));
+ }
+
+ @Override
+ public boolean allow(final Class<?> clazz) {
+ return validate(clazz) &&
allowedClasses.contains(clazz.getCanonicalName()) || super.allow(clazz);
+ }
+
+ @Override
+ public boolean allow(final Class<?> clazz, final Field field) {
+ if (!validate(field)) {
+ return false;
}
-
- /**
- * Required for compose().
- *
- * @param delegate the base to delegate to
- * @param allow the list of class canonical names
- */
- public ClassPermissions(final JexlPermissions delegate, final
Collection<String> allow) {
- super(Objects.requireNonNull(delegate));
- allowedClasses = new HashSet<>(Objects.requireNonNull(allow));
+ if (!clazz.isAssignableFrom(field.getDeclaringClass())) {
+ return false;
}
Review Comment:
`ClassPermissions.allow(Class<?>, Field)` checks assignability in the wrong
direction: `clazz.isAssignableFrom(field.getDeclaringClass())` will be false
for inherited fields when `clazz` is a subclass of the declaring class. This
can incorrectly deny access to public inherited fields. Swap the check to
ensure the field's declaring class is assignable from `clazz` (and consider
validating `clazz` here too).
##########
src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java:
##########
@@ -69,71 +89,112 @@ 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>
*/
- final class ClassPermissions extends JexlPermissions.Delegate {
-
- /** The set of explicitly allowed classes, overriding the delegate
permissions. */
- private final Set<String> allowedClasses;
-
- /**
- * Creates permissions based on the RESTRICTED set but allowing an
explicit set.
- *
- * @param allow the set of allowed classes
- */
- public ClassPermissions(final Class<?>... allow) {
- this(JexlPermissions.RESTRICTED,
- Arrays.stream(Objects.requireNonNull(allow))
- .map(Class::getCanonicalName)
- .collect(Collectors.toList()));
+ final class ClassPermissions extends JexlPermissions.Delegate {
+ /**
+ * The set of explicitly allowed classes, overriding the delegate
permissions.
+ */
+ private final Set<String> allowedClasses;
+
+ /**
+ * Creates permissions based on the RESTRICTED set but allowing an
explicit set.
+ *
+ * @param allow the set of allowed classes
+ */
+ public ClassPermissions(final Class<?>... allow) {
+ this(JexlPermissions.RESTRICTED, allow);
+ }
+
+ public ClassPermissions(final JexlPermissions permissions, final
Class<?>... allow) {
+ this(permissions,
Arrays.stream(Objects.requireNonNull(allow)).map(Class::getCanonicalName).collect(Collectors.toList()));
+ }
+
+ /**
+ * Required for compose().
+ *
+ * @param delegate the base to delegate to
+ * @param allow the list of class canonical names
+ */
+ public ClassPermissions(final JexlPermissions delegate, final
Collection<String> allow) {
+ super(Objects.requireNonNull(delegate));
+ allowedClasses = new HashSet<>(Objects.requireNonNull(allow));
+ }
+
+ @Override
+ public boolean allow(final Constructor<?> constructor) {
+ return validate(constructor) &&
+
(allowedClasses.contains(constructor.getDeclaringClass().getCanonicalName()) ||
super.allow(constructor));
+ }
+
+ @Override
+ public boolean allow(final Class<?> clazz) {
+ return validate(clazz) &&
allowedClasses.contains(clazz.getCanonicalName()) || super.allow(clazz);
Review Comment:
`ClassPermissions.allow(Class<?>)` is missing parentheses: `validate(clazz)
&& allowedClasses.contains(...) || super.allow(clazz)`. As written,
`super.allow(clazz)` is evaluated even when `clazz` is null/invalid, and the
`validate` guard does not apply to the delegated check. Wrap the `||`
expression so validation applies to the whole decision.
```suggestion
return validate(clazz)
&& (allowedClasses.contains(clazz.getCanonicalName()) ||
super.allow(clazz));
```
##########
src/main/java/org/apache/commons/jexl3/scripting/JexlScriptEngine.java:
##########
@@ -296,9 +298,11 @@ private static JexlEngine getEngine() {
.safe(false)
.logger(JexlScriptEngine.LOG)
.cache(JexlScriptEngine.CACHE_SIZE);
- if (PERMISSIONS != null) {
- builder.permissions(PERMISSIONS);
- }
+ // ensure the script object class is always allowed for
all permissions set
+ JexlPermissions permissions = new
JexlPermissions.ClassPermissions(
+ PERMISSIONS == null ? JexlPermissions.RESTRICTED :
PERMISSIONS,
+ JexlScriptObject.class);
+ builder.permissions(permissions);
Review Comment:
`JexlScriptEngine.setPermissions(null)` is documented to fall back to
`JexlBuilder`'s default permissions, but `getEngine()` now forces `RESTRICTED`
whenever `PERMISSIONS` is null. This can silently ignore a caller/test that has
set a custom default via `JexlBuilder.setDefaultPermissions(...)`. Consider
basing the wrapper on the builder's current permissions (or only calling
`builder.permissions(...)` when `PERMISSIONS` is non-null).
```suggestion
// preserve builder default permissions when PERMISSIONS
is null;
// otherwise ensure the script object class is always
allowed
if (PERMISSIONS != null) {
final JexlPermissions permissions = new
JexlPermissions.ClassPermissions(
PERMISSIONS,
JexlScriptObject.class);
builder.permissions(permissions);
}
```
##########
src/main/java/org/apache/commons/jexl3/scripting/JexlScriptEngine.java:
##########
@@ -442,15 +446,15 @@ public Bindings createBindings() {
public Object eval(final Reader reader, final ScriptContext context)
throws ScriptException {
// This is mandated by JSR-223 (see SCR.5.5.2 Methods)
Objects.requireNonNull(reader, "reader");
- Objects.requireNonNull(context, "context");
+ Objects.requireNonNull(context, CONTEXT_KEY);
return eval(readerToString(reader), context);
}
@Override
public Object eval(final String script, final ScriptContext context)
throws ScriptException {
// This is mandated by JSR-223 (see SCR.5.5.2 Methods)
- Objects.requireNonNull(script, "context");
- Objects.requireNonNull(context, "context");
+ Objects.requireNonNull(script, CONTEXT_KEY);
Review Comment:
In `eval(String, ScriptContext)`, the null-check uses
`Objects.requireNonNull(script, CONTEXT_KEY)`, which will report "context" when
`script` is null. This makes debugging harder and is inconsistent with other
null checks in the class; the message should refer to the `script` parameter.
```suggestion
Objects.requireNonNull(script, "script");
```
##########
src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsTest.java:
##########
@@ -506,10 +508,13 @@ public Pair cons(Object first, Object second) {
}
@Test
- void testPair0() {
+ void testPair0() throws Exception {
final Map<String, Object> funcs = new HashMap<>();
funcs.put("lisp", new Scheme());
- final JexlPermissions permissions =
JexlPermissions.RESTRICTED.compose("org.example.*");
+ final JexlPermissions permissions = JexlPermissions.RESTRICTED.compose(
+ "org.example.*",
+ "org.apache.commons.jexl3.internal.introspection {
+PermissionsTest$Scheme {}");
Review Comment:
The permissions compose string appears to be missing the closing `}` for the
package block: `"org.apache.commons.jexl3.internal.introspection {
+PermissionsTest$Scheme {}"`. The current parser may tolerate this because the
string ends, but it’s easy to misread and fragile if anything is appended.
Consider adding the closing brace to match the documented syntax.
```suggestion
"org.apache.commons.jexl3.internal.introspection {
+PermissionsTest$Scheme {} }");
```
##########
src/test/java/org/apache/commons/jexl3/JexlTestCase.java:
##########
@@ -17,220 +17,151 @@
package org.apache.commons.jexl3;
-import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.fail;
-
-import java.lang.reflect.Constructor;
-import java.lang.reflect.Method;
import java.util.Arrays;
-
import org.apache.commons.jexl3.internal.Debugger;
import org.apache.commons.jexl3.internal.OptionsContext;
import org.apache.commons.jexl3.internal.Util;
import org.apache.commons.jexl3.internal.introspection.Uberspect;
import org.apache.commons.jexl3.introspection.JexlPermissions;
import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+
/**
* Implements runTest methods to dynamically instantiate and invoke a test,
* wrapping the call with setUp(), tearDown() calls.
* Eases the implementation of main methods to debug.
*/
public class JexlTestCase {
- public static class PragmaticContext extends OptionsContext implements
JexlContext.PragmaProcessor, JexlContext.OptionsHandle {
- private final JexlOptions options;
-
- public PragmaticContext() {
- this(new JexlOptions());
- }
-
- public PragmaticContext(final JexlOptions o) {
- this.options = o;
- }
-
- @Override
- public JexlOptions getEngineOptions() {
- return options;
- }
-
- @Override
- public void processPragma(final JexlOptions opts, final String key,
final Object value) {
- if ("script.mode".equals(key) && "pro50".equals(value)) {
- opts.set(MODE_PRO50);
- }
- }
-
- @Override
- public void processPragma(final String key, final Object value) {
- processPragma(null, key, value);
- }
- }
- // The default options: all tests where engine lexicality is
- // important can be identified by the builder calling lexical(...).
- static {
- JexlOptions.setDefaultFlags("-safe", "+lexical");
- }
-
- /** No parameters signature for test run. */
- private static final Class<?>[] NO_PARMS = {};
-
- /** String parameter signature for test run. */
- private static final Class<?>[] STRING_PARM = {String.class};
-
- // define mode pro50
- static final JexlOptions MODE_PRO50 = new JexlOptions();
-
- static {
- MODE_PRO50.setFlags("+strict +cancellable +lexical +lexicalShade
-safe".split(" "));
- }
-
- /**
- * A very secure singleton.
- */
- public static final JexlPermissions SECURE = JexlPermissions.RESTRICTED;
-
- static JexlEngine createEngine() {
- return new JexlBuilder().create();
- }
-
- public static JexlEngine createEngine(final boolean lenient) {
- return createEngine(lenient, SECURE);
- }
- public static JexlEngine createEngine(final boolean lenient, final
JexlPermissions permissions) {
- return new JexlBuilder()
- .uberspect(new Uberspect(null, null, permissions))
- .arithmetic(new JexlArithmetic(!lenient)).cache(128).create();
- }
-
- static JexlEngine createEngine(final JexlFeatures features) {
- return new JexlBuilder().features(features).create();
- }
-
- /**
- * Will force testing the debugger for each derived test class by
- * recreating each expression from the JexlNode in the JexlEngine cache &
- * testing them for equality with the origin.
- * @throws Exception
- */
- public static void debuggerCheck(final JexlEngine ijexl) throws Exception {
- Util.debuggerCheck(ijexl);
- }
-
- /**
- * Compare strings ignoring white space differences.
- * <p>This replaces any sequence of whitespaces (ie \\s) by one space (ie
ASCII 32) in both
- * arguments then compares them.</p>
- * @param lhs left hand side
- * @param rhs right hand side
- * @return true if strings are equal besides whitespace
- */
- public static boolean equalsIgnoreWhiteSpace(final String lhs, final
String rhs) {
- final String lhsw = lhs.trim().replaceAll("\\s+", "");
- final String rhsw = rhs.trim().replaceAll("\\s+", "");
- return lhsw.equals(rhsw);
- }
-
- /**
- * Runs a test.
- * @param args where args[0] is the test class name and args[1] the test
class method
- * @throws Exception
- */
- public static void main(final String[] args) throws Exception {
- runTest(args[0], args[1]);
- }
-
- /**
- * Instantiate and runs a test method; useful for debugging purpose.
- * For instance:
- * <code>
- * public static void main(String[] args) throws Exception {
- * runTest("BitwiseOperatorTest","testAndVariableNumberCoercion");
- * }
- * </code>
- * @param tname the test class name
- * @param mname the test class method
- * @throws Exception
- */
- public static void runTest(final String tname, final String mname) throws
Exception {
- final String testClassName = "org.apache.commons.jexl3." + tname;
- final Class<JexlTestCase> clazz = null;
- JexlTestCase test = null;
- // find the class
- assertThrows(ClassNotFoundException.class, () ->
Class.forName(testClassName), () -> "no such class: " + testClassName);
- // find ctor & instantiate
- Constructor<JexlTestCase> ctor = null;
- try {
- ctor = clazz.getConstructor(STRING_PARM);
- test = ctor.newInstance("debug");
- } catch (final NoSuchMethodException xctor) {
- // instantiate default class ctor
- try {
- test = clazz.getConstructor().newInstance();
- } catch (final Exception xany) {
- fail("cant instantiate test: " + xany);
- return;
- }
- } catch (final Exception xany) {
- fail("cant instantiate test: " + xany);
- return;
- }
- // Run the test
- test.runTest(mname);
- }
-
- public static String toString(final JexlScript script) {
- final Debugger d = new Debugger().lineFeed("").indentation(0);
- d.debug(script);
- return d.toString();
- }
-
- /** A default JEXL engine instance. */
- protected final JexlEngine JEXL;
-
- public JexlTestCase(final String name) {
- this(name, new
JexlBuilder().imports(Arrays.asList("java.lang","java.math")).permissions(null).cache(128).create());
- }
-
- protected JexlTestCase(final String name, final JexlEngine jexl) {
- //super(name);
- JEXL = jexl;
- }
-
- /**
- * Dynamically runs a test method.
- * @param name the test method to run
- * @throws Exception if anything goes wrong
- */
- public void runTest(final String name) throws Exception {
- if ("runTest".equals(name)) {
- return;
- }
- Method method = null;
- try {
- method = this.getClass().getDeclaredMethod(name, NO_PARMS);
- }
- catch (final Exception xany) {
- fail("no such test: " + name);
- return;
- }
- try {
- setUp();
- method.invoke(this);
- } finally {
- tearDown();
- }
- }
-
- public void setUp() throws Exception {
- // nothing to do
- }
-
- public String simpleWhitespace(final String arg) {
- return arg.trim().replaceAll("\\s+", " ");
- }
-
- @AfterEach
- public void tearDown() throws Exception {
- debuggerCheck(JEXL);
- }
+ /**
+ * The restricted permissions that are used by default in JEXL tests.
+ * <p>Need to add jexl3 package but removes most internals</p>
+ */
+ public static final JexlPermissions TEST_PERMS =
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{} }",
+ "java.net -{ +URI { -toURL() } }");
Review Comment:
In `TEST_PERMS`, the rule `"java.net -{ +URI { -toURL() } }"` is misleading:
the parser only treats `+`/`-` prefixes on *class* restrictions, not on member
names. `-toURL()` will be parsed the same as `toURL()`, and under a `+URI { ...
}` (positive/whitelist) class it will actually *allow* `toURL`. If the intent
is to block `URI.toURL()`, the rule needs to be expressed using supported
syntax (or restructured to avoid a whitelist for URI).
```suggestion
"java.net -{ URI { toURL() } }");
```
##########
src/test/java/org/apache/commons/jexl3/internal/introspection/NoJexlTest.java:
##########
@@ -85,6 +86,7 @@ public interface InterNoJexl5 {
@Test
void testNoJexlPermissions() throws Exception {
+ final JexlPermissions r = Permissions.RESTRICTED;
Review Comment:
`final JexlPermissions r = Permissions.RESTRICTED;` is unused. With PMD
best-practices enabled for test sources, this will likely fail the build
(UnusedLocalVariable). Remove it or use it in an assertion.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
