SECURITY: Go per-user conversation scoping (cross-user data leak) — th-8fe998 - #298
Merged
Conversation
…a leak) SessionStore.ListConversations took no user filter, so list_conversations returned every user's conversations to any authenticated caller. The resume path and get_conversation_messages were not owner-checked either, so a caller could open and read another user's conversation by id. Conversations are now owned by the authenticated principal and every read is filtered by it: - Principal carries Email (JWT `email` claim); AccessContext.ConversationScope() derives the connection's visibility. Ownership comes from the principal only — the client-supplied userName/userEmail frame fields were the spoofing vector and no longer decide who may read what (userEmail remains the OTP contact). - ListConversations filters during selection, before the handler's limit; filtering after a limit silently returns short/empty pages. - get_session, get_conversation_messages, send_message and verify_otp all route session lookups through one owner-checked chokepoint (scopedSession), so the check lives once rather than being re-derived — and forgotten — per handler. - Not-yours is indistinguishable from never-existed: a denied read returns the identical SESSION_NOT_FOUND payload an unknown id returns, and resuming another user's conversation mints a fresh conversation exactly as an unknown id does. Neither path can be used as an existence oracle. - Fails closed: auth enabled with an emailless or rejected principal sees nothing rather than falling back to unscoped. - Auth disabled (no verifier — local/dev single-tenant) stays unscoped and is unchanged. That is the only unscoped path. BREAKING for SessionStore implementers, deliberately: ListConversations, CreateSession and ResumeSession take a required ConversationScope. Required rather than optional-defaulting-to-unscoped so downstream implementations get a compile error and must confront who may see what — a default-to-unscoped parameter is fail-open. ConversationScope's zero value denies everything. Tests are written from the attacker's side (the bug passed every test written from the honest user's side) and assert on full response payloads for the existence-oracle cases. Verified by mutation: reverting the scope check, the dispatcher filter, or the principal stamping each fails these tests.
🦋 Changeset detectedLatest commit: 4d89a57 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 |
brentrager
added a commit
that referenced
this pull request
Jul 20, 2026
…lless principals (#314) The rule shipped in #298 — `Unscoped || (Email != "" && owner == Email)` — denied everything to any connection whose principal carries no email claim. On an auth-enabled server that is an outage: AnonymousPrincipal has no email, so an anonymous visitor got an empty list, a refused resume, and (because the scopedSession chokepoint covers the write path too) a refused send_message on the session it had just created. The identical rule hung .NET CI on an ACL test that authenticates without an email claim and was reverted there (#309); Go had no such test, which is why it went unnoticed here. Allows() now owner-checks only conversations that HAVE an owner. An owned session stays readable/writable by its owner alone — the reported P0 (authenticated A reaching into authenticated B's owned session) remains closed on both paths. An ownerless session (anonymous, emailless-authenticated, or legacy auth-disabled) stays reachable, because there is no owner to enforce on behalf of. Keying ownerless sessions on `sub` was rejected: AnonymousPrincipal.Sub is the literal "anonymous" for every visitor, so it would pool all anonymous conversations into one bucket and leak them to each other. Email comparison moves to strings.EqualFold, matching the .NET and Python siblings — OIDC providers vary on email-claim casing. Unchanged: the scopedSession chokepoint, the identical SESSION_NOT_FOUND for not-yours vs never-existed, selection-side list filtering, and the auth-disabled unscoped path. Tests: adds the coverage Go never had — an anonymous connection AND an emailless authenticated principal, each against an auth-ENABLED server, creating, reading, listing and sending in their own session. Both fail against the old predicate. Also asserts the P0 stays closed on the write path (nothing appended to the victim's log) and that case folding matches only the same address. Claude-Session: https://claude.ai/code/session_01331RwggrhiP9UJVao9dr1F Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.
SECURITY — cross-user data leak (P0)
Go's
SessionStore.ListConversations(ctx)took no user filter, solist_conversationsreturned every user's conversations to any authenticated caller. The resume path andget_conversation_messageswere not owner-checked either, so a caller could also open and read another user's conversation by id.Any authenticated user could enumerate and read anyone else's chats. Reported by a downstream customer; being closed across all five language lanes.
The fix
Ownership is driven by the connection's authenticated principal, never by client-supplied input.
PrincipalcarriesEmail(JWTemailclaim).AccessContext.ConversationScope()derives the connection's visibility.create_conversation_sessionstamps the principal's email as owner. The client-supplieduserName/userEmailframe fields were the spoofing vector and no longer decide who may read what —userEmailsurvives only as the OTP delivery contact.ListConversationsfilters during selection, before the handler's limit. Filtering after a limit silently returns short/empty pages.get_session,get_conversation_messages,send_messageandverify_otproute session lookups through one owner-checked chokepoint (FrameDispatcher.scopedSession) — the check lives once, at the chokepoint, instead of being re-derived and forgotten per handler.No existence oracle
A denied session read returns the identical
SESSION_NOT_FOUNDpayload an unknown id returns — same code, same message, same envelope — so "not yours" and "never existed" are indistinguishable. On the resume path, another user's conversation id takes the exact same branch as an unknown one: it mints a fresh conversation. Neither can be used to probe which ids exist. Tests assert on full response payloads, not just the error code.Fail-closed rules
A rejected token keeps
AuthEnabledset, so a forged token gets the empty per-user view rather than the auth-disabled unscoped view.BREAKING interface change (deliberate)
ListConversations,CreateSessionandResumeSessionnow take a requiredConversationScope. Required rather than optional-defaulting-to-unscoped precisely so every downstreamSessionStoreimplementer gets a compile error and must confront who may see what — a default-to-unscoped parameter would be fail-open and leave downstream stores silently vulnerable. The compile break is the feature.ConversationScope's zero value denies everything, so a partially-migrated store leaks nothing. Migration notes are in the changeset.Verification
go test -race ./... && go vet ./... && gofmt -l .— 226 tests pass, vet clean, gofmt clean.New tests are written from the attacker's side, since the bug passed every test written from the honest user's side:
SESSION_NOT_FOUND, no message text leakedget_conversation_messagesandget_session)"" == ""compare would match)Mutation-verified: independently reverting (a) the scope check, (b) the dispatcher filter, and (c) the principal stamping each makes these tests fail. They are not tests that always pass.
Pearl: th-8fe998