From b9b31afcc905cd9d5e63739ff5920d849f7f20f0 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 17 Mar 2023 12:56:53 +0100 Subject: [PATCH] Improve diagnostics in SpEL for `matches` operator Supplying a large regular expression to the `matches` operator in a SpEL expression can result in errors that are not very helpful to the user. This commit improves the diagnostics in SpEL for the `matches` operator by throwing a SpelEvaluationException with a meaningful error message to better assist the user. Closes gh-30150 --- .../expression/spel/SpelMessage.java | 6 +++- .../expression/spel/ast/OperatorMatches.java | 33 ++++++++++++++----- .../expression/spel/EvaluationTests.java | 14 ++++++++ 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/SpelMessage.java b/spring-expression/src/main/java/org/springframework/expression/spel/SpelMessage.java index 4fc4eae7bec6..f1fd0f67f715 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/SpelMessage.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/SpelMessage.java @@ -264,7 +264,11 @@ public enum SpelMessage { /** @since 5.2.23 */ MAX_REPEATED_TEXT_SIZE_EXCEEDED(Kind.ERROR, 1076, - "Repeated text results in too many characters, exceeding the threshold of ''{0}''"); + "Repeated text results in too many characters, exceeding the threshold of ''{0}''"), + + /** @since 5.2.23 */ + MAX_REGEX_LENGTH_EXCEEDED(Kind.ERROR, 1077, + "Regular expression contains too many characters, exceeding the threshold of ''{0}''"); private final Kind kind; diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OperatorMatches.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OperatorMatches.java index 6b02dbc2dbf9..0863716c182d 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OperatorMatches.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OperatorMatches.java @@ -43,6 +43,12 @@ public class OperatorMatches extends Operator { private static final int PATTERN_ACCESS_THRESHOLD = 1000000; + /** + * Maximum number of characters permitted in a regular expression. + * @since 5.2.23 + */ + private static final int MAX_REGEX_LENGTH = 256; + private final ConcurrentMap patternCache; @@ -78,26 +84,28 @@ public OperatorMatches(ConcurrentMap patternCache, int startPos public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException { SpelNodeImpl leftOp = getLeftOperand(); SpelNodeImpl rightOp = getRightOperand(); - String left = leftOp.getValue(state, String.class); - Object right = rightOp.getValue(state); - if (left == null) { + String input = leftOp.getValue(state, String.class); + if (input == null) { throw new SpelEvaluationException(leftOp.getStartPosition(), SpelMessage.INVALID_FIRST_OPERAND_FOR_MATCHES_OPERATOR, (Object) null); } + + Object right = rightOp.getValue(state); if (!(right instanceof String)) { throw new SpelEvaluationException(rightOp.getStartPosition(), SpelMessage.INVALID_SECOND_OPERAND_FOR_MATCHES_OPERATOR, right); } + String regex = (String) right; try { - String rightString = (String) right; - Pattern pattern = this.patternCache.get(rightString); + Pattern pattern = this.patternCache.get(regex); if (pattern == null) { - pattern = Pattern.compile(rightString); - this.patternCache.putIfAbsent(rightString, pattern); + checkRegexLength(regex); + pattern = Pattern.compile(regex); + this.patternCache.putIfAbsent(regex, pattern); } - Matcher matcher = pattern.matcher(new MatcherInput(left, new AccessCount())); + Matcher matcher = pattern.matcher(new MatcherInput(input, new AccessCount())); return BooleanTypedValue.forValue(matcher.matches()); } catch (PatternSyntaxException ex) { @@ -110,6 +118,13 @@ public BooleanTypedValue getValueInternal(ExpressionState state) throws Evaluati } } + private void checkRegexLength(String regex) { + if (regex.length() > MAX_REGEX_LENGTH) { + throw new SpelEvaluationException(getStartPosition(), + SpelMessage.MAX_REGEX_LENGTH_EXCEEDED, MAX_REGEX_LENGTH); + } + } + private static class AccessCount { @@ -127,7 +142,7 @@ private static class MatcherInput implements CharSequence { private final CharSequence value; - private AccessCount access; + private final AccessCount access; public MatcherInput(CharSequence value, AccessCount access) { this.value = value; diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java index 773baf0457dc..6b5f02a05b69 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java @@ -199,6 +199,20 @@ void matchesWithPatternAccessThreshold() { evaluateAndCheckError(expression, SpelMessage.FLAWED_PATTERN); } + @Test + void matchesWithPatternLengthThreshold() { + String pattern = "(0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789" + + "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789" + + "01234567890123456789012345678901234567890123456789|abc)"; + assertThat(pattern).hasSize(256); + Expression expr = parser.parseExpression("'abc' matches '" + pattern + "'"); + assertThat(expr.getValue(context, Boolean.class)).isTrue(); + + pattern += "?"; + assertThat(pattern).hasSize(257); + evaluateAndCheckError("'abc' matches '" + pattern + "'", Boolean.class, SpelMessage.MAX_REGEX_LENGTH_EXCEEDED); + } + // mixing operators @Test public void testMixingOperators01() {