-
Notifications
You must be signed in to change notification settings - Fork 52.3k
fix(gateway): honor group/thread session isolation precedence in is_shared_multi_user_session #62370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
briandevans
wants to merge
2
commits into
NousResearch:main
Choose a base branch
from
briandevans:fix/gateway-session-isolation-mirror
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
fix(gateway): honor group/thread session isolation precedence in is_shared_multi_user_session #62370
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Thanks — I looked at this closely and am intentionally keeping the helper scoped to the isolation config decision rather than mirroring per-source participant presence, because all three consumers depend on that scoping and widening it would regress two of them:
Prompt contract (
build_session_context→ session.py):test_non_thread_group_shows_userdeliberately pins that a default-isolation group carrying auser_namebut nouser_idrenders**User:** "<name>"(single-user), and its siblingtest_shared_non_thread_group_prompt_hides_single_userflips to the multi-user note only whengroup_sessions_per_user=False. Keying the helper on missing participant-id would silently flip every no-user_idgroup prompt to multi-user, breaking that intended display (and the sender-prefix branch in run.py that follows it).Resume IDOR gate (
_resume_allowed, slash_commands.py): this is the security-critical one. The gate treats config-isolated sessions as not shared, then applies an explicit fail-closed participant check ("participant id is missing on one side: cannot prove the same owner — fail closed"). If the helper returned shared=True whenever the current source lacks a participant id, a caller with a missinguser_idin a config-isolated group would be classified shared and allowed to resume another identified member's per-user session, short-circuiting that fail-closed check. That widens the IDOR surface rather than closing it.So the helper answers "is participant-sharing enabled by config?" — the question its three call sites actually consume — and the key-shape divergence you spotted for the missing-
user_idcase is handled downstream (the resume gate's own fail-closed pid check; the prompt's single-user display). Mirroring it into the helper would break the prompt contract and loosen the resume gate, so I'm leaving the helper as-is on this PR.