SECURITY: owner-check every sessionId-taking action in the Rust server (th-1b7ed0) - #306
Merged
Merged
Conversation
The per-user scoping in th-8fe998 covered the Rust server's READ paths but left every WRITE path loading a session by raw client-supplied id. `send_message` was the worst case: an authenticated user A who knew or guessed user B's sessionId could send a message into B's session. The turn replays B's conversation history as context and streams the agent's reply back to A — so A reads B's conversation content by asking questions against B's session, defeating the read scoping entirely. `get_session`, `verify_otp`, `confirm_tool_action`, `submit_interaction` and `rename_conversation` had the same gap: nothing between the client's id and the session. Rather than patch send_message in isolation, this adopts the Go dispatcher's chokepoint: `scoped_session` is now the only way a handler turns a client-supplied sessionId into a session. It loads the session and hides it unless the connection's authenticated principal owns its conversation, returning None — exactly what an unknown id returns — so every caller emits the identical not-found event and none can distinguish "not yours" from "never existed". A storage error is a denial. The check lives once and cannot be forgotten by the next handler. Invariants preserved: `Unscoped` only when auth is not configured (the smooth-daemon / LocalServer single-user embedding keeps working), `Denied` fails closed, and org scoping stays alongside user scoping. Tests drive the attack from A's side: the headline case asserts B's message log is untouched and no turn was ever spawned; every denial is asserted byte-identical to a never-existed id; one test per routed handler; and the auth-disabled path is asserted unaffected. Verified adversarially — with the ownership check neutered, 9 of the new tests fail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01331RwggrhiP9UJVao9dr1F
🦋 Changeset detectedLatest commit: 791d490 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
The per-user scoping shipped in #302 (th-8fe998) owner-checked the Rust server's read paths but left every write path loading a session by raw client-supplied id.
send_messagewas the worst case:An authenticated user A who knows or guesses user B's
sessionIdcould send a message into B's session. The turn replays B's conversation history as context and streams the agent's reply back to A — so A reads B's conversation content by asking questions against B's session. That defeats the read scoping #302 just shipped, and it also writes into B's log.Complete inventory of sessionId-taking handlers
send_messageget_sessionget_sessionget_session, noscopeparam at allverify_otpget_session(..).is_none()confirm_tool_actionsubmit_interactionrename_conversationconversationId; any user could retitle another's conversationmay_read_conversationget_conversation_messagescreate_conversation_session(resume)list_conversationscancel(inserver.rs)Fix
Adopts the Go dispatcher's
scopedSessionpattern rather than patchingsend_messagein isolation.scoped_session(state, session_id, scope)is now the only way a handler turns a client-supplied sessionId into a session: it loads the session, then hides it unless the connection's authenticated principal owns its conversation, returningNone— exactly what an unknown id returns. Every caller emits the identical not-found event, so "not yours" is indistinguishable from "never existed" (no existence oracle for enumerating ids). A storage error is a denial.Preserved invariants:
Unscopedonly when auth is not configured (the smooth-daemon /LocalServersingle-user embedding is unaffected),Deniedfails closed, org scoping (auth_org) stays as defense in depth.Verification
SESSION_NOT_FOUND, exactly one event (no 202 ack ⇒ no turn ever spawned), and B's message log is asserted untouched.Unscoped) send/read asserted unaffected; existingLocalServer/daemon tests pass.cargo test -p smooai-smooth-operator-server→ 259 passed, 0 failed.cargo clippy→ 0 errors, 7 warnings, all pre-existing (th-da8207) and none in touched files.cargo fmt --checkclean on touched files (one pre-existing diff insmooth-operator/src/agent_config.rs, untouched here).One test-fixture change:
confirm_tool_action.rsspawned a parked turn without registering its session inAppState— unreachable in the live server (a park only exists aftersend_message, which requires a registered session). The fixture now registers it.🤖 Generated with Claude Code
https://claude.ai/code/session_01331RwggrhiP9UJVao9dr1F