This is an automated email from the ASF dual-hosted git repository. jsinovassin pushed a commit to branch backport/bound-matchesregex-3.0.x in repository https://gitbox.apache.org/repos/asf/unomi.git
commit e4d513b9c4a4450b44d069325c81407f27c70f01 Author: jsinovassin <[email protected]> AuthorDate: Mon Sep 28 12:23:51 2026 +0200 Bound regular-expression evaluation in the matchesRegex operator (unomi-3.0.x) Backport of #872 to unomi-3.0.x, re-cut from the current branch head. The matchesRegex operator compiles and evaluates a pattern that an unauthenticated caller supplies on the public context endpoint, against a subject the same caller chooses. java.util.regex is a backtracking engine, so one request can hold a thread. The operator now runs under guards that cap the pattern length at 512 and the input length at 10000, cache compiled patterns in a bounded LRU, and abort once a character-access budget of one million is exceeded. Each limit is overridable by system property under org.apache.unomi.conditions.regex, and well-behaved patterns keep the same semantics. Differences from master: - AllITs registers ConditionRegexSafetyIT alone; the other suites the master commit lists do not exist on this line. - ConditionRegexSafetyIT sends no API key, because the context endpoint is public here. That is the surface an unbounded evaluation is reachable from. - One test case is added on each side, in the unit suite and in the integration suite, for the bounded-quantifier shape. Both suites otherwise probe with (a+)+$, which uses an unbounded quantifier. Java 17 optimizes that shape, so it costs nothing and the case passes whether or not the guard is present: reverting the operator to the unguarded call leaves it green. The added case uses ^(a|a){0,60}z$ against 40 characters, which is not optimized and whose cost doubles with each added character, and reverting the operator turns it red on its timeout. Tests: PropertyConditionEvaluatorTest runs 15 cases and passes. Reverting the operator to Pattern.compile(...).matcher(...).matches() turns 3 of them red, one of which is the case added here. --- .../test/java/org/apache/unomi/itests/AllITs.java | 1 + .../unomi/itests/ConditionRegexSafetyIT.java | 133 +++++++++++++++++++++ .../conditions/PropertyConditionEvaluator.java | 99 ++++++++++++++- .../conditions/PropertyConditionEvaluatorTest.java | 59 +++++++++ 4 files changed, 291 insertions(+), 1 deletion(-) diff --git a/itests/src/test/java/org/apache/unomi/itests/AllITs.java b/itests/src/test/java/org/apache/unomi/itests/AllITs.java index be8f596a8..f05d5631b 100644 --- a/itests/src/test/java/org/apache/unomi/itests/AllITs.java +++ b/itests/src/test/java/org/apache/unomi/itests/AllITs.java @@ -53,6 +53,7 @@ import org.junit.runners.Suite.SuiteClasses; ModifyConsentIT.class, PatchIT.class, ContextServletIT.class, + ConditionRegexSafetyIT.class, RuleServiceIT.class, PrivacyServiceIT.class, GroovyActionsServiceIT.class, diff --git a/itests/src/test/java/org/apache/unomi/itests/ConditionRegexSafetyIT.java b/itests/src/test/java/org/apache/unomi/itests/ConditionRegexSafetyIT.java new file mode 100644 index 000000000..cfca1787f --- /dev/null +++ b/itests/src/test/java/org/apache/unomi/itests/ConditionRegexSafetyIT.java @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.itests; + +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.entity.ContentType; +import org.apache.http.entity.StringEntity; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.ops4j.pax.exam.junit.PaxExam; +import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy; +import org.ops4j.pax.exam.spi.reactors.PerSuite; + +/** + * A personalization condition on the public tracking endpoints lets the caller supply a regular + * expression outright, without administrator credentials. If its evaluation were unbounded, one small + * request would occupy a request thread indefinitely. + * <p> + * These tests assert the property that matters operationally — the request <em>returns</em> — rather + * than any particular status code. The inputs used are ones that took exponential time before the fix + * (seconds at 26 characters, and far beyond that at the lengths used here), so a regression does not + * produce a wrong answer, it produces a request that never comes back. Each is therefore bounded by a + * timeout: if the boundary regresses, the test fails by timing out rather than by assertion. + */ +@RunWith(PaxExam.class) +@ExamReactorStrategy(PerSuite.class) +public class ConditionRegexSafetyIT extends BaseIT { + + private final static String CONTEXT_URL = "/cxs/context.json"; + + /** Generous next to the sub-millisecond a bounded evaluation needs, decisive against an unbounded one. */ + private final static int RESPONSE_TIMEOUT_MS = 30_000; + + /** + * A personalization filter carrying a caller-supplied pattern. {@code (a+)+$} against a long run of + * 'a' followed by a non-matching character is the textbook catastrophic case; the caller controls + * both the pattern and, through the identifier, much of the subject. + */ + @Test(timeout = RESPONSE_TIMEOUT_MS) + public void testCallerSuppliedRegexConditionIsEvaluatedInBoundedTime() throws Exception { + final StringBuilder subject = new StringBuilder(); + for (int i = 0; i < 60; i++) { + subject.append('a'); + } + + final String payload = "{\"sessionId\":\"regex-condition-session\",\"profileId\":\"" + subject + "\"," + + "\"requiredProfileProperties\":[\"*\"],\"events\":[]," + + "\"filters\":[{\"id\":\"regex-safety-filter\",\"filters\":[{\"condition\":{" + + "\"type\":\"profilePropertyCondition\",\"parameterValues\":{" + + "\"propertyName\":\"itemId\",\"comparisonOperator\":\"matchesRegex\"," + + "\"propertyValue\":\"(a+)+$\"}}}]}]}"; + + assertReturnsPromptly(payload); + } + + /** + * The bounded-quantifier shape, which is the one that actually costs on a Java 17 engine. + * {@code (a+)+$} above uses an unbounded quantifier, and Java 17 optimizes that shape away, so + * the case above answers promptly whether or not the guard is present. {@code ^(a|a){0,60}z$} + * is not optimized: its cost doubles with each added character of the subject, and an unguarded + * evaluation of 40 characters runs for hours. + */ + @Test(timeout = RESPONSE_TIMEOUT_MS) + public void testBoundedQuantifierRegexConditionIsEvaluatedInBoundedTime() throws Exception { + final StringBuilder subject = new StringBuilder(); + for (int i = 0; i < 40; i++) { + subject.append('a'); + } + + final String payload = "{\"sessionId\":\"regex-bounded-session\",\"profileId\":\"" + subject + "\"," + + "\"requiredProfileProperties\":[\"*\"],\"events\":[]," + + "\"filters\":[{\"id\":\"regex-bounded-filter\",\"filters\":[{\"condition\":{" + + "\"type\":\"profilePropertyCondition\",\"parameterValues\":{" + + "\"propertyName\":\"itemId\",\"comparisonOperator\":\"matchesRegex\"," + + "\"propertyValue\":\"^(a|a){0,60}z$\"}}}]}]}"; + + assertReturnsPromptly(payload); + } + + /** A pattern far longer than any legitimate one must be refused rather than compiled and run. */ + @Test(timeout = RESPONSE_TIMEOUT_MS) + public void testOversizedCallerSuppliedRegexIsRefused() throws Exception { + final StringBuilder hugePattern = new StringBuilder(); + for (int i = 0; i < 2000; i++) { + hugePattern.append("a?"); + } + + final String payload = "{\"sessionId\":\"regex-oversize-session\",\"requiredProfileProperties\":[\"*\"]," + + "\"events\":[]," + + "\"filters\":[{\"id\":\"regex-oversize-filter\",\"filters\":[{\"condition\":{" + + "\"type\":\"profilePropertyCondition\",\"parameterValues\":{" + + "\"propertyName\":\"itemId\",\"comparisonOperator\":\"matchesRegex\"," + + "\"propertyValue\":\"" + hugePattern + "\"}}}]}]}"; + + assertReturnsPromptly(payload); + } + + /** + * Sends the payload to the public context endpoint and requires an answer. Any HTTP status is + * acceptable — rejecting the input and processing it are both fine outcomes. What is not acceptable + * is the request not completing, which is exactly what an unbounded evaluation causes. + */ + private void assertReturnsPromptly(final String payload) throws Exception { + final HttpPost request = new HttpPost(getFullUrl(CONTEXT_URL)); + // No credential: on this line the context endpoint is public, which is the surface an + // unbounded evaluation is reachable from. + request.setEntity(new StringEntity(payload, ContentType.APPLICATION_JSON)); + + final long startedAt = System.currentTimeMillis(); + try (CloseableHttpResponse response = httpClient.execute(request)) { + final long elapsed = System.currentTimeMillis() - startedAt; + Assert.assertNotNull("The public endpoint must answer rather than hang", response); + Assert.assertTrue("The public endpoint answered, but took " + elapsed + + "ms, which suggests the evaluation is no longer bounded", elapsed < RESPONSE_TIMEOUT_MS); + } + } +} diff --git a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java index e6c391fdf..2e245283f 100644 --- a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java +++ b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java @@ -38,6 +38,7 @@ import org.slf4j.LoggerFactory; import java.text.SimpleDateFormat; import java.util.*; import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; import java.util.stream.Collectors; import static org.apache.unomi.persistence.spi.conditions.DateUtils.getDate; @@ -56,6 +57,29 @@ public class PropertyConditionEvaluator implements ConditionEvaluator { private static final HardcodedPropertyAccessorRegistry hardcodedPropertyAccessorRegistry = new HardcodedPropertyAccessorRegistry(); private ExpressionFilterFactory expressionFilterFactory; + // The matchesRegex operator compiles and evaluates a regular expression that can originate from an + // untrusted, unauthenticated caller (a condition tree on the public context/eventcollector endpoints, + // or a GraphQL filter). java.util.regex is a backtracking engine, so an unbounded evaluation is a + // single-request CPU-exhaustion primitive. These guards bound the pattern length, the matched value + // length, and the total work the matcher may perform. All limits are overridable via system property + // for the rare deployment with legitimate larger needs. + private static final int MAX_REGEX_PATTERN_LENGTH = + Integer.getInteger("org.apache.unomi.conditions.regex.maxPatternLength", 512); + private static final int MAX_REGEX_INPUT_LENGTH = + Integer.getInteger("org.apache.unomi.conditions.regex.maxInputLength", 10000); + private static final long MAX_REGEX_CHAR_ACCESSES = + Long.getLong("org.apache.unomi.conditions.regex.maxCharAccesses", 1000000L); + private static final int MAX_CACHED_REGEX_PATTERNS = 1000; + + private static final Map<String, Pattern> compiledRegexCache = + Collections.synchronizedMap(new LinkedHashMap<String, Pattern>(16, 0.75f, true) { + @Override + protected boolean removeEldestEntry(Map.Entry<String, Pattern> eldest) { + return size() > MAX_CACHED_REGEX_PATTERNS; + } + }); + + public void setUsePropertyConditionOptimizations(boolean usePropertyConditionOptimizations) { this.usePropertyConditionOptimizations = usePropertyConditionOptimizations; } @@ -256,7 +280,7 @@ public class PropertyConditionEvaluator implements ConditionEvaluator { } else if (op.equals("endsWith")) { return actualValue.toString().endsWith(expectedValue); } else if (op.equals("matchesRegex")) { - return expectedValue != null && Pattern.compile(expectedValue).matcher(actualValue.toString()).matches(); + return expectedValue != null && matchesRegexSafely(expectedValue, actualValue.toString()); } else if (op.equals("in") || op.equals("inContains") || op.equals("notIn") || op.equals("hasSomeOf") || op.equals("hasNoneOf") || op.equals("all")) { Collection<?> expectedValues = ConditionContextHelper.foldToASCII((Collection<?>) condition.getParameter("propertyValues")); Collection<?> expectedValuesInteger = (Collection<?>) condition.getParameter("propertyValuesInteger"); @@ -300,6 +324,79 @@ public class PropertyConditionEvaluator implements ConditionEvaluator { return false; } + /** + * Evaluates the {@code matchesRegex} operator with ReDoS protections: the pattern and the matched + * value are length-capped, compiled patterns are cached, and the match runs against a budget-limited + * {@link CharSequence} so that catastrophic backtracking is cut off instead of pinning a request + * thread. Any guard violation or invalid pattern evaluates to {@code false} (the condition does not + * match) and is logged. Regex semantics are otherwise unchanged, so legitimate patterns behave as before. + */ + protected static boolean matchesRegexSafely(String regex, String value) { + if (regex.length() > MAX_REGEX_PATTERN_LENGTH) { + LOGGER.warn("matchesRegex pattern longer than {} characters rejected", MAX_REGEX_PATTERN_LENGTH); + return false; + } + if (value.length() > MAX_REGEX_INPUT_LENGTH) { + LOGGER.warn("matchesRegex evaluation skipped: value longer than {} characters", MAX_REGEX_INPUT_LENGTH); + return false; + } + try { + Pattern pattern = compiledRegexCache.computeIfAbsent(regex, Pattern::compile); + return pattern.matcher(new BudgetedCharSequence(value, new long[]{MAX_REGEX_CHAR_ACCESSES})).matches(); + } catch (PatternSyntaxException e) { + LOGGER.warn("matchesRegex evaluation skipped: invalid pattern: {}", e.getMessage()); + return false; + } catch (RegexBudgetExceededException e) { + LOGGER.warn("matchesRegex evaluation aborted after {} character accesses (possible ReDoS pattern)", + MAX_REGEX_CHAR_ACCESSES); + return false; + } + } + + private static final class RegexBudgetExceededException extends RuntimeException { + RegexBudgetExceededException() { + super(null, null, false, false); + } + } + + /** + * A {@link CharSequence} wrapper that throws once a shared budget of {@code charAt} accesses is + * exhausted, bounding the total work a backtracking regex engine can perform on it. The budget is a + * single-element array so it is shared across the sub-sequences the engine may create. + */ + private static final class BudgetedCharSequence implements CharSequence { + private final CharSequence delegate; + private final long[] budget; + + BudgetedCharSequence(CharSequence delegate, long[] budget) { + this.delegate = delegate; + this.budget = budget; + } + + @Override + public char charAt(int index) { + if (--budget[0] < 0) { + throw new RegexBudgetExceededException(); + } + return delegate.charAt(index); + } + + @Override + public int length() { + return delegate.length(); + } + + @Override + public CharSequence subSequence(int start, int end) { + return new BudgetedCharSequence(delegate.subSequence(start, end), budget); + } + + @Override + public String toString() { + return delegate.toString(); + } + } + protected Object getPropertyValue(Item item, String expression) throws Exception { if (usePropertyConditionOptimizations) { Object result = getHardcodedPropertyValue(item, expression); diff --git a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluatorTest.java b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluatorTest.java index 07bc4d2e8..64baea80e 100644 --- a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluatorTest.java +++ b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluatorTest.java @@ -32,6 +32,8 @@ import java.util.regex.Pattern; import static junit.framework.TestCase.assertEquals; import static junit.framework.TestCase.assertNull; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; public class PropertyConditionEvaluatorTest { @@ -202,4 +204,61 @@ public class PropertyConditionEvaluatorTest { } } + @Test + public void testMatchesRegexStillMatchesNormalPatterns() { + assertTrue("Simple regex should still match", + propertyConditionEvaluator.isMatch("matchesRegex", "hello123", "[a-z]+\\d+", null, null, null, null, null)); + assertFalse("Simple regex should still not match", + propertyConditionEvaluator.isMatch("matchesRegex", "123hello", "[a-z]+\\d+", null, null, null, null, null)); + assertFalse("Invalid pattern should evaluate to false instead of throwing", + propertyConditionEvaluator.isMatch("matchesRegex", "anything", "([unclosed", null, null, null, null, null)); + } + + @Test(timeout = 5000) + public void testMatchesRegexCatastrophicBacktrackingIsBounded() { + // '(a+)+$' against a long run of 'a' followed by a non-matching character is a textbook + // exponential-backtracking (ReDoS) input; unbounded evaluation would run effectively forever. + StringBuilder subject = new StringBuilder(); + for (int i = 0; i < 100; i++) { + subject.append('a'); + } + subject.append('!'); + assertFalse("Catastrophic pattern must be aborted and evaluate to false", + propertyConditionEvaluator.isMatch("matchesRegex", subject.toString(), "(a+)+$", null, null, null, null, null)); + } + + /** + * The bounded-quantifier shape, which is the one that actually costs on a Java 17 engine. + * {@code (a+)+$} above uses an unbounded quantifier, and Java 17 optimizes that shape away, so + * that test passes whether or not the guard is present and cannot discriminate on its own. + * {@code ^(a|a){0,60}z$} is not optimized: its cost doubles with each added character of the + * subject, and an unguarded evaluation of 40 characters runs for hours. + */ + @Test(timeout = 5000) + public void testMatchesRegexBoundedQuantifierIsAborted() { + StringBuilder subject = new StringBuilder(); + for (int i = 0; i < 40; i++) { + subject.append('a'); + } + assertFalse("Bounded-quantifier pattern must be aborted and evaluate to false", + propertyConditionEvaluator.isMatch("matchesRegex", subject.toString(), "^(a|a){0,60}z$", null, null, null, null, null)); + } + + @Test + public void testMatchesRegexOversizedInputsAreRejected() { + StringBuilder longPattern = new StringBuilder("^"); + for (int i = 0; i < 600; i++) { + longPattern.append('a'); + } + assertFalse("Over-long pattern must be rejected", + propertyConditionEvaluator.isMatch("matchesRegex", "aaa", longPattern.toString(), null, null, null, null, null)); + + StringBuilder longValue = new StringBuilder(); + for (int i = 0; i < 10001; i++) { + longValue.append('a'); + } + assertFalse("Over-long value must be rejected", + propertyConditionEvaluator.isMatch("matchesRegex", longValue.toString(), "a*", null, null, null, null, null)); + } + }
