feat(gateway): bind outbound emit + Slack reads to the calling session's branch#1799
Merged
Conversation
a02b75e to
527fd04
Compare
mistercrunch
approved these changes
Jul 6, 2026
mistercrunch
left a comment
Member
There was a problem hiding this comment.
Nice, thanks for tackling this.
0a32405 to
9c08248
Compare
527fd04 to
bb0a9ae
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bb0a9ae to
1750860
Compare
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.
The bug (confused deputy)
Gateway channels each have a
target_branch_id, and inbound routing is branch-scoped — but outbound was not.agor_gateway_emit_messagetakes an explicitgatewayChannelId, and its only authorization (ensureCanEmitFromChannel) early-returns for admin role, otherwise checking the user's permission on the channel's branch. Nothing tied the calling session's branch to the channel. Since assistant sessions run as admin users, any session could callagor_gateway_outbound_targets_list, discover another assistant's channel, and emit through it — impersonating that assistant's Slack bot. The same leak applied to reads viaagor_gateway_slack_thread_history_get.The fix
Whenever the caller has session context (
ctx.sessionIdpresent — always true for agent sessions), outbound emit and Slack thread reads now requiresession.branch_id === channel.target_branch_id:GatewayService.emitMessage— whenemittedBySessionIdis set (server-derived from MCPctx.sessionId, never tool input; the input zod schema is strict and rejects injected attribution fields), a newensureSessionBranchBoundToChannelcheck runs beforeensureCanEmitFromChannel, with no admin-role bypass (agents are admins — a bypass would make the fix a no-op; this is deliberate). Fails closed if the session can't be loaded. The denial message names the caller's branch but never echoes the channel's target branch (anti-enumeration).agor_gateway_slack_thread_history_get— both thesessionId-mapping path and the explicitgatewayChannelId+threadIdpath deny cross-branch reads for session-context callers, with the same anti-enumeration property. Existing admin/'all'-permission checks are unchanged for no-session callers.agor_gateway_outbound_targets_list— results are force-scoped to the calling session's branch (even for admins). A conflictingargs.branchIdreturnschannels: []plus a binding note (no throw); an empty result includes a hint to ask an operator.agor_sk_, i.e. human operators) keep today's user-permission model unchanged. No escape hatch in v1.Tests cover: same-branch emit success with session attribution on the audit row; cross-branch emit denied while admin with no Slack send and no audit row; fail-closed on missing session; unchanged no-session admin/member paths; strict-schema rejection of injected
emittedBySessionId/sessionId; targets-list scoping/conflict/hint/no-session cases; and thread-history cross-branch denial (mapped + explicit) alongside unchanged no-session admin reads.CI note
check:multitenancy-boundariesis pre-existing-red onmain(4 benign hits: 3 in daemon test files that drive tenant scope / flush macrotasks, plus one type-onlyDatabaseimport inwidgets/env-vars/index.ts) — all unchanged by this PR. It is a known-unrelated failure the team merges over; this PR deliberately does not modify the shared check script.Stacking
Stacked on #1745 (#1743 ← #1745 ← this). Base is
gateway-token-widget, notmain; this will be rebased ontomainafter #1743/#1745 merge.Residual risks (known, out of scope)
agor_sessions_createa session on the victim branch and have it emit — that's a separate authorization layer and the emit is then correctly attributed to a session genuinely on that branch; out of scope here.sessions.patchbranch_idre-parenting is a human-only surface; a possible one-line follow-up is addingbranch_idtoensureSessionImmutability.config.allowed_emitter_branch_idsis the designated future escape-hatch extension point if cross-branch emits ever need to be sanctioned per channel.🤖 Generated with Claude Code