This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch UNOMI-975-public-profile-cookie-binding in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 7f92f483ec6159380c4734eff3ab0085e002412a Author: Serge Huber <[email protected]> AuthorDate: Sun Aug 16 09:26:05 2026 +0200 UNOMI-975: Close the anonymous-session hole in the public caller binding The ownership rule the previous commit added only reached named sessions. An anonymous session records no owner at all - PrivacyService#getAnonymousProfile returns a profile with no itemId, so the session's profileId is null - which left two ways past it. The de-anonymising branch rebound such a session to whoever presented its id and saved it, and the invalidateSession guard tested "owner differs", which a null owner passed. Either one handed a visitor's session to a caller that merely knew the id. Ownership is now established positively rather than by absence of a mismatch, through a single isOwnedByCookieBearer() used by both call sites, so an unowned session answers "not yours". Binding an anonymous session back to a named profile is reserved for trusted callers; a public caller leaves it anonymous and the visitor picks up a named session once its client rotates the session id. That also stops the rebinding from retroactively re-attributing every event already recorded in the session to a real profile, which is the outcome anonymous browsing was asked for to begin with. The refusal to bind a body profileId now logs at WARN rather than DEBUG, matching the two sibling refusals: an integration that used to bind a profile this way stops working at that line, and DEBUG left an operator with nothing to find. The anonymous case stays at INFO on purpose - it cannot tell a takeover attempt from the visitor who just turned anonymity off, and it repeats until the session id rotates, so a WARN there would devalue the ones that mean something. Also in the same pass: sanitize the one new log statement that interpolated a request-supplied id raw, drop the null guard on a mandatory @Reference so a missing identity service fails loudly instead of silently downgrading every caller to untrusted, and reorder the profile-switch branch so the security condition reads without a double negative. Tests: the anonymous takeover is pinned at unit level for both the rebinding and invalidateSession routes, plus the trusted caller that must still be allowed through. ContextEndpointBaselineIT gains end-to-end coverage for the anonymous takeover, for /eventcollector - previously only exercised for compatibility, never for the hardening it shares with /context.json - and a runtime assertion that the profile cookie really is issued HttpOnly rather than only that the shipped default says so. The foreign-session test now also asserts the refused id is not echoed and that the rightful owner still holds the session afterwards. Docs: the migration guide gains the "Client-facing hardening (3.1)" section that four pages already linked to but which was never written, covering each field's 3.0 and 3.1 behaviour, the HttpOnly default and the widened cookie validation. Corrected the claim in builtin-event-types and recipes that cross-profile targetId and systemProperties.* writes require a trusted caller - that gate is source-address based and independent of this distinction - and the session rules in how-profile-tracking-works, which stated the guarantee more broadly than the code delivered. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../unomi/itests/ContextEndpointBaselineIT.java | 164 ++++++++++++++++++++- .../org/apache/unomi/itests/ContextServletIT.java | 21 ++- manual/src/main/asciidoc/builtin-event-types.adoc | 9 +- .../main/asciidoc/how-profile-tracking-works.adoc | 6 +- .../asciidoc/migrations/migrate-3.0-to-3.1.adoc | 44 +++++- manual/src/main/asciidoc/recipes.adoc | 8 +- .../apache/unomi/rest/exception/LogSanitizer.java | 8 +- .../rest/service/impl/RestServiceUtilsImpl.java | 88 +++++++++-- .../RestServiceUtilsImplProfileBindingTest.java | 106 ++++++++++++- 9 files changed, 414 insertions(+), 40 deletions(-) diff --git a/itests/src/test/java/org/apache/unomi/itests/ContextEndpointBaselineIT.java b/itests/src/test/java/org/apache/unomi/itests/ContextEndpointBaselineIT.java index 2c51a5b99..e8c917109 100644 --- a/itests/src/test/java/org/apache/unomi/itests/ContextEndpointBaselineIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/ContextEndpointBaselineIT.java @@ -42,6 +42,7 @@ import java.util.Objects; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; /** @@ -232,7 +233,14 @@ public class ContextEndpointBaselineIT extends BaseIT { } } - /** A public caller must not be able to adopt a session belonging to someone else. */ + /** + * A public caller must not be able to adopt a session belonging to someone else. + * <p> + * Three things have to hold, not just the first: the caller must not end up on the other's + * profile, the refused id must not be echoed back (or the client keeps replaying it), and the + * other visitor must still hold the session afterwards - refusing by handing the session to the + * caller anyway would satisfy the first assertion alone. + */ @Test public void hardened_publicCallerCannotAdoptAForeignSession() throws Exception { String otherSessionId = "baseline-other-sess-" + System.currentTimeMillis(); @@ -249,6 +257,160 @@ public class ContextEndpointBaselineIT extends BaseIT { assertEquals(200, attempt.getStatusCode()); assertTrue("the untrusted caller must not end up on the other's profile", !otherProfileId.equals(attempt.getContextResponse().getProfileId())); + assertNull("a refused session id must not be echoed back to the caller", + attempt.getContextResponse().getSessionId()); + + // The rightful owner comes back: the session must still be theirs and still be accepted. + TestUtils.RequestResponse ownerAgain = postContextJson(newContextRequest(otherSessionId), + other.getCookieHeaderValue(), otherSessionId); + assertEquals("the rightful owner must keep its profile", otherProfileId, + ownerAgain.getContextResponse().getProfileId()); + assertEquals("and must not have lost the session to the caller that was refused", + otherSessionId, ownerAgain.getContextResponse().getSessionId()); + } + + /** + * The same takeover, aimed at an <em>anonymous</em> session. + * <p> + * An anonymous session records no owner at all - {@code PrivacyService#getAnonymousProfile} + * returns a profile with no id, so the session's profileId is null - which left the ownership + * rule with nothing to compare the cookie against. Presenting the id was therefore enough to have + * the session rebound to the presenter's own profile and saved. The session must stay anonymous. + * <p> + * The check is made through the rightful owner's next request rather than by reading the session + * back: if the takeover had happened the session would now carry a real, foreign profile, and the + * owner would be refused by the ownership rule that covers named sessions. + */ + @Test + public void hardened_publicCallerCannotTakeOverAnAnonymousSession() throws Exception { + String victimSessionId = "baseline-anon-sess-" + System.currentTimeMillis(); + TestUtils.RequestResponse victim = postContextJson(newContextRequest(victimSessionId), null, victimSessionId); + String victimProfileId = victim.getContextResponse().getProfileId(); + + try { + // The visitor asks for anonymous browsing, then makes one request so the session picks the + // anonymous profile up. + privacyService.setRequireAnonymousBrowsing(victimProfileId, true, TEST_SCOPE); + keepTrying("Profile should require anonymous browsing", + () -> privacyService.isRequireAnonymousBrowsing(victimProfileId), + Boolean.TRUE::equals, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); + postContextJson(newContextRequest(victimSessionId), victim.getCookieHeaderValue(), victimSessionId); + keepTrying("Session should have become anonymous", + () -> profileService.loadSession(victimSessionId), + session -> session != null && session.getProfileId() == null, + DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); + + // Someone else presents that session id with their own cookie. + String attackerSessionId = "baseline-anon-attacker-" + System.currentTimeMillis(); + TestUtils.RequestResponse attacker = postContextJson(newContextRequest(attackerSessionId), null, attackerSessionId); + String attackerProfileId = attacker.getContextResponse().getProfileId(); + postContextJson(newContextRequest(victimSessionId), attacker.getCookieHeaderValue(), victimSessionId); + + keepTrying("The anonymous session must not be reassigned to the caller", + () -> profileService.loadSession(victimSessionId), + session -> session != null && !attackerProfileId.equals(session.getProfileId()), + DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); + + // And the rightful owner is still served by it. + TestUtils.RequestResponse ownerAgain = postContextJson(newContextRequest(victimSessionId), + victim.getCookieHeaderValue(), victimSessionId); + assertEquals("the anonymous visitor must keep its own profile", victimProfileId, + ownerAgain.getContextResponse().getProfileId()); + assertEquals("and must keep its session", victimSessionId, + ownerAgain.getContextResponse().getSessionId()); + } finally { + privacyService.setRequireAnonymousBrowsing(victimProfileId, false, TEST_SCOPE); + } + } + + /** + * The same body-profileId claim, aimed at {@code /eventcollector}. + * <p> + * Both endpoints share {@code initEventsRequest}, so this passes today - which is exactly why it + * is worth pinning. The collector reads its {@code sessionId}/{@code profileId} from a different + * request model and even falls back to a query parameter, so a change on that side could route + * around the binding rule without any context.json test noticing. + * <p> + * The collector's response body carries no profile id, so the assertion is on the profile cookie + * the request is answered with: that is the profile the server decided the caller is. + */ + @Test + public void hardened_eventCollectorIgnoresPublicBodyProfileId() throws Exception { + String otherProfileId = "baseline-ec-other-" + System.currentTimeMillis(); + Profile other = new Profile(otherProfileId); + profileService.save(other); + keepTrying("Other profile should be saved", () -> profileService.load(otherProfileId), + Objects::nonNull, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); + + try { + String sessionId = "baseline-ec-claim-" + System.currentTimeMillis(); + EventsCollectorRequest claim = newEventsRequest(sessionId); + claim.setProfileId(otherProfileId); + + HttpPost post = new HttpPost(getFullUrl(EVENT_COLLECTOR_URL)); + post.addHeader(UNOMI_API_KEY_HTTP_HEADER_KEY, testPublicKeyValue); + post.setEntity(new StringEntity(getObjectMapper().writeValueAsString(claim), ContentType.APPLICATION_JSON)); + + try (CloseableHttpResponse response = httpClient.execute(post)) { + String setCookie = response.getFirstHeader("Set-Cookie") == null + ? "" : response.getFirstHeader("Set-Cookie").getValue(); + assertFalse("the eventcollector must not bind a public caller to a body profileId, got: " + setCookie, + setCookie.contains(otherProfileId)); + } + } finally { + profileService.delete(otherProfileId, false); + } + } + + /** And the session half of the same rule, again through {@code /eventcollector}. */ + @Test + public void hardened_eventCollectorCannotAdoptAForeignSession() throws Exception { + String otherSessionId = "baseline-ec-sess-" + System.currentTimeMillis(); + TestUtils.RequestResponse other = postContextJson(newContextRequest(otherSessionId), null, otherSessionId); + String otherProfileId = other.getContextResponse().getProfileId(); + + String callerSessionId = "baseline-ec-caller-" + System.currentTimeMillis(); + TestUtils.RequestResponse caller = postContextJson(newContextRequest(callerSessionId), null, callerSessionId); + + HttpPost post = new HttpPost(getFullUrl(EVENT_COLLECTOR_URL)); + post.addHeader(UNOMI_API_KEY_HTTP_HEADER_KEY, testPublicKeyValue); + post.addHeader("Cookie", caller.getCookieHeaderValue()); + post.setEntity(new StringEntity(getObjectMapper().writeValueAsString(newEventsRequest(otherSessionId)), + ContentType.APPLICATION_JSON)); + + try (CloseableHttpResponse response = httpClient.execute(post)) { + String setCookie = response.getFirstHeader("Set-Cookie") == null + ? "" : response.getFirstHeader("Set-Cookie").getValue(); + assertFalse("presenting a foreign session id must not move the caller onto its owner's profile, got: " + + setCookie, setCookie.contains(otherProfileId)); + } + + // The owner still has the session. + TestUtils.RequestResponse ownerAgain = postContextJson(newContextRequest(otherSessionId), + other.getCookieHeaderValue(), otherSessionId); + assertEquals("the rightful owner must keep its profile", otherProfileId, + ownerAgain.getContextResponse().getProfileId()); + assertEquals("and its session", otherSessionId, ownerAgain.getContextResponse().getSessionId()); + } + + /** + * The profile cookie must actually be issued {@code HttpOnly} by a running server. + * <p> + * The shipped defaults are checked separately as configuration text; this asserts the value that + * survives the whole path from that default through {@code WebConfig} and + * {@code ConfigSharingService} into the {@code Set-Cookie} header. Binding a public caller to the + * profile its cookie names only means anything while page script cannot read that cookie, so the + * flag is part of the security model rather than a preference. + */ + @Test + public void hardened_profileCookieIsHttpOnly() throws Exception { + String sessionId = "baseline-httponly-" + System.currentTimeMillis(); + TestUtils.RequestResponse response = postContextJson(newContextRequest(sessionId), null, sessionId); + + String setCookie = response.getCookieHeaderValue(); + assertNotNull("a first visit must be issued the profile cookie", setCookie); + assertTrue("the profile cookie must be HttpOnly, got: " + setCookie, + setCookie.toLowerCase().contains("httponly")); } // ------------------------------------------------------------------ helpers diff --git a/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java b/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java index f68904a6f..36ce3393d 100644 --- a/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java @@ -473,11 +473,16 @@ public class ContextServletIT extends BaseIT { } /** - * End-to-end guard for anonymous browsing. The session-ownership check added for public callers - * deliberately skips anonymous profiles today; any future tightening of it must not detach the - * session of a visitor who is legitimately browsing anonymously. That failure would be invisible - * at unit level in the endpoint wiring, hence this IT: it asserts the visitor's own session id is - * still echoed back (a refused session is suppressed from the response) after anonymisation. + * End-to-end guard for anonymous browsing: a visitor who is legitimately browsing anonymously must + * never have their own session detached. That failure would be invisible at unit level in the + * endpoint wiring, hence this IT — it asserts the visitor's own session id is still echoed back, + * since a refused session is suppressed from the response. + * <p> + * The de-anonymising step at the end is the one place where the session-ownership rule cannot be + * applied: an anonymous session records no owner, so nothing can be matched against the cookie. + * A public caller therefore keeps an anonymous session instead of having it bound back to a named + * profile. What must hold either way is that the visitor is still served and still holds the + * session; being refused here would strand every visitor who turns anonymity off. */ @Test public void testAnonymousBrowsing_visitorKeepsItsOwnSession() throws Exception { @@ -515,7 +520,9 @@ public class ContextServletIT extends BaseIT { anonymousResponse.getContextResponse().getSessionId()); assertEquals(sessionId, anonymousResponse.getContextResponse().getSessionId()); - // And turning anonymity back off must keep working too (the de-anonymising branch). + // And turning anonymity back off must keep serving the visitor. For a public caller the + // session stays anonymous rather than being rebound (see this test's javadoc); the profile + // it is served as is still its own. privacyService.setRequireAnonymousBrowsing(profileId, false, TEST_SCOPE); keepTrying("Anonymous browsing should be disabled again", () -> privacyService.isRequireAnonymousBrowsing(profileId), @@ -532,6 +539,8 @@ public class ContextServletIT extends BaseIT { assertEquals(200, deanonymisedResponse.getStatusCode()); assertNotNull("Leaving anonymous browsing must not refuse the visitor's own session", deanonymisedResponse.getContextResponse().getSessionId()); + assertEquals("and must keep serving it under the visitor's own profile", profileId, + deanonymisedResponse.getContextResponse().getProfileId()); } finally { privacyService.setRequireAnonymousBrowsing(profileId, false, TEST_SCOPE); } diff --git a/manual/src/main/asciidoc/builtin-event-types.adoc b/manual/src/main/asciidoc/builtin-event-types.adoc index 92c5bf6a6..8a4402a31 100644 --- a/manual/src/main/asciidoc/builtin-event-types.adoc +++ b/manual/src/main/asciidoc/builtin-event-types.adoc @@ -242,9 +242,12 @@ image::form-event-type.png[] This event is usually used by user interfaces that make it possible to modify profile properties, for example a form where a user can edit his profile properties, or a management UI to modify. -Note that this event type is a protected event type that is only accepted from configured third-party servers -(or equivalently from a trusted private-key / administrator caller in 3.1). Cross-profile updates and -`systemProperties.*` writes also require a trusted caller — see <<_client_facing_hardening_3_1,client-facing hardening>>. +Note that this event type is a protected event type that is only accepted from configured third-party servers, which are +authorized by source IP address (and in V2 compatibility mode by an `X-Unomi-Peer` key valid for that address). That +gate is separate from the public-versus-trusted caller distinction that governs which profile and session a request may +bind to — see <<_client_facing_hardening_3_1,client-facing hardening>>. Once the event is accepted, its `targetId` +cross-profile update and its `systemProperties.*` writes are not subject to a further caller check, so the address +allowlist is what protects them. ===== Structure definition diff --git a/manual/src/main/asciidoc/how-profile-tracking-works.adoc b/manual/src/main/asciidoc/how-profile-tracking-works.adoc index 3b7c35f4c..c76dfafa4 100644 --- a/manual/src/main/asciidoc/how-profile-tracking-works.adoc +++ b/manual/src/main/asciidoc/how-profile-tracking-works.adoc @@ -241,14 +241,16 @@ Apache Unomi attempts to identify the visitor's profile through the following pr * **Public callers** (public API key / unauthenticated context): the profile cookie is the **only** profile bearer. A body or query `profileId` is **ignored** (even when no cookie is present). * **Trusted callers** (system administrator or tenant private-key / `TENANT_ADMINISTRATOR`): an explicit body/query `profileId` is honored and may differ from the cookie. * Cookie name defaults to `context-profile-id` (configurable via `org.apache.unomi.profile.cookie.name`). - * Cookie values are validated against a JSON schema — invalid values (for example containing script tags) cause a `400 Bad Request`. + * Cookie values are validated against a JSON schema — invalid values (for example containing script tags) cause a `400 Bad Request`. Since 3.1 the cookie is read on every request, so this also applies when the request supplies an explicit `profileId`; before, a malformed cookie went unnoticed on those requests. * The resolved profile ID is used to attempt loading the profile from the database. 2. **Session Profile Override** (if session exists): * If a session is found (see Step 2) and its profile differs from the request profile, Unomi switches to the session profile **only when**: ** the profile cookie already matches the session owner, **or** ** the caller is trusted (and is not keeping an explicit trusted body `profileId` override). - * Otherwise the session is **detached** for this request (Unomi does not adopt a foreign session profile for a public caller). + * Otherwise the session is **detached** for this request (Unomi does not adopt a foreign session profile for a public caller), and the supplied session id is **not echoed back** in the response — a client that saw its own id returned would keep replaying an id the server did not accept. + * **Anonymous sessions** are a special case. Anonymity removes the owner from the session (`getAnonymousProfile` returns a profile with no ID, so the session's `profileId` is `null`), which leaves the rule above nothing to compare the cookie against. Binding such a session back to a named profile — what happens when a visitor turns anonymous browsing off — is therefore reserved for **trusted** callers. For a public caller the session stays anonymous, and the visitor gets a named sessi [...] + * `invalidateSession=true` re-creates the session under the supplied id, so it observes the same rule: a public caller may only invalidate a session its own cookie owns, and a session with no recorded owner cannot be invalidated this way. 3. **Profile Creation**: If no profile ID is found or the profile doesn't exist: * If a profile ID was provided (from cookie, or from a trusted body/query `profileId`) but doesn't exist in the database, creates a new profile with that ID diff --git a/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc b/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc index a041bf2d2..59350e8ec 100644 --- a/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc +++ b/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc @@ -180,7 +180,49 @@ public void updateKeys(String publicKey, String privateKey) { ==== No API Contract Changes -All API endpoints remain the same between 3.0 and 3.1. The only differences are in the authentication mechanism and tenant resolution. Request/response payloads are unchanged. +All API endpoints remain the same between 3.0 and 3.1, and request/response payloads are unchanged in shape. The differences are in the authentication mechanism, tenant resolution, and which of the identity fields a public caller sends to `/context.json` and `/eventcollector` the server acts on — see <<_client_facing_hardening_3_1,Client-facing hardening (3.1)>> below. + +[#_client_facing_hardening_3_1] +==== Client-facing hardening (3.1) + +`/cxs/context.json` and `/cxs/eventcollector` are reachable without authentication, or with a tenant *public* API key. In 3.1 the identity fields those endpoints accept are treated as claims to be checked rather than as instructions. + +A caller is **trusted** when it holds a tenant private key or authenticates as a system administrator (`ROLE_UNOMI_ADMIN` or `ROLE_UNOMI_TENANT_ADMIN`). A tenant *public* API key is **not** trusted: it identifies the tenant, not the visitor. + +.What changed for public callers +[cols="1,2,2", options="header"] +|=== +| Field | 3.0 | 3.1 + +| Body/query `profileId` +| Selected the profile. +| Ignored. The profile cookie (`context-profile-id` by default) is the only profile bearer, including when no cookie is present. Trusted callers may still pass it, and it may differ from the cookie. + +| `sessionId` +| Any existing session was continued, and was rebound to the caller's profile when the two differed. +| Continued only when the profile cookie already owns that session. Otherwise the session is *detached* for the request rather than rebound, and its id is not echoed back in the response. + +| `sessionId` naming an *anonymous* session +| Rebound to the caller's profile when the caller was not itself browsing anonymously. +| Left anonymous. An anonymous session records no owner, so there is nothing to check the cookie against; only a trusted caller may bind it to a named profile. A visitor who turns anonymous browsing off keeps an anonymous session until their client uses a new session id. + +| `invalidateSession=true` +| Re-created the session under the supplied id. +| Allowed only when the profile cookie owns that session. A session with no recorded owner — an anonymous one — cannot be re-created this way by a public caller. +|=== + +Anonymous browsing, personas and profile overrides are otherwise unchanged, and a caller with no cookie is still issued a profile: that is how tracking has always started. + +===== Two behaviour changes to check before upgrading + +* **The profile cookie now defaults to `HttpOnly`.** The model above holds only while page script cannot read the cookie, so the default is part of the design rather than a preference. If your page code reads `document.cookie` for the profile id, read `profileId` from the JSON response instead. Setting `org.apache.unomi.profile.cookie.httpOnly=false` (env `UNOMI_PROFILE_COOKIE_HTTPONLY`) restores the old default, at the cost of that guarantee. +* **A malformed profile cookie is rejected on more requests than before.** The cookie is validated against a JSON schema and an invalid value answers `400`. In 3.0 the cookie was only read when no `profileId` was supplied, so a request carrying both an explicit `profileId` and a malformed cookie was still served; in 3.1 it fails. This mainly affects server-side callers that forward a browser's raw `Cookie` header. + +===== Migrating a client + +* Browser trackers need no change: the browser sends the cookie automatically, and the response still carries `profileId` and `sessionId`. +* A backend that named a profile through body `profileId` while authenticating with a *public* key must switch to a tenant private key, or to system administrator credentials. Until it does, the server logs a `WARN` naming the ignored `profileId` on every such request. +* A client that supplied a session id it did not own now finds `sessionId` absent from the response. Treat that as "start a new session" and generate a fresh id rather than replaying the old one. === Migrating your existing data diff --git a/manual/src/main/asciidoc/recipes.adoc b/manual/src/main/asciidoc/recipes.adoc index b40361ea3..84b74a1ab 100644 --- a/manual/src/main/asciidoc/recipes.adoc +++ b/manual/src/main/asciidoc/recipes.adoc @@ -134,8 +134,12 @@ event data to the profile. This is simpler than it sounds, as usually all it req defining the corresponding JSON schema and you're ready to update profiles using events. - Use the protected built-in "updateProperties" event. This event is designed to be used for administrative purposes -only. Cross-profile updates and `systemProperties.*` writes require a **trusted** caller (tenant private key or system -administrator). Prefer custom events for public visitors. Again, prefer the custom events solution because as this is a +only. Being a protected event type, it is only accepted from an authorized source address — a configured third-party +server, or in V2 compatibility mode an `X-Unomi-Peer` key valid for that source address. Note that this is an +address-based gate: it is independent of the public-versus-trusted caller distinction that governs profile and session +binding (see <<_client_facing_hardening_3_1,client-facing hardening>>), and once the event is accepted its `targetId` +and `systemProperties.*` writes are not further restricted. Prefer custom events for public visitors. Again, prefer the +custom events solution because as this is a protected event it will require sending trusted credentials, and as Unomi only supports a single key for the moment it could be problematic if the key is intercepted. But at least by using an event you will get the benefits of auditing and historical property modification tracing (see <<_request_tracing_explain,request tracing>>). 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 5747a9722..ce13731eb 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 @@ -49,9 +49,11 @@ final class LogSanitizer { * {@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. + * Delegates to {@link org.apache.unomi.api.utils.LogSanitizer}, which holds the one + * implementation of this filter. It lives in {@code api} so that bundles outside {@code rest} + * which log request-derived values can reuse it instead of growing a second copy; as of this + * change its only callers are in {@code rest}. This class keeps 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. 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 0b67ce71d..7ed3a0987 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 @@ -138,6 +138,11 @@ public class RestServiceUtilsImpl implements RestServiceUtils { } final String requestedBodyProfileId = profileId; + // Read unconditionally, where before the cookie was only consulted when no body profileId was + // supplied. Every check below needs to know the cookie bearer even when the body names someone + // else - that mismatch is the thing being guarded against. Note the widened side effect: + // getProfileIdCookieValue rejects a schema-invalid cookie with a 400, so a request carrying + // both an explicit profileId and a malformed cookie now fails where it used to be served. 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. @@ -148,8 +153,11 @@ public class RestServiceUtilsImpl implements RestServiceUtils { if (!trustedCaller) { // Public callers: the cookie is the only profile bearer, so a body profileId never selects // the profile — including when no cookie is present, where there is nothing to match against. + // Logged at WARN like the two other refusals below, and for the same reason: this is an + // identity claim being rejected. An integration that used to bind a profile this way + // stops working at this line, and DEBUG would leave an operator with nothing to find. if (requestedBodyProfileId != null && !requestedBodyProfileId.equals(cookieProfileIdAtRequest)) { - LOGGER.debug("Ignoring body profileId {} from public caller (cookie profileId is {})", + LOGGER.warn("Ignoring body profileId {} from public caller (cookie profileId is {})", LogSanitizer.forLogging(requestedBodyProfileId), LogSanitizer.forLogging(cookieProfileIdAtRequest)); } profileId = cookieProfileIdAtRequest; @@ -174,8 +182,13 @@ public class RestServiceUtilsImpl implements RestServiceUtils { // to it: a public caller may only invalidate a session its own cookie already owns. if (invalidateSession && !trustedCaller && StringUtils.isNotBlank(effectiveSessionId)) { Session existingSession = profileService.loadSession(effectiveSessionId); - if (existingSession != null && existingSession.getProfileId() != null - && !existingSession.getProfileId().equals(cookieProfileIdAtRequest)) { + // An unknown session id is fine - there is nothing to take over, and the request goes on to + // create one. What must not pass is an existing session the cookie bearer cannot be shown to + // own, which includes a session with no owner recorded at all: an anonymous session has a + // null profileId, so an "owner differs" test would wave it through and re-create it under + // the caller's profile. + if (existingSession != null + && !isOwnedByCookieBearer(existingSession.getProfileId(), 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()), @@ -222,9 +235,11 @@ public class RestServiceUtilsImpl implements RestServiceUtils { // 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)) { + boolean cookieOwnsSession = isOwnedByCookieBearer(sessionProfile.getItemId(), cookieProfileIdAtRequest); + if (trustedExplicitProfileOverride) { + LOGGER.debug("Keeping trusted body profileId {} despite session/cookie mismatch", + LogSanitizer.forLogging(eventsRequestContext.getProfile().getItemId())); + } else if (cookieOwnsSession || trustedCaller) { Profile sessionProfileWithId = profileService.load(sessionProfile.getItemId()); if (sessionProfileWithId != null) { eventsRequestContext.setProfile(sessionProfileWithId); @@ -233,9 +248,6 @@ public class RestServiceUtilsImpl implements RestServiceUtils { 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("Refusing to switch profile from {} to session profile {} without matching cookie bearer; " + "detaching session {} for this request", @@ -261,10 +273,36 @@ public class RestServiceUtilsImpl implements RestServiceUtils { 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); + // User does not want to browse anonymously anymore, update the sessionProfile to real profile. + // + // Only a trusted caller may do this. An anonymous session records no owner at + // all - PrivacyService#getAnonymousProfile returns a profile with no itemId, so + // the session's profileId is null - which leaves the ownership rule enforced + // everywhere else in this method with nothing to check the cookie against. The + // rebinding is a write, so honouring it for a public caller would hand the + // session to whoever presents its id, which is exactly what that rule exists to + // prevent. A public caller therefore leaves the session anonymous; the visitor + // picks up a named session again once the client rotates its session id. + // + // Refusing also avoids retroactively re-attributing every event already recorded + // in that session to a real profile, which is the outcome anonymous browsing was + // asked for in the first place. + if (trustedCaller) { + sessionProfile = eventsRequestContext.getProfile(); + eventsRequestContext.getSession().setProfile(sessionProfile); + eventsRequestContext.addChanges(EventService.SESSION_UPDATED); + } else { + // INFO rather than WARN, unlike the refusals above: those fire on a claim + // that is demonstrably wrong, while this one cannot tell a takeover attempt + // from the visitor who legitimately just turned anonymity off - that is the + // whole difficulty. It also repeats on every request until the client picks + // a new session id, so a WARN here would train operators to ignore the ones + // that do mean something. + LOGGER.info("Not rebinding anonymous session {} to profile {} for a public caller: " + + "an anonymous session has no recorded owner to match the cookie bearer against", + LogSanitizer.forLogging(effectiveSessionId), + LogSanitizer.forLogging(eventsRequestContext.getProfile().getItemId())); + } } 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(); @@ -474,9 +512,31 @@ public class RestServiceUtilsImpl implements RestServiceUtils { * <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. + * <p> + * {@code securityService} is a mandatory static {@code @Reference}, so this component is never + * active without it. Deliberately not null-guarded: defaulting a missing identity service to + * "untrusted" would silently strip every trusted integration of its binding rights with nothing + * in the log to explain it, which is far harder to diagnose than the NPE that says so outright. */ private boolean isTrustedProfileCaller() { - return securityService != null && securityService.hasSystemAccess(); + return securityService.hasSystemAccess(); + } + + /** + * Whether the profile named by the caller's cookie is the recorded owner of a session. + * <p> + * The one place the ownership rule is written down, so the two call sites that need it cannot + * drift apart. Ownership has to be positively established, so an absent cookie and an unowned + * session both answer {@code false}. A session with a {@code null} owner is not "owned by + * nobody, so anyone may have it" - it is a session whose owner cannot be checked, which for this + * purpose is the same answer. + * + * @param ownerProfileId the profile id recorded as owning the session, may be {@code null} + * @param cookieProfileIdAtRequest the profile id carried by the caller's cookie, may be {@code null} + * @return {@code true} only when the cookie bearer demonstrably owns the session + */ + private boolean isOwnedByCookieBearer(String ownerProfileId, String cookieProfileIdAtRequest) { + return cookieProfileIdAtRequest != null && cookieProfileIdAtRequest.equals(ownerProfileId); } /** 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 index 86bebdfea..2a20a245f 100644 --- 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 @@ -281,9 +281,11 @@ class RestServiceUtilsImplProfileBindingTest { // --------------------------------------------------------------------------------------- // 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. + // pinned here, because the session-ownership check cannot reach an anonymous session: such a + // session records no owner (getAnonymousProfile returns a profile with no itemId), so there is + // nothing to match the cookie bearer against. Branch 3 is therefore the one place where caller + // trust decides the outcome; the other three must stay untouched by that rule so a legitimately + // anonymous visitor is not detached on every request. // --------------------------------------------------------------------------------------- /** @@ -341,13 +343,13 @@ class RestServiceUtilsImplProfileBindingTest { } /** - * 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 untrusted caller reaches with a reused anonymous session id — so it must keep - * working for the legitimate case while the fix is designed. + * Branch 3, trusted caller: rebinding an anonymous session to a real profile is a write that + * assigns an ownerless session to a named owner, so it stays available to a caller whose + * authority to name a profile has been established. */ @Test - void anonymousBrowsing_leaving_rebindsSessionToTheRealProfile() { + void anonymousBrowsing_leaving_rebindsSessionToTheRealProfileForTrustedCaller() { + when(securityService.hasSystemAccess()).thenReturn(true); Profile cookieProfile = new Profile("cookie-profile"); Session session = new Session("anon-sess", anonymous(), new Date(), "systemscope"); @@ -364,6 +366,66 @@ class RestServiceUtilsImplProfileBindingTest { assertNotNull(ctx.getSession()); assertEquals("cookie-profile", ctx.getSession().getProfile().getItemId(), "leaving anonymity must bind the session back to the visitor's real profile"); + assertTrue((ctx.getChanges() & EventService.SESSION_UPDATED) != 0, + "the session change must be flagged so it is persisted"); + } + + /** + * Branch 3, public caller: the same rebinding is refused. + * <p> + * This is the anonymous-session takeover. An anonymous session carries no owner, so presenting + * its id was enough to have it rebound to the presenter's own profile and saved — the ownership + * rule enforced for named sessions had nothing to bite on. The session must stay anonymous and + * unchanged, so nothing is persisted and the real visitor keeps it. + */ + @Test + void anonymousBrowsing_leaving_isRefusedForPublicCaller() { + Profile callerProfile = new Profile("caller-profile"); + Session session = new Session("anon-sess", anonymous(), new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "caller-profile")}); + when(profileService.load("caller-profile")).thenReturn(callerProfile); + when(profileService.loadSession("anon-sess")).thenReturn(session); + when(privacyService.isRequireAnonymousBrowsing(callerProfile)).thenReturn(false); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "anon-sess", null, null, + false, false, request, response, new Date()); + + assertNotNull(ctx.getSession()); + assertTrue(ctx.getSession().getProfile().isAnonymousProfile(), + "a public caller must not take over an anonymous session by presenting its id"); + assertNull(ctx.getSession().getProfileId(), + "the session must keep no owner rather than being assigned to the caller"); + assertEquals(0, ctx.getChanges() & EventService.SESSION_UPDATED, + "nothing changed, so the session must not be persisted under the caller's profile"); + assertEquals("caller-profile", ctx.getProfile().getItemId(), + "the caller still acts as its own cookie profile"); + } + + /** + * The same takeover attempted through {@code invalidateSession}, which re-creates the session + * under the supplied id. The ownership guard for that path keys off the session's profileId, + * which is null for an anonymous session, so it used to wave this through. + */ + @Test + void anonymousBrowsing_invalidateSession_cannotTakeOverAnonymousSessionForPublicCaller() { + Profile callerProfile = new Profile("caller-profile"); + Session session = new Session("anon-sess", anonymous(), new Date(), "systemscope"); + + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(COOKIE_NAME, "caller-profile")}); + when(profileService.load("caller-profile")).thenReturn(callerProfile); + when(profileService.loadSession("anon-sess")).thenReturn(session); + when(privacyService.isRequireAnonymousBrowsing(callerProfile)).thenReturn(false); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "anon-sess", null, null, + false, true, request, response, new Date()); + + assertTrue(ctx.isSessionRefused(), + "a public caller must not recreate an anonymous session it cannot be shown to own"); + assertNull(ctx.getSession(), + "the refused session must be detached rather than reissued under the caller's profile"); } /** Branch 4: the ordinary non-anonymous case — the session is bound to the caller's profile. */ @@ -485,6 +547,34 @@ class RestServiceUtilsImplProfileBindingTest { assertFalse("cookie-profile".equals(ctx.getProfile().getItemId())); } + /** + * invalidateProfile together with a session the cookie owns: the session's profile wins and the + * visitor gets their old profile back, so the invalidation does not take effect. + * <p> + * Pre-existing behaviour, pinned rather than changed - the session-adoption branch runs after the + * new profile is minted and overwrites it. It is also the only route by which an untrusted caller + * reaches {@code cookieOwnsSession == true}, so without this test that condition looks dead and + * would be an easy thing to "simplify" away. Callers that mean to start over must invalidate the + * session too. + */ + @Test + void invalidateProfile_withOwnedSession_isUndoneBySessionAdoption() { + Profile cookieProfile = new Profile("cookie-profile"); + Session session = 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); + when(profileService.loadSession("own-sess")).thenReturn(session); + + EventsRequestContext ctx = restServiceUtils.initEventsRequest( + "systemscope", "own-sess", null, null, + true, false, request, response, new Date()); + + assertFalse(ctx.isSessionRefused(), "the caller's own session must not be refused"); + assertEquals("cookie-profile", ctx.getProfile().getItemId(), + "the session it owns hands the visitor its previous profile back"); + } + /** * 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
