SECURITY: re-land the .NET write-path session chokepoint under Option B - #311
Merged
Conversation
…tion B #308 owner-checked the WRITE path (send_message, verify_otp, confirm_tool_action) via a single ScopedSessionAsync chokepoint, closing a P0: an authenticated user who knew another user's sessionId could send INTO that session, replaying the victim's history as turn context and streaming the agent's reply back to the attacker — a read dressed up as a write that defeated the read scoping entirely. #309 reverted it. The chokepoint was right; the RULE was too strict. th-966fab's predicate denied a session with no recorded owner, and denied any principal whose token carries no email claim. Applied to reads that only hid old history; applied to writes it locks those principals out of the product: create_conversation_session stamps ownerEmail = null for an emailless or anonymous principal, and the very next send_message refuses the session it just minted. That killed anonymous/public-agent chat, and hung main's .NET CI — the ACL integration test converses over exactly that path with a token carrying no email, and its ReceiveAsync (CancellationToken.None) waits forever for a frame that never comes. This re-lands the chokepoint under Option B: a session that HAS an owner is owner-checked; a session with NO owner has nobody to enforce against and stays reachable. That closes the reported attack (owned sessions are reachable only by their owner) without the lockout. Ownerless sessions remain absent from list_conversations and non-resumable by conversationId, so reaching one requires already holding its sessionId — not an enumeration surface. Option A (email ?? sub) was rejected for now: Go's AnonymousPrincipal uses the literal sub "anonymous" for every visitor, so keying on sub would pool all anonymous visitors into one bucket. All five sessionId-taking handlers route through ScopedSessionAsync, so the next handler cannot forget the check. Every non-disputed invariant is kept: byte-identical SESSION_NOT_FOUND for not-yours vs never-existed, unscoped only when auth is disabled, OrdinalIgnoreCase comparison, SQL-side scope filtering for the list. Tests: the #308 attacker-side suite, plus the regression that forced the revert — an emailless authenticated principal AND an anonymous connection to an auth-enabled server can each create, read and send in their own session. Neutering the check fails 8 of the 24 scoping tests. Full suite 402 passed, integration 46/46, run 2x plus the integration project 3x more, terminating normally each time (~11s). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01331RwggrhiP9UJVao9dr1F
🦋 Changeset detectedLatest commit: eda27a2 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.
Re-lands the write-path chokepoint from #308 (reverted by #309) under the Option B visibility rule.
The P0
get_session/get_conversation_messages/ resume were owner-checked, butsend_messagestill loaded any session by client-suppliedsessionId. An authenticated user A who knew or guessed user B'ssessionIdcould send a message into B's session — the turn replayed B's conversation history as context and streamed the agent's reply back to A. A read of someone else's conversation dressed up as a write, defeating the read scoping entirely.verify_otp(marking a foreign session identity-verified, unlocking itsend_user-gated tools) andconfirm_tool_action(resolving a foreign parked write) were open the same way.Why #308 was reverted, and what changed
The chokepoint was right; the rule was too strict. th-966fab's predicate denied a session with no recorded owner, and denied any principal whose token carries no
emailclaim. On the read paths that only hid old history. On the write path it locks those principals out of the product:create_conversation_sessionstampsownerEmail = nullfor an emailless or anonymous principal, and the very nextsend_messagerefuses the session it just minted.That killed anonymous/public-agent chat — a supported Smoo AI scenario — and hung main's .NET CI.
WebSocketProtocolIntegrationTests.Acl_PrivateDoc_OnlyReachesEntitledUser_OverWebSocketconverses over exactly that path (aTrustedTokenwith noemailclaim, and a second anonymous connection to the same auth-enabled server); itsReceiveAsyncusesCancellationToken.None, so a frame that never comes is a permanent hang and the job was cancelled at ~14 min.Option B:
A session that has an owner is owner-checked — which is exactly the reported attack. A session with no owner has nobody to enforce against and stays reachable, as before. Ownerless sessions are still absent from
list_conversations(SQL-side scope filter) and still not resumable byconversationId, so reaching one requires already holding itssessionId— not an enumeration surface.Option A (
email ?? sub) was considered and rejected for now: Go'sAnonymousPrincipaluses the literal sub"anonymous"for every visitor, so keying onsubwould pool all anonymous visitors into one shared bucket and leak their chats to each other.The chokepoint
ScopedSessionAsyncis the only way a handler may turn a client-suppliedsessionIdinto a session. It loads, then hides the session unless the principal may reach it — returningnull, exactly what an unknown id returns, so every caller emits the identical not-found response. All five sessionId-taking handlers route through it, so the next handler cannot forget the check:get_sessionget_conversation_messagessend_messageconfirm_tool_actionverify_otp_store.GetSessionAsyncnow has exactly one caller in the repo: the chokepoint itself.Invariants kept: byte-identical
SESSION_NOT_FOUNDfor not-yours vs never-existed (no existence oracle), unscoped only when auth is disabled,OrdinalIgnoreCasecomparison, SQL-side filtering for the list.Verification
dotnet build: 12 projects, 0 errors, 0 warnings.dotnet test: 402 passed, 0 failed, run twice, terminating normally in ~11s and ~19s (the regression that forced the revert was a hang, so exit was checked, not just green).Acl_PrivateDoc_OnlyReachesEntitledUser_OverWebSocketpasses.[Theory]over both).#308attacker-side tests are kept: the headline case asserts the turn never runs and the victim's message log is untouched, plus a byte-identical no-oracle assertion per handler.🤖 Generated with Claude Code
https://claude.ai/code/session_01331RwggrhiP9UJVao9dr1F