fix(matrix-communication): don't report verify success on MAC mismatch; fetch keys in --listen#48
Merged
Conversation
…h; fetch keys in --listen Follow-up to #47. - MAC handler: nio's receive_mac_event silently sets the SAS state to canceled on a key mismatch (it never raises), so the previous code sent m.key.verification.done and reported success even on a genuine mismatch (possible MITM). Distinguish the benign state-machine race from a real failure via sas.verified_devices; on a real mismatch, send a cancellation and mark the attempt cancelled instead of claiming success. - --listen mode returned right after verification, skipping the post-verification room-key fetch that the request path performs. Extract the fetch into a shared fetch_room_keys() helper and run it in both flows. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Matrix device verification script to handle nio's SAS state-machine race conditions more robustly, ensuring genuine key mismatches trigger cancellation messages. It also extracts the post-verification room key fetching logic into a reusable fetch_room_keys function. The review feedback suggests two important improvements: filtering for encrypted rooms before slicing the room list to guarantee up to 10 encrypted rooms are checked, and wrapping the post-verification client.sync calls in a try-except block to prevent transient network errors from crashing the script after successful verification.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
… and sync errors Addresses review feedback on PR #48: - Filter rooms to encrypted ones before applying the [:10] limit, so up to 10 encrypted rooms are always checked instead of 10 arbitrary rooms that could all be unencrypted. - Guard the post-fetch sync loop so a transient sync error cannot crash the script (and report failure) after verification has already succeeded. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
|
CybotTM
added a commit
that referenced
this pull request
Jun 27, 2026
) From a cross-session retrospective (2026-06-27). Adds one learning to the E2EE guide (`skills/matrix-communication/references/e2ee-guide.md`) — references-only, no script behavior change. ## What changed Two related items added to `references/e2ee-guide.md`: 1. **Gate device trust on the verification result.** Documents the security rule that `client.verify_device(...)` must be gated on the SAS/emoji outcome (`sas.verified` / `sas.verified_devices`) — an unconditional `verify_device` trusts a device even on a MAC mismatch (a MITM bypass). Captures the matrix-nio subtleties (`receive_mac_event` never raises on a bad MAC; `sas.verified` can be `False` after a successful exchange due to a state-machine race, so `verified_devices` is the authoritative signal). This bypass was caught by the automated commit security review and fixed in #48. 2. **matrix-nio API notes (0.25.2)** — maintainer notes verified against the installed nio 0.25.2: - `nio.__version__` does not exist (raises `AttributeError`) — use `importlib.metadata.version("matrix-nio")`. - `DeviceStore` is **not** importable from `nio.store` (`ImportError`); the real path is `from nio.crypto import DeviceStore`. - `device.id` is a read-only property aliasing `device_id` and does **not** raise — a reviewer claim that it errors is incorrect. All three nio facts were re-verified against matrix-nio 0.25.2 in this environment before documenting. ## Version No version bump — references-only change. The `check-version-parity` hook only triggers on `plugin.json` / `SKILL.md` / `composer.json`, none of which were touched (hook skipped cleanly in pre-commit). ## Validation `pre-commit run --files` on the changed file: trailing-whitespace, end-of-file-fixer, markdownlint-cli2 all **Passed**; version-parity and other hooks correctly skipped (no relevant files).
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.
Follow-up to #47 (already merged), addressing two issues found in review of that PR.
1.
matrix-e2ee-verify.pyreported verification success on a genuine MAC mismatchnio's
Sas.receive_mac_eventnever raises on a bad MAC — on a key mismatch (or invalid/unexpected MAC event) it silently sets the SAS state toSasState.canceledand returns (crypto/sas.py). The merged code caught only exceptions, so a genuine mismatch (a possible MITM) fell through toself.verified = True, sentm.key.verification.done, and printed✅ VERIFICATION COMPLETE/🎉 Device verified and keys synced!.#47 correctly gated the trust-persisting
verify_device()call onsas.verified, so a bad device was not trusted in the local store — but the script still told the user it succeeded, which is false assurance for a security-critical step.This change distinguishes the benign state-machine race (
sas.verifiedcan beFalseeven after a successful exchange) from a real failure usingsas.verified_devices— a device id lands there only once its MAC has been cryptographically validated, and a later cancel does not clear it. On a real mismatch it now sends a cancellation (sas.get_cancellation()) and marks the attempt cancelled instead of claiming success.2.
--listenmode skipped the post-verification room-key fetchThe
--listenbranch returned immediately after verification, so it never ran the room-key fetch that the request path performs (the documented purpose: "complete verification and fetch room keys"). A user verifying via--listensawDevice verified!but had to runmatrix-fetch-keys.pymanually before encrypted history would decrypt.The fetch is extracted into a shared
fetch_room_keys()helper and now runs in both the request and--listenflows.Notes
matrix-niosource (crypto/sas.py: silentcanceledon mismatch;verified_devicespopulated only on validated MAC;get_cancellation()available).ruff checkandruff format --checkpass;py_compileclean.Test plan
matrix-e2ee-verify.pywith matching emojis →VERIFICATION SUCCESSFUL, device trusted, keys fetched.matrix-e2ee-verify.pywith mismatched emojis (decline in Element) →VERIFICATION FAILED, no success message, device not trusted.matrix-e2ee-verify.py --listen→ after Element-initiated verification, room keys are fetched (not justDevice verified!).🤖 Generated with Claude Code