Anonymous widget visitors were locked out of their own sessions - #548
Open
brentrager wants to merge 2 commits into
Open
Anonymous widget visitors were locked out of their own sessions#548brentrager wants to merge 2 commits into
brentrager wants to merge 2 commits into
Conversation
P0, 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. userEmail alone was the
trigger; the same create without it streamed fine.
The visitor's email lands on its own `user` participant, which makes 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 locked out of the session it had just created. Its recovery
path created another session carrying the same email and was denied
identically, so real visitors saw an infinite retry loop rather than a
blip. th-909995 recurring for the emailful case, which
anonymous_scope()'s own "the anonymous widget flow keeps working"
comment assumed could not happen.
An anonymous connection can never satisfy an ownership check, so it now
skips that axis — but only for a read it reached BY ID, where the
unguessable id is the visitor's whole capability. Listing stays strict
for everyone: anonymous listing falls back to the SEED org, which is
where widget conversations pool, so widening it there would have leaked
visitors' chats to each other. A negative control caught exactly that
before it shipped. An authenticated principal with no email claim still
fails closed, the tenant boundary is untouched, and the fused
SESSION_NOT_FOUND from #545 still leaks nothing.
Fixed at the one may_read_conversation chokepoint, so all of
send_message, get_session, get_conversation_messages,
confirm_tool_action, submit_interaction, verify_otp and resume are
covered by the one change.
Tests: the create-WITH-userEmail-then-send round trip every existing
test skipped (they covered capture and ownership separately, never the
round trip), plus four negative controls.
A negative control caught the first cut widening list_conversations: an anonymous connection could enumerate the SEED org's owned conversations, which is exactly where widget conversations pool, so it would have leaked visitors' chats to each other. The exception is now Reach::ById only.
🦋 Changeset detectedLatest commit: 68d91c6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The outage
Live on smoo.ai: the public chat was broken for every real visitor. The widget's pre-chat form collects name + email, so every real visitor sent
userEmail, and every real visitor's session was dead on arrival.Deterministic repro against prod
wss://ai.smoo.ai/wswithOrigin: https://smoo.ai(2/2 each way, credit to the reporting agent):{ agentId }send_messagestreams fine{ agentId, userName }{ agentId, browserFingerprint }{ agentId, userEmail }send_message→SESSION_NOT_FOUND{ agentId, userName, userEmail }SESSION_NOT_FOUNDuserEmailwas the trigger and nothing else. The widget correctly detectedSESSION_NOT_FOUND, recovered by creating a fresh session and re-sending with the new id — and the new session was dead too. Infinite loop; the visitor saw "We couldn't reach the chat."Mechanism
The session row always existed. This was never a failed create — it was an authorization denial wearing the not-found response, because
scoped_sessiondeliberately fuses "does not exist" with "is not yours" (#545, anti-enumeration).server::anonymous_scope— on a multi-user deployment a connection with no verified principal (every public widget visitor) getsUserScope::Denied. Its own doc comment asserts "It can still create a fresh session, so the anonymous widget flow keeps working." That assertion is the bug: it could create, but not use.handler::may_read_conversationcomputesowned= anyuserparticipant with a non-blank email, thenUserScope::Denied => !owned.userEmailputs the email on the visitor's ownuserparticipant ⇒owned⇒ theDeniedarm returns false ⇒scoped_session→Ok(None)⇒SESSION_NOT_FOUND.So the visitor was owner-checked against an identity it does not have and cannot ever have, and locked out of the session it had created one frame earlier.
This is th-909995 recurring for the emailful case — the same failure the doc comment already records happening once ("fail-closing it instead denied those principals their own sessions… and the .NET revert in #309"). The ownership boundary was designed for authenticated multi-user deployments (stop user B driving user A's session) on the assumption that widget conversations are ownerless. The pre-chat form makes them owned.
Ruled out
capture_crm_contactis best-effort and swallows every failure; it cannot make a session unresolvable.The fix
One line at the single
may_read_conversationchokepoint, sosend_message,get_session,get_conversation_messages,confirm_tool_action,submit_interaction,verify_otpand conversation resume all change together:An anonymous connection can never satisfy an ownership check, so it no longer faces one — narrowly:
Reachenum). Where the caller already holds an unguessable id, that id is the visitor's entire capability — exactly the model in force before scoping shipped.list_conversations(Reach::Listing) stays strict for everyone.auth_org.is_none()is set only by the tokenless and degraded-token branches ofresolve_ws_access, so an authenticated principal whose token carries noemailclaim still fails closed.SESSION_NOT_FOUNDstill leaks nothing — "not found" and "not yours" stay byte-identical.A negative control caught a real widening before it shipped
The first cut applied the exception in
may_read_conversationunconditionally.an_anonymous_visitor_still_cannot_reach_an_authenticated_users_sessionfailed immediately: anonymous listing falls back to the SEED org, which is precisely where widget conversations pool, so that version would have let any anonymous visitor enumerate other visitors' chats. Hence theReachsplit.Tests
rust/smooth-operator-server/tests/user_scoping.rsgains 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.anonymous_visitor_with_an_email_can_send_into_the_session_it_created— asserts the conversation really isowned(or the test proves nothing), asserts the session row exists after the create, then that the send reaches the turn (LLM_UNAVAILABLE, i.e. past the ACL gate) and that resume binds back.Four negative controls:
the_not_found_response_is_still_reachable_for_an_anonymous_visitor—SESSION_NOT_FOUNDis still producible for that same caller on an unknown id, so theassert_ne!above means something.an_authenticated_emailless_principal_still_cannot_reach_an_owned_session— the exception is keyed on "no verified principal", not onDenied.another_user_still_cannot_reach_the_visitors_owned_session— including that nothing lands in the visitor's message log.an_anonymous_visitor_still_cannot_reach_an_authenticated_users_session— no enumeration.Positive control: with the fix line reverted,
anonymous_visitor_with_an_email_can_send_into_the_session_it_createdfails reproducing production's exact string,session '<uuid>' not found— and the session-exists assertion passes before that failure, which is the direct proof that the row was there all along. All four negative controls still pass under the revert, so none of them ride on the fix.Gates
cargo test -p smooai-smooth-operator-server— all green (29/29 inuser_scoping, ~250 across the crate).cargo fmt --all --check— clean.cargo clippy -p smooai-smooth-operator-server --all-targets -- -D warnings— clean..changeset/anonymous-visitor-owned-session.md(patch).Known follow-ups (not in this PR)
ConversationScope.Allows), Python, TypeScript and .NET all refuse an owned conversation to an emailless scope. Production runs the Rust server, so this PR ends the outage, but parity is real. It is not a mechanical port: Go's anonymous principal carries the org"public", soauth_org.is_none()has no direct analogue and each language needs its own key. Deliberately deferred rather than rushed under a live P0.