Skip to content

fix(matrix): fail closed for approval reactions - #33328

Closed
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/matrix-approval-reaction-fail-closed
Closed

fix(matrix): fail closed for approval reactions#33328
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/matrix-approval-reaction-fail-closed

Conversation

@Dusk1e

@Dusk1e Dusk1e commented May 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Enforces rigid fail-closed authorization criteria on Matrix communication event loops by passing authoritative gateway reference chains to validate emoji reaction approvals against the global default-deny security model.

Why

A security containment defect existed within the Matrix adapter's reaction handler loops. Previously, reaction senders were validated strictly against MATRIX_ALLOWED_USERS. Whenever this platform variable remained unconfigured, the event loop bypassed verification gates entirely—even if global gateway access was actively guarded by GATEWAY_ALLOWED_USERS. This permitted untrusted room members to resolve high-risk command prompts or dismiss pending agent gates simply by applying reaction emoticons to the bot's validation interface. This patch bounds reaction lifecycles to core runner verification layers.

Scope of Changes

  • gateway/platforms/matrix.py: Patched the incoming event loop to interface directly with authoritative runtime user validation modules instead of relying on fragile standalone environment evaluations.
  • gateway/run.py: Connected the running gateway daemon pipeline references down into the active Matrix adaptors during startup sequences.
  • tests/gateway/test_matrix_exec_approval.py: Introduced explicit regression tests ensuring proper callback cancellation on allowlist mismatch conditions.

Verified Test Cases

Targeted validation blocks and dynamic context checks within the Matrix pipeline completed successfully with absolute zero failures:

  • test_matrix_empty_platform_allowlist_with_global_mismatch_fails_closed
  • test_matrix_global_allowlist_match_successfully_authorizes_reaction
  • test_matrix_unconfigured_allowlist_matrix_defaults_to_strict_deny
  • Full file regression execution for test_matrix_exec_approval.py
Targeted Matrix Control Sweeps: 5 passed, 0 failed

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery platform/matrix Matrix adapter (E2EE) P0 Critical — data loss, security, crash loop labels May 27, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of open PR #30062 (same fix: fail-closed Matrix approval reactions when MATRIX_ALLOWED_USERS is empty). Both add gateway auth fallback to run.py verification.

@teknium1

Copy link
Copy Markdown
Contributor

Superseded by #34567 (merged to main as 784d8dd).

Thanks for catching the Matrix approval reaction fail-open — it's a real bug and you're right that it needed fail-closed treatment.

We went with the minimal shape from #30062: a direct GATEWAY_ALLOW_ALL_USERS short-circuit inline at the check site, mirroring the already-shipped Telegram precedent (_is_callback_user_authorized, telegram.py:558). Your PR's _is_reaction_approval_authorized helper routed through gateway_runner._is_user_authorized via a build_source round-trip — more general, but it adds cross-object plumbing (the adapter reaching back into the runner) and an except Exception fallback that could itself fail-open if build_source throws unexpectedly. For a security boundary we preferred the smaller, locally-verifiable surface that matches the existing Telegram idiom.

Both approaches preserve the key no-regression case (operators who set GATEWAY_ALLOW_ALL_USERS=true keep open approvals). Appreciate the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P0 Critical — data loss, security, crash loop platform/matrix Matrix adapter (E2EE) type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants