diff --git a/.changeset/anonymous-visitor-owned-session.md b/.changeset/anonymous-visitor-owned-session.md new file mode 100644 index 00000000..27a4ff96 --- /dev/null +++ b/.changeset/anonymous-visitor-owned-session.md @@ -0,0 +1,62 @@ +--- +"@smooai/smooth-operator": patch +--- + +fix(server): an anonymous widget visitor who gives an email is no longer locked out of its own session + +Seen in production on smoo.ai, as a total outage of the public chat. +`create_conversation_session { agentId, userEmail }` answered 200, and the very +next `send_message` on the same socket answered `SESSION_NOT_FOUND` for a +session that plainly existed. `userEmail` alone was the trigger — the same +create without it, or with only `userName` or `browserFingerprint`, streamed +fine. + +The widget's pre-chat form collects name + email, so the email lands on the +visitor's own `user` participant, and `may_read_conversation` counts any `user` +participant with a non-blank email as making the conversation **owned**. A +public widget visitor has no verified principal, which on a multi-user +deployment is `UserScope::Denied`, whose arm was `!owned`. So the visitor was +owner-checked against an identity it does not have, and denied the session it +had itself created one frame earlier. The widget's recovery path then created a +fresh session carrying the same email and was denied identically — so real +visitors saw an unbounded retry loop and "We couldn't reach the chat", not a +transient blip. This is th-909995 recurring for the emailful case, against +`server::anonymous_scope`'s own assertion that "it can still create a fresh +session, so the anonymous widget flow keeps working": it could create, but not +use. + +An anonymous connection can never satisfy an ownership check, so it no longer +faces one — narrowly: + +- The exception applies only to a read reached **by id** (new `Reach::ById`), + where the unguessable session/conversation id is the visitor's entire + capability, exactly as it was before scoping shipped. +- `list_conversations` (`Reach::Listing`) stays strict for everyone. Anonymous + listing falls back to the SEED org, which is precisely where widget + conversations pool, so granting the exception there would have leaked + visitors' chats to each other. A negative control caught that before it + shipped. +- Keyed on "no verified principal" (`auth_org.is_none()`, set only by the + tokenless and degraded-token branches of `resolve_ws_access`), not on the + scope — an authenticated principal whose token carries no `email` claim still + fails closed. +- The tenant boundary is untouched, and the fused `SESSION_NOT_FOUND` from the + storage-blip work still leaks nothing: "not found" and "not yours" remain + byte-identical. + +Fixed at the single `may_read_conversation` chokepoint, so `send_message`, +`get_session`, `get_conversation_messages`, `confirm_tool_action`, +`submit_interaction`, `verify_otp` and conversation resume all change together. + +`rust/smooth-operator-server/tests/user_scoping.rs` gains the create-**with**- +`userEmail`-then-send round trip, which is what every existing test missed: they +exercised capture and ownership separately and never the round trip a real +visitor makes. It asserts the session row exists after the create (the failure +was always an authorization denial, never a failed create), then that the send +reaches the turn. Four negative controls ride with it: `SESSION_NOT_FOUND` is +still producible for that same caller on an unknown id, an authenticated +emailless principal still cannot reach an owned session, another authenticated +user still cannot reach the visitor's session or see it in a list, and an +anonymous connection still cannot enumerate authenticated users' conversations. + +No API change. diff --git a/rust/smooth-operator-server/src/handler.rs b/rust/smooth-operator-server/src/handler.rs index 3d286c2f..b12050e6 100644 --- a/rust/smooth-operator-server/src/handler.rs +++ b/rust/smooth-operator-server/src/handler.rs @@ -92,6 +92,23 @@ fn same_org(auth_org: Option<&str>, row_org: &str) -> bool { auth_org.is_none_or(|o| o == row_org) } +/// How the caller arrived at the conversation, which is what decides whether the +/// anonymous exception in [`may_read_conversation`] applies. +/// +/// [`Reach::ById`] means the caller already holds an unguessable id — for a +/// public widget visitor that id IS its capability, the only credential it has. +/// [`Reach::Listing`] means `list_conversations` turned the conversation up by +/// enumeration. Letting an identity-less connection past the ownership axis is +/// defensible for the first and never for the second: it must not be handed ids +/// it could not already name. Listing is org-bounded but falls back to the SEED +/// org for an anonymous caller, which is precisely where widget conversations +/// pool — so widening it there would leak visitors' chats to each other. +#[derive(Clone, Copy, PartialEq, Eq)] +enum Reach { + ById, + Listing, +} + /// Whether this connection may read `conversation_id`. /// /// Two boundaries, outer first. **Tenant**: a connection carrying a verified org @@ -104,8 +121,27 @@ fn same_org(auth_org: Option<&str>, row_org: &str) -> bool { /// anonymous or emailless principal, or predating ownership) stays readable by /// everyone, as it was before scoping shipped; fail-closing it instead denied /// those principals their own sessions (th-909995, and the .NET revert in #309). -/// An owned conversation still needs a matching `User(email)`, so `Denied` — and -/// any other user — is refused. +/// An owned conversation still needs a matching `User(email)`, so an +/// *authenticated* `Denied` — and any other user — is refused. +/// +/// **The anonymous exception (th-anon-owned).** `Denied` covers two very +/// different connections: an authenticated principal whose token carries no +/// `email` claim, and a connection with no verified principal at all — every +/// public widget visitor (see `server::anonymous_scope`). Only the first can +/// meaningfully fail the owner check; the second can NEVER satisfy it, because +/// it has no identity to match with. Applying ownership to it broke the widget +/// outright the moment its pre-chat form started sending `userEmail`: that email +/// lands on the visitor's own `user` participant, makes the conversation +/// `owned`, and the visitor is then locked out of the session it just created — +/// `send_message` answers `SESSION_NOT_FOUND` for a session that plainly exists, +/// and the widget's recovery loop re-creates and is denied identically, forever. +/// This is th-909995 recurring for the emailful case. So an anonymous connection +/// (`auth_org.is_none()` — set only by the tokenless and degraded-token branches +/// of `server::resolve_ws_access`) skips the ownership axis *for a +/// [`Reach::ById`] read only*, and is bounded by session-id unguessability, +/// exactly as it was before scoping shipped. An authenticated emailless +/// principal still fails closed, and [`Reach::Listing`] stays strict for +/// everyone so nothing becomes enumerable. /// /// A storage error is a denial — an owner check that can't be completed must not /// pass. @@ -114,6 +150,7 @@ async fn may_read_conversation( conversation_id: &str, auth_org: Option<&str>, scope: &UserScope, + reach: Reach, ) -> bool { // Nothing to check on either axis — skip the participant read entirely, as // this function did for `Unscoped` before the tenant check existed. @@ -153,7 +190,12 @@ async fn may_read_conversation( .iter() .any(|p| smooth_operator::adapter::is_owner(p, email)) } - UserScope::Denied => !owned, + // Ownerless ⇒ open. Owned ⇒ refused for an authenticated + // emailless principal, and refused for everyone while + // enumerating — but NOT for an anonymous connection that already + // named the id, which has no identity the check could ever be + // satisfied by. + UserScope::Denied => !owned || (auth_org.is_none() && reach == Reach::ById), UserScope::Unscoped => true, // handled above } } @@ -198,11 +240,15 @@ async fn scoped_session( if !same_org(auth_org, &session.organization_id) { return Ok(None); } - Ok( - may_read_conversation(state, &session.conversation_id, auth_org, scope) - .await - .then_some(session), + Ok(may_read_conversation( + state, + &session.conversation_id, + auth_org, + scope, + Reach::ById, ) + .await + .then_some(session)) } /// The `error` event for a session read that storage could not answer. Distinct @@ -510,7 +556,8 @@ async fn handle_create_session( // conversations by their ids alone. let resume = match parsed.get("conversationId").and_then(Value::as_str) { Some(cid) - if !cid.is_empty() && may_read_conversation(state, cid, auth_org, scope).await => + if !cid.is_empty() + && may_read_conversation(state, cid, auth_org, scope, Reach::ById).await => { state.storage.get_conversation(cid).await.ok().flatten() } @@ -953,7 +1000,7 @@ async fn handle_list_conversations( const MSG_CAP: usize = 200; let mut rows: Vec<(i64, Value)> = Vec::new(); for conv in conversations { - if !may_read_conversation(state, &conv.id, auth_org, scope).await { + if !may_read_conversation(state, &conv.id, auth_org, scope, Reach::Listing).await { continue; } let mut query = smooth_operator::adapter::MessageQuery::new(&conv.id, MSG_CAP); @@ -1083,7 +1130,9 @@ async fn handle_rename_conversation( // retitle another user's conversation. Not-ours is reported exactly as // never-existed. match state.storage.get_conversation(conversation_id).await { - Ok(Some(_)) if may_read_conversation(state, conversation_id, auth_org, scope).await => {} + Ok(Some(_)) + if may_read_conversation(state, conversation_id, auth_org, scope, Reach::ById) + .await => {} Ok(_) => { let _ = sink.send(protocol::error( request_id, diff --git a/rust/smooth-operator-server/tests/user_scoping.rs b/rust/smooth-operator-server/tests/user_scoping.rs index 7339c261..6ca4d88f 100644 --- a/rust/smooth-operator-server/tests/user_scoping.rs +++ b/rust/smooth-operator-server/tests/user_scoping.rs @@ -52,15 +52,29 @@ fn scoped(email: &str) -> UserScope { UserScope::User(email.to_string()) } -/// Drive one frame as `scope` and return the first emitted event. +/// Drive one frame as `scope` on an *authenticated* connection (one carrying a +/// verified org) and return the first emitted event. async fn drive(state: &AppState, scope: &UserScope, frame: &Value) -> Value { + drive_as(state, Some(SEED_ORG_ID), scope, frame).await +} + +/// [`drive`], with the connection's `auth_org` spelled out. `None` is the +/// anonymous connection — no verified principal, which is what every public +/// widget visitor is (`server::resolve_ws_access` sets `org_id: None` on both +/// the tokenless and the degraded-token branches). +async fn drive_as( + state: &AppState, + auth_org: Option<&str>, + scope: &UserScope, + frame: &Value, +) -> Value { let (tx, mut rx) = unbounded_channel::(); handler::handle_frame( state, &AccessContext::anonymous(), "conn-test", None, - Some(SEED_ORG_ID), + auth_org, scope, &frame.to_string(), &tx, @@ -90,6 +104,18 @@ async fn create_session( storage: &InMemoryStorageAdapter, scope: &UserScope, claimed_email: Option<&str>, +) -> Created { + create_session_as(state, storage, Some(SEED_ORG_ID), scope, claimed_email).await +} + +/// [`create_session`], with the connection's `auth_org` spelled out (`None` = +/// the anonymous widget connection). +async fn create_session_as( + state: &AppState, + storage: &InMemoryStorageAdapter, + auth_org: Option<&str>, + scope: &UserScope, + claimed_email: Option<&str>, ) -> Created { let mut frame = json!({ "action": "create_conversation_session", @@ -99,7 +125,7 @@ async fn create_session( if let Some(email) = claimed_email { frame["userEmail"] = Value::from(email); } - let ev = drive(state, scope, &frame).await; + let ev = drive_as(state, auth_org, scope, &frame).await; assert_eq!(ev["type"], "immediate_response", "got: {ev}"); let created = Created { @@ -478,13 +504,24 @@ async fn anonymous_create_still_honors_the_frame_email() { /// Drive a frame and collect EVERY event it emits (a spawned turn would emit an /// ack + stream events; a denied one emits exactly one error). async fn drive_all(state: &AppState, scope: &UserScope, frame: &Value) -> Vec { + drive_all_as(state, Some(SEED_ORG_ID), scope, frame).await +} + +/// [`drive_all`], with the connection's `auth_org` spelled out (`None` = the +/// anonymous widget connection). +async fn drive_all_as( + state: &AppState, + auth_org: Option<&str>, + scope: &UserScope, + frame: &Value, +) -> Vec { let (tx, mut rx) = unbounded_channel::(); let handle = handler::handle_frame( state, &AccessContext::anonymous(), "conn-test", None, - Some(SEED_ORG_ID), + auth_org, scope, &frame.to_string(), &tx, @@ -963,3 +1000,231 @@ async fn an_ownerless_conversation_is_reachable_by_every_scope() { .await .contains(&alice.conversation_id)); } + +// ---- the anonymous widget visitor who typed an email (th-anon-owned) ------- +// +// P0 outage, live on smoo.ai: `create_conversation_session { agentId, userEmail }` +// answered 200, and the very next `send_message` on the SAME socket answered +// `SESSION_NOT_FOUND` for a session that existed. Deterministic, and `userEmail` +// alone was the trigger — the same create without it streamed fine. +// +// Mechanism: the widget's pre-chat form collects name + email, so the visitor's +// own `user` participant carries an email, which makes the conversation `owned`. +// The visitor's connection is anonymous, which on a multi-user deployment is +// `UserScope::Denied`, whose arm was `!owned` — so the visitor was owner-checked +// against an identity it does not have and locked out of its own session. Every +// recovery attempt created a session with the same email and was denied the same +// way, which is why real visitors saw an infinite retry loop, not a blip. +// +// Note what the section above tests and this one does not: `ownerless()` creates +// WITHOUT `userEmail`, so it never produced an owned-and-anonymous conversation. +// That is the exact gap — the tests covered capture and ownership separately and +// never the create-then-send round trip a real visitor makes. + +/// One anonymous widget visitor who submitted the pre-chat form: no verified +/// principal (`auth_org: None` ⇒ `UserScope::Denied`), `userEmail` supplied, so +/// the conversation it creates is OWNED and owned by nobody it can prove it is. +async fn anonymous_visitor_with_email() -> (AppState, Arc, Created) { + let storage = Arc::new(InMemoryStorageAdapter::new()); + let state = AppState::new(storage.clone(), base_config()); + let created = create_session_as( + &state, + &storage, + None, + &UserScope::Denied, + Some("visitor@example.com"), + ) + .await; + seed_message(&storage, &created.conversation_id, "visitor's question").await; + (state, storage, created) +} + +/// Assert the conversation really is owned, so the tests below are exercising +/// the ownership arm and not passing because the email never landed. +async fn assert_owned(storage: &InMemoryStorageAdapter, conversation_id: &str) { + let participants = storage + .list_participants_by_conversation(conversation_id) + .await + .expect("list participants"); + assert!( + participants.iter().any(|p| { + p.participant_type == smooth_operator::domain::ParticipantType::User + && p.email.as_deref().is_some_and(|e| !e.trim().is_empty()) + }), + "the visitor's email must be on a user participant, or this test proves nothing: {participants:?}" + ); +} + +#[tokio::test] +async fn anonymous_visitor_with_an_email_can_send_into_the_session_it_created() { + let (state, storage, mine) = anonymous_visitor_with_email().await; + let me = UserScope::Denied; + assert_owned(&storage, &mine.conversation_id).await; + + // (a) vs (b): the session row EXISTS. The outage was never a failed create — + // `scoped_session` loads it fine and then hides it. + assert!( + state.get_session(&mine.session_id).is_some(), + "the session must exist in storage after a create that answered 200" + ); + + // The exact round trip every existing test skipped. + let events = drive_all_as(&state, None, &me, &send_frame(&mine.session_id)).await; + assert_ne!( + events[0]["error"]["code"], "SESSION_NOT_FOUND", + "the visitor must reach the session it just created: {events:?}" + ); + assert_eq!( + events[0]["error"]["code"], "LLM_UNAVAILABLE", + "past the ACL gate, the only thing left is the absent gateway: {events:?}" + ); + + // And the rest of the by-id surface that routes through the same predicate. + let read = drive_as( + &state, + None, + &me, + &json!({ + "action": "get_conversation_messages", + "requestId": "gcm", + "sessionId": mine.session_id, + }), + ) + .await; + assert_eq!(read["type"], "immediate_response", "got: {read}"); + + // ...but NOT listing. The exception is deliberately by-id only: an + // identity-less connection may use an id it already holds and must never be + // handed ids it could not name. Anonymous listing falls back to the SEED org, + // which is exactly where widget conversations pool, so widening it would leak + // visitors' chats to each other. The visitor keeps its own thread through + // resume-by-id below, which is what the widget actually uses. + assert!( + !list_ids_as(&state, None, &me) + .await + .contains(&mine.conversation_id), + "an owned conversation must stay out of an anonymous list" + ); + + // Resume binds back rather than minting a fresh conversation each reload — + // the loop the widget was stuck in. + let resumed = drive_as( + &state, + None, + &me, + &json!({ + "action": "create_conversation_session", + "requestId": "cs", + "agentId": "agent-fixed", + "conversationId": mine.conversation_id, + }), + ) + .await; + assert_eq!( + resumed["data"]["conversationId"], mine.conversation_id, + "got: {resumed}" + ); +} + +#[tokio::test] +async fn the_not_found_response_is_still_reachable_for_an_anonymous_visitor() { + // Negative control for the test above: `assert_ne!(SESSION_NOT_FOUND)` only + // means something if this handler can still produce that code for this + // caller. An id that never existed must still come back not-found. + let (state, _storage, _mine) = anonymous_visitor_with_email().await; + let ghost = uuid::Uuid::new_v4().to_string(); + + let events = drive_all_as(&state, None, &UserScope::Denied, &send_frame(&ghost)).await; + assert_eq!(events.len(), 1, "no turn may be spawned: {events:?}"); + assert_eq!( + events[0]["error"]["code"], "SESSION_NOT_FOUND", + "got: {events:?}" + ); +} + +#[tokio::test] +async fn an_authenticated_emailless_principal_still_cannot_reach_an_owned_session() { + // Negative control: the exception is keyed on "no verified principal", NOT + // on the `Denied` scope. A connection that DID authenticate but carries no + // `email` claim keeps failing closed — that half of th-909995 is unchanged. + let (state, storage, visitor) = anonymous_visitor_with_email().await; + assert_owned(&storage, &visitor.conversation_id).await; + + let read = get_messages(&state, &UserScope::Denied, &visitor.session_id).await; + assert_eq!(read["error"]["code"], "SESSION_NOT_FOUND", "got: {read}"); + + let events = drive_all(&state, &UserScope::Denied, &send_frame(&visitor.session_id)).await; + assert_eq!(events.len(), 1, "no turn may be spawned: {events:?}"); + assert_eq!(events[0]["error"]["code"], "SESSION_NOT_FOUND"); +} + +#[tokio::test] +async fn another_user_still_cannot_reach_the_visitors_owned_session() { + // Negative control: making the conversation reachable by its anonymous + // creator must not make it reachable by a DIFFERENT authenticated user. + let (state, storage, visitor) = anonymous_visitor_with_email().await; + assert_owned(&storage, &visitor.conversation_id).await; + let bob = scoped("bob@example.com"); + + let read = get_messages(&state, &bob, &visitor.session_id).await; + assert_eq!(read["error"]["code"], "SESSION_NOT_FOUND", "got: {read}"); + + let events = drive_all(&state, &bob, &send_frame(&visitor.session_id)).await; + assert_eq!(events.len(), 1, "no turn may be spawned: {events:?}"); + assert_eq!(events[0]["error"]["code"], "SESSION_NOT_FOUND"); + + assert!( + !list_ids(&state, &bob) + .await + .contains(&visitor.conversation_id), + "the visitor's conversation must not appear in another user's list" + ); + + // Nothing landed in the visitor's log. + let messages = storage + .list_messages_by_conversation(smooth_operator::adapter::MessageQuery::new( + &visitor.conversation_id, + 50, + )) + .await + .expect("list messages") + .messages; + assert_eq!( + messages.len(), + 1, + "only the visitor's own message: {messages:?}" + ); +} + +#[tokio::test] +async fn an_anonymous_visitor_still_cannot_reach_an_authenticated_users_session() { + // Negative control: the exception lets an anonymous connection past the + // OWNERSHIP axis, not past ownership *of an authenticated user's* session + // in a way that widens listing. Alice's conversation is owned by a real + // principal; an anonymous connection knowing its id is the residual risk + // called out in the doc comment, but it must not be ENUMERABLE. + let (state, storage, _a, _b) = two_users().await; + let alice_ids = list_ids_as(&state, None, &UserScope::Denied).await; + assert!( + alice_ids.is_empty(), + "an anonymous connection must not enumerate authenticated users' conversations: {alice_ids:?}" + ); + drop(storage); +} + +async fn list_ids_as(state: &AppState, auth_org: Option<&str>, scope: &UserScope) -> Vec { + let ev = drive_as( + state, + auth_org, + scope, + &json!({ "action": "list_conversations", "requestId": "lc" }), + ) + .await; + assert_eq!(ev["type"], "immediate_response", "got: {ev}"); + ev["data"]["conversations"] + .as_array() + .expect("conversations array") + .iter() + .map(|c| c["conversationId"].as_str().expect("id").to_string()) + .collect() +}