Copilot commented on code in PR #414:
URL: https://github.com/apache/commons-jexl/pull/414#discussion_r3893665187
##########
src/main/java/org/apache/commons/jexl3/internal/Interpreter.java:
##########
@@ -1377,12 +1378,37 @@ protected Object visit(final ASTEQSNode node, final
Object data) {
@Override
protected Object visit(final ASTERNode node, final Object data) {
final Object left = node.jjtGetChild(0).jjtAccept(this, data);
- final Object right = node.jjtGetChild(1).jjtAccept(this, data);
+ final JexlNode rightNode = node.jjtGetChild(1);
+ final Object right = resolvePattern(rightNode,
rightNode.jjtAccept(this, data));
// note the arguments inversion between 'in'/'matches' and 'contains'
// if x in y then y contains x
return operators.contains(node, JexlOperator.CONTAINS, right, left);
}
+ /**
+ * If the right operand of {@code =~} / {@code !~} is a string literal,
compile it to a Pattern once and
+ * cache the result in the node's value slot (same mechanism as negated
numeric literals).
+ * Dynamic string values (from variables) are returned unchanged.
+ * The regex string length is validated before compilation (matches
JexlArithmetic.REGEX_PATTERN_MAX_LENGTH).
+ */
+ private static Object resolvePattern(final JexlNode rightNode, final
Object right) {
+ if (right instanceof CharSequence && rightNode instanceof
JexlNode.Constant) {
+ final Object cached = rightNode.jjtGetValue();
+ if (cached instanceof Pattern) {
+ return cached;
+ }
+ final String regex = right.toString();
+ final int maxLen = 2048;
Review Comment:
The regex max-length is duplicated as a magic number here, while
`JexlArithmetic` defines `REGEX_PATTERN_MAX_LENGTH`. This creates a risk of
divergence (and the PR description explicitly claims they match). Consider
centralizing the limit by making the constant accessible from `internal` (e.g.,
`public static final` on a shared class, or a small utility method) and
referencing it here.
##########
src/main/java/org/apache/commons/jexl3/internal/Interpreter.java:
##########
@@ -1377,12 +1378,37 @@ protected Object visit(final ASTEQSNode node, final
Object data) {
@Override
protected Object visit(final ASTERNode node, final Object data) {
final Object left = node.jjtGetChild(0).jjtAccept(this, data);
- final Object right = node.jjtGetChild(1).jjtAccept(this, data);
+ final JexlNode rightNode = node.jjtGetChild(1);
+ final Object right = resolvePattern(rightNode,
rightNode.jjtAccept(this, data));
// note the arguments inversion between 'in'/'matches' and 'contains'
// if x in y then y contains x
return operators.contains(node, JexlOperator.CONTAINS, right, left);
}
+ /**
+ * If the right operand of {@code =~} / {@code !~} is a string literal,
compile it to a Pattern once and
+ * cache the result in the node's value slot (same mechanism as negated
numeric literals).
+ * Dynamic string values (from variables) are returned unchanged.
+ * The regex string length is validated before compilation (matches
JexlArithmetic.REGEX_PATTERN_MAX_LENGTH).
+ */
+ private static Object resolvePattern(final JexlNode rightNode, final
Object right) {
+ if (right instanceof CharSequence && rightNode instanceof
JexlNode.Constant) {
+ final Object cached = rightNode.jjtGetValue();
+ if (cached instanceof Pattern) {
+ return cached;
+ }
+ final String regex = right.toString();
+ final int maxLen = 2048;
+ if (regex.length() > maxLen) {
+ throw new ArithmeticException("regular expression too long: "
+ regex.length() + " > " + maxLen);
+ }
+ final Pattern compiled = Pattern.compile(regex);
+ rightNode.jjtSetValue(compiled);
+ return compiled;
+ }
+ return right;
+ }
Review Comment:
Caching the compiled Pattern by mutating the AST node (`jjtSetValue`)
introduces shared mutable state in the script tree. If the same parsed
script/AST is executed concurrently, this is a data race (even if benign most
of the time) and can violate expected thread-safety guarantees. A safer
approach is to cache per-script (immutable structure created at parse/compile
time) or per-interpreter execution (e.g., an Interpreter-local map keyed by
node identity). If you keep node-level caching, consider making it thread-safe
(e.g., safe publication/atomic install) and document the thread-safety
implications.
##########
src/main/java/org/apache/commons/jexl3/JexlArithmetic.java:
##########
@@ -1374,9 +1422,13 @@ public Object multiply(final Object left, final Object
right) {
final double r = toDouble(strictCast, right);
return l * r;
}
- // otherwise treat as BigInteger
+ // otherwise treat as BigInteger; pre-check bit-length sum to avoid
O(n²) on huge operands
final BigInteger l = toBigInteger(strictCast, left);
final BigInteger r = toBigInteger(strictCast, right);
+ final int precision = getMathContext().getPrecision();
+ if (precision > 0 && l.bitLength() + r.bitLength() > precision * 10 /
3 + 1) {
+ throw new ArithmeticException("BigInteger precision exceeded");
Review Comment:
This newly introduced exception message is much less actionable than
`checkBigIntegerPrecision(...)` (which includes bit-length and context
precision). Since this is a security/operational hardening boundary, having
consistent, contextual error messages helps debugging and support. Consider
including the computed limit and observed operand bit-lengths (or delegating to
the same formatter used elsewhere) while keeping sensitive data exposure in
mind.
##########
src/main/java/org/apache/commons/jexl3/parser/NumberParser.java:
##########
@@ -32,6 +34,30 @@ public final class NumberParser implements Serializable {
*/
private static final long serialVersionUID = 1L;
+ /**
+ * Hard upper bound on BigInteger literal digits when no engine precision
is configured.
+ * Acts as a parse-time DoS guard independent of any MathContext
(JEXL-security f014).
+ */
+ static final int MAX_BIGINTEGER_DIGITS = 256;
+
+ /**
+ * Returns the maximum digit count allowed for a BigInteger literal.
+ * When a JEXL engine with a bounded MathContext is active on the current
thread, the
+ * engine's precision (in decimal digits) is used as the limit; otherwise
MAX_BIGINTEGER_DIGITS applies.
+ * At runtime, JexlArithmetic.checkBigIntegerPrecision() enforces a
stricter bit-length limit
+ * based on the same precision (f014, f013).
+ */
+ private static int maxBigIntegerDigits() {
+ final JexlEngine engine = JexlEngine.getThreadEngine();
+ if (engine != null) {
+ final int precision =
engine.getArithmetic().getMathContext().getPrecision();
+ if (precision > 0) {
+ return precision;
+ }
+ }
+ return MAX_BIGINTEGER_DIGITS;
+ }
Review Comment:
The doc states the engine precision is interpreted as a limit \"in decimal
digits\", but the subsequent checks compare that limit to the literal length in
whatever base is being parsed (e.g., hex/octal digit counts). That makes the
enforced limit inconsistent with the documented unit (decimal digits) and can
reject valid literals earlier than intended (or accept more than intended)
depending on base. Consider converting the precision-based limit to an
equivalent max digit-count for the current base (using a log-based conversion)
or enforce a base-independent bound (e.g., max bits) consistently at parse time.
##########
src/test/java/org/apache/commons/jexl3/ArithmeticTest.java:
##########
@@ -2409,4 +2409,75 @@ void setOptions(final JexlOptions options) {
assertEquals("zero", jexl.createExpression("array.0").evaluate(jc));
assertEquals("one", jexl.createExpression("array.1").evaluate(jc));
}
+
+ // ----- security fixes f012 / f013 / f014 -----
+
+ /**
+ * f014: BigInteger literal with more digits than MAX_BIGINTEGER_DIGITS
must be rejected at parse time.
+ */
+ @Test
+ void testBigIntegerLiteralTooLong() {
+ // normal H-literal still works
+ assertNotNull(JEXL.createScript("42H"));
+ // a literal just over the cap (256 + 1 digits + 'H') must fail to
parse
+ final char[] digits = new char[256 + 1];
+ java.util.Arrays.fill(digits, '1');
+ final String huge = new String(digits) + "H";
+ assertThrows(JexlException.Parsing.class, () ->
JEXL.createScript(huge));
+ }
+
+ /**
+ * f013: BigInteger arithmetic results that exceed the MathContext
precision must throw.
+ */
+ @Test
+ void testBigIntegerArithmeticPrecisionCap() {
+ // precision=3 caps at ~11 bits (formula: 3 * 10 / 3 + 1 = 11), so
results > 2047 are rejected
+ final JexlArithmetic bounded = new JexlArithmetic(true, new
MathContext(3), JexlArithmetic.BIGD_SCALE);
+ final JexlEngine jexl = new JexlBuilder().arithmetic(bounded).create();
+ // small values are fine
+ assertEquals(new BigInteger("3"), jexl.createScript("a + b", "a", "b")
+ .execute(null, BigInteger.ONE, BigInteger.valueOf(2L)));
+ // result > 2047 is rejected: 1500 + 1000 = 2500, bitLength=12 > 11
+ assertThrows(JexlException.class, () ->
+ jexl.createScript("a + b", "a", "b")
+ .execute(null, BigInteger.valueOf(1500L),
BigInteger.valueOf(1000L)));
+ // multiply pre-check: 64 (7 bits) * 64 (7 bits), sum of operand bits
= 14 > 11
+ assertThrows(JexlException.class, () ->
+ jexl.createScript("a * b", "a", "b")
+ .execute(null, BigInteger.valueOf(64L),
BigInteger.valueOf(64L)));
+ }
+
+ /**
+ * f012: a regex pattern string longer than REGEX_PATTERN_MAX_LENGTH must
throw.
+ */
+ @Test
+ void testRegexPatternTooLong() {
+ final JexlEngine jexl = new JexlBuilder().strict(true).create();
+ final JexlScript script = jexl.createScript("x =~ y", "x", "y");
+ final char[] chars = new char[JexlArithmetic.REGEX_PATTERN_MAX_LENGTH
+ 1];
+ java.util.Arrays.fill(chars, 'a');
+ final String longPattern = new String(chars);
+ assertThrows(JexlException.class, () -> script.execute(null, "abc",
longPattern));
+ }
+
+ /**
+ * f012: regex matching must respond to thread interruption so a
catastrophic-backtracking
+ * pattern does not hang a cancellable engine indefinitely.
+ */
+ @Test
+ void testRegexMatchingInterruptible() {
+ final JexlEngine jexl = new JexlBuilder().cancellable(true).create();
+ // Classic catastrophic-backtracking: (a+)+b against a non-matching
string
+ final JexlScript script = jexl.createScript("x =~ y", "x", "y");
+ final String evilPattern = "(a+)+b";
+ final char[] chars = new char[20];
+ java.util.Arrays.fill(chars, 'a');
+ final String evilValue = new String(chars) + "c";
+ try {
+ Thread.currentThread().interrupt();
+ assertThrows(JexlException.Cancel.class, () ->
script.execute(null, evilValue, evilPattern));
+ } finally {
+ Thread.interrupted(); // clear so it does not leak into other tests
+ }
+ }
Review Comment:
This test interrupts the thread before execution starts, so it can pass even
if the regex engine never checks for interruption during matching (e.g., if
cancellation is detected earlier in the evaluation pipeline). To specifically
validate the new `InterruptibleCharSequence` behavior, add a test that starts
the regex match in a separate thread, waits until it is actively running, then
interrupts that thread and asserts it terminates promptly with
`JexlException.Cancel`.
--
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]
