This is an automated email from the ASF dual-hosted git repository.
asf-gitbox-commits pushed a commit to branch
UNOMI-972-credentials-profile-binding-privileged-rest
in repository https://gitbox.apache.org/repos/asf/unomi.git
The following commit(s) were added to
refs/heads/UNOMI-972-credentials-profile-binding-privileged-rest by this push:
new fae021f6d UNOMI-972: close the systemProperties gate on the bare key,
not just the prefix
fae021f6d is described below
commit fae021f6dd36514315dde55e6a27856f549a8e41
Author: Serge Huber <[email protected]>
AuthorDate: Sun Aug 9 09:35:26 2026 +0200
UNOMI-972: close the systemProperties gate on the bare key, not just the
prefix
The gate added for the reported cross-profile write matched
prop.startsWith("systemProperties."), so the exact key "systemProperties" -
no dot
- walked straight through it. That is not a near-miss. For a flat name
PropertyHelper skips its nested-resolution loop and calls
BeanUtils.setProperty(target, "systemProperties", value), which invokes
Profile#setSystemProperties(Map) and replaces the entire map: a superset of
the
per-key write the gate exists to block, and enough to plant
mergeIdentifier, which
is the field MergeProfilesOnPropertyAction keys on.
Both gates - the update mapping and the delete mapping - now go through one
isSystemPropertiesWrite predicate rather than repeating the condition, so
the two
cannot drift apart the way the prefix check drifted from the field it was
meant to
protect.
The regression test was written first and confirmed failing against the
unfixed
gate (expected NO_CHANGE, got PROFILE_UPDATED - the untrusted caller really
did
replace the map). A trusted caller may still set it, so this stays a trust
check
rather than a ban.
Found by the Copilot review on the pull request. Its diagnosis was right
and the
multi-agent review pass had missed it.
Note the shape of the underlying problem is not fully addressed:
processProperties
has no allowlist at all, so segments, scores, consents and a wholesale
properties
replacement are reachable by an untrusted caller through the same route. An
allowlist would be the robust fix but could break existing integrations, so
it is
proposed separately rather than folded in here.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
---
.../baseplugin/actions/UpdatePropertiesAction.java | 25 +++++++-
.../actions/UpdatePropertiesActionTest.java | 71 ++++++++++++++++++++++
2 files changed, 93 insertions(+), 3 deletions(-)
diff --git
a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesAction.java
b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesAction.java
index c5ea1c5f6..78070d144 100644
---
a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesAction.java
+++
b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesAction.java
@@ -49,8 +49,10 @@ public class UpdatePropertiesAction implements
ActionExecutor {
public static final String TARGET_TYPE_PROFILE = "profile";
+ /** The reserved profile field that only a caller with system access may
write. */
+ private static final String SYSTEM_PROPERTIES_KEY = "systemProperties";
/** Prefix of the profile properties that only a caller with system access
may write. */
- private static final String SYSTEM_PROPERTIES_PREFIX = "systemProperties.";
+ private static final String SYSTEM_PROPERTIES_PREFIX =
SYSTEM_PROPERTIES_KEY + ".";
private static final Logger LOGGER =
LoggerFactory.getLogger(UpdatePropertiesAction.class.getName());
@@ -122,7 +124,7 @@ public class UpdatePropertiesAction implements
ActionExecutor {
List<String> propsToDelete = (List<String>)
event.getProperties().get(PROPS_TO_DELETE);
if (propsToDelete != null) {
for (String prop : propsToDelete) {
- if (!trustedCaller &&
prop.startsWith(SYSTEM_PROPERTIES_PREFIX)) {
+ if (!trustedCaller && isSystemPropertiesWrite(prop)) {
LOGGER.warn("Refusing systemProperties delete for
untrusted caller: {}", LogSanitizer.forLogging(prop));
continue;
}
@@ -164,7 +166,7 @@ public class UpdatePropertiesAction implements
ActionExecutor {
private boolean processProperties(Profile target, Map<String, Object>
propsMap, String strategy, boolean trustedCaller) {
boolean isProfileOrPersonaUpdated = false;
for (String prop : propsMap.keySet()) {
- if (!trustedCaller && prop.startsWith(SYSTEM_PROPERTIES_PREFIX)) {
+ if (!trustedCaller && isSystemPropertiesWrite(prop)) {
LOGGER.warn("Refusing systemProperties update for untrusted
caller: {}", LogSanitizer.forLogging(prop));
continue;
}
@@ -187,6 +189,23 @@ public class UpdatePropertiesAction implements
ActionExecutor {
return isProfileOrPersonaUpdated;
}
+
+ /**
+ * Whether a property name writes the reserved {@code systemProperties}
area.
+ * <p>
+ * Matching the {@code systemProperties.} prefix alone was not enough: the
bare key
+ * {@code systemProperties} has no dot, so it slipped through, and for a
flat name
+ * {@link org.apache.unomi.persistence.spi.PropertyHelper#setProperty}
falls through to
+ * {@code BeanUtils.setProperty}, which invokes {@code
Profile#setSystemProperties(Map)} and
+ * replaces the entire map. That is strictly more than the per-key write
this gate blocks — it
+ * is enough to plant {@code mergeIdentifier} and drive the profile-merge
action.
+ *
+ * @param propertyName the event-supplied property name
+ * @return true when the name targets systemProperties, whether wholesale
or a single entry
+ */
+ private static boolean isSystemPropertiesWrite(String propertyName) {
+ return SYSTEM_PROPERTIES_KEY.equals(propertyName) ||
propertyName.startsWith(SYSTEM_PROPERTIES_PREFIX);
+ }
/**
* Whether the caller holds system access, i.e. the administrator or
tenant administrator role.
* <p>
diff --git
a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesActionTest.java
b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesActionTest.java
index 8045d3572..5f77b847e 100644
---
a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesActionTest.java
+++
b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesActionTest.java
@@ -97,6 +97,77 @@ public class UpdatePropertiesActionTest {
assertEquals(null,
caller.getSystemProperties().get("mergeIdentifier"));
}
+ /**
+ * The gate matched on the {@code systemProperties.} prefix only, so the
exact key
+ * {@code systemProperties} slipped past it. That is not a harmless
near-miss: for a flat name
+ * PropertyHelper falls through to {@code BeanUtils.setProperty}, which
calls
+ * {@code Profile#setSystemProperties(Map)} and replaces the whole map — a
superset of the
+ * per-key write the gate exists to block, and enough to plant {@code
mergeIdentifier} and drive
+ * the profile-merge action.
+ */
+ @Test
+ public void untrustedCaller_cannotReplaceTheWholeSystemPropertiesMap() {
+ Profile caller = new Profile("caller");
+ caller.getSystemProperties().put("mergeIdentifier",
"[email protected]");
+
+ Map<String, Object> replacement = new HashMap<>();
+ replacement.put("mergeIdentifier", "[email protected]");
+ Map<String, Object> updateMap = new HashMap<>();
+ updateMap.put("systemProperties", replacement);
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(UpdatePropertiesAction.PROPS_TO_UPDATE, updateMap);
+
+ Event event = new Event("updateProperties", null, caller,
"systemscope", null, null, eventProps, new Date(), true);
+
+ int changes = actionExecutor.execute(new Action(), event);
+
+ assertEquals(EventService.NO_CHANGE, changes);
+ assertEquals("the untrusted caller must not replace the
systemProperties map",
+ "[email protected]",
caller.getSystemProperties().get("mergeIdentifier"));
+ }
+
+ /**
+ * The delete mapping is not exploitable the same way - PropertyHelper's
remove strategy bails out
+ * for a name with no dot - so this passes even against the unfixed gate.
Kept as a guard: if that
+ * remove path ever learns to handle flat names, this catches it rather
than the next reporter.
+ */
+ @Test
+ public void untrustedCaller_cannotDeleteTheWholeSystemPropertiesMap() {
+ Profile caller = new Profile("caller");
+ caller.getSystemProperties().put("mergeIdentifier",
"[email protected]");
+
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(UpdatePropertiesAction.PROPS_TO_DELETE,
+ java.util.Collections.singletonList("systemProperties"));
+
+ Event event = new Event("updateProperties", null, caller,
"systemscope", null, null, eventProps, new Date(), true);
+
+ actionExecutor.execute(new Action(), event);
+
+ assertEquals("the untrusted caller must not clear the systemProperties
map",
+ "[email protected]",
caller.getSystemProperties().get("mergeIdentifier"));
+ }
+
+ /** A trusted caller is still allowed to set it, so the gate is a trust
check and not a ban. */
+ @Test
+ public void trustedAdmin_mayReplaceTheSystemPropertiesMap() {
+ when(securityService.hasSystemAccess()).thenReturn(true);
+
+ Profile caller = new Profile("caller");
+ Map<String, Object> replacement = new HashMap<>();
+ replacement.put("mergeIdentifier", "[email protected]");
+ Map<String, Object> updateMap = new HashMap<>();
+ updateMap.put("systemProperties", replacement);
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(UpdatePropertiesAction.PROPS_TO_UPDATE, updateMap);
+
+ Event event = new Event("updateProperties", null, caller,
"systemscope", null, null, eventProps, new Date(), true);
+
+ actionExecutor.execute(new Action(), event);
+
+ assertEquals("[email protected]",
caller.getSystemProperties().get("mergeIdentifier"));
+ }
+
@Test
public void trustedAdmin_canUpdateAnotherProfile() {
when(securityService.hasSystemAccess()).thenReturn(true);
