This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch bound-diagnostic-and-script-caches in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 9a4f6b65b19310e001c0b70386fdc3d2a42eb43a Author: Serge Huber <[email protected]> AuthorDate: Tue Sep 8 21:01:08 2026 +0200 Bound diagnostic type tracking and the MVEL expression cache. Public input can feed unique ids and rejected scripts; keep those maps from growing without a cap. --- .../apache/unomi/scripting/MvelScriptExecutor.java | 46 ++++++++++++++++++---- .../unomi/scripting/MvelScriptExecutorTest.java | 34 ++++++++++++++++ .../services/impl/TypeResolutionServiceImpl.java | 34 ++++++++++++++-- .../services/impl/events/EventServiceImpl.java | 5 ++- .../impl/TypeResolutionServiceImplTest.java | 22 +++++++++++ 5 files changed, 129 insertions(+), 12 deletions(-) diff --git a/scripting/src/main/java/org/apache/unomi/scripting/MvelScriptExecutor.java b/scripting/src/main/java/org/apache/unomi/scripting/MvelScriptExecutor.java index cb706050f..dc01f3835 100644 --- a/scripting/src/main/java/org/apache/unomi/scripting/MvelScriptExecutor.java +++ b/scripting/src/main/java/org/apache/unomi/scripting/MvelScriptExecutor.java @@ -21,8 +21,13 @@ import org.mvel2.ParserConfiguration; import org.mvel2.ParserContext; import java.io.Serializable; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.Collections; +import java.util.HexFormat; +import java.util.LinkedHashMap; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; /** * MVEL script executor implementation @@ -31,7 +36,22 @@ public class MvelScriptExecutor implements ScriptExecutor { private final static String INVALID_SCRIPT_MARKER = "--- Invalid Script Marker ---"; - private Map<String, Serializable> mvelExpressions = new ConcurrentHashMap<>(); + private static final int DEFAULT_EXPRESSIONS_CACHE_MAX_SIZE = 1000; + + private final int expressionsCacheMaxSize = Integer.getInteger( + "org.apache.unomi.scripting.mvel.expressions.cache.max.size", DEFAULT_EXPRESSIONS_CACHE_MAX_SIZE); + + /** + * Size-bounded LRU cache keyed by a fixed-size hash of the script text. Rejected scripts can be + * unique and large, so an unbounded map keyed by the raw script would grow without limit. + */ + private final Map<String, Serializable> mvelExpressions = Collections.synchronizedMap( + new LinkedHashMap<String, Serializable>(16, 0.75f, true) { + @Override + protected boolean removeEldestEntry(Map.Entry<String, Serializable> eldest) { + return size() > expressionsCacheMaxSize; + } + }); private SecureFilteringClassLoader secureFilteringClassLoader = new SecureFilteringClassLoader(getClass().getClassLoader()); private ExpressionFilterFactory expressionFilterFactory; @@ -51,10 +71,12 @@ public class MvelScriptExecutor implements ScriptExecutor { try { Thread.currentThread().setContextClassLoader(secureFilteringClassLoader); - if (!mvelExpressions.containsKey(script)) { + String scriptCacheKey = getScriptCacheKey(script); + Serializable compiledScript = mvelExpressions.get(scriptCacheKey); + if (compiledScript == null) { if (expressionFilterFactory.getExpressionFilter("mvel").filter(script) == null) { - mvelExpressions.put(script, INVALID_SCRIPT_MARKER); + compiledScript = INVALID_SCRIPT_MARKER; } else { ParserConfiguration parserConfiguration = new ParserConfiguration(); parserConfiguration.setClassLoader(secureFilteringClassLoader); @@ -71,11 +93,12 @@ public class MvelScriptExecutor implements ScriptExecutor { parserContext.addImport("ThreadLocal", String.class); parserContext.addImport("SecurityManager", String.class); - mvelExpressions.put(script, MVEL.compileExpression(script, parserContext)); + compiledScript = MVEL.compileExpression(script, parserContext); } + mvelExpressions.put(scriptCacheKey, compiledScript); } - if (mvelExpressions.containsKey(script) && mvelExpressions.get(script) != INVALID_SCRIPT_MARKER) { - return MVEL.executeExpression(mvelExpressions.get(script), context); + if (compiledScript != INVALID_SCRIPT_MARKER) { + return MVEL.executeExpression(compiledScript, context); } else { return null; } @@ -83,4 +106,13 @@ public class MvelScriptExecutor implements ScriptExecutor { Thread.currentThread().setContextClassLoader(tccl); } } + + private static String getScriptCacheKey(String script) { + try { + byte[] hash = MessageDigest.getInstance("SHA-256").digest(script.getBytes(StandardCharsets.UTF_8)); + return HexFormat.of().formatHex(hash); + } catch (NoSuchAlgorithmException e) { + return script; + } + } } diff --git a/scripting/src/test/java/org/apache/unomi/scripting/MvelScriptExecutorTest.java b/scripting/src/test/java/org/apache/unomi/scripting/MvelScriptExecutorTest.java index f7b60e0b2..cd7e8ad89 100644 --- a/scripting/src/test/java/org/apache/unomi/scripting/MvelScriptExecutorTest.java +++ b/scripting/src/test/java/org/apache/unomi/scripting/MvelScriptExecutorTest.java @@ -23,6 +23,7 @@ import org.junit.Test; import java.io.File; import java.io.IOException; +import java.lang.reflect.Field; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -32,6 +33,7 @@ import java.util.regex.Pattern; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; public class MvelScriptExecutorTest { @@ -126,6 +128,38 @@ public class MvelScriptExecutorTest { assertFalse("Vulnerability successfully executed ! File created at " + vulnFile.getCanonicalPath(), vulnFile.exists()); } + @Test + public void testRejectedScriptCacheIsBounded() throws Exception { + System.setProperty("org.apache.unomi.scripting.mvel.expressions.cache.max.size", "10"); + try { + MvelScriptExecutor boundedExecutor = new MvelScriptExecutor(); + boundedExecutor.setExpressionFilterFactory(new ExpressionFilterFactory() { + @Override + public ExpressionFilter getExpressionFilter(String filterCollection) { + Set<Pattern> allowedExpressions = new HashSet<>(); + Set<Pattern> forbiddenExpressions = new HashSet<>(); + return new ExpressionFilter(allowedExpressions, forbiddenExpressions); + } + }); + String padding = "x".repeat(4096); + Map<String, Object> ctx = new HashMap<>(); + for (int i = 0; i < 1000; i++) { + boundedExecutor.execute("rejected-script-" + i + "-" + padding, ctx); + } + Field mvelExpressionsField = MvelScriptExecutor.class.getDeclaredField("mvelExpressions"); + mvelExpressionsField.setAccessible(true); + Map<?, ?> cache = (Map<?, ?>) mvelExpressionsField.get(boundedExecutor); + assertTrue("Rejected-script cache must be size-bounded but grew to " + cache.size(), + cache.size() <= 10); + for (Object cacheKey : cache.keySet()) { + assertTrue("Cache keys must be fixed-size hashes, not the raw script text", + ((String) cacheKey).length() <= 64); + } + } finally { + System.clearProperty("org.apache.unomi.scripting.mvel.expressions.cache.max.size"); + } + } + private void assertPublicEvalDoesNotReturnTwo(String expression) { Object result = null; try { diff --git a/services/src/main/java/org/apache/unomi/services/impl/TypeResolutionServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/TypeResolutionServiceImpl.java index 329756a75..0e99cc571 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/TypeResolutionServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/TypeResolutionServiceImpl.java @@ -45,6 +45,12 @@ public class TypeResolutionServiceImpl implements TypeResolutionService { private static final Logger LOGGER = LoggerFactory.getLogger(TypeResolutionServiceImpl.class.getName()); private static final int MAX_RECURSION_DEPTH = 1000; + + /** + * Upper bound for diagnostic de-duplication collections. Some identifiers come from public + * input (for example condition type ids on context filters), so these sets must not grow without limit. + */ + private static final int MAX_TRACKED_DIAGNOSTIC_ENTRIES = 1000; private volatile DefinitionsService definitionsService; @@ -127,7 +133,7 @@ public class TypeResolutionServiceImpl implements TypeResolutionService { } ConditionType conditionType = definitionsService.getConditionType(conditionTypeId); if (conditionType == null) { - if (unresolvedConditionTypes.add(conditionTypeId)) { + if (addBoundedDiagnostic(unresolvedConditionTypes, conditionTypeId)) { LOGGER.warn("Couldn't resolve condition type: {} for {}", conditionTypeId, contextObjectName); } return false; @@ -217,6 +223,20 @@ public class TypeResolutionServiceImpl implements TypeResolutionService { } } + /** + * Adds a value to a bounded diagnostic de-duplication set. Returns {@code true} if the value + * was newly stored (the caller should log). Once the cap is reached, further values are ignored. + */ + private static boolean addBoundedDiagnostic(Set<String> set, String value) { + if (set.contains(value)) { + return false; + } + if (set.size() >= MAX_TRACKED_DIAGNOSTIC_ENTRIES) { + return false; + } + return set.add(value); + } + @Override public boolean resolveActionTypes(Rule rule, boolean ignoreErrors) { if (definitionsService == null) { @@ -228,7 +248,7 @@ public class TypeResolutionServiceImpl implements TypeResolutionService { if (rule.getActions() == null) { if (!ignoreErrors) { // Only warn once per rule to avoid log spam - if (warnedRulesWithNullActions.add(ruleId)) { + if (addBoundedDiagnostic(warnedRulesWithNullActions, ruleId)) { LOGGER.warn("Rule {}:{} has null actions", ruleId, rule.getMetadata() != null ? rule.getMetadata().getName() : "unknown"); } } @@ -237,7 +257,7 @@ public class TypeResolutionServiceImpl implements TypeResolutionService { if (rule.getActions().isEmpty()) { if (!ignoreErrors) { // Only warn once per rule to avoid log spam - if (warnedRulesWithNullActions.add(ruleId)) { + if (addBoundedDiagnostic(warnedRulesWithNullActions, ruleId)) { LOGGER.warn("Rule {}:{} has empty actions", ruleId, rule.getMetadata() != null ? rule.getMetadata().getName() : "unknown"); } } @@ -262,7 +282,7 @@ public class TypeResolutionServiceImpl implements TypeResolutionService { unresolvedActionTypes.remove(action.getActionTypeId()); action.setActionType(actionType); } else { - if (unresolvedActionTypes.add(action.getActionTypeId())) { + if (addBoundedDiagnostic(unresolvedActionTypes, action.getActionTypeId())) { LOGGER.warn("Couldn't resolve action type : {}", action.getActionTypeId()); } return false; @@ -454,6 +474,12 @@ public class TypeResolutionServiceImpl implements TypeResolutionService { } Map<String, InvalidObjectInfo> typeMap = invalidObjects.computeIfAbsent(objectType, k -> new ConcurrentHashMap<>()); + + if (!typeMap.containsKey(objectId) && typeMap.size() >= MAX_TRACKED_DIAGNOSTIC_ENTRIES) { + LOGGER.debug("Invalid object tracking for type {} is full ({} entries), not tracking {}", + objectType, MAX_TRACKED_DIAGNOSTIC_ENTRIES, objectId); + return; + } InvalidObjectInfo newInfo = new InvalidObjectInfo( objectType, diff --git a/services/src/main/java/org/apache/unomi/services/impl/events/EventServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/events/EventServiceImpl.java index 3442a59ac..493cc03ca 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/events/EventServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/events/EventServiceImpl.java @@ -48,6 +48,7 @@ public class EventServiceImpl implements EventService { /** Event-type chains already logged at recursion limit (see {@link #recursionChainKey}). */ private static final Set<String> LOGGED_RECURSION_CHAINS = ConcurrentHashMap.newKeySet(); + private static final int MAX_LOGGED_RECURSION_CHAINS = 1000; /** * Simple data class to hold event information for recursion tracking. @@ -221,7 +222,9 @@ public class EventServiceImpl implements EventService { // Original allowed depths 0-10 (11 calls), blocking at depth 11 if (eventStack.size() > MAX_RECURSION_DEPTH) { String chainKey = recursionChainKey(eventStack); - if (LOGGED_RECURSION_CHAINS.add(chainKey)) { + if (!LOGGED_RECURSION_CHAINS.contains(chainKey) + && LOGGED_RECURSION_CHAINS.size() < MAX_LOGGED_RECURSION_CHAINS + && LOGGED_RECURSION_CHAINS.add(chainKey)) { EventInfo currentEventInfo = new EventInfo(event); if (tracer != null) { tracer.trace("Max recursion depth reached for event: " + event.getEventType(), event.getItemId()); diff --git a/services/src/test/java/org/apache/unomi/services/impl/TypeResolutionServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/TypeResolutionServiceImplTest.java index 9e62633e8..db69770ae 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TypeResolutionServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TypeResolutionServiceImplTest.java @@ -1362,5 +1362,27 @@ public class TypeResolutionServiceImplTest { assertTrue(info.getReason().contains("missingLeafType"), "Reason text must name the actual failing leaf type"); } + + @Test + public void unresolvedConditionTypeTracking_isBounded() { + when(definitionsService.getConditionType(startsWith("unknownType-"))).thenReturn(null); + + for (int i = 0; i < 2500; i++) { + Condition condition = new Condition(); + condition.setConditionTypeId("unknownType-" + i); + assertFalse(typeResolutionService.resolveConditionType(condition, "boundedness test")); + } + + java.lang.reflect.Field field; + try { + field = TypeResolutionServiceImpl.class.getDeclaredField("unresolvedConditionTypes"); + field.setAccessible(true); + Set<?> tracked = (Set<?>) field.get(typeResolutionService); + assertTrue(tracked.size() <= 1000, + "unresolved condition type tracking must stay bounded, but had " + tracked.size() + " entries"); + } catch (ReflectiveOperationException e) { + throw new AssertionError(e); + } + } }
