SECURITY: fix cross-user conversation-history leak in Python get_conversation_messages - #304
Merged
Merged
Conversation
…ership chokepoint The th-8fe998 scoping fix added `_visible_session` and routed get_session, send_message and verify_otp through it, but _handle_get_conversation_messages still called `self._store.get_session` directly. Any authenticated user could therefore read any other user's full conversation history by sessionId — the exact endpoint the security epic was reported about. It now goes through the chokepoint and reports a session it does not own with the byte-identical SESSION_NOT_FOUND payload used for an id that never existed, so the endpoint is not an existence oracle. Audited every other sessionId-taking handler: get_session, send_message and verify_otp were already routed; confirm_tool_action takes no store lookup and resolves against a per-connection ConfirmationRegistry, so it cannot reach another principal's session. This was the only bypass. Tests: the headline cross-user read (asserted on the payload, not just the error code), the not-yours-vs-never-existed byte-identical case, the owner's own read still working, and a structural test asserting `_visible_session` is the ONLY `self._store.get_session` call site — so a future handler that skips the chokepoint fails a test. All four fail without the fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01331RwggrhiP9UJVao9dr1F
🦋 Changeset detectedLatest commit: a9605ca 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.
Problem
P0 SECURITY. The per-user conversation scoping fix (th-8fe998, #295) added the
_visible_sessionownership chokepoint to the Python dispatcher and routedget_session,send_messageandverify_otpthrough it — but_handle_get_conversation_messagesstill calledself._store.get_sessiondirectly:No owner check. Any authenticated user could read any other user's full conversation history by sessionId — the exact endpoint the security epic was reported about. The Go, TypeScript, Rust and C# lanes were clean; Python had the helper but one handler skipped it.
Solution
get_conversation_messagesnow routes through_visible_session, and reports a session it does not own with the byte-identicalSESSION_NOT_FOUNDpayload already emitted for an id that never existed — no new error code, no reworded message, so the endpoint is not an existence oracle for enumerating other users' session ids.Handler audit
Every sessionId-taking handler in the dispatch chain:
get_conversation_messages_store.get_session_visible_sessionget_session_visible_sessionsend_message_visible_sessionverify_otp_visible_sessionconfirm_tool_actionConfirmationRegistry(server.py:131, one per WebSocket connection), so a foreign sessionId is aNO_PENDING_CONFIRMATIONno-op and cannot reach another principal's sessioncreate_conversation_session,list_conversations,cancel,pingcancelis connection-local; the other two were scoped by #295)self._store.get_sessionnow has exactly one call site: inside_visible_sessionitself.Making the bypass structurally hard to repeat
_visible_session's doc comment now states it is the ONLY permitted way to turn a client-supplied sessionId into a session (mirroring Go'sscopedSession), and names this bug as what happens otherwise.test_visible_session_is_the_only_lookupasserts the dispatcher source contains exactly one directself._store.get_session(call — a future handler that skips the chokepoint fails a test.Verification
Added to the existing security test file from #295:
test_get_conversation_messages_leaks_no_history_across_users— the headline case: user A reads user B's sessionId ⇒SESSION_NOT_FOUND, and the assertion is on the payload (nobob secret, nomessageskey), not just the error code.get_conversation_messagesadded to both existing parametrized suites, includingtest_not_yours_is_byte_identical_to_never_existed(the oracle test).test_owner_can_read_their_own_conversation_messages— the legitimate read still works.test_get_conversation_messages.pysuite (unscoped, single-tenant) passes unchanged.All four new/extended cases were confirmed to FAIL against the pre-fix code (fix temporarily reverted, 4 failed / 19 passed) and pass with it.
Full Python gates green:
No PyPI publish in this PR.
Pearl: th-1b7ed0. Follow-up to SECURITY epic th-8fe998 (#295).
🤖 Generated with Claude Code
https://claude.ai/code/session_01331RwggrhiP9UJVao9dr1F