SECURITY(go): scope owned conversations only — unbreak anonymous/emailless chat - #314
Merged
Conversation
…lless principals 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. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01331RwggrhiP9UJVao9dr1F
🦋 Changeset detectedLatest commit: 4065e1a 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
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.
Pearl th-909995. Go lane of the Option B fix. Companion to #309 (the .NET revert that surfaced this).
The problem
PR #298 shipped, and released:
On an auth-enabled server, a connection whose principal carries no email gets
Email: ""and is denied everything — empty conversation list, refused resume, and (becausescopedSessionis the chokepoint for every sessionId-taking handler, writes included) a refusedsend_messageon the session it just created.That population is real, not hypothetical.
go/server/auth.go:No email. So every anonymous visitor to an auth-enabled server — public-agent chat, a supported Smoo AI scenario — could not converse at all. Same for any verified token whose IdP issues no
emailclaim.The identical rule in .NET hung CI on a WebSocket ACL test that authenticates with
sub/org/role/groupsand no email, forcing #309 to revert the .NET write-path fix. Go had no equivalent test, which is exactly why it shipped here unnoticed.The fix — Option B
A session that has an owner is owner-checked — the reported P0 (authenticated A reaching into authenticated B's owned session) stays closed on both the read and the write path. A session with no owner stays reachable, as before.
Option A (
email ?? sub) was rejected specifically because ofAnonymousPrincipal:subis the literal"anonymous"for every visitor, so keying on it would pool all anonymous conversations into one shared bucket and leak them to each other.Also switches email comparison to
strings.EqualFold, aligning with the .NET and Python siblings — OIDC providers vary on email-claim casing, and a bare==locks a user out of their own conversation the day their IdP changes it.Deliberately unchanged
scopedSessionchokepoint, verbatim (it is the reference implementation the other lanes were told to copy)SESSION_NOT_FOUNDfor not-yours vs never-existed — no existence oracleUnscopedonly when auth is disabledlist_conversationsTests
Adds
go/server/anonymous_scoping_test.go— the coverage Go never had:SESSION_NOT_FOUNDreading and sending into B's owned session, and B's message log is asserted unchanged — the attacker's text never landsUpdated
user_scoping_test.gowhere it encoded the old behavior (TestAuthEnabledWithoutEmailSeesNothing→...SeesNoOwnedConversations).Verified the tests actually bite
Reverted the predicate locally and re-ran — a test that passes either way is worthless here:
TestEmaillessPrincipalCanConverseOnAuthEnabledServerFAILS (both subtests: anonymous + emailless),TestAuthEnabledWithoutEmailSeesNoOwnedConversationsFAILS, 2TestConversationScopeAllowsrows FAILEqualFold→==TestOwnerEmailComparisonIsCaseInsensitiveFAILS,TestConversationScopeAllows/match_ignores_email_casingFAILSGates
gofmt -l .clean ·go vet ./...clean ·go test -race ./...233 pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01331RwggrhiP9UJVao9dr1F