This is an automated email from the ASF dual-hosted git repository. henrib pushed a commit to branch JEXL-470 in repository https://gitbox.apache.org/repos/asf/commons-jexl.git
commit aeeb669fcf44698ece7e04e1fac9c095195d108b Author: Henrib <[email protected]> AuthorDate: Thu Aug 20 12:07:54 2026 +0200 [JEXL-470] Parser/string correctness fixes Tier-2 B2 wave — small, well-scoped parser and interpreter correctness fixes: - f018 StringParser.readUnicodeChar: reject 'g'/'h' as hex digits (was `c <= 'h'` / `c <= 'H'`); only 0-9/a-f/A-F are valid, so `\ug000` now passes through literally instead of decoding a bogus char. - f020 Parser.jjt ArrayAccess: key the safe-navigation bit off the bracket (child) index rather than the count of `?[` tokens, and saturate past 64 brackets, so `a[b]?[c]` marks the correct child. Parser regenerated. - f019 ASTRegexLiteral/StringParser: pass regex bodies through verbatim (only translating `\/` -> `/`) so regex escapes like \b, \d, \w survive; wrap Pattern.compile in try/catch -> JexlException.Parsing; add escapeRegex as the round-trip inverse used by the Debugger. - f028 Interpreter empty()/size(): always rethrow JexlException.Cancel and, in a strict engine, rethrow other errors instead of mapping them to true/0 (which silently swallowed failures, including cancellation). - f029 Interpreter switch statement: let `continue` propagate to the enclosing loop (as in Java) instead of being swallowed and falling through into following cases. Also fixes a checkstyle import-order violation in PermissionsRestrictedSweepTest introduced by the earlier hardening commit. Co-Authored-By: Claude Opus 4.8 <[email protected]> --- .../apache/commons/jexl3/internal/Debugger.java | 3 +- .../apache/commons/jexl3/internal/Interpreter.java | 40 ++++++++++++-- .../commons/jexl3/parser/ASTRegexLiteral.java | 10 +++- .../org/apache/commons/jexl3/parser/Parser.jjt | 4 +- .../apache/commons/jexl3/parser/StringParser.java | 63 +++++++++++++++++++--- .../commons/jexl3/ArithmeticOperatorTest.java | 28 ++++++++++ .../org/apache/commons/jexl3/ArithmeticTest.java | 38 +++++++++++++ .../org/apache/commons/jexl3/Issues400Test.java | 37 +++++++++++++ .../java/org/apache/commons/jexl3/SwitchTest.java | 34 ++++++++++++ .../apache/commons/jexl3/parser/ParserTest.java | 18 +++++++ 10 files changed, 262 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/apache/commons/jexl3/internal/Debugger.java b/src/main/java/org/apache/commons/jexl3/internal/Debugger.java index fff4f7b7..f8940a2e 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/Debugger.java +++ b/src/main/java/org/apache/commons/jexl3/internal/Debugger.java @@ -1294,7 +1294,8 @@ public class Debugger extends ParserVisitor implements JexlInfo.Detail { @Override protected Object visit(final ASTRegexLiteral node, final Object data) { - final String img = StringParser.escapeString(node.toString(), '/'); + // emit the regex verbatim (only '/' is escaped), mirroring the verbatim buildRegex (JEXL-security f019) + final String img = StringParser.escapeRegex(node.toString()); return check(node, "~" + img, data); } diff --git a/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java b/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java index 569b2fbf..1b57b839 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java +++ b/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java @@ -1313,11 +1313,42 @@ public class Interpreter extends InterpreterBase { return result; } + /** + * Evaluates the argument of {@code empty()}/{@code size()} while preserving cancellation and + * strict-mode error propagation (JEXL-security f028). + * <p> + * {@link JexlArithmetic#evaluate(Log, Supplier)} logs and swallows <em>every</em> {@link JexlException} + * (including {@link JexlException.Cancel}), masking the failure as the empty/0 result even in a strict + * engine. Here cancellation is always rethrown and, in a strict engine, so is any other error; only a + * lenient engine maps a failed argument evaluation to the {@link JexlEngine#TRY_FAILED} sentinel. + * </p> + * + * @param arg the argument supplier + * @return the evaluated value, or {@link JexlEngine#TRY_FAILED} on a swallowed lenient failure + */ + private Object evaluateArgument(final Supplier<Object> arg) { + try { + return arg.get(); + } catch (final JexlException.Cancel xcancel) { + // cancellation (and timeouts) must never be masked as an empty/0 result + throw xcancel; + } catch (final JexlException xjexl) { + if (isStrictEngine()) { + // a strict engine surfaces the underlying error instead of masking it + throw xjexl; + } + if (logger != null && logger.isWarnEnabled()) { + logger.warn(xjexl.getMessage(), xjexl.clean()); + } + return JexlEngine.TRY_FAILED; + } + } + @Override protected Object visit(final ASTEmptyFunction node, final Object data) { final JexlNode arg = node.jjtGetChild(0); final Supplier<Object> eval = () -> arg.jjtAccept(this, data); - Object value = arithmetic.evaluate(logger, eval); + Object value = evaluateArgument(eval); return value == JexlEngine.TRY_FAILED ? true : operators.empty(node, value); } @@ -2009,7 +2040,7 @@ public class Interpreter extends InterpreterBase { protected Object visit(final ASTSizeFunction node, final Object data) { final JexlNode arg = node.jjtGetChild(0); final Supplier<Object> eval = () -> arg.jjtAccept(this, data); - Object value = arithmetic.evaluate(logger, eval); + Object value = evaluateArgument(eval); return value == JexlEngine.TRY_FAILED ? 0 : operators.size(node, value); } @@ -2069,9 +2100,10 @@ public class Interpreter extends InterpreterBase { value = node.jjtGetChild(i).jjtAccept(this, data); } catch (final JexlException.Break xbreak) { break; // break out of the switch - } catch (final JexlException.Continue xcontinue) { - // continue to next case } + // NOTE: JexlException.Continue is intentionally NOT caught here. Like Java, 'continue' + // targets the enclosing loop, not the switch, so it must propagate out of the switch + // rather than fall through into the following cases (JEXL-security f029). } return value; } diff --git a/src/main/java/org/apache/commons/jexl3/parser/ASTRegexLiteral.java b/src/main/java/org/apache/commons/jexl3/parser/ASTRegexLiteral.java index e6dd45a0..c15c5327 100644 --- a/src/main/java/org/apache/commons/jexl3/parser/ASTRegexLiteral.java +++ b/src/main/java/org/apache/commons/jexl3/parser/ASTRegexLiteral.java @@ -18,6 +18,9 @@ package org.apache.commons.jexl3.parser; import java.util.Objects; import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; + +import org.apache.commons.jexl3.JexlException; public final class ASTRegexLiteral extends JexlNode implements JexlNode.Constant<Pattern> { @@ -54,7 +57,12 @@ public final class ASTRegexLiteral extends JexlNode implements JexlNode.Constant } void setLiteral(final String literal) { - this.literal = Pattern.compile(literal); + try { + // report a malformed pattern as a parse error rather than leaking PatternSyntaxException (JEXL-security f019) + this.literal = Pattern.compile(literal); + } catch (final PatternSyntaxException xpattern) { + throw new JexlException.Parsing(jexlInfo(), "invalid regular expression: " + literal).clean(); + } } @Override diff --git a/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt b/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt index 2a8a0cfa..198d3666 100644 --- a/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt +++ b/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt @@ -1263,7 +1263,9 @@ void ArrayAccess() : { int s = 0; } { - (LOOKAHEAD(2) (<LBRACKET>|<QLBRACKET> { safe |= (1L << s++); }) Expression() <RBRACKET>)+ { jjtThis.setSafe(safe); } + // advance the bracket index 's' for *every* bracket so the safe-navigation bit is keyed to the + // child (bracket) position, not to the count of '?[' seen (JEXL-security f020); saturate past 64. + (LOOKAHEAD(2) (<LBRACKET> { s++; } | <QLBRACKET> { if (s < Long.SIZE) { safe |= (1L << s); } s++; }) Expression() <RBRACKET>)+ { jjtThis.setSafe(safe); } } void MemberAccess() #void : {} diff --git a/src/main/java/org/apache/commons/jexl3/parser/StringParser.java b/src/main/java/org/apache/commons/jexl3/parser/StringParser.java index fb97b105..98ea21ec 100644 --- a/src/main/java/org/apache/commons/jexl3/parser/StringParser.java +++ b/src/main/java/org/apache/commons/jexl3/parser/StringParser.java @@ -53,13 +53,35 @@ public class StringParser { private static final char FIRST_ASCII = 32; /** - * Builds a regex pattern string, handles escaping '/' through '\/' syntax. + * Builds a regex pattern string from a {@code ~/.../} literal image. + * <p> + * The regex body is passed through <em>verbatim</em> to preserve regex escape sequences + * ({@code \b}, {@code \d}, {@code \w}, {@code \s}, ...); the only translation performed is the + * documented {@code \/} unescape, which lets a literal slash appear inside the delimiters + * (JEXL-security f019). Applying string-literal escaping here would corrupt regex escapes + * (e.g. turning {@code \b} into a backspace character). + * </p> * - * @param str The string to build from - * @return The built string + * @param str The raw {@code ~/.../} token image to build from + * @return The regex source string (delimiters removed, {@code \/} collapsed to {@code /}) */ public static String buildRegex(final CharSequence str) { - return buildString(str.subSequence(1, str.length()), true); + final int last = str.length() - 1; // the closing '/' + final StringBuilder strb = new StringBuilder(str.length()); + // image is '~' '/' body '/'; body starts after the leading '~/' + int i = 2; + while (i < last) { + final char c = str.charAt(i); + if (c == '\\' && i + 1 < last && str.charAt(i + 1) == '/') { + // the only recognized escape: '\/' yields a literal '/' + strb.append('/'); + i += 2; + } else { + strb.append(c); + i += 1; + } + } + return strb.toString(); } /** @@ -138,6 +160,35 @@ public class StringParser { return Objects.toString(strb, str); } + /** + * Escapes a regex source back into a {@code /.../} literal body. + * <p> + * This is the inverse of {@link #buildRegex(CharSequence)}: the body is emitted verbatim, escaping + * only embedded slashes as {@code \/} (backslashes are <em>not</em> doubled, so regex escapes such as + * {@code \b} or {@code \d} round-trip unchanged — JEXL-security f019). + * </p> + * + * @param str The regex source (without delimiters) + * @return The delimited {@code /.../} representation, or null if the input is null + */ + public static String escapeRegex(final CharSequence str) { + if (str == null) { + return null; + } + final int length = str.length(); + final StringBuilder strb = new StringBuilder(length + 2); + strb.append('/'); + for (int i = 0; i < length; ++i) { + final char c = str.charAt(i); + if (c == '/') { + strb.append('\\'); + } + strb.append(c); + } + strb.append('/'); + return strb.toString(); + } + /** * Escapes a String representation, expand non-ASCII characters as Unicode escape sequence. * @@ -308,9 +359,9 @@ public class StringParser { final char c = str.charAt(begin + offset); if (c >= '0' && c <= '9') { value = c - '0'; - } else if (c >= 'a' && c <= 'h') { + } else if (c >= 'a' && c <= 'f') { value = c - 'a' + BASE10; - } else if (c >= 'A' && c <= 'H') { + } else if (c >= 'A' && c <= 'F') { value = c - 'A' + BASE10; } else { return 0; diff --git a/src/test/java/org/apache/commons/jexl3/ArithmeticOperatorTest.java b/src/test/java/org/apache/commons/jexl3/ArithmeticOperatorTest.java index 003c5885..0d8ac0a4 100644 --- a/src/test/java/org/apache/commons/jexl3/ArithmeticOperatorTest.java +++ b/src/test/java/org/apache/commons/jexl3/ArithmeticOperatorTest.java @@ -587,6 +587,34 @@ class ArithmeticOperatorTest extends JexlTestCase { asserter.setVariable("str", "4/6"); asserter.assertExpression("str =~ ~/\\d\\/\\d/", Boolean.TRUE); } + + /** + * Regex escapes such as {@code \b} (word boundary) must be passed through verbatim to + * {@link Pattern}, not interpreted as string-literal escapes that would corrupt them into a + * backspace character (JEXL-security f019). + */ + @Test + void testRegexEscapesPassThrough() throws Exception { + asserter.setVariable("str", "a cat b"); + asserter.assertExpression("str =~ ~/.*\\bcat\\b.*/", Boolean.TRUE); + asserter.setVariable("str", "category"); + asserter.assertExpression("str =~ ~/.*\\bcat\\b.*/", Boolean.FALSE); + // \d (digit) and \/ (literal slash) still behave as before + asserter.setVariable("str", "4/6"); + asserter.assertExpression("str =~ ~/\\d\\/\\d/", Boolean.TRUE); + } + + /** + * A malformed regex literal must surface as a parse error rather than leaking a raw + * {@link java.util.regex.PatternSyntaxException} (JEXL-security f019). + */ + @Test + void testRegexInvalidPattern() { + final JexlEngine jexl = new JexlBuilder().create(); + final JexlException.Parsing xparse = assertThrows(JexlException.Parsing.class, + () -> jexl.createScript("x =~ ~/(unbalanced/")); + assertNotNull(xparse.getMessage()); + } void testSelfAssignOperators(final String text, final int x, final int y0, final int x0) { //String text = "y.add(x++)"; final JexlEngine jexl = new JexlBuilder().safe(true).create(); diff --git a/src/test/java/org/apache/commons/jexl3/ArithmeticTest.java b/src/test/java/org/apache/commons/jexl3/ArithmeticTest.java index 2893f0b3..257dcb4c 100644 --- a/src/test/java/org/apache/commons/jexl3/ArithmeticTest.java +++ b/src/test/java/org/apache/commons/jexl3/ArithmeticTest.java @@ -1449,6 +1449,44 @@ class ArithmeticTest extends JexlTestCase { } } + /** + * A strict engine must surface argument-evaluation errors from {@code empty()}/{@code size()} + * instead of masking them as {@code true}/{@code 0} (JEXL-security f028); a lenient engine keeps + * the historical empty/0 fallback. + */ + @Test + void testEmptySizeStrictPropagatesError() { + final JexlEngine strict = new JexlBuilder().strict(true).safe(false).create(); + assertThrows(JexlException.class, () -> strict.createScript("empty(x.y)", "x").execute(null, (Object) null)); + assertThrows(JexlException.class, () -> strict.createScript("size(x.y)", "x").execute(null, (Object) null)); + final JexlEngine lenient = new JexlBuilder().strict(false).safe(true).create(); + assertEquals(Boolean.TRUE, lenient.createScript("empty(x.y)", "x").execute(null, (Object) null)); + assertEquals(0, lenient.createScript("size(x.y)", "x").execute(null, (Object) null)); + } + + /** + * Cancellation during {@code empty()}/{@code size()} argument evaluation must never be masked + * as empty/0, even in a lenient engine (JEXL-security f028). + */ + @Test + void testEmptySizeDoNotSwallowCancel() { + final JexlEngine jexl = new JexlBuilder().strict(false).cancellable(true).create(); + final JexlScript empty = jexl.createScript("empty(x)", "x"); + final JexlScript size = jexl.createScript("size(x)", "x"); + try { + Thread.currentThread().interrupt(); + assertThrows(JexlException.Cancel.class, () -> empty.execute(null, "abc")); + } finally { + Thread.interrupted(); // clear the interrupted status so it does not leak into other tests + } + try { + Thread.currentThread().interrupt(); + assertThrows(JexlException.Cancel.class, () -> size.execute(null, "abc")); + } finally { + Thread.interrupted(); + } + } + @Test void testEmptyDouble() { Object x; diff --git a/src/test/java/org/apache/commons/jexl3/Issues400Test.java b/src/test/java/org/apache/commons/jexl3/Issues400Test.java index 50626de9..856e390b 100644 --- a/src/test/java/org/apache/commons/jexl3/Issues400Test.java +++ b/src/test/java/org/apache/commons/jexl3/Issues400Test.java @@ -233,6 +233,43 @@ public class Issues400Test { assertArrayEquals(new String[]{"C"}, (String[]) result); } + /** + * The safe-navigation bit of an array access must be keyed to the bracket (child) position, + * not to the count of '?[' seen (JEXL-security f020). A mixed chain like 'a[b]?[c]' must mark + * only the second child as safe; round-tripping through the Debugger proves the right child + * carries the '?'. + */ + @Test + void testSafeNavBitIndexing() { + // @formatter:off + final JexlEngine jexl = new JexlBuilder() + .cache(64) + .strict(true) + .safe(false) + .create(); + // @formatter:on + // Debugger reconstructs '?[' from isSafeChild(i); mixed chains must round-trip verbatim. + final String[] sources = { + "a[b]?[c]", + "a?[b][c]", + "a[b][c]?[d]", + "a?[b]?[c]", + "a[b][c]" + }; + for (final String src : sources) { + final JexlScript script = jexl.createScript(src, "a", "b", "c", "d"); + assertEquals(src, script.getParsedText(), src); + } + // and the leading-non-safe / trailing-safe split behaves at evaluation time: + // a is present, a[b] is null -> the trailing ?[c] shields the null, yielding null (not a throw). + final Map<String, Object> a = Collections.singletonMap("x", 42); + final JexlScript shielded = jexl.createScript("a[b]?[c]", "a", "b", "c"); + assertNull(shielded.execute(null, a, "missing", "c")); + // whereas the leading, non-safe [b] on a null base still throws. + final JexlScript unshielded = jexl.createScript("a[b]?[c]", "a", "b", "c"); + assertThrows(JexlException.class, () -> unshielded.execute(null, null, "b", "c")); + } + @Test void test406a() { // @formatter:off diff --git a/src/test/java/org/apache/commons/jexl3/SwitchTest.java b/src/test/java/org/apache/commons/jexl3/SwitchTest.java index e310c026..45f5a7f8 100644 --- a/src/test/java/org/apache/commons/jexl3/SwitchTest.java +++ b/src/test/java/org/apache/commons/jexl3/SwitchTest.java @@ -37,6 +37,40 @@ public class SwitchTest extends JexlTestCase { super("SwitchTest"); } + /** + * 'continue' inside a switch statement must target the enclosing loop (as in Java), not be + * swallowed and fall through into the following cases (JEXL-security f029). 'break' by contrast + * only breaks out of the switch, leaving the rest of the loop body to run. + */ + @Test + void testContinueInSwitchStatement() { + final JexlEngine jexl = new JexlBuilder().create(); + // continue skips the remainder of the loop iteration -> the append is not reached for i == 2 + final String continueSrc = + "var r = '';\n" + + "for (var i : [1, 2, 3]) {\n" + + " switch (i) {\n" + + " case 2 : continue;\n" + + " default : {}\n" + + " }\n" + + " r += i;\n" + + "}\n" + + "r"; + assertEquals("13", jexl.createScript(continueSrc).execute(null)); + // break only leaves the switch -> the append still runs for every i + final String breakSrc = + "var r = '';\n" + + "for (var i : [1, 2, 3]) {\n" + + " switch (i) {\n" + + " case 2 : break;\n" + + " default : {}\n" + + " }\n" + + " r += i;\n" + + "}\n" + + "r"; + assertEquals("123", jexl.createScript(breakSrc).execute(null)); + } + @Test void testSwitchExpression() { final JexlEngine jexl = new JexlBuilder().safe(false).strict(true).create(); diff --git a/src/test/java/org/apache/commons/jexl3/parser/ParserTest.java b/src/test/java/org/apache/commons/jexl3/parser/ParserTest.java index ef5b93c4..6e777a02 100644 --- a/src/test/java/org/apache/commons/jexl3/parser/ParserTest.java +++ b/src/test/java/org/apache/commons/jexl3/parser/ParserTest.java @@ -53,6 +53,24 @@ class ParserTest { } } + /** + * Test unicode escape sequences: only 4 hex digits (0-9, a-f, A-F) form a unicode char; + * an out-of-range digit (g/h/...) is not a valid escape and passes through literally. + */ + @Test + void testUnicodeEscape() { + // valid unicode escapes decode to the expected character + assertEquals("A", StringParser.buildString("'\\u0041'", true)); + assertEquals(String.valueOf((char) 0x00ff), StringParser.buildString("'\\u00ff'", true)); + assertEquals(String.valueOf((char) 0x00ff), StringParser.buildString("'\\u00FF'", true)); + assertEquals("Z9", StringParser.buildString("'\\u005a\\u0039'", true)); + // out-of-range hex digits ('g'/'h'/'G'/'H') are NOT consumed as a unicode escape: + // the escape is not recognized, so the '\\u' is re-emitted followed by the raw characters. + assertEquals("\\ug000", StringParser.buildString("'\\ug000'", true)); + assertEquals("\\u00fh", StringParser.buildString("'\\u00fh'", true)); + assertEquals("\\uH000", StringParser.buildString("'\\uH000'", true)); + } + @Test void testErrorAmbiguous() throws Exception { final Parser parser = new Parser(";");
