This is an automated email from the ASF dual-hosted git repository.
jsinovassin pushed a commit to branch unomi-3.0.x
in repository https://gitbox.apache.org/repos/asf/unomi.git
The following commit(s) were added to refs/heads/unomi-3.0.x by this push:
new c23ec1746 Bound regular-expression evaluation in the matchesRegex
operator (unomi-3.0.x) (#888)
c23ec1746 is described below
commit c23ec17463e76d4db73ad47609c88832c96573d9
Author: Jonathan SINOVASSIN-NAIK <[email protected]>
AuthorDate: Mon Sep 28 17:16:01 2026 +0200
Bound regular-expression evaluation in the matchesRegex operator
(unomi-3.0.x) (#888)
---
.../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 4fad7c472..454680c8d 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,
SchemaIdPatternSafetyIT.class,
RuleServiceIT.class,
PrivacyServiceIT.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));
+ }
+
}
