This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch deny-mvel-public-eval-apis in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 7bb972c529c72e4b464d5b1124e3a3b1d667f0fc Author: Serge Huber <[email protected]> AuthorDate: Wed Sep 9 09:38:53 2026 +0200 Keep MVEL enabled and rely on the class-loader deny. Compatibility with 3.0 stays: script:: still compiles. Public eval APIs remain unreachable regardless of how the class name is written. Do not treat request sanitizing as the control that must catch every prefix encoding. --- docker/README.md | 1 - manual/src/main/asciidoc/configuration.adoc | 16 +++------ .../asciidoc/migrations/migrate-3.0-to-3.1.adoc | 7 +--- .../main/resources/etc/custom.system.properties | 5 --- .../spi/conditions/ConditionContextHelper.java | 39 ---------------------- .../spi/conditions/ConditionContextHelperTest.java | 9 ----- .../unomi/rest/endpoints/ContextJsonEndpoint.java | 2 +- .../rest/endpoints/ContextJsonEndpointTest.java | 6 ---- .../apache/unomi/scripting/MvelScriptExecutor.java | 29 +--------------- .../unomi/scripting/MvelScriptExecutorTest.java | 23 ++----------- 10 files changed, 9 insertions(+), 128 deletions(-) diff --git a/docker/README.md b/docker/README.md index d3bbe3b94..a5dc49bde 100644 --- a/docker/README.md +++ b/docker/README.md @@ -128,7 +128,6 @@ Note: Linux doesn't support the host.docker.internal DNS lookup method yet, it s - `UNOMI_HEALTHCHECK_PASSWORD`: Required health-check (`health`) password — no known default - `UNOMI_AUTO_START`: Boolean to specify if unomi auto start with karaf (defaults to `true`) - `UNOMI_DISTRIBUTION`: Specifies the Unomi Distribution Feature to use (`unomi-distribution-elasticsearch` or `unomi-distribution-opensearch`, defaults to `unomi-distribution-elasticsearch`) -- `UNOMI_SCRIPTING_MVEL_ENABLED`: Enable MVEL `script::` expressions (defaults to `false` in 3.1; set `true` only for existing rules/conditions that still need MVEL) ### ElasticSearch-specific Variables - `UNOMI_ELASTICSEARCH_ADDRESSES`: ElasticSearch host:port (default: localhost:9200) diff --git a/manual/src/main/asciidoc/configuration.adoc b/manual/src/main/asciidoc/configuration.adoc index 0ef39dfc7..c67b8a034 100644 --- a/manual/src/main/asciidoc/configuration.adoc +++ b/manual/src/main/asciidoc/configuration.adoc @@ -534,10 +534,6 @@ curl -X POST "http://localhost:8181/cxs/segments"; \ === Scripting security -Starting with Apache Unomi 3.1, MVEL scripting is *disabled by default*. Expressions that start with `script::` are not compiled or executed unless you set `org.apache.unomi.scripting.mvel.enabled` to `true` (environment variable `UNOMI_SCRIPTING_MVEL_ENABLED=true`). - -Leave it off unless existing rules or conditions still need MVEL. When it is on, the allow-list, forbid list, and filtering class loader still apply. Built-in definitions that use `script::` (for example `sessionDurationCondition` and the session size update in `_i6iqbc3x8_sessionPageReferrer`) also require the engine to be enabled. - ==== Multi-layer scripting filtering system The scripting security system is multi-layered. @@ -554,7 +550,9 @@ The second layer is the expression filtering system, that uses an allow-listing expressions (through configuration and deployment on the server side). Any unrecognized expression will not be accepted. Finally, once the script starts executing in the scripting engine, a filtering class loader will only let the script -access classes that have been allowed. +access classes that have been allowed. Public MVEL eval APIs are always denied, including when they are referenced +through inner classes, array types, or names with invisible characters. That class-loader check is the control that +must hold even if an expression reaches the engine. This multi-layered approach makes it possible to retain a high level of security even if one layer is poorly configured or abused. @@ -647,7 +645,6 @@ From https://github.com/apache/unomi/blob/unomi-1.5.x/plugins/baseplugin/src/mai As we see in the above example, we use an MVEL script with the setPropertyAction to set a property value. Starting with version 1.5.2, any expression use in rules MUST be allow-listed. -Starting with version 3.1, MVEL itself is also off by default: set `org.apache.unomi.scripting.mvel.enabled=true` before those expressions will run. By default, Apache Unomi comes with some built-in allowed expressions that cover all the internal uses cases. @@ -691,11 +688,6 @@ Alongside with the allow-listing technology, there are new configuration paramet [source] ---- -# MVEL compilation and execution for script:: expressions. Default false starting with Unomi 3.1. -# Set to true if existing rules or conditions still use script:: (allow-list and class-loader filters still apply). -# Must be the string true (not 1). -org.apache.unomi.scripting.mvel.enabled=${env:UNOMI_SCRIPTING_MVEL_ENABLED:-false} - # These parameters control the list of classes that are allowed or forbidden when executing expressions. # Forbid is applied first. Public MVEL eval APIs are always denied, even if this list is emptied. org.apache.unomi.scripting.allow=${env:UNOMI_ALLOW_SCRIPTING_CLASSES:-org.apache.unomi.api.Event,org.apache.unomi.api.Profile,org.apache.unomi.api.Session,org.apache.unomi.api.Item,org.apache.unomi.api.CustomItem,java.lang.Object,java.util.Map,java.util.HashMap,java.lang.Integer,org.mvel2.*} @@ -943,7 +935,7 @@ org.apache.unomi.services.actions;version ==== Scripting roadmap -Starting with 3.1, MVEL is off unless operators opt in (see above). Scripting will probably undergo further changes in later versions of Apache Unomi, with the likely retirement of MVEL in favor of Groovy Actions detailed above. +Scripting will probably undergo major changes in future versions of Apache Unomi, with the likely retirement of MVEL in favor of Groovy Actions detailed above. These changes will not happen on maintenance versions of Apache Unomi, only in the next major version. Maintenance versions will of course maintain compatibility with existing scripting solutions. diff --git a/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc b/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc index b858b35eb..59350e8ec 100644 --- a/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc +++ b/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc @@ -294,10 +294,6 @@ In addition to tenant API keys, Unomi 3.1 hardens several contracts that can bre |`mergeProfilesOnProperty` / cross-profile `updateProperties` / `systemProperties.*` |Sometimes driven from public context events |Cross-profile merge/update and `systemProperties` writes require a trusted caller (system or tenant admin). Public callers may still update the **current** cookie-bound profile's normal properties when the event type allows it. - -|MVEL `script::` expressions -|Compiled and executed when they passed the allow-list -|**Disabled by default.** Set `org.apache.unomi.scripting.mvel.enabled=true` (env `UNOMI_SCRIPTING_MVEL_ENABLED=true`) if existing rules or conditions still need MVEL. Allow-list and class-loader filters still apply when it is on. |=== [WARNING] @@ -337,7 +333,6 @@ console (port 8102) is not covered by that check. * Ops scripts and Docker: export both password env vars; replace `karaf:karaf` with `karaf:$UNOMI_ROOT_PASSWORD`. * Windows deployments: confirm the passwords actually took effect after upgrading — the startup check warns but cannot halt `karaf.bat` (see the warning above). * Automation that uploaded Groovy actions or managed Router import/export with a tenant private key: switch to system administrator credentials. -* Deployments whose rules or conditions still use `script::` MVEL expressions (including built-in `sessionDurationCondition` and the session size action on `_i6iqbc3x8_sessionPageReferrer`): set `org.apache.unomi.scripting.mvel.enabled=true` / `UNOMI_SCRIPTING_MVEL_ENABLED=true`. Expression allow-lists and class-loader filters still apply when the engine is on. New 3.1 deployments can leave the default (`false`). ==== Benefits of Multi-Tenancy in 3.1 @@ -356,7 +351,7 @@ Before starting the migration, please ensure that: - You are currently running Apache Unomi 3.0 (or a later 3.0.x version) - You understand the multi-tenancy impact on your data model - You have a plan to update client applications to tenant API keys (or temporary <<_v2_compatibility_mode,V2 compatibility mode>> only if coming from 2.x) -- You have reviewed the <<_client_facing_hardening_3_1,client-facing hardening>> notes (cookie-only public profile binding, HttpOnly default, required passwords, privileged REST roles, MVEL off by default) +- You have reviewed the <<_client_facing_hardening_3_1,client-facing hardening>> notes (cookie-only public profile binding, HttpOnly default, required passwords, privileged REST roles) - You know how to obtain plaintext API keys after upgrade (regenerate via `/cxs/tenants/{id}/apikeys`; create responses expose masked keys only) === Migration Process diff --git a/package/src/main/resources/etc/custom.system.properties b/package/src/main/resources/etc/custom.system.properties index 05c4a9e95..645d742ba 100644 --- a/package/src/main/resources/etc/custom.system.properties +++ b/package/src/main/resources/etc/custom.system.properties @@ -30,11 +30,6 @@ ${optionals}=unomi.custom.system.properties org.apache.unomi.security.root.password=${env:UNOMI_ROOT_PASSWORD} org.apache.unomi.healthcheck.password=${env:UNOMI_HEALTHCHECK_PASSWORD} -# MVEL compilation and execution for script:: expressions. Default false starting with Unomi 3.1. -# Set to true if existing rules or conditions still use script:: (allow-list and class-loader filters still apply). -# Must be the string true (not 1). -org.apache.unomi.scripting.mvel.enabled=${env:UNOMI_SCRIPTING_MVEL_ENABLED:-false} - # These parameters control the list of classes that are allowed or forbidden when executing expressions. # The forbid list is applied first, and public MVEL eval APIs are always denied even if this list is emptied. # org.mvel2.* compiler classes stay on the allow wildcard so allow-listed expressions can still compile. diff --git a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java index 2c5192e6d..bb37c1985 100644 --- a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java +++ b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java @@ -704,45 +704,6 @@ public class ConditionContextHelper { return false; } - /** - * Detects {@code script::} / {@code parameter::} after stripping invisible characters, trimming, - * and ignoring case (and fullwidth colons). Used by public-request sanitizers so prefix tricks - * cannot skip filtering. Resolution still requires the exact prefixes in - * {@link #isParameterReference(Object)}. - * - * @param value the value to inspect - * @return {@code true} when the value looks like a script or parameter reference - */ - public static boolean looksLikeScriptOrParameterReference(Object value) { - if (!(value instanceof String)) { - return false; - } - String normalized = stripInvisibleCharacters((String) value).trim().replace('\uFF1A', ':'); - return startsWithIgnoreCase(normalized, SCRIPT_EXPRESSION_PREFIX) || - startsWithIgnoreCase(normalized, PARAMETER_REFERENCE_PREFIX); - } - - private static boolean startsWithIgnoreCase(String value, String prefix) { - return value.regionMatches(true, 0, prefix, 0, prefix.length()); - } - - static String stripInvisibleCharacters(String input) { - StringBuilder stripped = new StringBuilder(input.length()); - for (int i = 0; i < input.length(); ) { - int codePoint = input.codePointAt(i); - i += Character.charCount(codePoint); - if (codePoint == 0) { - continue; - } - int type = Character.getType(codePoint); - if (type == Character.FORMAT || type == Character.CONTROL || type == Character.SURROGATE) { - continue; - } - stripped.appendCodePoint(codePoint); - } - return stripped.toString(); - } - /** * Folds an object's string representation to ASCII. * diff --git a/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelperTest.java b/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelperTest.java index ef043215f..3ee6e6f11 100644 --- a/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelperTest.java +++ b/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelperTest.java @@ -632,15 +632,6 @@ public class ConditionContextHelperTest { assertFalse(ConditionContextHelper.isParameterReference(null)); assertFalse(ConditionContextHelper.isParameterReference(42)); assertFalse(ConditionContextHelper.isParameterReference("parameter:not-a-reference")); - assertFalse(ConditionContextHelper.looksLikeScriptOrParameterReference("equals")); - assertFalse(ConditionContextHelper.looksLikeScriptOrParameterReference("notascript::payload")); - assertFalse(ConditionContextHelper.isParameterReference(" SCRIPT::evil")); - assertTrue(ConditionContextHelper.looksLikeScriptOrParameterReference(" SCRIPT::evil")); - assertTrue(ConditionContextHelper.looksLikeScriptOrParameterReference("Script::evil")); - assertTrue(ConditionContextHelper.looksLikeScriptOrParameterReference("\uFEFFscript::evil")); - assertTrue(ConditionContextHelper.looksLikeScriptOrParameterReference("scr\u200Bipt::evil")); - assertTrue(ConditionContextHelper.looksLikeScriptOrParameterReference("PARAMETER::key")); - assertTrue(ConditionContextHelper.looksLikeScriptOrParameterReference("script\uFF1A\uFF1Aevil")); } @Test diff --git a/rest/src/main/java/org/apache/unomi/rest/endpoints/ContextJsonEndpoint.java b/rest/src/main/java/org/apache/unomi/rest/endpoints/ContextJsonEndpoint.java index c9530c083..c36b663dc 100644 --- a/rest/src/main/java/org/apache/unomi/rest/endpoints/ContextJsonEndpoint.java +++ b/rest/src/main/java/org/apache/unomi/rest/endpoints/ContextJsonEndpoint.java @@ -500,7 +500,7 @@ public class ContextJsonEndpoint { private Object sanitizeValue(Object value) { if (value instanceof String) { String stringValue = (String) value; - if (ConditionContextHelper.looksLikeScriptOrParameterReference(value)) { + if (ConditionContextHelper.isParameterReference(value)) { LOGGER.warn("Scripting detected in context request, filtering out. See debug level for more information"); LOGGER.debug("Scripting detected in context request with value {}, filtering out...", value); return null; diff --git a/rest/src/test/java/org/apache/unomi/rest/endpoints/ContextJsonEndpointTest.java b/rest/src/test/java/org/apache/unomi/rest/endpoints/ContextJsonEndpointTest.java index a2d8df779..b29946802 100644 --- a/rest/src/test/java/org/apache/unomi/rest/endpoints/ContextJsonEndpointTest.java +++ b/rest/src/test/java/org/apache/unomi/rest/endpoints/ContextJsonEndpointTest.java @@ -74,12 +74,6 @@ class ContextJsonEndpointTest { void sanitizeValue_filtersScriptAndParameterReferences() throws Exception { assertNull(invokeSanitizeValue("script::Runtime.getRuntime().exec(\"touch /tmp/evil\")")); assertNull(invokeSanitizeValue("parameter::eventTypeId")); - assertNull(invokeSanitizeValue(" SCRIPT::evil")); - assertNull(invokeSanitizeValue("Script::evil")); - assertNull(invokeSanitizeValue("\uFEFFscript::evil")); - assertNull(invokeSanitizeValue("scr\u200Bipt::evil")); - assertNull(invokeSanitizeValue("PARAMETER::eventTypeId")); - assertEquals("mentions script:: in documentation", invokeSanitizeValue("mentions script:: in documentation")); } @Test 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 007f47f81..cb706050f 100644 --- a/scripting/src/main/java/org/apache/unomi/scripting/MvelScriptExecutor.java +++ b/scripting/src/main/java/org/apache/unomi/scripting/MvelScriptExecutor.java @@ -19,43 +19,22 @@ package org.apache.unomi.scripting; import org.mvel2.MVEL; import org.mvel2.ParserConfiguration; import org.mvel2.ParserContext; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import java.io.Serializable; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.atomic.AtomicBoolean; /** * MVEL script executor implementation */ public class MvelScriptExecutor implements ScriptExecutor { - /** - * System property that enables MVEL compilation and execution for {@code script::} expressions. - * Default is {@code false} starting with Unomi 3.1. Set to {@code true} to restore the previous behaviour - * for deployments that still rely on MVEL in rules or conditions. - */ - public static final String ENABLED_PROPERTY = "org.apache.unomi.scripting.mvel.enabled"; - - private static final Logger LOGGER = LoggerFactory.getLogger(MvelScriptExecutor.class.getName()); - private static final String INVALID_SCRIPT_MARKER = "--- Invalid Script Marker ---"; - private static final AtomicBoolean DISABLED_NOTICE_LOGGED = new AtomicBoolean(); + private final static String INVALID_SCRIPT_MARKER = "--- Invalid Script Marker ---"; private Map<String, Serializable> mvelExpressions = new ConcurrentHashMap<>(); private SecureFilteringClassLoader secureFilteringClassLoader = new SecureFilteringClassLoader(getClass().getClassLoader()); private ExpressionFilterFactory expressionFilterFactory; - /** - * Whether MVEL compilation and execution are enabled. - * - * @return {@code true} only when {@value #ENABLED_PROPERTY} is {@code true} - */ - public static boolean isEnabled() { - return Boolean.parseBoolean(System.getProperty(ENABLED_PROPERTY, "false")); - } - /** * Sets the factory used to obtain expression filters per script language. * @@ -67,12 +46,6 @@ public class MvelScriptExecutor implements ScriptExecutor { @Override public Object execute(String script, Map<String, Object> context) { - if (!isEnabled()) { - if (DISABLED_NOTICE_LOGGED.compareAndSet(false, true)) { - LOGGER.info("MVEL scripting is disabled ({}=false); script expressions will not run. Set it to true to restore MVEL for existing rules and conditions.", ENABLED_PROPERTY); - } - return null; - } final ClassLoader tccl = Thread.currentThread().getContextClassLoader(); try { 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 78780c562..f7b60e0b2 100644 --- a/scripting/src/test/java/org/apache/unomi/scripting/MvelScriptExecutorTest.java +++ b/scripting/src/test/java/org/apache/unomi/scripting/MvelScriptExecutorTest.java @@ -18,7 +18,6 @@ package org.apache.unomi.scripting; import org.apache.unomi.api.CustomItem; import org.apache.unomi.api.Event; -import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -33,7 +32,6 @@ 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.assertNull; public class MvelScriptExecutorTest { @@ -46,34 +44,18 @@ public class MvelScriptExecutorTest { @Before public void setup() { - System.clearProperty(MvelScriptExecutor.ENABLED_PROPERTY); scriptExecutor.setExpressionFilterFactory(emptyAllowList()); } - @After - public void restoreMvelEnabledProperty() { - System.clearProperty(MvelScriptExecutor.ENABLED_PROPERTY); - } - - @Test - public void testMvelDisabledByDefaultEvenWhenExpressionIsAllowed() { - scriptExecutor.setExpressionFilterFactory(allowAllExpressions()); - Object result = scriptExecutor.execute("1+1", new HashMap<String, Object>()); - assertNull(result); - assertFalse(MvelScriptExecutor.isEnabled()); - } - @Test - public void testMvelEnabledEvaluatesAllowlistedExpression() { - System.setProperty(MvelScriptExecutor.ENABLED_PROPERTY, "true"); + public void testAllowlistedArithmeticStillRuns() { scriptExecutor.setExpressionFilterFactory(allowAllExpressions()); Object result = scriptExecutor.execute("1+1", new HashMap<String, Object>()); assertEquals(2, ((Number) result).intValue()); } @Test - public void testNestedPublicEvalDoesNotRunWhenEnabled() { - System.setProperty(MvelScriptExecutor.ENABLED_PROPERTY, "true"); + public void testNestedPublicEvalDoesNotRun() { scriptExecutor.setExpressionFilterFactory(allowAllExpressions()); assertPublicEvalDoesNotReturnTwo("org.mvel2.MVEL.eval(\"1+1\")"); assertPublicEvalDoesNotReturnTwo("org.mvel2.MVEL.eval ( \"1+1\" )"); @@ -83,7 +65,6 @@ public class MvelScriptExecutorTest { @Test public void testMVELSecurity() throws IOException { - System.setProperty(MvelScriptExecutor.ENABLED_PROPERTY, "true"); Map<String, Object> ctx = new HashMap<>(); Event mockEvent = generateMockEvent(); ctx.put("event", mockEvent);
