Skip to content

fix(matrix): fail-closed approval reaction auth (parity with Telegram) - #34567

Merged
teknium1 merged 1 commit into
mainfrom
fix/32877-matrix-approval-fail-closed
May 29, 2026
Merged

fix(matrix): fail-closed approval reaction auth (parity with Telegram)#34567
teknium1 merged 1 commit into
mainfrom
fix/32877-matrix-approval-fail-closed

Conversation

@teknium1

Copy link
Copy Markdown
Contributor

The bug (confirmed live on main)

gateway/platforms/matrix.py:2239 gated approval reactions with:

if self._allowed_user_ids and sender not in self._allowed_user_ids:
    return  # ignore unauthorized

When MATRIX_ALLOWED_USERS is empty, self._allowed_user_ids is falsy, the
whole guard short-circuits, and any user in the room can resolve a
dangerous-command approval prompt
— fail-open.

The fix

_allow_all = os.getenv("GATEWAY_ALLOW_ALL_USERS", "").lower() in {"true", "1", "yes"}
if not _allow_all and not (self._allowed_user_ids and sender in self._allowed_user_ids):
    return  # ignore unauthorized

Fail-closed by default; the deliberate-open-access operator
(GATEWAY_ALLOW_ALL_USERS=true) is explicitly preserved. Mirrors the
already-shipped Telegram callback-auth shape (_is_callback_user_authorized,
telegram.py:558 — same GATEWAY_ALLOW_ALL_USERS fallback).

No regression — every supported config preserved

Configuration Before After
MATRIX_ALLOWED_USERS set (solo/team operator) works works (unchanged)
Paired via /pair works works (unchanged)
GATEWAY_ALLOW_ALL_USERS=true (deliberate open) any user OK any user OK (preserved)
No allowlist, no allow-all any room member can approve ⚠ BUG fail-closed

The only behavior that changes is the last row — and those senders can't pass
Layer-1 admission (_is_user_authorized defaults to deny) to issue a command
in the first place, so no legitimate user loses anything. The bug was an
inconsistency where an unauthorized user could still approve a prompt they
were never authorized to touch.

Verification

tests/gateway/test_matrix_approval_reaction_fail_closed.py — 4 tests, all
pass on current main:

  • test_no_allowlist_no_allow_all_denies — the bug, now closed
  • test_no_allowlist_allow_all_permitsno-regression guard (allow-all operator keeps open approvals)
  • test_listed_sender_permits / test_unlisted_sender_denies — allowlist path unchanged

Part of the approval-gate-bypass cluster (issue #32877 names the single-gate
problem; this closes the Matrix reaction-auth surface). Salvage of #30062.

Closes #30062

Co-authored-by: EloquentBrush0x 283442588+EloquentBrush0x@users.noreply.github.com

…SERS is empty

The _on_reaction approval handler used:

    if self._allowed_user_ids and sender not in self._allowed_user_ids:

When MATRIX_ALLOWED_USERS is not configured, _allowed_user_ids is an
empty set. The short-circuit on the empty set caused the deny block to
never execute, allowing any Matrix room member to approve or deny tool
calls via ✅/❎ reactions — even users that run.py's _is_user_authorized
would reject for regular messages.

Fix mirrors the Telegram _is_callback_user_authorized fix (commit
89d3205, PR #28494): deny by default when no allowlist is configured,
unless GATEWAY_ALLOW_ALL_USERS=true is explicitly set.
@github-actions

Copy link
Copy Markdown
Contributor

🔎 Lint report: fix/32877-matrix-approval-fail-closed vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 9437 on HEAD, 9435 on base (🆕 +2)

🆕 New issues (2):

Rule Count
unresolved-import 1
unresolved-attribute 1
First entries
tests/gateway/test_matrix_approval_reaction_fail_closed.py:16: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_matrix_approval_reaction_fail_closed.py:88: [unresolved-attribute] unresolved-attribute: Unresolved attribute `resolve_gateway_approval` on type `ModuleType`

✅ Fixed issues: none

Unchanged: 4899 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

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

Copy link
Copy Markdown
Collaborator

Competing with open PRs #30062 and #33328 (both fix the same Matrix fail-open approval auth bug). This PR implements the fix differently — using GATEWAY_ALLOW_ALL_USERS env var for parity with Telegram's callback-auth shape.

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

Labels

area/auth Authentication, OAuth, credential pools 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