Do not send an encrypted event no one can read - #19
Merged
Merged
Conversation
Three ways an encrypted send could report success while reaching nobody. All the same shape: a failure or an absence read as an answer. mx_send_encrypted() filtered out its own device by device_id alone. Matrix device ids are scoped to a user, not globally unique, so every other account whose device happened to share the name was silently dropped -- two bots both called BOT, and the only recipient disappears while the send still returns an event id. The self-filter compares (user_id, device_id) now. Devices are skipped when their keys or one-time keys do not verify, and that stays: one bad device must not make a room unusable. Skipping all of them is different. The room key then reaches nobody, mx_crypto_encrypt_for_devices() builds the event for an empty recipient list without complaint, and the caller gets an event id for a message every recipient will fail to decrypt. Reaching zero aborts. A room with no other devices at all is not that case -- it is a room of one, and sending to it is fine, which is tested so the guard cannot swallow it. And /keys/query and /keys/claim both answer 200 with a `failures` map when a server could not be reached, returning whatever they did manage. Reading only the successful half made an unreachable homeserver indistinguishable from a user with no devices. Both helpers gain `strict`: warn by default, error under it. mx_send_encrypted() asks for strict, because encrypting to a user whose devices could not be listed is exactly how a message ends up readable by nobody. The decrypt path keeps the default, where an unlisted device means an unverified sender rather than a lost message. Found by auditing the two calls I had named as the next place to look after the same pattern turned up four times in chat.api's adapter. 148 assertions, three mutations, all caught. R CMD check clean but for the days-since-last-update NOTE.
TroyHernandez
added a commit
to cornball-ai/chat.api
that referenced
this pull request
Aug 5, 2026
cornball-ai/mx.client#19 fixes three ways mx_send_encrypted() could report success while the message reached nobody: a self-filter that compared device_id without user_id and so dropped other accounts sharing a device name, an empty recipient list that still posted the event, and partial /keys/query and /keys/claim responses read as though the successful half were the whole answer. This adapter calls straight into that function and cannot work around any of them, so the floor goes to 0.2.0.1 and CI enforces it rather than declaring it. Every step of reading that floor is checked: a parse that quietly finds nothing would wave through exactly what the check exists to catch, which is what corteza's first version of this did on macOS. CI is red until #19 merges, by construction.
The zero-recipient guard measured what survived mx_crypto_verify_device_map(), which drops what it cannot verify. So a room whose only other device is malformed looked exactly like a room with no other device: nothing left to encrypt to either way, and only the second of those is a reason to go ahead. The unreadable event went out with an event id attached. mx_crypto_verify_device_map() now reports every (user_id, device_id) it was handed on a "seen" attribute, verified or not, and the guard measures against that with our own device removed. A member who has no devices at all still sends: there is genuinely nobody to share a key with, and the event belongs in the room for whatever device they log in later. Both halves are tested, because the difference between them is the whole point. And an explicit recipients = list() is refused. Supplying recipients skips discovery and every guard along with it, so the same unreadable event went out for a caller who did their own lookup and came back with nothing. This is the case that escaped the last round: the tests covered devices skipped for want of a one-time key, which happens after verification, and not devices dropped during it. 160 assertions, three mutations, all caught.
Contributor
Author
|
Both fixed in The guard was measuring the wrong set. Correct — It measures against what the homeserver named now: On your second case — a member with no devices at all — I went the other way, and want to flag it rather than bury it. That one still sends. There is genuinely nobody to share a key with, and the event belongs in the room for whatever device that member logs in later, which can then get the key by request or from backup. Refusing would break a real and ordinary case. Both halves are tested precisely because the difference between them is the whole point of the guard. Explicit You are right about why it escaped: the tests covered devices skipped for want of a one-time key, which happens after verification, and never a device dropped during it. The two paths look the same from the outside and are not. 160 assertions, three more mutations, all caught. chat.api's floor tracks to 0.2.0.2. |
TroyHernandez
added a commit
to cornball-ai/chat.api
that referenced
this pull request
Aug 5, 2026
* Own Matrix E2EE in the adapter Olm/Megolm state moves onto the chat_matrix() client, so E2EE becomes a property of the client a consumer already holds rather than a second API it has to learn. corteza carried this in R/matrix_crypto.R, in parallel to the contract, and reached around chat_poll()$raw to decrypt. - chat_matrix() gains e2ee and crypto_store. e2ee = FALSE is the default and is the previous behaviour exactly: no crypto context, no store touched, no keys published. - chat_send() routes encrypted rooms through mx_send_encrypted(), building the same m.room.message content the cleartext path PUTs, so markdown and mentions render the same either way. The room is asked about per send rather than once at init: a room that turns on encryption between polls must not get one cleartext message first. - chat_poll() folds decrypted events into the messages list it already builds, as ordinary chat_message records. A decrypt that throws warns and loses that traffic; the poll still returns the cleartext messages and advances the cursor, so one missing Megolm session cannot stall the loop. - chat_message() gains encrypted and sender_verified. sender_verified is NULL on cleartext, where the transport asserts the sender and there is nothing to verify, and FALSE on an encrypted message whose claimed sender did not bind to a verified device -- a real answer, not a missing one. - chat_capabilities()$e2ee answers for this client, from whether it holds a crypto context, not from whether mx.crypto happens to be installed. Reporting TRUE off an install would invite a consumer to hand a secret to a client that PUTs it in the clear. Two fixes to what was ported. The crypto store is keyed on the app namespace via mx_crypto_store_dir() instead of dirname(config)/crypto: corteza's version tied the device identity to wherever the config file sat, so moving the config silently minted a new identity and lost every Megolm session. Because that key is the app name alone, an e2ee client built from an explicit path with no app is now an error rather than a guess -- two bots built that way would have shared one Olm account and the second would have come up wearing the first's device keys. And the encrypted-room cache is one room id per line instead of JSON, since chat.api has no dependencies and a character vector needs no parser. A .crypto seam replaces the four crypto operations, which is what lets the e2ee = TRUE paths be tested on a runner with neither mx.crypto nor a Rust toolchain. 69 new assertions; each was checked against a mutated source. 299/299 pass, R CMD check --as-cran clean but for the expected new-submission NOTE. mx.client bound raised to 0.2.0 for mx_crypto_known_devices() and mx_crypto_process_sync(devices = ), which is what makes sender_verified answerable. * rformat + document * Intern the crypto context per identity One identity is one Olm account. A consumer that rebuilds its client per use -- corteza does, deliberately, so the access token that rotates mid-loop is never cached in a stale config -- would otherwise load a second account and republish 50 one-time keys on every build, with two contexts writing over each other's pickles in the same store. Interning reconciles a short-lived client with a long-lived crypto identity: the token is derived at use, the account is not. The key is a plain identity string (the store path, or "app:<name>") rather than a resolved directory, because resolving one calls into mx.client and the .crypto seam exists so these paths run without it. * Fail closed on every E2EE boundary Eight defects in the first pass, all at the edges where the adapter decides between the encrypted path and the clear one. Each had the same shape: a failure treated as an answer. 1. An unanswerable encryption state is no longer a plaintext room. matrix_room_is_encrypted() turned every lookup error -- expired token, timeout, 500 -- into FALSE, and chat_send() then used the cleartext path, so a room not already in the cache could leak one message whenever its state could not be checked. It now aborts the send. A room already known encrypted still short-circuits, so a transient failure cannot block one. 2. Attachments are refused in encrypted rooms. The encryption check ran after the upload loop, so files reached the homeserver in the clear and only the text took the Megolm path -- and an attachment-only send never consulted crypto at all. The check now runs before anything is uploaded, and chat_capabilities()$files reports FALSE on an e2ee client rather than advertising something that fails in exactly the rooms such a client exists for. 3. Stores and interned contexts are keyed to a Matrix device. The cache key was the app name or a raw store string, so two clients built the documented way -- chat_matrix(mx = ..., e2ee = TRUE) -- collapsed onto one entry and shared an Olm account; the explicit-path guard never covered ready mx clients, and is gone in favour of a key that cannot collide by construction. An Olm account belongs to a device, so the key and the store path are (user_id, device_id), both required. The directory name is still sanitized and so still not injective, which is why the exact identity is written into the store and compared on every open: a collision, a copied store, or a changed device_id is an error instead of a silent account swap. 4. A sync is not consumed until its crypto state is on disk. The cursor was committed inside mx_sync_update(); a crash between that and the session save skipped the sync carrying a room key permanently, since the homeserver never re-sends it. On e2ee clients the cursor is now written after the crypto state, through a .save seam. And decrypt errors propagate: mx.client already skips an individual event it has no Megolm session for, so a throw here is to-device processing or persistence, and swallowing it while keeping the advanced cursor acknowledged a sync whose keys were lost. Cleartext clients keep the cursor inside the sync, where the poison-pill protection wants it. 5. Crypto initialization happens after a request has succeeded, not at construction. Publishing keys is authenticated, and the stored token may already be rejected at process start; building in chat_matrix() put that upload ahead of any relogin, so the constructor threw, the poll loop never ran, and every restart repeated with the same dead token. The context is built on first poll or send, off the config that just worked. chat_capabilities()$e2ee therefore reads the setting rather than the context, so it cannot flip after the first poll. 6. Failed membership discovery no longer reports a successful send. mx_send_encrypted() derives its recipients from member_ids, so the character() that a swallowed lookup error produced shared the room key with nobody, posted the m.room.encrypted event anyway, and returned an event id -- unreadable to the room, recorded as sent by the caller. Membership failure and an empty member list both abort. Send errors propagate too, matching the cleartext path. 7. Sender verification can no longer be falsely negative forever. sender_bound is stamped once, when a room key arrives over to-device, and persisted with the session; a later sync carrying a timeline message from that sender does not rebind it. The device query asked only about timeline senders, so a key arriving in a to-device-only sync -- the normal case, since the key is shared before the message -- was recorded unverified permanently. To-device envelope senders are now included. 8. Mixed timelines keep the homeserver's order. Decrypted records were appended after the cleartext ones, so an encrypted message followed by a plain reply came back reversed, which reorders a room's commands against the messages they act on. Both sets are now folded back into sync order; anything the sync did not position sorts last, in arrival order. Two of these initially escaped their own tests: matrix_room_is_encrypted() and matrix_crypto_send() sit behind the .crypto seam, so mutating them changed nothing the suite could see. They are now driven directly against stubbed mx.client/mx.api entry points in test_matrix_mxclient.R, along with matrix_crypto_init()'s store binding. 354 -> 386 assertions, ten mutations checked, all caught. R CMD check clean but for the expected new-submission NOTE. * Hold the in-memory cursor too, and stop forcing mx.client on every client Two blockers from review, one of which was turning CI red. The advanced cursor was kept off disk on a crypto failure but left live on the client, which only moved the skip. A caller that caught the decrypt error and polled the same client again resumed from the token the failed sync produced, and the room keys in it are gone for good -- the homeserver does not re-send them. Reproduced against 0.0.1.4: after a decrypt error, env$mx$sync_token was the new token, not the old one. The e2ee path now holds the pre-sync cursor on the client until the crypto state is safe, and only then makes the advanced one live. A relogin's refreshed credentials survive the rollback, because a rotated token is not what makes a sync consumed -- dropping it would have the retry authenticate with the one the homeserver just rejected. Cleartext clients are untouched: nothing between their sync and their cursor can fail, and the sync already wrote it. And save_fn was defaulted in the constructor. The other four seams read `.seam %||% mx.client::fn`, and R's lazy `%||%` never forces the mx.client side when a seam is supplied -- which is exactly what makes the documented four-seams-without-mx.client configuration work. A fifth default resolved unconditionally forced mx.client on every client, cleartext ones included, and both CI legs failed with "there is no package called 'mx.client'". It is resolved where it is used now, on the deferred-save path, which only e2ee reaches. Verified the way the claim is made: chat.api installed into a library holding nothing but base, where mx.client is absent. The documented configuration polls and sends, and test_matrix.R runs 236 assertions clean. It could not have before -- that file had also picked up a matrix_crypto_store() assertion that resolves a path through mx.client, which has moved to test_matrix_mxclient.R where the skip is announced. CI now installs mx.api and mx.client so test_matrix_mxclient.R runs rather than skipping. That file holds the direct tests for the two crypto boundary functions that leak if they fail open, and they sit behind the .crypto seam where nothing in test_matrix.R can reach them -- so without this they would have gone back to skipping into a green check the moment the constructor stopped needing mx.client. mx.crypto is Linux-only, from the r2u binary, since building it needs a Rust toolchain; its one test is conditional and reports its absence rather than passing silently. Also corrects the e2ee documentation, which claimed first "poll or send" publishes keys after a relogin. mx_with_relogin() wraps only the sync, so that is true of a first poll and not of a send that precedes one. 403 assertions, 0 fails, both new fixes mutation-checked. R CMD check clean but for the expected new-submission NOTE. * CI: install curl and jsonlite before mx.api chat.api has no dependencies of its own, so install_deps brings in nothing and mx.api's Imports were missing. * Key the crypto cache on the device identity alone The cache key carried the store as well as (user_id, device_id), and the tests said two stores for one device were two contexts. Matrix gives a device one long-lived ed25519 and one curve25519 key for the life of that device_id, so that was wrong in two ways: two stores minted two Olm accounts, and therefore two identity keys for one device; and two spellings of one directory produced two independent mutable contexts over one set of pickles, each overwriting the other's Megolm sessions. identity.txt already enforced store -> identity. This is the direction that file cannot see, identity -> store. The key is now the identity and nothing else, and the store request is recorded beside the context. A second store for a device that already has one is refused rather than silently ignored -- the caller asked for a store and would otherwise have got a different one without being told -- and rather than silently honoured, which is the bug. Re-homing a device's account is a re-provision, and a re-provisioned device gets a new device_id; matrix_crypto_forget() is the way out for anything that really means to start over. Store requests normalize first, so `/tmp/s` and `/tmp/s/` are one request. Trailing separators are stripped by hand because normalizePath() leaves a path that does not exist yet exactly as given, and a store's first use is exactly when it does not exist. corteza's default path never triggered any of this -- one config, one store -- but chat_matrix() is public and this is its contract. Also corrects two comments that outlived their claims: the initialization comment said the context is built after a successful request, which is true from chat_poll() and not from a chat_send() that precedes any poll (the public docs were already fixed); and corteza's floor rationale explained 0.0.1.3 while requiring 0.0.1.5. 412 assertions, 245 of them on a bare runner with no mx.* installed. Three mutations checked, all caught. * Check the account against the keys the homeserver already published The one-store-per-device rule lived only in .crypto_cache, which meant it lived only for the length of a process. Restart, or call matrix_crypto_forget(), and a changed crypto_store would mint a fresh Olm account and go on to publish different long-lived keys under the old device_id. "Re-homing needs a new device_id" was a comment, not a check. So init now asks the homeserver what keys it already holds for this device and refuses if they are not this account's. That record outlives the process, which is the property the cache could never have. A device the server has never seen is the first run and proceeds. A query that cannot be answered is an error: init is about to publish keys to that same homeserver, so being unable to ask it anything is not a state to publish from, and the caller retries on the next poll. The check runs before the upload, so a mismatch never reaches it. Three things now hold the invariant, at three lifetimes: identity.txt binds a store to a device, the cache binds a device to one store within a process, and this binds a device to its keys for as long as the homeserver remembers them. Path normalization is lexical now, not normalizePath(). That resolves symlinks, which this does not, but it returns a path that does not exist yet exactly as given -- and a store's first use is exactly when it does not exist. Two calls for one directory would disagree depending on whether it had been created between them, which is worse than not following symlinks: the comparison has to be stable over the store's whole life, not accurate at one moment of it. So `/missing/a/../b` and `/missing/b` are one spec, `/missing/./b` too, and `C:/` keeps its slash -- the trailing-separator strip this replaces ate the slash off a Windows drive root and left `C:`. 431 assertions, 256 of them on a bare runner. Four mutations, all caught. * Tell an unverifiable device record from an absent one matrix_crypto_check_published() went through mx_crypto_known_devices(), which verifies signatures and drops what fails with a warning. That is right for choosing who to encrypt to and wrong for asking what this device already published: a tampered or signature-stripped entry for our own device came back as no entry, collapsed "present but unverifiable" into "absent", and got published over. The same failure-read-as-absence this whole review keeps finding, one layer down. It reads the raw /keys/query response now, with three outcomes that stay three: absent is a first run, present and valid is compared, present and unverifiable is an error. The tests stub the query rather than the already-filtered helper, so the third case is reachable at all -- it was not before. Still out of reach: a homeserver that omits the device on purpose looks exactly like a first run over this channel. Closing that needs key pinning or cross-signing, noted in the code and the docs rather than implied away. Path roots are no longer all "/". Collapsing every leading slash run made //server/share/x, \\server\share\x and /server/share/x one spec, and on Windows the first two are a UNC share while the third is a local path -- so a request naming one store would have been handed another's context, which is the collision the check exists to stop. Three or more leading slashes are POSIX, not UNC, and still collapse. "C:relative" keeps its drive prefix instead of being resolved against getwd(): Windows resolves it against the current directory on that drive, which R cannot reconstruct portably, and a spec that cannot be resolved is better left unmerged than merged wrong. 444 assertions, 266 of them on a bare runner. Three mutations, all caught. * test_irc.R: define %||% before using it Base R only grew `%||%` in 4.4.0. The file defined its own at line 17 and used one at line 7, so every run so far had been borrowing base's -- invisible on the R this usually runs against, an error on the R 4.0 the package's Depends claims to support. The Ubuntu CI leg picked distro R 4.3.3 instead of r2u's 4.6.1 this run, which is an apt-source flake rather than anything here, but it is the only reason the bug ever surfaced. * Spell out the last inherited %||% in the tests test_matrix_mxclient.R had one too, in the stub helper added this round. Every test file now supplies its own or does without, verified by running each of the five with an inherited %||% poisoned to error. * Treat a partial /keys/query as unanswered, and refuse drive-relative stores Two more boundaries where a failure was reading as an absence. /keys/query answers 200 with a `failures` map when it could not reach a server, returning whatever it did manage. The check ignored it, so an empty device_keys beside a non-empty failures map was classified as a first run and published over. This query names exactly one user -- ours -- so any failure at all means the question went unanswered, and a request that throws and one that succeeds with nothing in it are the same state to a caller about to publish keys. An empty failures map is still the ordinary first-run answer and is tested to stay that way. And "C:store" is drive-relative: Windows resolves it against the current directory of that drive, which R cannot read. Treating "C:" as a root to fold against made "C:../x", "C:x" and "C:a/../../x" one spec when they are three directories, so two requested stores could still land on one cached context. Keeping unresolved leading ".." would make the spec honest without making it canonical -- two spellings of one directory would stay two -- so there is no correct answer available and this asks for one that is. Absolute drive paths are unaffected. 451 assertions, 270 on a bare runner. Two mutations, both caught. * Require the mx.client that will not send to nobody cornball-ai/mx.client#19 fixes three ways mx_send_encrypted() could report success while the message reached nobody: a self-filter that compared device_id without user_id and so dropped other accounts sharing a device name, an empty recipient list that still posted the event, and partial /keys/query and /keys/claim responses read as though the successful half were the whole answer. This adapter calls straight into that function and cannot work around any of them, so the floor goes to 0.2.0.1 and CI enforces it rather than declaring it. Every step of reading that floor is checked: a parse that quietly finds nothing would wave through exactly what the check exists to catch, which is what corteza's first version of this did on macOS. CI is red until #19 merges, by construction. * Track the mx.client floor to 0.2.0.2 0.2.0.2 measures the zero-recipient guard against the devices the homeserver named rather than the ones that verified, so a room whose only other device is malformed no longer reads as a room of one, and refuses an explicitly empty recipient list.
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
1 participant
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.
Three ways
mx_send_encrypted()could report success while the message reached nobody. All the same shape: a failure or an absence read as an answer.The self-filter ignored user identity
mx_send_encrypted()excluded every device whosedevice_idmatched its own. Matrix device ids are scoped to a user, not globally unique — so if Alice and Bob both have a device calledBOT, filtering bydevice_idalone drops Bob. The only recipient disappears and the send still returns an event id.The filter compares
(user_id, device_id)now.Zero usable recipients still posted an event
Devices are skipped when their
device_keysor one-time keys fail verification. That is deliberate and stays: one bad device must not make a room unusable.Skipping all of them is a different thing. The room key reaches nobody,
mx_crypto_encrypt_for_devices()builds the event for an empty recipient list without complaint,mx_send_encrypted()posts it, and the caller is handed an event id for a message every recipient will fail to decrypt.Reaching zero now aborts. A room with no other devices at all is not that case — that is a room of one, and sending to it is fine. There is a test for it, so the guard cannot quietly swallow the solo-room case.
Partial responses were read as empty ones
/keys/queryand/keys/claimboth answer 200 with afailuresmap when a server could not be reached, and return whatever they did manage. Both were read as though the successful half were the whole answer, so an unreachable homeserver was indistinguishable from a user with no devices.mx_crypto_known_devices()andmx_crypto_claim_otks()gainstrict: they warn by default and error under it.mx_send_encrypted()asks for strict, because encrypting to a user whose devices could not be listed is exactly how a message ends up readable by nobody. The decrypt path keeps the default, where an unlisted device means an unverified sender rather than a lost message — a warning is the right level there.strictdefaults to FALSE, so both exported signatures stay backwards compatible.Provenance
Found by auditing the two calls in this path I had flagged as the next place to look, after the same pattern turned up four times in chat.api's Matrix adapter during review of cornball-ai/chat.api#5.
mx_crypto_publish_keys()was audited too and has no analogous hole: transport errors propagate and the account is marked published only after the upload returns.148 assertions, three mutations run, all caught.
R CMD check --as-cranclean but for the days-since-last-update NOTE.Downstream
chat.api's
mx.clientfloor wants raising to 0.2.0.1 once this merges — its adapter calls straight intomx_send_encrypted()and cannot work around any of the three. That puts this PR at the head of the chain: mx.client, then cornball-ai/chat.api#5, then the drat, then cornball-ai/corteza#169.