Skip to content

Own Matrix E2EE in the adapter - #5

Merged
TroyHernandez merged 14 commits into
mainfrom
feat/e2ee-adapter
Aug 5, 2026
Merged

TroyHernandez merged 14 commits into
mainfrom
feat/e2ee-adapter

Conversation

@TroyHernandez

@TroyHernandez TroyHernandez commented Aug 5, 2026 •

Copy link
Copy Markdown
Contributor

Moves Olm/Megolm state 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.

Two review rounds have landed on top of the original change; this body describes the current state.

What the adapter does

  • 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, not read off a cached set, so a room that turns on encryption between polls does not get one cleartext message first.
  • chat_poll() folds decrypted events into the messages list, in the homeserver's own timeline order rather than appended after the cleartext ones.
  • 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.
  • chat_capabilities() reports e2ee from the client's setting, and files = FALSE on an e2ee client.

Slack, IRC, and loopback keep e2ee = FALSE untouched.

Everything fails closed

The first pass got this wrong in six places, all the same shape: a failure treated as an answer. Current behaviour:

  • An unanswerable encryption state aborts the send. A lookup error used to become FALSE, and the message went out in the clear. A room already known encrypted still short-circuits on the cached set, so a transient failure cannot block one.
  • Attachments are refused in encrypted rooms, before anything is uploaded. mx_send_media() posts a cleartext m.file event; the check used to run after the upload loop, and an attachment-only send never consulted crypto at all.
  • Membership discovery is not best-effort. An empty member_ids has mx_send_encrypted() share the room key with nobody, post the event anyway, and return an event id — unreadable to the room, recorded as sent. Both the lookup error and an empty list abort. Send errors propagate, matching the cleartext path.
  • A sync is not consumed until its crypto state is stored, on disk and on the client. The cursor is written after the crypto state via a .save seam, and the pre-sync cursor stays live on the client until everything has succeeded — otherwise a caller that catches the error and re-polls the same client skips the sync whose room keys were lost. Decrypt errors propagate; mx.client already skips an individual event it has no session for, so a throw here is to-device processing or persistence. Cleartext clients keep the cursor inside the sync, where the poison-pill protection wants it.
  • Crypto initialization is deferred out of the constructor. Publishing keys is authenticated and the stored token may already be rejected at startup, so building in the constructor put that upload ahead of any relogin: chat_matrix() threw, the poll loop never ran, and every restart repeated with the same dead token. Built on first poll or send now. (mx_with_relogin() wraps only the sync, so a send preceding any poll publishes with the token it has — the same exposure a direct send already carries.)
  • Sender verification cannot be falsely negative forever. sender_bound is stamped once when a room key arrives over to-device and persisted with the session, with no rebinding path. The device query asked only about timeline senders, so a key arriving in a to-device-only sync — the normal case — was recorded unverified permanently. To-device envelope senders are now included.

Identity

A Matrix device has one Olm identity for its whole life, and three things hold that, at three lifetimes:

  • identity.txt in the store binds it to one (user_id, device_id), and refuses to open for another.
  • The in-process cache gives one device one context and one store, so a second store is an error rather than a second account.
  • On first use the account is checked against the keys the homeserver already holds for that device_id. This is the only one that survives a restart, and so the only one that catches a store swapped between runs -- the other two are per-store and per-process. It reads the raw /keys/query response and keeps three outcomes distinct: absent is a first run, present-and-matching is this account, and present-but-differing or present-but-unverifiable is an error. A query that cannot be answered is also an error -- whether it throws, or returns 200 with a failures map and an empty result beside it -- since init is about to publish to that same homeserver. What it cannot see is a homeserver that omits the device on purpose, which needs key pinning or cross-signing.

The store and the interned context are keyed on (user_id, device_id), both required. An Olm account belongs to a device, so anything coarser is a store two identities can share. The directory name is sanitized and so not injective — @a/b:ex and @a_b:ex collide — 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 errors instead of silently swapping accounts.

Contexts are interned per identity so a consumer can rebuild its client freely. corteza does that on purpose, so the token that rotates mid-loop is never read from a stale config; without interning every send would reload the account pickle and republish 50 one-time keys.

The four-seams contract still holds

chat_matrix() documents mx plus .sync/.extract/.send/.media as enough to poll and send with no mx.client installed. That works because %||% is lazy. A fifth default resolved unconditionally broke it for every client and turned both CI legs red; the save function is resolved where it is used instead. Verified against a library containing nothing but base: the documented configuration polls and sends, and test_matrix.R runs 270 assertions clean.

Testing

A .crypto seam replaces the four crypto operations, so the e2ee = TRUE paths run on a runner with neither mx.crypto nor a Rust toolchain. Two fixes initially escaped it — matrix_room_is_encrypted() and matrix_crypto_send() live behind that seam, so reverting 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.

CI installs mx.api and mx.client so that file runs rather than skipping, and fails loudly if they are absent. mx.crypto is Linux-only (building it needs Rust); its one conditional test reports its absence rather than passing silently.

296 -> 451 assertions, 270 of them verified on a bare runner with nothing but base installed. Twenty-four mutations run across the review, all caught. 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.

Downstream

cornball-ai/corteza#169 deletes R/matrix_crypto.R and drops mx.crypto from its Suggests. It is a draft until this merges, and its CI now clones chat.api main, so it stays red until then by construction.

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.
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.
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.
@TroyHernandez

Copy link
Copy Markdown
Contributor Author

All eight findings confirmed and fixed in 140b1a6. Each was verified against the code before changing anything, and the CI evidence problem checked out too — I'll cover that below.

The six blockers

1. Encryption-state lookup failed open. matrix_room_is_encrypted() now aborts the send when the state cannot be determined, instead of answering FALSE. A room already known encrypted still short-circuits on the cached set, so a transient failure cannot block one; only an unknown room is affected, which is exactly the case that could leak.

2. Attachments in encrypted rooms. The check moved ahead of the upload loop and chat_send() refuses files in an encrypted room. Attachment-only sends are covered by the same guard — they previously never consulted crypto at all. chat_capabilities()$files is FALSE on an e2ee client, per your point that TRUE is not honest for one.

3. Store and cache identity. Both are now keyed on (user_id, device_id), both required. The explicit-path guard is gone rather than extended — a key that cannot collide by construction is better than a guard that only covered one of the two ways to build a client. The sanitization is still non-injective, so the exact identity is written into the store and compared on every open: @a/b:ex and @a_b:ex still land in the same directory, and the second one to open it now errors. corteza stops passing its own user_id-only store entirely.

4. Cursor before crypto state. On e2ee clients the cursor is written after the crypto state, through a new .save seam, and decrypt errors propagate. You're right that the swallow was the worse half: individual missing Megolm sessions are already skipped inside mx.client, so a throw reaching that handler was to-device processing or persistence. Cleartext clients keep the cursor inside the sync, so the poison-pill protection is only traded where durability demands it.

5. Init before relogin. The context is built on the first poll or send, off the config that just succeeded — so key publication happens after mx_with_relogin() rather than before it. chat_capabilities()$e2ee reads the setting rather than the context, since a capability that flipped after the first poll would be its own problem.

6. Membership discovery. Both the lookup error and an empty member list abort. Encrypted send errors propagate too, matching the cleartext path.

The two correctness issues

7. To-device senders are included in the device query. Confirmed the mechanism: sender_bound is stamped once at mx_crypto_process_sync() step 1 and persisted with the session, with no rebinding path, so a key arriving in a to-device-only sync — the normal case — was recorded unverified permanently.

8. Ordering. Both sets fold back into the sync's own event order, taken from the timeline walk rather than origin_server_ts (two events in a room can share a millisecond). Unpositioned events sort last in arrival order.

Two of these escaped their own tests

matrix_room_is_encrypted() and matrix_crypto_send() sit behind the .crypto seam, so my first mutation run reverted both fixes and the suite stayed green. 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 run, all caught.

CI

Confirmed, and worse than "did not install": the run log shows chat.api, mx.api, mx.client all "suggested but not available", and test_matrix_transport.R:25 exits the file, so every transport assertion on that branch was skipped. corteza's workflow now installs the stack from source and fails if either declared floor is unmet, before the tests run. Floors are read from the checkout's own DESCRIPTION so they cannot drift.

Version is 0.0.1.4. Nothing merged, drat still unpushed, corteza #169 still draft.

…ient

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.
chat.api has no dependencies of its own, so install_deps brings in
nothing and mx.api's Imports were missing.
@TroyHernandez

Copy link
Copy Markdown
Contributor Author

Both blockers fixed in a8136de, CI fixed in 04d5a01, and CI is now green on both legs — for the first time on this branch, and on something real.

1. The in-memory cursor. Reproduced exactly as you described against installed 0.0.1.4: after a decrypt error, env$mx$sync_token was "new". You're right that keeping it off disk only moved the skip — the reusable client is the durability boundary, not the file.

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: a rotated token is not what makes a sync consumed, and 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.

Three new assertions cover it, including the one that was missing: catch the error, poll the same client again, and check where the second sync resumed from.

2. The eager .save. Correct diagnosis. The other four defaults survive a bare runner only because %||% is lazy and never forces the mx.client side when a seam is supplied; a fifth resolved unconditionally broke that for every client. It is resolved at the deferred-save call site now.

Verified the way the claim is made rather than by inspection: chat.api installed into a library holding nothing but base, mx.client 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.

3. Coverage. Fixed, and it needed fixing twice: mx.api's Imports are not chat.api's, so the first attempt failed on missing curl/jsonlite. CI now installs mx.api and mx.client and fails the job if either is absent, which is stronger than asserting inside a file that can skip itself. Confirmed from the passing run: mx.api 0.3.0, mx.client 0.2.0, mx.crypto 0.2.1 on Linux. mx.crypto is Linux-only from the r2u binary since building it needs Rust; macOS logs mx.crypto absent (crypto-init test will not run) rather than passing silently.

4. Stale descriptions. This body is rewritten to describe the current state. And you're right about the documentation overstating it: mx_with_relogin() wraps only the sync, so "first poll or send publishes after a relogin" is true of a poll and not of a send that precedes one. Reworded to distinguish them.

403 assertions, 0 fails. Twelve mutations run across the two rounds, all caught.

corteza #169 is red on exactly one thing, which is the intended blocked state:

Error: chat.api 0.0.1.2 is below the 0.0.1.5 this corteza requires

Both legs, same reason. Its verify step also had the failure mode you'd expect me to have introduced: the first version came from a DESCRIPTION parse that silently found no floors on macOS and waved 0.0.1.2 through while Ubuntu rejected it. A verification step that can quietly check nothing is the same defect the job exists to prevent, so it reads corteza's own runtime constants now and errors if it cannot.

Nothing merged, drat still unpushed.

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.
@TroyHernandez

Copy link
Copy Markdown
Contributor Author

Finding 1 fixed in 4c00c3e; the two low ones too. chat.api CI stays green on both legs.

The cache is now one context per identity, full stop. You were right that the store had no business in the key, and the second bullet is the worse half: two spellings of one directory gave two independent mutable contexts over one set of pickles, each overwriting the other's Megolm sessions. That is corruption, not just a spec violation.

The key is (user_id, device_id) alone. identity.txt was only ever enforcing store → identity; this is the direction it cannot see.

What to do about a conflicting request took some thought. Silently returning the existing context is wrong — the caller named a store and would get a different one without being told. Silently honouring it is the bug. So a second store for a device that already has one is refused, and matrix_crypto_forget() is the escape hatch for anything that genuinely means to re-home. Re-homing a device's Olm account is a re-provision, and a re-provisioned device gets a new device_id.

Store requests normalize first, so /tmp/s and /tmp/s/ are one request. Trailing separators are stripped by hand rather than left to normalizePath(), which returns a nonexistent path unchanged — and a store's first use is exactly when it does not exist yet. My first attempt did leave it to normalizePath() and the suite caught it immediately.

corteza's fixed default never triggered any of this, as you say, but chat_matrix() is public and this is its contract.

Both low findings are fixed: the initialization comment no longer claims the context follows a successful request unconditionally — it now separates the poll case from the send case the way the public docs already did — and corteza's floor rationale lists every step with its reason instead of explaining a version two behind what it requires. corteza's stale CI comment and PR body are corrected too; the body now says plainly that the workflow clones each repo's default branch and reads floors from corteza's runtime constants, and names the macOS parse failure that made the DESCRIPTION version unsafe.

412 assertions, 245 of them verified on a bare runner with no mx.* installed at all. Three mutations on this round, all caught. Fifteen across the review.

corteza #169 is red on the one line it should be, both legs:

Error: chat.api 0.0.1.2 is below the 0.0.1.6 this corteza requires

chat.api is 0.0.1.6, corteza 0.7.1.8. Nothing merged, drat still local at two commits.

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.
@TroyHernandez

Copy link
Copy Markdown
Contributor Author

Both fixed in 1cd8b8a, and the body is refreshed.

1. Durable identity continuity. You're right that the cache only ever held this for the length of a process, and that "re-homing needs a new device_id" was commentary. A restart does it automatically, which is the case that actually matters for a bot systemd keeps restarting.

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 no in-process structure could have had. Behaviour:

  • device the server has never seen → first run, proceed
  • keys match → this account is that device, proceed
  • keys differ → refuse, naming both key prefixes
  • query fails → error, because init is about to publish keys to that same homeserver, so being unable to ask it anything is not a state to publish from. Retried on the next poll.

The check runs before the upload, so a mismatch never publishes. Asserted separately, since the ordering is the whole point.

Three things hold the invariant now, 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. Only the third survives a restart, and that gap is what you found.

I did consider a local pointer file for identity → store, but it has the same defect as the cache one level up: the pointer's own location depends on app, so changing app walks around it. The homeserver is the authority on what keys a device_id has, so that is what to ask.

2. Path normalization. All three cases reproduced, including C:/ → C:. It's lexical now, not normalizePath().

The deciding argument is one you didn't quite state but that falls out of your first bullet: normalizePath() returns a nonexistent path unchanged, so the same store path gave one spec before it existed and another after. Two calls in one process could disagree because the directory got created in between. Stability over the store's whole life matters more here than following symlinks at one moment of it, so the trade is deliberate and noted in the comment. /missing/a/../b, /missing/./b, C:/, C:\x\y, /../x, and the exists-vs-not-exists pair are all covered.

3. Body updated: 431 assertions, 256 bare, nineteen mutations. The initialization heading is now "deferred out of the constructor", which is what it actually claims. Added a section spelling out the three identity mechanisms and which lifetime each covers.

431 assertions, 256 of them on a runner with nothing but base installed. Four mutations this round, all caught. Both CI legs green.

chat.api is 0.0.1.7, corteza 0.7.1.9 with the floor raised to match. Nothing merged; drat still local at two commits.

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.
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.
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.
@TroyHernandez

Copy link
Copy Markdown
Contributor Author

Both fixed in f0e7fb0, plus one thing CI turned up on its own.

1. Unverified is not absent. Correct, and it is the same shape as everything else this review has found — one layer further down than I looked. mx_crypto_known_devices() verifies and drops with a warning, which is right for choosing who to encrypt to and wrong for asking what this device already published.

It reads the raw /keys/query response now, and the three outcomes stay three: absent → first run, present and matching → this account, present and either differing or unverifiable → error. The tests stub the query rather than the already-filtered helper, so the third case is reachable at all; it was not before, which is why nothing caught it.

The deliberate-omission gap is called out in the code and the docs rather than implied away. Agreed it needs pinning or cross-signing, and that is not this PR.

2. UNC paths. Reproduced all three collapsing to /server/share/x. The root is no longer always /: ^//[^/] takes the host and share as the root, three or more leading slashes stay POSIX and collapse, and dot segments fold inside a UNC path without climbing past the share. 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.

3. corteza #169's body now says 0.0.1.8, as does its blocker notice.

And one you did not ask for. The Ubuntu leg failed once mid-round on could not find function "%||%" in test_irc.R. Two separate things:

  • An apt flake: the runner picked distro R 4.3.3 instead of r2u's 4.6.1. Transient, and it is back on 4.6.1 now.
  • A real latent bug it exposed. test_irc.R defined %||% at line 17 and used one at line 7, so every run so far had been silently borrowing base's — which only exists from R 4.4.0, while this package's Depends says R 4.0. test_matrix_mxclient.R had one too, in a helper I added this round.

Both fixed, and since CI is back on 4.6.1 and would not re-prove it, I checked directly: each of the five test files runs with an inherited %||% poisoned to error. All five pass, so none of them is relying on base's any more.

444 assertions, 266 on a bare runner. Three mutations this round, twenty-two across the review, all caught. Both legs green.

chat.api 0.0.1.8, corteza 0.7.1.10. Nothing merged; drat still local at two commits.

…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.
@TroyHernandez

Copy link
Copy Markdown
Contributor Author

Both fixed in dcb6a83. Both legs green.

1. Partial /keys/query. Confirmed — the code read resp$device_keys and never looked at resp$failures, so a 200 carrying an unreachable server plus an empty result was classified as a first run and published over.

Since the query names exactly one user, ours, any non-empty failures means the question went unanswered, so it aborts before looking at the entry at all. 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 there is a test pinning that so this does not start erroring on the common path.

Your suggested fixture is what the test uses.

2. Drive-relative ... Reproduced all three collapsing to C:x. Of your two options I took the second: drive-relative stores are rejected.

Preserving unresolved leading .. would make the spec honest without making it canonical — C:../x and C:a/../../x are the same directory and would still be two specs, so the cache could refuse a legitimate second request rather than merge two illegitimate ones. Less bad, still wrong. Since the meaning depends on per-drive state R cannot read, there is no canonical form available, and asking for an absolute path is the only answer that is actually correct. Absolute drive paths are unaffected.

451 assertions, 270 on a bare runner. Two mutations this round, twenty-four across the review, all caught.

chat.api 0.0.1.9, corteza 0.7.1.11. Both PR bodies updated. Nothing merged; drat still local at two commits.

Eight rounds, and the pattern has held every time: the invariant was right at one scope and absent at the next one out. In-process, then across restart, then through the verification helper, now through a partial response. If there is a ninth I would look at the same seam again — mx_crypto_publish_keys() and mx_crypto_claim_otks() are the two calls in this path I have not audited for the same shape.

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.
@TroyHernandez

Copy link
Copy Markdown
Contributor Author

The audit turned up both, and they are real. Fixed in cornball-ai/mx.client#19, which now sits at the head of this chain.

Device ids are scoped to a user. mx_send_encrypted() filtered its own device by device_id alone, so Alice and Bob both having a device called BOT meant Bob was dropped — the only recipient gone, the send still returning an event id. The filter compares (user_id, device_id) now.

Zero usable recipients posted anyway. Confirmed the whole path: mx_crypto_encrypt_for_devices() builds the event for an empty recipient list without complaint, mx_send_encrypted() posts it, caller gets an event id for a message nobody can decrypt.

Reaching zero aborts. I kept the distinction you drew — skipping some devices stays skip-and-warn — and added one more: a room with no other devices at all is a room of one, not a failure, and there is a test pinning that so the guard cannot swallow the solo case.

Partial responses. Both helpers gain strict: warn by default, error under it. mx_send_encrypted() asks for strict. The decrypt path deliberately does not — there an unlisted device means an unverified sender, not a lost message, so a warning is the right level. strict = FALSE keeps both exported signatures backwards compatible.

mx_crypto_publish_keys() I audited too and it has no analogous hole, matching what you found.

This PR is now red on purpose

chat.api's mx.client floor goes to 0.2.0.1, because this adapter calls straight into mx_send_encrypted() and cannot work around any of the three. Both legs now fail with:

Error: mx.client 0.2.0 is below the 0.2.0.1 chat.api declares

The floor is enforced rather than declared, and every step of reading it 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.

Merge order is now four deep: mx.client#19 → this → drat → corteza#169. Each one is red until its predecessor merges, by construction rather than by convention.

One release note: 0.2.0.1 is a dev marker. A CRAN submission of chat.api will need mx.client's fix published as 0.2.1 first, and the floor moved to match. That is sequencing for you rather than something I should decide.

mx.client#19: 148 assertions, three mutations, both legs green.

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.
@TroyHernandez
TroyHernandez merged commit 660e8d0 into main Aug 5, 2026
2 of 4 checks passed
@TroyHernandez
TroyHernandez deleted the feat/e2ee-adapter branch August 5, 2026 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant