fix(teams): bind approval card actions to conversation, enforce render permissions

[spfcraze]

What does this PR do?

Fixes two approval-escalation holes in the Teams card-action handler. The Action.Execute invoke payload round-trips through the Teams client and is untrusted (Microsoft's own guidance), but the handler trusted both session_key and hermes_action from it: a crafted invoke could approve a DIFFERENT session's pending dangerous command (cross-session IDOR — QQ and Feishu bind the clicker to the session; Teams bound nothing), and could resubmit choices the render side withheld (approve_always despite allow_permanent=False or smart_denied) — which writes the dangerous pattern into the permanent on-disk allowlist. The session_key must now contain the invoking conversation's id, and every choice is enforced server-side against the pending approval's recorded permissions (allow_session / allow_permanent / smart_denied) via a new get_blocking_approval_data() helper in tools/approval.py.

Related Issue

No GitHub issue — discovered via code review and reproduced live (see below). Happy to file one first if preferred.

Changes Made

• fix/teams-approval-idor — 3 file(s) changed vs base:
  • plugins/platforms/teams/adapter.py
  • tests/gateway/test_teams.py
  • tools/approval.py

How to Test

Validation completed (recorded by prp):

1. Sabotage check: pre-fix code fails the regression tests (3 failed), with the fix all pass (24 passed, 0 failed) — target tests/gateway/test_teams.py.
2. Suite tests/gateway/test_teams.py tests/tools/test_approval.py tests/tools/test_approval_heartbeat.py: branch 114 passed / 0 failed vs baseline 110 passed / 0 failed — zero branch-only failures.
3. uvx ruff clean; git diff --check clean; live repro verified both vectors pre-fix (cross-session approved, withheld always honored) and post-fix (both blocked), legit owner path unaffected
4. The full repo-wide suite was not run for this change; GitHub CI owns full-suite validation.

Logs

Sabotage verification output:

# base leg (pre-fix code + branch tests):
#   tests: 21 passed, 3 failed
# head leg (with fix):
#   tests: 24 passed, 0 failed

[teknium1]

Thanks for addressing a real Teams approval-boundary gap. Current main accepts session_key and hermes_action from the invoke payload and resolves the queue without a conversation or server-side render-permission check (plugins/platforms/teams/adapter.py:1015-1077); the card renderer does withhold scoped choices (plugins/platforms/teams/adapter.py:1129-1138).

Problems

• The proposed conversation_id in session_key test in 1817e86e2470431d250272056fde8a3684df7501 is not an exact binding. Teams uses conversation.id as chat_id (plugins/platforms/teams/adapter.py:888), and session keys are structured components (gateway/session.py:1079-1086); substring matching can match unrelated portions of the opaque key.
• The proposed read of approval permissions and later resolve are not atomic. The queue explicitly allows concurrent entries (tools/approval.py:2149-2152), while resolution separately pops FIFO (tools/approval.py:2214-2224), so concurrent invokes can validate one entry and resolve another.

Suggested changes

• Bind cards to an opaque approval ID and atomically validate conversation, allowed choice, and target entry before consuming it.
• Add a two-entry/concurrent-invoke regression test with differing permission flags.

Automated hermes-sweeper review.

[spfcraze]

Reworked in eff4239. The substring binding is gone: each _ApprovalEntry now carries an opaque uuid, the card payload embeds only that approval_id (the session key no longer round-trips through the Teams client), and send_exec_approval binds the entry to the render-time conversation.id server-side. On invoke, a new resolve_gateway_approval_by_id() holds the queue's _lock across the whole validate-and-consume: it locates the exact entry by id, requires an exact conversation-id match, enforces the render-time permissions (smart_denied / allow_session / allow_permanent), and removes exactly that entry — concurrent invokes against a multi-entry queue can no longer validate one entry and pop another.

Regression coverage added: a two-entry/concurrent-invoke test with differing permission flags (each invoke consumes only its own entry), a valid-id-withheld-choice rejection, a substring conversation-mismatch rejection, and a forged-id rejection.

Verification: tests/gateway/test_teams.py 30/30 pass; approval-adjacent suites (discord/feishu/matrix/slack/telegram/tui + approvals commands) green; revert-verified — the 10 security tests fail on pre-fix main and pass with the change.

[egilewski]

looks mergeable

The replayed Teams approval change closes the prior cross-session and withheld-choice paths: card invokes carry only an opaque queue-entry id, the server binds it to the render-time conversation, and one locked resolver validates permissions before signaling the exact waiting entry. No additional source-backed security issue was found.

Security evidence:

• trust boundary: Teams Adaptive Card invoke data (hermes_action, approval_id, cmd, and desc) is attacker-controlled. The sink is the pending gateway approval entry: its result/event unblocks the agent command guard and can authorize execution. The configured user authorization and in-process queue are trusted server-side state.
• source/sink/invariant: The replay no longer accepts a client session_key. send_exec_approval binds the opaque entry id to the conversation; resolve_gateway_approval_by_id performs exact-id lookup, requires the exact bound conversation and stored allow_session/allow_permanent/smart_denied permissions, then removes only that entry before signaling it.
• current-main reproduction: The prior implementation accepted a client-supplied session key, checked only whether that session had a pending approval, and FIFO-resolved it; the replay removes that path and uses exact opaque-id lookup, conversation binding, and stored choice permissions.
• PR-head or patch-replay validation: Patch-replay validation passed focused Teams and queue-approval tests covering cross-conversation, forged and legacy IDs, withheld choices, concurrent entries, and render-time binding.
• positive/negative cases: Positive: an authorized click with a matching conversation and offered choice resolves its own entry. Negative: mismatched conversation, unknown or legacy ID, withheld session/always choices, and concurrent two-entry clicks leave entries unresolved or consume only their own entry.
• residual bypass search: Reviewed the changed Teams, gateway, and approval call sites plus sibling approval handlers. Other adapters retain their session-routed handlers, while Teams card data cannot select a queue or broaden render-time permissions. No residual Teams card bypass was found.
• reviewer validation: Manual source review covered the relevant security logic, and focused replay tests passed.

Review setup: I reviewed a run-owned local rebase or patch replay against current GitHub main because the submitted branch is stale or conflicted; this does not mean the submitted branch itself merges cleanly.

Not checked:

• CodeRabbit review
• Full repository test suite
• ruff lint

Signed: GPT-5.6-luna-max in Codex