Conversation
|
suggesting changes The change blocks the reported cross-user approval attack on same-platform per-user channel events, preserves the pending prompt after a rejected click, and intentionally permits shared-session members. However, its exact full-session-key comparison breaks the real Discord relay callback path: ordinary relay messages retain source.platform=discord, while Discord component callbacks are normalized by _discord_interaction_to_event with source.platform=relay. The key derived for the legitimate owner therefore never equals the pending Discord session key, so exec approvals, slash confirmations, and clarifications remain pending and cannot be resolved through their native buttons. This deterministic regression should be fixed and covered with an end-to-end-shaped test before merge.
Security evidence:
Uncertainty: The full focused pytest module and broader gateway suite were not runnable in the available Python environment because pytest was not installed. No live connector/provider end-to-end environment was available; the Discord regression was validated through the actual repository event constructors and resolver path in-process. Signed: GPT-5.6-sol-xhigh in Codex |
The relay adapter resolves an approval / slash-confirm / clarify button click in _consume_prompt_response, which runs *before* handle_message (see _on_passthrough). The normal _is_user_authorized gate therefore never runs for a consumed prompt, and the resolver keys only off the button's prompt_id -> session_key, never the clicker's identity. In a per-user channel session (group_sessions_per_user, the default), any other member of the channel who can see the buttons could click "Always" and resolve -- and permanently allowlist -- another user's dangerous-command approval (CWE-639). The native adapters already guard this: qqbot's _is_authorized_interaction_for_session and whatsapp_cloud's _is_interactive_sender_authorized require the clicker to own the session, and NousResearch#41226 fixed the same "any channel member can click Approve" class for the Slack/Feishu/Discord native adapters -- but the relay passthrough path was never covered. Re-derive the caller's session key exactly as handle_message does (build_session_key + the same config) and require an exact match before resolving. A shared (non-per-user) session collapses every member's source to one key so all members stay allowed; a per-user key carries the participant id so only its owner matches. DMs are 1:1 and fail open. The check peeks (does not pop) so a rejected stray click cannot evict a still-pending prompt from under its real owner.
33ce10e to
2d0d496
Compare
…d session key Review follow-up: the owner check re-derived `build_session_key` from the callback's source and required an exact match against the pending key. The two relay lanes normalize the same conversation differently, so that comparison rejected the *legitimate* owner of every Discord native prompt button. Ordinary Discord traffic keeps the underlying platform through `ws_transport._event_from_wire` (`agent:main:discord:...`), while a component press travels the passthrough plane, where `_discord_interaction_to_event` deliberately stamps `Platform.RELAY` — it must, so the key matches the one the connector bound the follow-up capability under — and carries no `thread_id` (`agent:main:relay:channel:...`). Probing the shipped code, an owner-authored "always" click on their own approval was consumed and dropped: pending `agent:main:discord:channel:chan42:owner` vs callback `agent:main:relay:channel:chan42:owner`. Threads diverge on two more segments (`thread:th9:th9` vs `channel:th9`), so normalizing the platform slot alone would not have been enough. All three prompt kinds were affected. Bind ownership to the participant id instead — the one identifier both lanes carry unchanged. `_capture_scope` now records, per session key it sees inbound, the participant that key isolates (`_remember_session_owner`); a key that does not end in its sender's own id is a shared group/thread session and is deliberately left unowned. `_caller_owns_prompt_session` reads that back and compares ids, the same `operator == session_user` shape the native qqbot adapter uses — without its session-key parser, which `gateway/run.py`'s `_parse_session_key` refuses to write because the trailing segment may be a user id or a thread id. Fails open where there is no single owner to protect (empty key, shared session, DM, or a session never seen inbound — a cron- or API-started turn, unchanged from before the gate existed) and closed on an author-less answer. The peek-don't-pop behaviour is unchanged. Tests are rebuilt around the real lanes: sources now come from `_event_from_wire` (which also fixes the `AttributeError: 'str' object has no attribute 'value'` the earlier hand-built `platform="relay"` fixtures raised in `build_session_key`), and two new cases drive a Discord button through `_on_passthrough`/`_discord_interaction_to_event` against a `Platform.DISCORD`-keyed session — owner resolves, co-member does not. A third covers the per-user thread lane, and a fourth pins the unowned-session fall-open. On the previous head the two cross-lane owner tests and the fall-open test fail; on `main` the two non-owner tests fail.
b620b81 to
58d039f
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real relay authorization boundary: current main consumes Discord prompt buttons before handle_message (gateway/relay/adapter.py:653-659) and resolves pending approvals after an unconditional pop (:1846-1868). The participant-based follow-up correctly avoids the Discord relay/native session-key mismatch described in the earlier review.
Problems
gateway/relay/adapter.py:403-411bounds the owner map by evicting oldest entries even while their prompts remain live. Once evicted,:1443-1445deliberately fails open, so cache churn can restore cross-user resolution for an unexpired approval.gateway/relay/adapter.py:389-401uses the raw participant identifier, but currentmaincanonicalizes WhatsApp group participant identities ingateway/session.py:1108-1113. A mapped LID/JID therefore leaves the prompt unowned and reaches the same fail-open branch.
Suggested changes
- Bind the owner to each pending prompt, or prevent eviction while that prompt is unexpired; add a cache-churn regression.
- Canonicalize the stored WhatsApp participant consistently with
build_session_key, with a mapped-identity group test.
Automated hermes-sweeper review.
| # insertion order, so the tail is the most recently seen. | ||
| excess = len(self._session_owner_by_key) - _SESSION_OWNER_CACHE_MAX | ||
| if excess > 0: | ||
| for stale in list(self._session_owner_by_key)[:excess]: |
There was a problem hiding this comment.
This cap can evict an owner while that session still has an unexpired prompt. _caller_owns_prompt_session() then treats the missing entry as unowned and returns True at lines 1443-1445, allowing a co-member to resolve it. Bind the owner to the pending prompt or retain this entry until the prompt expires, and add a cache-churn regression.
| "thread_sessions_per_user", False | ||
| ), | ||
| ) | ||
| if not key.endswith(f":{participant}"): |
There was a problem hiding this comment.
build_session_key() canonicalizes WhatsApp group participant IDs on current main (gateway/session.py:1108-1113), but participant here is raw. For a mapped LID/JID, this suffix check fails, no owner is stored, and the fail-open branch permits the callback. Canonicalize consistently or avoid raw suffix inference, with a mapped WhatsApp group test.
|
not enough evidence A security conclusion cannot be made from a coherent current-main integration. Review setup attempted a run-owned deterministic patch replay of PR head 58d039f onto current GitHub Security evidence:
Uncertainty: The conflict-resolved implementation that would actually be proposed for current main is unavailable; runtime positive and negative security cases were not executed because the checkout lacks pytest and a pytest-capable virtual environment; it is unknown whether conflict resolution would preserve current-main relay behavior while applying the ownership capture and pre-consumption authorization check; integration-specific residual bypasses cannot be excluded without a coherent replay and executable focused tests. Signed: GPT-5.6-sol-xhigh in Codex |
|
suggesting changes
Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
Summary
The relay adapter resolves an approval / slash-confirm / clarify button click
in
_consume_prompt_response, which runs beforehandle_message(see_on_passthrough). The normal_is_user_authorizedgate therefore never runsfor a consumed prompt, and the resolver keys only off the button's
prompt_id → session_key— never the clicker's identity.In a per-user channel session (
group_sessions_per_user, the default), anyother member of the channel who can see the buttons can click "✅ Always" and
resolve — and permanently allowlist — another user's dangerous-command
approval (CWE-639, IDOR).
Problem
_on_passthrough(Discord interactions over the relay/connector passthroughplane) decodes a component press into a
MessageEventand calls_consume_prompt_response(event)beforehandle_message:_consume_prompt_responselooks the prompt up byprompt_id, reads the storedsession_key, and callsresolve_gateway_approval(session_key, choice)(andthe slash-confirm / clarify equivalents). The pending-prompt state minted in
send_exec_approvalis only{"session_key", "chat_id"}— no owner — andevent.source.user_id(the clicker, taken from the raw Discord interactionbody in
_discord_interaction_to_event) is never compared against it.Because resolution precedes
handle_message, the click never reaches_is_user_authorized; the only thing standing between a co-member and anotheruser's approval is an 8-hex
prompt_idthat is printed into the button everychannel member can see.
Exploit (default config, Discord over the relay, a shared guild channel where
group_sessions_per_useris on so each member has their own session):send_exec_approvalposts"
into the channel, bound to the victim's
session_key.signature and forwards the interaction on the passthrough plane.
_on_passthrough → _consume_prompt_responseresolvesresolve_gateway_approval(victim_session, "always")— approving, andpermanently allowlisting, the victim's command. The clicker's identity is
discarded.
The same gap applies to the
slash_confirmandclarifyarms.Prior art shows this class is in scope for the project: the native adapters
already guard it —
qqbot's_is_authorized_interaction_for_session(
operator == session_user) andwhatsapp_cloud's_is_interactive_sender_authorized— and #41226 fixed the identical"any channel member can click Approve" bug for the Slack / Feishu / Discord
native adapters. The relay passthrough path was never covered (#41226 and
the open delegation PR #47863 both leave
gateway/relay/adapter.pyuntouched).Fix
Before resolving, require the clicker to own the session. Re-derive the
caller's session key exactly as
handle_messagedoes — the samebuild_session_key(event.source, …)with the same config — and require anexact match:
group_sessions_per_user=False, or a shared thread)collapses every member's source to the same key, so all members stay allowed
— no regression to legitimate shared-channel approvals.
session_keyfails open; a key-derivation error fails closed.The check peeks (it does not pop) so a rejected stray click can't evict a
still-pending prompt from under its real owner — the legitimate owner can still
answer afterward. A rejected click is consumed (returns
True) so thecommand-shaped text isn't re-dispatched as the clicker's chat.
This mirrors the native adapters' owner check without a fragile session-key
parser, and without touching the generic
run.pyapproval path.Scope
gateway/relay/adapter.py— new_caller_owns_prompt_sessionhelper +a peek-and-check guard at the top of
_consume_prompt_response(covers allthree prompt kinds: exec approval, slash-confirm, clarify).
tests/gateway/relay/test_relay_interactive.py— 3 regression tests.No signature changes; no change to
run.pyor the native adapters.Testing
New tests (channel sessions):
test_prompt_response_rejects_non_owner_in_per_user_channel— a co-member's"always" click does not call
resolve_gateway_approval, the event isconsumed, and the prompt is left pending for its real owner.
test_prompt_response_allows_owner_in_per_user_channel— the owner's clickresolves as before.
test_prompt_response_shared_channel_allows_any_member— withgroup_sessions_per_user=False, any member resolves (shared sessionpreserved).
The existing interactive tests are unchanged and still pass: they use DM
sources, which the DM fast-path leaves untouched.