Copilot commented on code in PR #846:
URL: https://github.com/apache/unomi/pull/846#discussion_r3742458450
##########
plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesAction.java:
##########
@@ -140,11 +161,15 @@ public int execute(Action action, Event event) {
}
}
- private boolean processProperties(Profile target, Map<String, Object>
propsMap, String strategy) {
+ 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)) {
+ LOGGER.warn("Refusing systemProperties update for untrusted
caller: {}", LogSanitizer.forLogging(prop));
+ continue;
+ }
Review Comment:
Checking only the dotted prefix leaves an exact `systemProperties` key
unrestricted. `PropertyHelper.setProperty` can call
`Profile.setSystemProperties(Map)` for that key, allowing an untrusted update
to replace the entire internal map and bypass the intended `systemProperties.*`
gate.
##########
rest/src/main/java/org/apache/unomi/rest/service/impl/RestServiceUtilsImpl.java:
##########
@@ -132,12 +137,51 @@ public EventsRequestContext initEventsRequest(String
scope, String sessionId, St
}
}
- if (profileId == null) {
- // Get profile id from the cookie
- profileId = getProfileIdCookieValue(request);
+ final String requestedBodyProfileId = profileId;
+ final String cookieProfileIdAtRequest =
getProfileIdCookieValue(request);
+ // Resolved once: the caller's identity cannot change during a single
request, and the
+ // checks below must all agree on it.
+ final boolean trustedCaller = isTrustedProfileCaller();
+ // When a public caller presents a foreign sessionId, we must not
overwrite that session.
+ String effectiveSessionId = sessionId;
+
+ if (!trustedCaller) {
+ // Public callers: the cookie is the only profile bearer. Ignore
body profileId entirely
+ // (including when no cookie is present — otherwise knowing a UUID
is enough to load it).
+ if (requestedBodyProfileId != null &&
!requestedBodyProfileId.equals(cookieProfileIdAtRequest)) {
+ LOGGER.debug("Ignoring body profileId {} from public caller
(cookie profileId is {})",
+ LogSanitizer.forLogging(requestedBodyProfileId),
LogSanitizer.forLogging(cookieProfileIdAtRequest));
+ }
+ profileId = cookieProfileIdAtRequest;
+ } else if (profileId == null) {
+ profileId = cookieProfileIdAtRequest;
+ }
+ // else trusted caller keeps explicit body profileId (may differ from
cookie)
+
+ // Trusted callers may intentionally bind to a body profileId that
differs from the cookie.
+ final boolean trustedExplicitProfileOverride = trustedCaller
+ && requestedBodyProfileId != null
+ && cookieProfileIdAtRequest != null
+ && !requestedBodyProfileId.equals(cookieProfileIdAtRequest);
Review Comment:
A trusted caller's explicit body profile is not treated as an override when
no profile cookie is present. If that request also supplies an existing
session, the trusted-caller branch below replaces the requested profile with
the session owner, contrary to the documented ability for trusted integrations
to bind an explicit profile. Treat any non-null trusted body `profileId` that
differs from the cookie—including a missing cookie—as explicit.
##########
extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java:
##########
@@ -520,9 +621,11 @@ private Map<String, ScriptMetadata> getScriptMetadataMap()
{
@Override
public void save(String actionName, String groovyScript) {
validateNotEmpty(actionName, "Action name");
+ validateNoControlCharacters(actionName, "Action name");
validateNotEmpty(groovyScript, "Groovy script");
long startTime = System.currentTimeMillis();
+ auditScriptChange("save", actionName, groovyScript);
Review Comment:
The audit record is emitted before compilation and persistence, and even
before the unchanged-script early return. Failed or no-op uploads therefore
produce the same `AUDIT ... save` record as a deployed change, making the new
audit trail materially misleading. Emit a success record only after persistence
(and optionally a separately labeled attempt/failure record).
##########
manual/src/main/asciidoc/5-min-quickstart.adoc:
##########
@@ -35,6 +35,8 @@ services:
environment:
- UNOMI_ELASTICSEARCH_ADDRESSES=elasticsearch:9200
- UNOMI_THIRDPARTY_PROVIDER1_IPADDRESSES=0.0.0.0/0,::1,127.0.0.1
+ - UNOMI_ROOT_PASSWORD=choose-a-strong-password
+ - UNOMI_HEALTHCHECK_PASSWORD=choose-a-strong-health-password
Review Comment:
These literal credentials make the copy-paste compose example start with
publicly known passwords, contradicting the PR's goal of requiring operators to
choose credentials deliberately. They also leave the host-side
`$UNOMI_ROOT_PASSWORD` used by later curl commands unset. Require exported
values here as the actual compose files do.
This issue also appears on line 89 of the same file.
##########
extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java:
##########
@@ -584,7 +687,9 @@ private void saveActionType(Action action) {
@Override
public void remove(String actionName) {
validateNotEmpty(actionName, "Action name");
+ validateNoControlCharacters(actionName, "Action name");
+ auditScriptChange("remove", actionName, null);
Review Comment:
This logs a completed-looking `remove` audit event before `removeItem` runs.
A missing action or persistence failure can therefore be recorded as a
successful privileged change. Move the success audit after removal, or encode
attempt/outcome explicitly.
##########
rest/src/main/java/org/apache/unomi/rest/service/impl/RestServiceUtilsImpl.java:
##########
@@ -161,52 +205,73 @@ public EventsRequestContext initEventsRequest(String
scope, String sessionId, St
// Try to recover existing session
Profile sessionProfile;
- if (StringUtils.isNotBlank(sessionId) && !invalidateSession) {
+ if (StringUtils.isNotBlank(effectiveSessionId) &&
!invalidateSession) {
-
eventsRequestContext.setSession(profileService.loadSession(sessionId));
+
eventsRequestContext.setSession(profileService.loadSession(effectiveSessionId));
if (eventsRequestContext.getSession() != null) {
sessionProfile =
eventsRequestContext.getSession().getProfile();
boolean anonymousSessionProfile =
sessionProfile.isAnonymousProfile();
if
(!eventsRequestContext.getProfile().isAnonymousProfile() &&
!anonymousSessionProfile &&
!eventsRequestContext.getProfile().getItemId().equals(sessionProfile.getItemId()))
{
Review Comment:
Anonymous session profiles bypass the new ownership check because this
condition only runs when `anonymousSessionProfile` is false. The subsequent
anonymous-handling branch can then rebind that foreign session to the caller's
cookie/new profile, so knowing another visitor's anonymous session ID still
permits session takeover. Apply the cookie/trust ownership check whenever
profile IDs differ, including anonymous session profiles.
##########
manual/src/main/asciidoc/javascript-tracker-guide.adoc:
##########
@@ -136,14 +137,16 @@ generateUUID: function() {
return v.toString(16);
});
}
-
- // Fallback to Math.random() for very old browsers (not cryptographically
secure)
- // Note: This is less secure and should only be used as a last resort
- return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, function(c)
{
- const r = Math.random() * 16 | 0;
- const v = c === 'x' ? r : (r & 0x3 | 0x8);
- return v.toString(16);
- });
+
+ // No cryptographically secure source available: fail closed.
+ // Do NOT fall back to Math.random() — its output is predictable and a
+ // guessable session ID lets an attacker take over a visitor's session.
+ throw new Error(
+ 'Unomi tracker: no cryptographically secure random source available ' +
+ '(neither crypto.randomUUID nor crypto.getRandomValues). This browser
is ' +
+ 'unsupported. Serve the page over HTTPS (the Web Crypto API is
restricted to ' +
+ 'secure contexts) or install a Web Crypto polyfill.'
Review Comment:
`crypto.getRandomValues()` is explicitly available outside secure contexts;
only `crypto.randomUUID()` requires HTTPS. Because the fallback checks
`getRandomValues`, plain HTTP alone does not reach this failure branch on a
current browser. The error should describe missing Web Crypto support rather
than prescribe HTTPS as the fix.
This issue also appears in the following locations of the same file:
- line 1061
- line 1247
- line 1255
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
